From 14705d08b96508323323a2202d5ef0ac3bf593d4 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 12 Jun 2026 07:15:21 -0700 Subject: [PATCH] fix: route v0.42.41.0's raw exitCode writers through the verdict channel; reconcile merged structural pins (#2084) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI fallout from merging the v0.42.41.0 triage wave into the #2084 exit-seam design — both waves fixed the same timer-placement bug independently: - doctor.ts + extract.ts set failure exit codes via raw `process.exitCode =` writes (v0.42.41.0's process.exit -> exitCode conversion); the #2084 exit seam reads only the gbrain-owned verdict channel, so doctor FAILs exited 0 (Tier 1 RLS e2e + half-migrated-Minions tests). Converted to setCliExitVerdict, same as the wallclock-124 site in the merge commit. - cli-force-exit-teardown-arming.test.ts pinned v0.42.41.0's inline finally-armed timer, which the merge replaced with finishCliTeardown; rewritten to pin the merged invariant (no pre-try arming in cli.ts; the backstop arms inside the helper before the drain). - eval-capture drain timing bound 1s -> 2s: flaked at 1023ms under CI shard load after the new test files shifted LPT shard packing (13x budget slack still proves bounded-not-hung). --- src/commands/doctor.ts | 3 +- src/commands/extract.ts | 3 +- test/cli-force-exit-teardown-arming.test.ts | 81 ++++++++++----------- test/eval-capture-drain.test.ts | 5 +- 4 files changed, 48 insertions(+), 44 deletions(-) diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 028a8ab4d..f27772ac8 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,4 +1,5 @@ import type { BrainEngine } from '../core/engine.ts'; +import { setCliExitVerdict } from '../core/cli-force-exit.ts'; import * as db from '../core/db.ts'; import { LATEST_VERSION, getIdleBlockers } from '../core/migrate.ts'; import { checkResolvable } from '../core/check-resolvable.ts'; @@ -7228,7 +7229,7 @@ export async function runDoctor( // Use process.exitCode instead of process.exit() so cleanup handlers // (e.g. Bun unload events, open database connections) still run before // the process terminates. process.exit() is a hard kill that bypasses them. - process.exitCode = hasFail ? 1 : 0; + setCliExitVerdict(hasFail ? 1 : 0); } // --------------------------------------------------------------------------- diff --git a/src/commands/extract.ts b/src/commands/extract.ts index e6b5a4077..01f5dbe43 100644 --- a/src/commands/extract.ts +++ b/src/commands/extract.ts @@ -29,6 +29,7 @@ */ import { readFileSync, readdirSync, lstatSync, existsSync } from 'fs'; +import { setCliExitVerdict } from '../core/cli-force-exit.ts'; import { join, relative, dirname } from 'path'; import type { BrainEngine, LinkBatchInput, TimelineBatchInput } from '../core/engine.ts'; import type { PageType } from '../core/types.ts'; @@ -864,7 +865,7 @@ Status (v0.42): (r.first_batch_error ? ` (first error: ${r.first_batch_error})` : '') + ` — timeline is incomplete.`, ); - process.exitCode = 1; + setCliExitVerdict(1); } } else if (byMention || ner) { // v0.41.18.0 (T7): combined --by-mention + --ner walk shares one diff --git a/test/cli-force-exit-teardown-arming.test.ts b/test/cli-force-exit-teardown-arming.test.ts index ff9743e30..5d33b8602 100644 --- a/test/cli-force-exit-teardown-arming.test.ts +++ b/test/cli-force-exit-teardown-arming.test.ts @@ -1,56 +1,55 @@ /** - * Structural regression — the DISCONNECT_HARD_DEADLINE_MS force-exit timer in - * cli.ts main() must be armed at TEARDOWN ENTRY (inside the finally, before - * the drain + disconnect), never before the op-dispatch try block. + * Structural regression — the teardown hard-deadline must be armed at + * TEARDOWN ENTRY, never before the op-dispatch body. * - * Pre-fix bug: the 10s unref'd setTimeout was armed BEFORE the try, so any op - * whose handler ran past 10s wall-clock was killed mid-flight with - * process.exit(0) and ZERO stdout — an empty "success" indistinguishable from - * no results (a healthy `gbrain search` on a slow Postgres pooler hit this on - * every run). Armed in the finally, the timer still bounds a hung - * drain/disconnect (the C13 contract) but can no longer kill a - * slow-but-progressing op body. + * Pre-fix bug (closed independently by v0.42.41.0 and the #2084 wave, merged): + * a 10s unref'd setTimeout armed BEFORE the try killed any op whose handler + * ran past 10s wall-clock with process.exit(0) and ZERO stdout — an empty + * "success" indistinguishable from no results. * - * Source-grep is the right tool here (same rationale as - * fix-wave-structural.test.ts): the rule is "this arming must stay at this - * location". A behavioral test would need >10s of real wall-clock plus a - * deliberately slow op handler in a spawned CLI — slow and flaky by - * construction. + * Post-merge shape (#2084): the deadline lives inside `finishCliTeardown` + * (src/core/cli-force-exit.ts), armed as the helper's first act — i.e. at + * teardown entry, because every cli.ts call site invokes the helper from a + * `finally`. The op body's wallclock is bounded separately by the read-scope + * withTimeout wrap (v0.42.41.0). Source-grep is the right tool here (same + * rationale as fix-wave-structural.test.ts): a behavioral test would need + * >10s of real wall-clock in a spawned CLI. */ import { describe, test, expect } from 'bun:test'; import { readFileSync } from 'fs'; describe('cli.ts — disconnect hard-deadline armed at teardown entry, not before the op body', () => { - test('forceExitTimer setTimeout lives inside the finally, gated on the daemon guard, before the drain', () => { - const src = readFileSync('src/cli.ts', 'utf8'); + test('no timer arming exists between op-dispatch setup and the try; the deadline arms inside finishCliTeardown before the drain', () => { + const cli = readFileSync('src/cli.ts', 'utf8'); - const decl = src.indexOf('const DISCONNECT_HARD_DEADLINE_MS'); - expect(decl).toBeGreaterThan(-1); - const tryIdx = src.indexOf('try {', decl); + // The old pre-try arming constant must stay gone (its return is the + // kill-slow-ops-with-exit-0 regression). + expect(cli).not.toContain('DISCONNECT_HARD_DEADLINE_MS'); + + // Between the op-dispatch engine connect and the try there is no + // setTimeout call site (`setTimeout(` matches calls only; the + // ReturnType annotation stays allowed). + const connectIdx = cli.indexOf('// Local engine path (unchanged behavior for local installs).'); + expect(connectIdx).toBeGreaterThan(-1); + const tryIdx = cli.indexOf('try {', connectIdx); expect(tryIdx).toBeGreaterThan(-1); - const finallyIdx = src.indexOf('} finally {', tryIdx); + expect(cli.slice(connectIdx, tryIdx)).not.toContain('setTimeout('); + + // The op-dispatch finally routes through the shared teardown helper. + const finallyIdx = cli.indexOf('} finally {', tryIdx); expect(finallyIdx).toBeGreaterThan(-1); - const armIdx = src.indexOf('forceExitTimer = setTimeout', decl); + const teardownCallIdx = cli.indexOf('finishCliTeardown({ engine, drainTimeoutMs: 1000 })', finallyIdx); + expect(teardownCallIdx).toBeGreaterThan(finallyIdx); + + // Inside the helper, the backstop arms BEFORE the drain runs — teardown + // entry, bounding drain + disconnect and nothing else. + const helper = readFileSync('src/core/cli-force-exit.ts', 'utf8'); + const armIdx = helper.indexOf('const backstop = setTimeout('); expect(armIdx).toBeGreaterThan(-1); - const drainIdx = src.indexOf('drainAllBackgroundWorkForCliExit', finallyIdx); - expect(drainIdx).toBeGreaterThan(-1); - - // NO arming between the deadline declaration and the op-body try — a - // pre-try timer kills slow-but-progressing op handlers mid-flight with - // exit 0 and empty stdout. (`setTimeout(` matches only a call site; the - // `ReturnType` type annotation stays allowed.) - expect(src.slice(decl, tryIdx)).not.toContain('setTimeout('); - - // The arming sits AFTER the finally opens (teardown entry) and BEFORE the - // drain + disconnect it exists to bound. - expect(armIdx).toBeGreaterThan(finallyIdx); - expect(armIdx).toBeLessThan(drainIdx); - - // Still gated on the daemon-survival guard so `serve` stays alive, and - // still unref'd + cleared on clean teardown. - expect(src.slice(finallyIdx, drainIdx)).toMatch(/if \(shouldForceExitAfterMain\(\)\)/); - expect(src.slice(finallyIdx, drainIdx)).toContain('forceExitTimer.unref?.()'); - expect(src.slice(drainIdx)).toContain('if (forceExitTimer) clearTimeout(forceExitTimer)'); + const drainIdx = helper.indexOf('await drain({ timeoutMs: drainTimeoutMs })', armIdx); + expect(drainIdx).toBeGreaterThan(armIdx); + // Cleared on clean teardown. + expect(helper.indexOf('clearTimeout(backstop)', drainIdx)).toBeGreaterThan(drainIdx); }); }); diff --git a/test/eval-capture-drain.test.ts b/test/eval-capture-drain.test.ts index 2823027a3..7ab615d40 100644 --- a/test/eval-capture-drain.test.ts +++ b/test/eval-capture-drain.test.ts @@ -72,6 +72,9 @@ describe('awaitPendingEvalCaptures', () => { const r = await awaitPendingEvalCaptures(150); const elapsed = Date.now() - start; expect(r.unfinished).toBe(1); - expect(elapsed).toBeLessThan(1000); + // Bound proves "bounded, not a hang" — the alternative is infinite. 2s + // (13x the 150ms budget) absorbs CI shard-load timer jitter; the old 1s + // bound flaked at 1023ms on a loaded GitHub runner. + expect(elapsed).toBeLessThan(2000); }); });