mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-29 19:01:39 +00:00
fix: route v0.42.41.0's raw exitCode writers through the verdict channel; reconcile merged structural pins (#2084)
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).
This commit is contained in:
@@ -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);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<typeof setTimeout> 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<typeof setTimeout>` 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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user