diff --git a/src/core/brain-writer.ts b/src/core/brain-writer.ts index a65e6864b..d7ff74082 100644 --- a/src/core/brain-writer.ts +++ b/src/core/brain-writer.ts @@ -409,6 +409,11 @@ export interface ScanOpts { visitDir?: (dirPath: string) => void; } +/** Timeout-arm winner for the COUNT-vs-deadline race in scanBrainSources. + * A unique object so it can never collide with a legitimate COUNT result + * (number | null). Module-private. */ +const DEADLINE_SENTINEL: unique symbol = Symbol('gbrain.scan.deadline'); + export async function scanBrainSources( engine: BrainEngine, opts: ScanOpts = {}, @@ -480,41 +485,43 @@ export async function scanBrainSources( // pool can make this await hang past the budget. Without the race, we'd // wait indefinitely AND defeat the wall-clock guarantee. let dbPageCount: number | null = null; + // Set when the deadline race's timeout arm wins: the verdict that the + // budget is spent, independent of any later Date.now() reading. Timer + // callbacks on loaded runners can fire measurably EARLY relative to the + // wall clock (a +1ms pad was drifted past in practice — see the flake + // lineage in test/brain-writer-partial-scan.test.ts and issue #2946), so + // the hung-COUNT path must not re-derive "did the deadline fire?" from + // the clock the timer just raced against. + let deadlineHit = false; if (opts.dbPageCountForSource) { try { if (opts.deadline) { const remainingMs = opts.deadline - Date.now(); if (remainingMs <= 0) { dbPageCount = null; + deadlineHit = true; } else { - // Race COUNT against the deadline so a hung query can't eat the budget. - // - // Boundary overshoot (+1ms): the post-await deadline check at line - // ~512 uses `Date.now() >= deadline`. setTimeout fires AT OR AFTER - // the requested delay, so in theory the check always passes. In - // practice on heavily-loaded CI runners (8 parallel shards × 4 - // concurrent test files = ~32 concurrent bun processes) we saw - // intermittent failures where the timer callback resolved - // microseconds BEFORE the wall-clock boundary, leaving Date.now() - // a tick below deadline and the skip-check evaluating false. The - // src-a scan then ran on a populated dir before src-b's - // between-source check caught up — causing - // `firstSource.status === 'skipped'` to receive 'scanned'. - // - // Adding 1ms guarantees the timer fires past the deadline by at - // least one millisecond regardless of runner timer drift. Cost is - // 1ms additional wall-clock latency on hung COUNT queries, which - // is operationally negligible. Flake repro: - // https://github.com/garrytan/gbrain/actions/runs/77611667786 - dbPageCount = await Promise.race([ + // Race COUNT against the deadline so a hung query can't eat the + // budget. The timeout arm resolves a private sentinel — NOT null — + // so a deadline win is distinguishable from a COUNT that resolved + // null (failed/absent count keeps its existing semantics). + const raced = await Promise.race([ opts.dbPageCountForSource(src.id), - new Promise(resolve => setTimeout(() => resolve(null), remainingMs + 1)), + new Promise(resolve => + setTimeout(() => resolve(DEADLINE_SENTINEL), remainingMs)), ]); + if (raced === DEADLINE_SENTINEL) { + dbPageCount = null; + deadlineHit = true; + } else { + dbPageCount = raced; + } } } else { dbPageCount = await opts.dbPageCountForSource(src.id); } } catch { + // A throwing COUNT is a failed count, not a deadline verdict. dbPageCount = null; } } @@ -524,11 +531,11 @@ export async function scanBrainSources( // status='partial' with files_scanned=0, which is misleading ("partial // scan" when actually nothing was scanned). Mark this source + remainder // as 'skipped' so the doctor message is honest. - // `>=` matches the between-source check above (line 445). The Promise.race - // setTimeout resolves null at exactly `remainingMs` from now, so post-await - // Date.now() often equals deadline within integer-ms precision — strict `>` - // missed those landings on CI and let the next scanOneSource run anyway. - if (opts.signal?.aborted || (opts.deadline && Date.now() >= opts.deadline)) { + // `deadlineHit` is the authoritative verdict for the hung-COUNT path (the + // sentinel above); the wall-clock re-check (`>=`, matching the + // between-source check at line ~445) still covers a COUNT that RESOLVED + // slowly enough to eat the budget without the timer winning. + if (opts.signal?.aborted || deadlineHit || (opts.deadline && Date.now() >= opts.deadline)) { if (abortedAtSource === null) { abortedAtSource = src.id; } diff --git a/test/brain-writer-partial-scan.test.ts b/test/brain-writer-partial-scan.test.ts index 611c98dc6..e9b780134 100644 --- a/test/brain-writer-partial-scan.test.ts +++ b/test/brain-writer-partial-scan.test.ts @@ -174,6 +174,21 @@ describe('scanBrainSources partial-scan state', () => { expect(report.aborted_at_source).toBe('src-a'); }); + // #2946: the deadline race's timeout arm resolves a SENTINEL, not null — + // a COUNT that legitimately resolves null (failed/absent count) before the + // deadline must NOT be mistaken for a deadline hit: the source still gets + // scanned, with db_page_count simply unavailable. + test('COUNT resolving null before the deadline is not a deadline verdict — source still scans', async () => { + const start = Date.now(); + const report = await scanBrainSources(engine, { + deadline: start + 5_000, + dbPageCountForSource: async () => null, + }); + const firstSource = report.per_source.find(r => r.source_id === 'src-a')!; + expect(firstSource.status).toBe('scanned'); + expect(firstSource.db_page_count == null).toBe(true); + }); + // Codex adversarial #4 regression: even when dbPageCountForSource itself // would hang indefinitely, the Promise.race against the deadline must // resolve null and the scan must abort cleanly.