fix(cycle): surface stamp-write failure on CycleReport and degrade status (#3504) (#3589)

Co-Authored-By: Ryan Ayers <rayers@dividia.net>

src/core/cycle.ts

@@ -362,8 +362,22 @@ export interface CycleReport {
    *   - 'failed'  : lock acquired but all attempted phases failed
    */
   status: CycleStatus;
-  /** Present when status = 'skipped'. E.g., 'cycle_already_running' or 'no_database'. Also 'aborted' when the cycle was cancelled mid-flight (#1972). */
+  /** Present when status = 'skipped'. E.g., 'cycle_already_running' or 'no_database'. Also 'aborted' when the cycle was cancelled mid-flight (#1972), or 'stamp_write_failed' (#3504). */
   reason?: string;
+  /**
+   * #3504: the cycle ran, but persisting `last_source_cycle_at` /
+   * `last_full_cycle_at` threw. Set ONLY on a real write error — never for a
+   * pack that merely omits optional phases (those come back 'skipped' and
+   * `deriveStatus` correctly ignores them).
+   *
+   * When present, `status` is degraded away from success, because a cycle that
+   * cannot record that it finished is not a cycle that finished as far as every
+   * downstream freshness reader is concerned. Before this existed the failure
+   * was a `console.warn` only: `dream --json` reported `status: 'ok'`, doctor
+   * separately reported `cycle_freshness` stale, and nothing connected the two,
+   * so re-running (the advice doctor gives) could never fix it.
+   */
+  stamp_write_failed?: { source_id: string; error: string };
   /**
    * #1972: dead-holder sync/cycle locks the cycle-start reaper cleared this
    * run (count + lock ids). Omitted when nothing was reaped or no engine.
@@ -2549,9 +2563,14 @@ export async function runCycle(
   //   - status is 'failed' or 'skipped' (don't mark a non-run as fresh)
   //   - dryRun (writes are out of scope)
   //
-  // Best-effort: a write failure does NOT change the CycleReport status.
-  // The cost of writing the wrong timestamp post-failure is higher than
-  // the cost of missing a successful write (next cycle will redo work).
+  // #3504: the write is still best-effort in the sense that it never throws out
+  // of runCycle and never aborts the run (the phases already did their work).
+  // But a failure is no longer invisible: it is recorded on the report and
+  // degrades `status` away from success, so a cycle that could not persist its
+  // "done" stamp stops claiming it finished. The cost of writing the wrong
+  // timestamp post-failure is still higher than missing a successful write, so
+  // the stamp itself is unchanged — only the reporting is.
+  let stampWriteFailed: { source_id: string; error: string } | undefined;
   if (opts.sourceId && engine && !dryRun && !aborted && (status === 'ok' || status === 'clean' || status === 'partial')) {
     try {
       const nowIso = new Date().toISOString();
@@ -2567,17 +2586,29 @@ export async function runCycle(
         last_full_cycle_at: nowIso,
       });
     } catch (e) {
-      // Best-effort; cycle already succeeded by the time we get here.
-      console.warn(`[cycle] failed to write last_source_cycle_at for source ${opts.sourceId}: ${e instanceof Error ? e.message : String(e)}`);
+      const message = e instanceof Error ? e.message : String(e);
+      // Record it so `--json` consumers and the autopilot runner can see it.
+      // stderr alone does not survive a cron run, which is how #2251 stayed
+      // invisible while every stamp write failed for weeks.
+      stampWriteFailed = { source_id: opts.sourceId, error: message };
+      console.warn(`[cycle] failed to write last_source_cycle_at for source ${opts.sourceId}: ${message}`);
     }
   }
 
+  // #3504: a stamp-write failure degrades a successful run to 'partial'. It
+  // cannot upgrade or downgrade anything else: 'partial' is already non-success,
+  // and 'failed'/'skipped' never reach the stamp block at all. `aborted` still
+  // wins the reason slot, since an aborted run is the more fundamental fact.
+  const degradedByStamp = stampWriteFailed !== undefined && (status === 'ok' || status === 'clean');
+  const effectiveStatus: CycleStatus = aborted ? 'partial' : degradedByStamp ? 'partial' : status;
+
   return {
     schema_version: '1',
     timestamp,
     duration_ms,
-    status: aborted ? 'partial' : status,
-    ...(aborted ? { reason: 'aborted' } : {}),
+    status: effectiveStatus,
+    ...(aborted ? { reason: 'aborted' } : stampWriteFailed ? { reason: 'stamp_write_failed' } : {}),
+    ...(stampWriteFailed ? { stamp_write_failed: stampWriteFailed } : {}),
     ...(reapedLocks ? { reaped_dead_holder_locks: reapedLocks } : {}),
     brain_dir: opts.brainDir,
     phases: phaseResults,

test/cycle-last-full-cycle-at.test.ts

@@ -9,7 +9,9 @@
  *   - status is 'ok' | 'clean' | 'partial' (failed/skipped don't mark fresh)
  *   - dryRun is false
  *
- * Best-effort: a write failure does NOT change the CycleReport status.
+ * Best-effort in that it never throws out of runCycle. As of #3504 a write
+ * failure IS surfaced: it sets `stamp_write_failed` on the report and degrades
+ * a successful status to 'partial'. See test/cycle-stamp-write-failure.test.ts.
  */
 import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test';
 import { PGLiteEngine } from '../src/core/pglite-engine.ts';

test/cycle-stamp-write-failure.test.ts

@@ -0,0 +1,159 @@
+/**
+ * #3504 — a cycle that cannot persist its freshness stamp must stop reporting
+ * success.
+ *
+ * Before this, `updateSourceConfig` throwing was a `console.warn` and nothing
+ * else. `gbrain dream --json` reported `status: 'ok'`, doctor separately
+ * reported `cycle_freshness` stale, and no signal connected them — so the fix
+ * doctor recommends (re-run the cycle) could never work, because the cycle was
+ * already succeeding. That is the loop #2251 sat in while every stamp write
+ * failed on a corrupted `sources.config`.
+ *
+ * Contract pinned here:
+ *   - a stamp-write error sets `stamp_write_failed: {source_id, error}`
+ *   - it degrades 'ok' / 'clean' to 'partial' and sets reason 'stamp_write_failed'
+ *   - it NEVER throws out of runCycle (the phases already did their work)
+ *   - a pack that merely omits optional phases is NOT affected: those phases come
+ *     back 'skipped', `deriveStatus` ignores them by design, and the status stays
+ *     a success status. This is the conflation the maintainer flagged on #3504.
+ */
+import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test';
+import { PGLiteEngine } from '../src/core/pglite-engine.ts';
+import { resetPgliteState } from './helpers/reset-pglite.ts';
+import { withEnv } from './helpers/with-env.ts';
+import { runCycle } from '../src/core/cycle.ts';
+import { mkdtempSync } from 'fs';
+import { tmpdir } from 'os';
+import { join } from 'path';
+
+let engine: PGLiteEngine;
+let brainDir: string;
+// Per-test GBRAIN_HOME isolation: the PGLite cycle path takes a file lock at
+// `~/.gbrain/cycle.lock`, unscoped by source. Without isolation, a sibling
+// worktree running its own tests makes runCycle return 'skipped' and the stamp
+// hook silently no-ops. Same rationale as cycle-last-full-cycle-at.test.ts.
+let gbrainHome: string;
+
+const SOURCE = 'stamp-fail-src';
+
+beforeAll(async () => {
+  engine = new PGLiteEngine();
+  await engine.connect({});
+  await engine.initSchema();
+});
+
+afterAll(async () => {
+  await engine.disconnect();
+});
+
+beforeEach(async () => {
+  await resetPgliteState(engine);
+  brainDir = mkdtempSync(join(tmpdir(), 'gbrain-stamp-brain-'));
+  gbrainHome = mkdtempSync(join(tmpdir(), 'gbrain-stamp-home-'));
+  await engine.executeRaw(
+    `INSERT INTO sources (id, name, config) VALUES ($1, $2, $3::text::jsonb)
+     ON CONFLICT (id) DO NOTHING`,
+    [SOURCE, 'Stamp Fail Source', '{}'],
+  );
+});
+
+/** Run a per-source cycle with updateSourceConfig forced to throw. */
+async function runWithFailingStamp(message: string) {
+  const original = engine.updateSourceConfig.bind(engine);
+  let calls = 0;
+  (engine as unknown as { updateSourceConfig: unknown }).updateSourceConfig = async () => {
+    calls += 1;
+    throw new Error(message);
+  };
+  try {
+    const report = await withEnv({ GBRAIN_HOME: gbrainHome }, () =>
+      runCycle(engine, { brainDir, sourceId: SOURCE, phases: ['lint'] }),
+    );
+    return { report, calls };
+  } finally {
+    (engine as unknown as { updateSourceConfig: unknown }).updateSourceConfig = original;
+  }
+}
+
+describe('#3504 stamp-write failure is surfaced on the report', () => {
+  test('sets stamp_write_failed with the source id and the error message', async () => {
+    const { report, calls } = await runWithFailingStamp('jsonb_each on a non-object');
+    expect(calls).toBeGreaterThan(0);
+    expect(report.stamp_write_failed).toBeDefined();
+    expect(report.stamp_write_failed!.source_id).toBe(SOURCE);
+    expect(report.stamp_write_failed!.error).toContain('jsonb_each on a non-object');
+  });
+
+  test('degrades a successful status to partial with reason stamp_write_failed', async () => {
+    const { report } = await runWithFailingStamp('write blew up');
+    expect(report.status).toBe('partial');
+    expect(report.reason).toBe('stamp_write_failed');
+  });
+
+  test('does NOT throw out of runCycle — the phases already ran', async () => {
+    const { report } = await runWithFailingStamp('write blew up');
+    // The run still produced a report with its phase results intact.
+    expect(report.schema_version).toBe('1');
+    expect(report.phases.length).toBeGreaterThan(0);
+  });
+});
+
+describe('#3504 no false positives', () => {
+  test('a healthy per-source cycle has no stamp_write_failed and keeps a success status', async () => {
+    const report = await withEnv({ GBRAIN_HOME: gbrainHome }, () =>
+      runCycle(engine, { brainDir, sourceId: SOURCE, phases: ['lint'] }),
+    );
+    expect(report.stamp_write_failed).toBeUndefined();
+    expect(report.reason).toBeUndefined();
+    expect(['ok', 'clean']).toContain(report.status);
+  });
+
+  test('a pack that omits optional phases is not conflated with a stamp failure', async () => {
+    // The distinction the maintainer called out on #3504: `deriveStatus`
+    // deliberately ignores 'skipped' phases, so omitting optional phases is not
+    // a failure. Only a real write error may degrade the status.
+    const report = await withEnv({ GBRAIN_HOME: gbrainHome }, () =>
+      runCycle(engine, { brainDir, sourceId: SOURCE, phases: ['lint'] }),
+    );
+    const skipped = report.phases.filter((p) => p.status === 'skipped');
+    // Whether or not any phase skipped in this environment, the invariant holds:
+    // a success status must not carry a stamp-failure marker.
+    expect(report.stamp_write_failed).toBeUndefined();
+    if (skipped.length > 0) {
+      expect(['ok', 'clean']).toContain(report.status);
+    }
+  });
+
+  test('dryRun does not attempt the write and cannot report a stamp failure', async () => {
+    const original = engine.updateSourceConfig.bind(engine);
+    let called = false;
+    (engine as unknown as { updateSourceConfig: unknown }).updateSourceConfig = async () => {
+      called = true;
+      throw new Error('should never run under dryRun');
+    };
+    try {
+      const report = await withEnv({ GBRAIN_HOME: gbrainHome }, () =>
+        runCycle(engine, { brainDir, sourceId: SOURCE, phases: ['lint'], dryRun: true }),
+      );
+      expect(called).toBe(false);
+      expect(report.stamp_write_failed).toBeUndefined();
+    } finally {
+      (engine as unknown as { updateSourceConfig: unknown }).updateSourceConfig = original;
+    }
+  });
+
+  test('a legacy caller with no sourceId cannot report a stamp failure', async () => {
+    const original = engine.updateSourceConfig.bind(engine);
+    (engine as unknown as { updateSourceConfig: unknown }).updateSourceConfig = async () => {
+      throw new Error('should never run without sourceId');
+    };
+    try {
+      const report = await withEnv({ GBRAIN_HOME: gbrainHome }, () =>
+        runCycle(engine, { brainDir, phases: ['lint'] }),
+      );
+      expect(report.stamp_write_failed).toBeUndefined();
+    } finally {
+      (engine as unknown as { updateSourceConfig: unknown }).updateSourceConfig = original;
+    }
+  });
+});