From 039aed79c38941394ae5975348d8617c5836837f Mon Sep 17 00:00:00 2001 From: sudacode Date: Mon, 10 Aug 2026 23:50:48 -0700 Subject: [PATCH] fix(stats): fix duplicate-line cleanup edge cases - Drain the full write queue before scanning, not just one batch, so pending burst rows aren't missed - Floor lookback-days before the positivity check so a sub-day value no longer collapses to a zero-day window - Let the parsed cue list override the streaming dedup heuristic wherever it covers a line - Reject combining --lifetime with --duplicate-lines and non-positive --lookback-days values - Widen the burst frame bound to catch heavier typesetting while still sparing longer-spaced runs - Keep the cleanup modal open until the user closes it so they can read the result before it unmounts --- docs-site/immersion-tracking.md | 2 ++ docs-site/usage.md | 1 + launcher/parse-args.test.ts | 24 +++++++++++++ .../services/__tests__/stats-server.test.ts | 31 ++++++++++++++++ .../services/immersion-tracker-service.ts | 26 +++++++++++--- .../__tests__/duplicate-line-cleanup.test.ts | 35 ++++++++++++++++++- .../duplicate-line-cleanup.ts | 13 +++++-- .../services/stats-server/route-support.ts | 6 ++-- .../services/subtitle-line-dedup-gate.test.ts | 17 +++++++++ src/core/services/subtitle-line-dedup-gate.ts | 8 +++-- .../vocabulary/DuplicateLineCleanup.tsx | 16 ++++++--- 11 files changed, 163 insertions(+), 16 deletions(-) diff --git a/docs-site/immersion-tracking.md b/docs-site/immersion-tracking.md index ade702e5..2b1d16ad 100644 --- a/docs-site/immersion-tracking.md +++ b/docs-site/immersion-tracking.md @@ -147,6 +147,8 @@ subminer stats cleanup --duplicate-lines --dry-run --lookback-days 30 subminer stats cleanup --duplicate-lines --lookback-days 30 ``` +`--duplicate-lines` (short: `-d`) picks the cleanup mode, so it cannot be combined with `--lifetime`, and `--dry-run` and `--lookback-days ` only apply to it. Omitting `--lookback-days` scans all history; the value must be at least one day. + Runs never cross a session boundary, so rewatching an episode keeps both watches. Session telemetry (watch time, lines seen, tokens seen) and the rollups derived from it are left as recorded: they are cumulative samples taken during playback, and cannot be recomputed for sessions whose raw rows have since been pruned. ## Retention Defaults diff --git a/docs-site/usage.md b/docs-site/usage.md index 4fb56288..b2971ada 100644 --- a/docs-site/usage.md +++ b/docs-site/usage.md @@ -96,6 +96,7 @@ subminer stats -b # Start/reuse the background stats daemon subminer stats -s # Stop the background stats daemon subminer stats cleanup # Backfill vocabulary metadata, prune stale rows subminer stats cleanup -d --dry-run # Preview cleanup of repeated typeset subtitle lines +subminer stats cleanup -d --lookback-days 30 # Clean only lines recorded in the last 30 days subminer stats rebuild # Rebuild rollup data subminer doctor --refresh-known-words # Refresh the known-word cache subminer logs -e # Export a sanitized log ZIP and print its path diff --git a/launcher/parse-args.test.ts b/launcher/parse-args.test.ts index 063e7fe8..c55c6150 100644 --- a/launcher/parse-args.test.ts +++ b/launcher/parse-args.test.ts @@ -255,6 +255,30 @@ test('parseArgs rejects duplicate-line flags without the duplicate-lines mode', assert.match(error.stderr, /--dry-run and --lookback-days require --duplicate-lines/); }); +test('parseArgs rejects combining lifetime and duplicate-line cleanup modes', () => { + const error = withProcessExitIntercept(() => { + parseArgs(['stats', 'cleanup', '--lifetime', '--duplicate-lines'], 'subminer', {}); + }); + + assert.equal(error.code, 1); + assert.match(error.stderr, /Stats cleanup runs one mode at a time/); +}); + +test('parseArgs rejects unusable lookback windows', () => { + for (const value of ['0', '-5', 'soon']) { + const error = withProcessExitIntercept(() => { + parseArgs( + ['stats', 'cleanup', '--duplicate-lines', '--lookback-days', value], + 'subminer', + {}, + ); + }); + + assert.equal(error.code, 1); + assert.match(error.stderr, /--lookback-days must be a positive number of days/); + } +}); + test('parseArgs rejects cleanup-only stats flags without cleanup action', () => { const error = withProcessExitIntercept(() => { parseArgs(['stats', '--vocab'], 'subminer', {}); diff --git a/src/core/services/__tests__/stats-server.test.ts b/src/core/services/__tests__/stats-server.test.ts index 205faab4..ee0bba02 100644 --- a/src/core/services/__tests__/stats-server.test.ts +++ b/src/core/services/__tests__/stats-server.test.ts @@ -1064,6 +1064,37 @@ describe('stats server API routes', () => { assert.deepEqual(seenOptions, { dryRun: true, lookbackDays: 30 }); }); + it('POST /api/stats/maintenance/duplicate-lines ignores a window shorter than a day', async () => { + let seenOptions: unknown = null; + const app = createStatsApp( + createMockTracker({ + cleanupDuplicateSubtitleLines: async (options: unknown) => { + seenOptions = options; + return { + dryRun: true, + lookbackDays: null, + scannedLines: 0, + burstGroups: 0, + removedLines: 0, + removedWordOccurrences: 0, + removedKanjiOccurrences: 0, + samples: [], + }; + }, + }), + ); + + const res = await app.request('/api/stats/maintenance/duplicate-lines', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ dryRun: true, lookbackDays: 0.5 }), + }); + + assert.equal(res.status, 200); + // Half a day must not floor to a zero-day window; it means no limit. + assert.deepEqual(seenOptions, { dryRun: true, lookbackDays: null }); + }); + it('POST /api/stats/maintenance/duplicate-lines treats a missing body as an apply over all history', async () => { let seenOptions: unknown = null; const app = createStatsApp( diff --git a/src/core/services/immersion-tracker-service.ts b/src/core/services/immersion-tracker-service.ts index 1223b885..01e49b79 100644 --- a/src/core/services/immersion-tracker-service.ts +++ b/src/core/services/immersion-tracker-service.ts @@ -601,14 +601,14 @@ export class ImmersionTrackerService { } /** - * Collapse animation bursts that earlier versions recorded frame by frame. Pending - * writes are flushed first so a burst that is still queued is scanned as stored rows - * rather than surviving the cleanup and reappearing seconds later. + * Collapse animation bursts that earlier versions recorded frame by frame. The whole + * queue is drained first so a burst still waiting to be written is scanned as stored + * rows rather than surviving the cleanup and landing a moment after it. */ async cleanupDuplicateSubtitleLines( options: DuplicateSubtitleLineCleanupOptions = {}, ): Promise { - this.flushNow(); + this.drainQueue(); return cleanupDuplicateSubtitleLines(this.db, options); } @@ -1816,6 +1816,24 @@ export class ImmersionTrackerService { } } + /** + * Write out everything queued, not just the next batch. + * + * `flushNow` writes at most `batchSize` entries and does nothing at all while the write + * lock is held, so a maintenance pass that runs straight after it can still be reading + * a database that is missing rows. Each pass has to shrink the queue to continue: a + * failed flush puts its batch back, and looping on that would never finish. + */ + private drainQueue(): void { + while (this.queue.length > 0) { + const pendingBefore = this.queue.length; + this.flushNow(); + if (this.queue.length >= pendingBefore) { + return; + } + } + } + private flushSingle(write: QueuedWrite): void { executeQueuedWrite(write, this.preparedStatements); } diff --git a/src/core/services/immersion-tracker/__tests__/duplicate-line-cleanup.test.ts b/src/core/services/immersion-tracker/__tests__/duplicate-line-cleanup.test.ts index 62dc12a4..9b5e56f4 100644 --- a/src/core/services/immersion-tracker/__tests__/duplicate-line-cleanup.test.ts +++ b/src/core/services/immersion-tracker/__tests__/duplicate-line-cleanup.test.ts @@ -59,11 +59,12 @@ function seed(db: DatabaseSync, lines: SeedLine[]): void { lines.forEach((line, index) => { const lineId = index + 1; + const lineIndex = index + 1; const createdMs = line.createdMs ?? BASE_MS; insertLine.run( lineId, line.session, - lineId, + lineIndex, line.startMs, line.endMs, line.text, @@ -174,6 +175,38 @@ test('ordinary repeated dialogue survives', () => { } }); +test('a long run of quarter-second frames is still a burst', () => { + // Between the timing-only bound (0.1s) and the animation-frame bound (0.3s): heavier + // typesetting lands here, and the run length is what makes it conclusive. + const { db, dbPath } = createDb(karaokeFrames(1, '飛び上がる', 10_000, 6, 250)); + + try { + const summary = cleanupDuplicateSubtitleLines(db); + + assert.equal(summary.burstGroups, 1); + assert.equal(summary.removedLines, 5); + assert.equal(countLines(db), 1); + assert.equal(wordFrequency(db), 1); + } finally { + db.close(); + cleanupDbPath(dbPath); + } +}); + +test('a run of frames longer than the animation bound survives', () => { + const { db, dbPath } = createDb(karaokeFrames(1, '飛び上がる', 10_000, 6, 400)); + + try { + const summary = cleanupDuplicateSubtitleLines(db); + + assert.equal(summary.burstGroups, 0); + assert.equal(countLines(db), 6); + } finally { + db.close(); + cleanupDbPath(dbPath); + } +}); + test('a short run below the threshold survives', () => { const { db, dbPath } = createDb(karaokeFrames(1, '飛び上がる', 1_000, 4, 40)); diff --git a/src/core/services/immersion-tracker/duplicate-line-cleanup.ts b/src/core/services/immersion-tracker/duplicate-line-cleanup.ts index c86f053b..b6cd8402 100644 --- a/src/core/services/immersion-tracker/duplicate-line-cleanup.ts +++ b/src/core/services/immersion-tracker/duplicate-line-cleanup.ts @@ -8,9 +8,16 @@ * * Only timing is available here: the stored text has been stripped of ASS markup, so the * authoring evidence the file-level parser uses (`\t`, `\move`, karaoke timing, a - * changing override signature) is long gone. The rule is therefore the strict, - * metadata-free one -- a long run of identical, contiguous, short-lived lines inside a - * single session -- and every bound is configurable so a cautious run can ask for more. + * changing override signature) is long gone. What is left is a run of identical, + * contiguous, short-lived lines inside a single session. + * + * The run has to be as long as the timing-only rule in `subtitle-cue-dedup` demands, but + * each line may be as long as the animation-frame bound rather than the much tighter + * timing-only one. Five or more repeats of the same text, each ending where the next + * begins, is already conclusive on its own -- no dialogue does that -- and the tighter + * bound would walk straight past the heavier typesetting that motivated this, where + * frames sit nearer a quarter of a second. Both bounds are options, so a cautious run can + * ask for more, and a dry run always reports before anything is removed. * * Scope: subtitle lines, their word/kanji occurrences, and the `imm_words`/`imm_kanji` * aggregates those occurrences feed. Session telemetry (`lines_seen`, `tokens_seen`) and diff --git a/src/core/services/stats-server/route-support.ts b/src/core/services/stats-server/route-support.ts index d9a8eb3a..75892b26 100644 --- a/src/core/services/stats-server/route-support.ts +++ b/src/core/services/stats-server/route-support.ts @@ -98,10 +98,12 @@ export function parseDuplicateLineCleanupBody(body: unknown): { } { const source = body && typeof body === 'object' ? (body as Record) : {}; const rawLookback = source.lookbackDays; - const lookbackDays = - typeof rawLookback === 'number' && Number.isFinite(rawLookback) && rawLookback > 0 + // Floor before the bounds check, or a fraction of a day arrives as a zero-day window. + const wholeDays = + typeof rawLookback === 'number' && Number.isFinite(rawLookback) ? Math.floor(rawLookback) : null; + const lookbackDays = wholeDays !== null && wholeDays >= 1 ? wholeDays : null; return { dryRun: source.dryRun === true, lookbackDays }; } diff --git a/src/core/services/subtitle-line-dedup-gate.test.ts b/src/core/services/subtitle-line-dedup-gate.test.ts index 4921f9ce..6f190511 100644 --- a/src/core/services/subtitle-line-dedup-gate.test.ts +++ b/src/core/services/subtitle-line-dedup-gate.test.ts @@ -43,6 +43,23 @@ test('parsed cues keep separate lines that merely repeat', () => { assert.equal(recorded.length, 3); }); +test('parsed cues outrank the streaming heuristic for short repeated cues', () => { + // Long enough to trip the timing-only rule, but the parser saw these with full + // lookahead and kept them, so every one of them is a line the sidebar shows. + const cues: SubtitleCue[] = Array.from({ length: 8 }, (_, index) => ({ + startTime: 3 + index * 0.08, + endTime: 3 + (index + 1) * 0.08, + text: 'えっ', + })); + const gate = createSubtitleLineDedupGate({ getParsedCues: () => cues }); + + const recorded = cues.filter((cue) => + gate.shouldRecord({ text: cue.text, startSec: cue.startTime, endSec: cue.endTime }), + ); + + assert.equal(recorded.length, 8); +}); + test('a line whose timing does not match any cue still records', () => { // A shifted track, an embedded sub nobody parsed: no match, no drop. const cues: SubtitleCue[] = [{ startTime: 10, endTime: 14, text: '飛び上がる' }]; diff --git a/src/core/services/subtitle-line-dedup-gate.ts b/src/core/services/subtitle-line-dedup-gate.ts index 6d06b5de..d01512b2 100644 --- a/src/core/services/subtitle-line-dedup-gate.ts +++ b/src/core/services/subtitle-line-dedup-gate.ts @@ -169,10 +169,14 @@ export function createSubtitleLineDedupGate( return true; } + // The parsed cue list has the final say wherever it covers this line. Falling + // through to the streaming heuristic would let it drop cues the parser looked at + // with full lookahead and deliberately kept apart, which is the disagreement + // between sidebar and stats this gate exists to prevent. const spans = lookupSpans(text); - if (spans && isMergedAwayFrame(spans, sample.startSec)) { + if (spans) { run = null; - return false; + return !isMergedAwayFrame(spans, sample.startSec); } return advanceStreamingRun(text, sample); diff --git a/stats/src/components/vocabulary/DuplicateLineCleanup.tsx b/stats/src/components/vocabulary/DuplicateLineCleanup.tsx index 0155d456..986a8a18 100644 --- a/stats/src/components/vocabulary/DuplicateLineCleanup.tsx +++ b/stats/src/components/vocabulary/DuplicateLineCleanup.tsx @@ -43,7 +43,6 @@ export function DuplicateLineCleanup({ onClose, onCleaned }: DuplicateLineCleanu } else { setApplied(result); setPreview(null); - onCleaned(); } } catch (cause) { setError(cause instanceof Error ? cause.message : String(cause)); @@ -51,9 +50,18 @@ export function DuplicateLineCleanup({ onClose, onCleaned }: DuplicateLineCleanu setBusy(null); } }, - [lookbackDays, onCleaned], + [lookbackDays], ); + // Reloading the vocabulary tables unmounts this modal along with the rest of the tab, + // so it waits for the user to close: they get to read what was removed first. + const close = useCallback(() => { + if (applied && applied.removedLines > 0) { + onCleaned(); + } + onClose(); + }, [applied, onCleaned, onClose]); + const result = applied ?? preview; const nothingToDo = preview !== null && preview.removedLines === 0; @@ -63,7 +71,7 @@ export function DuplicateLineCleanup({ onClose, onCleaned }: DuplicateLineCleanu type="button" aria-label="Close duplicate line cleanup" className="absolute inset-0 bg-ctp-crust/70 backdrop-blur-[2px]" - onClick={onClose} + onClick={close} />
@@ -71,7 +79,7 @@ export function DuplicateLineCleanup({ onClose, onCleaned }: DuplicateLineCleanu