diff --git a/src/commands/autopilot.ts b/src/commands/autopilot.ts index 554bc26fd..d43bd661f 100644 --- a/src/commands/autopilot.ts +++ b/src/commands/autopilot.ts @@ -38,6 +38,7 @@ import { logSelfUpgrade } from '../core/audit/self-upgrade-audit.ts'; import { detectInstallMethod } from './upgrade.ts'; import { evaluateQuietHours } from '../core/minions/quiet-hours.ts'; import { inspectLock } from '../core/db-lock.ts'; +import { registerCleanup } from '../core/process-cleanup.ts'; /** * v0.37.7.0 #1162 — classify autopilot reconnect-loop errors. @@ -433,6 +434,37 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { let stopping = false; let childSupervisor: ChildWorkerSupervisor | null = null; + // #1872: graceful engine shutdown. On PGLite the cycle steps run INLINE in + // this process, so a hard `process.exit` mid-write (systemctl stop → + // SIGTERM) kills WASM Postgres with the WAL dirty and can corrupt the + // brain. Two exit paths must both close the engine: + // - autopilot's own shutdown() below (owns SIGINT + internal stops like + // max_crashes / cycle-failure-cap), and + // - process-cleanup's SIGTERM handler (installed at cli.ts module load; + // it runs the cleanup registry with a 3s deadline and then exits) — + // which is why closeEngine is ALSO registered there. + // closeEngine aborts the in-flight inline cycle (runCycle checks the + // signal between phases and threads it into phase sub-work), gives it a + // short bounded window to wind down, then disconnects. PGLite's + // disconnect() drains the pending query and checkpoints before closing; + // a second call is a no-op (disconnect snapshots + nulls the handle), so + // both paths firing is safe. + const shutdownAbort = new AbortController(); + let inflightInlineCycle: Promise | null = null; + const closeEngine = async () => { + shutdownAbort.abort(new Error('autopilot shutdown')); + if (inflightInlineCycle) { + // ponytail: 2s cap keeps us inside process-cleanup's 3s deadline; a + // between-phase abort resolves instantly, a mid-phase one may not. + await Promise.race([ + inflightInlineCycle.catch(() => { /* cycle errors already logged by the loop */ }), + new Promise((r) => setTimeout(r, 2_000)), + ]); + } + try { await engine.disconnect(); } catch { /* best-effort */ } + }; + const deregisterEngineClose = registerCleanup('autopilot-engine-close', closeEngine); + if (spawnManagedWorker) { const cliPath = resolveGbrainCliPath(); // Cgroup-aware auto-sized RSS watchdog cap (issue #1678). The old flat @@ -520,6 +552,10 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { childSupervisor.killChild('SIGKILL'); } } + // #1872: abort the in-flight inline cycle and close the engine BEFORE + // process.exit — a hard exit mid-write corrupts PGLite's WASM Postgres. + await closeEngine(); + deregisterEngineClose(); try { unlinkSync(lockPath); } catch { /* already gone */ } process.exit(0); }; @@ -1024,16 +1060,21 @@ export async function runAutopilot(engine: BrainEngine, args: string[]) { // path's phase set). Now both converge on the same primitive. try { const { runCycle } = await import('../core/cycle.ts'); - const report = await runCycle(engine, { + // #1872: track the promise so closeEngine can drain it on shutdown, + // and pass the abort signal so the cycle winds down between phases. + const cyclePromise = runCycle(engine, { brainDir: repoPath, // Autopilot daemon path: pulls by default (matches // pre-v0.17 autopilot behavior). CLI dream defaults false // for cron safety; that choice is scoped to dream only. pull: true, + signal: shutdownAbort.signal, yieldBetweenPhases: async () => { await new Promise(r => setImmediate(r)); }, }); + inflightInlineCycle = cyclePromise; + const report = await cyclePromise.finally(() => { inflightInlineCycle = null; }); // Only 'failed' (every attempted phase failed) trips the autopilot // circuit breaker. 'partial' means at least one phase warned or // failed while others ran — that's a soft signal, not a fatal diff --git a/src/commands/dream.ts b/src/commands/dream.ts index d2bc62bb5..71b598c6d 100644 --- a/src/commands/dream.ts +++ b/src/commands/dream.ts @@ -26,6 +26,7 @@ import type { BrainEngine } from '../core/engine.ts'; import { runCycle, + resolveSourceForDir, ALL_PHASES, type CyclePhase, type CycleReport, @@ -380,9 +381,9 @@ Options: --source Scope the cycle to one source so doctor's cycle_freshness check sees a fresh stamp on - completion. Without this, gbrain dream's - timestamp never lands and federated brains - see "stale cycle" forever. + completion. When omitted, gbrain derives the + source from --dir / the configured checkout + when it matches a source's local_path (#1869). --source-id Alias for --source. Matches the v0.37.7.0+ naming used by import/extract/graph-query. @@ -634,6 +635,25 @@ export async function runDream(engine: BrainEngine | null, args: string[]): Prom ); process.exit(1); } + + // #1869: a path-scoped run (--dir, or the configured sync.repo_path) whose + // directory matches a registered source's local_path IS that source's cycle + // — derive the source id so runCycle writes last_source_cycle_at / + // last_full_cycle_at on success and doctor's cycle_freshness check stops + // reading perpetually stale. Explicit --source still wins (resolved above). + // Fixed here at the command level, NOT in runCycle's stamp gate, so legacy + // global callers (autopilot-global-maintenance runs GLOBAL_PHASES with a + // brainDir and no sourceId) can't falsely stamp per-source freshness. + // A derived match on an archived source is skipped silently (falls back to + // legacy unscoped behavior) — stamping it would mask staleness on restore, + // mirroring the explicit --source archived guard above. + if (resolvedSourceId === undefined && engine !== null && brainDir !== null) { + const derived = await resolveSourceForDir(engine, brainDir); + if (derived !== undefined) { + const src = await fetchSource(engine, derived); + if (src?.archived !== true) resolvedSourceId = derived; + } + } // ─── issue #1678: bounded single-hold extract_atoms drain ────────── if (opts.drain) { if (engine === null) { diff --git a/src/core/cycle.ts b/src/core/cycle.ts index 0b5195795..a6a15fbc1 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -855,8 +855,16 @@ interface SyncPhaseResult extends PhaseResult { * Resolve the source id for a brain directory by looking up the sources * table. Returns undefined when no registered source matches (falls back * to pre-v0.18 global config.sync.* keys). + * + * Exported for dream.ts (#1869): a `gbrain dream --dir ` run whose + * path matches a registered source's local_path is a per-source cycle in + * everything but name, so dream derives the source id up front and passes + * it as opts.sourceId — landing the freshness stamp without changing + * runCycle's stamp/lock semantics for legacy global callers (the + * autopilot-global-maintenance handler runs GLOBAL_PHASES with a brainDir + * and MUST NOT stamp per-source freshness; see rejected PR #2549). */ -async function resolveSourceForDir( +export async function resolveSourceForDir( engine: BrainEngine, brainDir: string | null, ): Promise { diff --git a/test/autopilot-shutdown-engine-close.test.ts b/test/autopilot-shutdown-engine-close.test.ts new file mode 100644 index 000000000..8d2e3ae01 --- /dev/null +++ b/test/autopilot-shutdown-engine-close.test.ts @@ -0,0 +1,63 @@ +/** + * #1872 — autopilot SIGTERM/SIGINT must close the engine before exit. + * + * On PGLite the cycle steps run INLINE in the autopilot process, so a hard + * `process.exit` mid-write (systemctl stop → SIGTERM) kills WASM Postgres + * with the WAL dirty and can corrupt the brain. Two exit paths must both + * close the engine: + * + * - autopilot's own shutdown() (owns SIGINT + internal stops like + * max_crashes / cycle-failure-cap), and + * - process-cleanup's SIGTERM handler (installed at cli.ts module load, + * which exits within its 3s cleanup deadline) — reached via the + * registered 'autopilot-engine-close' cleanup callback. + * + * Because the shutdown path is deep inside `runAutopilot()` (a long-running + * daemon loop that ends in process.exit), a behavioral test would have to + * spawn + signal a real daemon. Following the established precedent + * (test/autopilot-supervisor-wiring.test.ts, test/autopilot-fanout-wiring.test.ts), + * these static-shape regressions pin the load-bearing wiring instead. + */ +import { describe, expect, it } from 'bun:test'; +import { readFileSync } from 'fs'; +import { join } from 'path'; + +const AUTOPILOT_SRC = readFileSync( + join(import.meta.dir, '..', 'src', 'commands', 'autopilot.ts'), + 'utf8', +); + +describe('autopilot.ts graceful engine shutdown (#1872)', () => { + it('registers an engine-close callback in the process-cleanup registry (SIGTERM path)', () => { + // process-cleanup owns SIGTERM (installed at cli.ts:10) and hard-exits + // after its cleanup pass; without this registration the engine is never + // closed on `systemctl stop`. + expect(AUTOPILOT_SRC).toContain( + "import { registerCleanup } from '../core/process-cleanup.ts';", + ); + expect(AUTOPILOT_SRC).toContain( + "registerCleanup('autopilot-engine-close', closeEngine)", + ); + }); + + it('closeEngine aborts the in-flight inline cycle then disconnects the engine', () => { + // Abort first (runCycle checks the signal between phases and threads it + // into phase sub-work), bounded drain, then disconnect. + expect(AUTOPILOT_SRC).toMatch( + /const closeEngine = async \(\) => \{[\s\S]{0,900}shutdownAbort\.abort\([\s\S]{0,900}engine\.disconnect\(\)/, + ); + }); + + it('the inline runCycle call carries the shutdown abort signal and is tracked as in-flight', () => { + // PGLite / --inline path: the cycle runs in-process, so shutdown must be + // able to (a) signal it to wind down and (b) await it before closing. + expect(AUTOPILOT_SRC).toMatch(/signal:\s*shutdownAbort\.signal/); + expect(AUTOPILOT_SRC).toMatch(/inflightInlineCycle\s*=\s*cyclePromise/); + }); + + it('shutdown() awaits closeEngine() before process.exit(0) (SIGINT + internal-stop path)', () => { + expect(AUTOPILOT_SRC).toMatch( + /await closeEngine\(\);[\s\S]{0,400}process\.exit\(0\)/, + ); + }); +}); diff --git a/test/dream-dir-source-stamp.test.ts b/test/dream-dir-source-stamp.test.ts new file mode 100644 index 000000000..ef2ca34c6 --- /dev/null +++ b/test/dream-dir-source-stamp.test.ts @@ -0,0 +1,99 @@ +/** + * #1869 — `gbrain dream --dir ` stamps cycle freshness when the path + * matches a registered source's local_path. + * + * Pre-fix, only `--source ` runs wrote last_source_cycle_at / + * last_full_cycle_at (runCycle's stamp gate reads opts.sourceId, and dream + * never derived one from --dir), so a path-scoped brain showed doctor's + * cycle_freshness as perpetually stale. + * + * The fix lives in dream.ts (derive the source id from the resolved brain + * dir via resolveSourceForDir), NOT in runCycle's stamp gate — a runCycle- + * wide change would make the autopilot-global-maintenance handler (global + * phases, brainDir set, no sourceId) falsely stamp per-source freshness + * (the #2194 poisoning class; see rejected PR #2549). + * + * Same real-PGLite/no-mocks discipline as test/dream.test.ts; same + * GBRAIN_HOME isolation as test/cycle-last-full-cycle-at.test.ts (the + * cycle's PGLite file lock lives under ~/.gbrain). + */ +import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test'; +import { mkdtempSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { runDream } from '../src/commands/dream.ts'; +import { withEnv } from './helpers/with-env.ts'; + +let engine: PGLiteEngine; +let brainDir: string; +let gbrainHome: string; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}, 60_000); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); + brainDir = mkdtempSync(join(tmpdir(), 'gbrain-dream-stamp-')); + gbrainHome = mkdtempSync(join(tmpdir(), 'gbrain-dream-stamp-home-')); +}, 60_000); + +afterEach(() => { + rmSync(brainDir, { recursive: true, force: true }); + rmSync(gbrainHome, { recursive: true, force: true }); +}); + +async function seedSource(id: string, archived = false): Promise { + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, archived, created_at) + VALUES ($1, $2, $3, '{}'::jsonb, $4, NOW())`, + [id, id, brainDir, archived], + ); +} + +async function readLastFullCycleAt(sourceId: string): Promise { + const rows = await engine.executeRaw<{ config: Record | null }>( + `SELECT config FROM sources WHERE id = $1`, + [sourceId], + ); + const raw = rows[0]?.config?.last_full_cycle_at; + return typeof raw === 'string' ? raw : null; +} + +describe('gbrain dream --dir freshness stamp (#1869)', () => { + test('--dir matching a source local_path stamps last_full_cycle_at', async () => { + await withEnv({ GBRAIN_HOME: gbrainHome }, async () => { + await seedSource('path-scoped'); + expect(await readLastFullCycleAt('path-scoped')).toBeNull(); + + const report = await runDream(engine, ['--dir', brainDir, '--phase', 'lint', '--json']); + expect(report).toBeTruthy(); + if (report) expect(['ok', 'clean']).toContain(report.status); + + // Pre-fix this stays null forever: dream never passed a sourceId, so + // runCycle's stamp gate skipped the write. + expect(await readLastFullCycleAt('path-scoped')).not.toBeNull(); + }); + }, 60_000); + + test('--dir matching an ARCHIVED source does not stamp it', async () => { + await withEnv({ GBRAIN_HOME: gbrainHome }, async () => { + await seedSource('mothballed', true); + + const report = await runDream(engine, ['--dir', brainDir, '--phase', 'lint', '--json']); + expect(report).toBeTruthy(); + + // Stamping an archived source would mask data staleness when it is + // later restored (mirrors the explicit --source archived guard). + expect(await readLastFullCycleAt('mothballed')).toBeNull(); + }); + }, 60_000); +}); diff --git a/test/dream.test.ts b/test/dream.test.ts index ebff52620..232921228 100644 --- a/test/dream.test.ts +++ b/test/dream.test.ts @@ -589,12 +589,22 @@ describe('runDream — --source / --source-id (v0.41.13)', () => { // ─── Back-compat: bare `gbrain dream` does NOT write per-source stamp ─ - test('gbrain dream (no --source) leaves all sources untouched (back-compat regression)', async () => { - await seedSource('alpha'); - await seedSource('beta'); + test('gbrain dream (no --source) stamps only the source whose local_path matches --dir (#1869)', async () => { + // Pre-#1869 this asserted NO source was ever stamped without an explicit + // --source — which is exactly the bug: a path-scoped `gbrain dream --dir` + // run never landed a freshness stamp and doctor's cycle_freshness stayed + // stale forever. New truth: the source whose local_path matches the + // resolved brain dir is derived and stamped; unrelated sources stay + // untouched (cross-source isolation). + await seedSource('alpha'); // local_path = repo → derived + stamped + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, archived, created_at) + VALUES ($1, $2, $3, '{}'::jsonb, false, NOW())`, + ['beta', 'beta', '/somewhere/else'], + ); const report = await runDream(engine, ['--dir', repo, '--phase', 'lint', '--json']); expect(report).toBeTruthy(); - expect(await readLastFullCycleAt('alpha')).toBeNull(); + expect(await readLastFullCycleAt('alpha')).not.toBeNull(); expect(await readLastFullCycleAt('beta')).toBeNull(); }, 60_000);