mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
fix(brain-writer): deadline race verdict from the sentinel, not the clock the timer raced (#2947)
Part of #2946. The hung-COUNT deadline race derived its verdict from a post-await wall-clock re-check, which races the timer's own drift: on loaded CI runners setTimeout callbacks fire measurably EARLY relative to Date.now(), so each padding/boundary adjustment (>=, +1ms) only moved which wrong status the partial-scan test received ('scanned', then 'partial'). The race's timeout arm now resolves a module-private sentinel; the sentinel winning IS the deadline verdict (deadlineHit), consulted by the post-await check without re-reading the clock. A COUNT that resolves null (failed/absent count) stays distinguishable and does not skip the scan; a COUNT that resolves slowly without the timer winning is still caught by the retained wall-clock re-check. The +1ms pad is gone — the sentinel makes timer drift irrelevant for the hung path. Verified: partial-scan suite green 8 consecutive runs incl. the new null-vs-sentinel distinction test. Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+33
-26
@@ -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<null>(resolve => setTimeout(() => resolve(null), remainingMs + 1)),
|
||||
new Promise<typeof DEADLINE_SENTINEL>(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;
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user