mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-14 00:48:18 +00:00
fix(doctor): sync_freshness falls back to content-lag when the clone is unavailable — stop false stale/FAIL after stateless-container restarts (#2908)
On stateless deploys (Docker on EB/K8s/Fly — what the cloud recipes produce), a container restart wipes federated clones; each is only re-materialized when that source's next sync job runs. Until then the v0.41.27.0 git short-circuit cannot probe HEAD at all, and the check fell through to raw wall-clock age — which no-op syncs never advance — so every QUIET source read as stale/FAIL right after a restart. Observed live: 16-source brain, 12 clones gone after a config-update restart, doctor 70 -> 25-35, monitor alert storm (score < threshold) while every clone that DID exist was byte-identical to origin HEAD. Fix: classify the probe three ways (probeSourceGitState: unchanged / changed / unavailable). 'unavailable' + chunker match borrows the REMOTE path's newest_content_at lag (v0.41.32.0) — DB-only, no subprocess — so a quiet source reads healthy while real missed work (content newer than last sync) still reports stale. 'changed' (readable clone, HEAD moved / dirty) keeps wall-clock exactly as before, and a chunker mismatch disables the fallback (D7: a pending re-chunk is never masked). isSourceUnchangedSinceSync stays as a boolean facade so source-health.ts is untouched. Tests: 6 new doctor cases (F1-F6, incl. three-bucket invariant) + 7 probeSourceGitState unit cases; existing suites green (doctor 90, git-head 21, source-health 28), tsc --noEmit clean.
This commit is contained in:
+30
-8
@@ -40,7 +40,7 @@ import {
|
||||
buildBasenameIndex,
|
||||
queryBasenameIndex,
|
||||
} from '../core/link-extraction.ts';
|
||||
import { isSourceUnchangedSinceSync } from '../core/git-head.ts';
|
||||
import { probeSourceGitState } from '../core/git-head.ts';
|
||||
// v0.41.32.0: remote staleness reads the stored newest_content_at column via
|
||||
// this pure comparator (no git subprocess on the HTTP MCP doctor path).
|
||||
import { lagFromContentMs } from '../core/source-health.ts';
|
||||
@@ -4062,29 +4062,51 @@ export async function checkSyncFreshness(
|
||||
// All four must hold; otherwise fall through to the time-based check.
|
||||
// The chunker version match is computed here (not in the helper)
|
||||
// because it depends on engine state, not git state.
|
||||
//
|
||||
// Clone-unavailable fallback: on stateless deploys (Docker on EB /
|
||||
// K8s / Fly — the platforms the cloud recipes produce), a container
|
||||
// restart wipes `local_path` and each clone is only re-materialized
|
||||
// when that source's next sync job runs. Until then the HEAD probe
|
||||
// cannot run at all ('unavailable'), which previously fell through to
|
||||
// raw wall-clock age — and since a no-op sync doesn't advance
|
||||
// `last_sync_at`, every QUIET source read as stale/FAIL after a
|
||||
// restart (score-sinking alert storm; observed live: 16-source brain,
|
||||
// 12 clones gone after a config-update restart, doctor 70→30).
|
||||
// 'unavailable' + chunker match now reuses the v0.41.32.0 REMOTE lag
|
||||
// signal (newest_content_at) below — DB-only, no subprocess, and it
|
||||
// still reports staleness whenever content really is newer than the
|
||||
// last sync. 'changed' (readable clone with real work) keeps
|
||||
// wall-clock exactly as before, and a chunker mismatch is never
|
||||
// masked (D7): it disables the fallback too.
|
||||
let cloneUnavailable = false;
|
||||
if (localOnly) {
|
||||
const gitUnchanged = isSourceUnchangedSinceSync(
|
||||
const gitState = probeSourceGitState(
|
||||
source.local_path,
|
||||
source.last_commit,
|
||||
{ requireCleanWorkingTree: 'ignore-untracked' },
|
||||
);
|
||||
const chunkerMatch = source.chunker_version === currentChunkerVersion;
|
||||
if (gitUnchanged && chunkerMatch) {
|
||||
if (gitState === 'unchanged' && chunkerMatch) {
|
||||
unchanged_count++;
|
||||
continue;
|
||||
}
|
||||
cloneUnavailable = gitState === 'unavailable' && chunkerMatch;
|
||||
}
|
||||
|
||||
// v0.41.32.0: REMOTE path (doctorReportRemote, !localOnly) computes lag
|
||||
// from the stored newest_content_at column — NO git subprocess on a
|
||||
// DB-supplied local_path (preserves the v0.41.27.0 trust boundary). A
|
||||
// quiet repo whose newest commit predates its last sync reports 0; NULL
|
||||
// column → wall-clock fallback. LOCAL fall-through keeps wall-clock: the
|
||||
// short-circuit already failed, so the source genuinely has work and
|
||||
// "hours since last sync" is the right staleness measure. The `ageMs < 0`
|
||||
// skew check above still runs on raw wall-clock for both paths (A1).
|
||||
// column → wall-clock fallback. LOCAL fall-through keeps wall-clock when
|
||||
// the clone is READABLE: the short-circuit failed on real evidence
|
||||
// (HEAD moved / dirty tree), so the source genuinely has work and
|
||||
// "hours since last sync" is the right staleness measure. A local clone
|
||||
// that is UNAVAILABLE (not yet re-materialized, see above) carries no
|
||||
// evidence either way, so it borrows this same DB-only lag. The
|
||||
// `ageMs < 0` skew check above still runs on raw wall-clock for both
|
||||
// paths (A1).
|
||||
let thresholdAgeMs = ageMs;
|
||||
if (!localOnly) {
|
||||
if (!localOnly || cloneUnavailable) {
|
||||
const contentMs = source.newest_content_at
|
||||
? new Date(source.newest_content_at).getTime()
|
||||
: null;
|
||||
|
||||
+55
-13
@@ -96,29 +96,71 @@ export interface GitFreshnessOpts {
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true iff `localPath` is a git repo whose current HEAD matches
|
||||
* `lastCommit`, AND (when `requireCleanWorkingTree`) the working tree
|
||||
* is clean.
|
||||
* Three-state git probe verdict for a federated source clone.
|
||||
*
|
||||
* - `'unchanged'`: HEAD matches `last_commit` (and, when requested, the
|
||||
* working tree is clean). Sync has nothing to do.
|
||||
* - `'changed'`: the clone is readable but HEAD moved, the tree is
|
||||
* dirty, or the DB never recorded a `last_commit` —
|
||||
* sync genuinely has (or may have) work.
|
||||
* - `'unavailable'`: the HEAD probe itself could not run — the clone
|
||||
* directory is missing, not a git repo, or git errored.
|
||||
* On stateless deploys (containers on EB / K8s / Fly,
|
||||
* where `local_path` dies with the filesystem and is
|
||||
* lazily re-materialized by the next per-source sync)
|
||||
* this is a NORMAL steady state for quiet sources, not
|
||||
* evidence of pending work. Callers can fall back to a
|
||||
* DB-only freshness signal instead of wall-clock age.
|
||||
*/
|
||||
export type SourceGitState = 'unchanged' | 'changed' | 'unavailable';
|
||||
|
||||
/**
|
||||
* Probe a source clone and classify it (see `SourceGitState`).
|
||||
*
|
||||
* This is NOT a full mirror of `gbrain sync`'s "do work?" predicate.
|
||||
* Chunker-version match is computed by the caller because it depends on
|
||||
* engine state (`sources.chunker_version` vs `CURRENT_CHUNKER_VERSION`).
|
||||
* See `src/commands/doctor.ts:checkSyncFreshness` for the AND
|
||||
* combination at the call site.
|
||||
*
|
||||
* NULL-input guard stays first: a NULL `last_commit` (legacy row) returns
|
||||
* `'changed'` WITHOUT running the head probe — same short-circuit contract
|
||||
* `isSourceUnchangedSinceSync` always had (pinned by doctor.test.ts case 4).
|
||||
*/
|
||||
export function probeSourceGitState(
|
||||
localPath: string | null | undefined,
|
||||
lastCommit: string | null | undefined,
|
||||
opts?: GitFreshnessOpts,
|
||||
): SourceGitState {
|
||||
if (!localPath || !lastCommit) return 'changed';
|
||||
const head = _headProbe(localPath);
|
||||
if (head === null) return 'unavailable';
|
||||
if (head !== lastCommit) return 'changed';
|
||||
if (opts?.requireCleanWorkingTree) {
|
||||
const ignoreUntracked = opts.requireCleanWorkingTree === 'ignore-untracked';
|
||||
const isClean = _cleanProbe(localPath, ignoreUntracked);
|
||||
// null (probe error) AND false (known dirty) both fail the gate. A clean
|
||||
// probe error with a READABLE head is not classified 'unavailable' —
|
||||
// fail toward "may have work" so the gate can only relax, never mask.
|
||||
if (isClean !== true) return 'changed';
|
||||
}
|
||||
return 'unchanged';
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true iff `localPath` is a git repo whose current HEAD matches
|
||||
* `lastCommit`, AND (when `requireCleanWorkingTree`) the working tree
|
||||
* is clean.
|
||||
*
|
||||
* Boolean façade over `probeSourceGitState` — `'unavailable'` and
|
||||
* `'changed'` both collapse to `false`, preserving the v0.41.27.0
|
||||
* fail-open contract for callers that only care about the short-circuit
|
||||
* (`src/core/source-health.ts`).
|
||||
*/
|
||||
export function isSourceUnchangedSinceSync(
|
||||
localPath: string | null | undefined,
|
||||
lastCommit: string | null | undefined,
|
||||
opts?: GitFreshnessOpts,
|
||||
): boolean {
|
||||
if (!localPath || !lastCommit) return false;
|
||||
const head = _headProbe(localPath);
|
||||
if (head === null || head !== lastCommit) return false;
|
||||
if (opts?.requireCleanWorkingTree) {
|
||||
const ignoreUntracked = opts.requireCleanWorkingTree === 'ignore-untracked';
|
||||
const isClean = _cleanProbe(localPath, ignoreUntracked);
|
||||
// null (probe error) AND false (known dirty) both fail the gate.
|
||||
if (isClean !== true) return false;
|
||||
}
|
||||
return true;
|
||||
return probeSourceGitState(localPath, lastCommit, opts) === 'unchanged';
|
||||
}
|
||||
|
||||
@@ -12,6 +12,7 @@ import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import {
|
||||
isSourceUnchangedSinceSync,
|
||||
probeSourceGitState,
|
||||
_setGitHeadProbeForTests,
|
||||
_setGitCleanProbeForTests,
|
||||
type GitHeadProbe,
|
||||
@@ -176,3 +177,60 @@ describe('isSourceUnchangedSinceSync — requireCleanWorkingTree (D7)', () => {
|
||||
expect(cleanCalls).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('probeSourceGitState — three-state verdict', () => {
|
||||
test('state 1: HEAD matches + clean → unchanged', () => {
|
||||
_setGitHeadProbeForTests(() => 'abc123');
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
expect(probeSourceGitState('/tmp/repo', 'abc123', { requireCleanWorkingTree: 'ignore-untracked' }))
|
||||
.toBe('unchanged');
|
||||
});
|
||||
|
||||
test('state 2: HEAD probe null (clone missing / not a repo / git error) → unavailable', () => {
|
||||
_setGitHeadProbeForTests(() => null);
|
||||
expect(probeSourceGitState('/tmp/gone', 'abc123')).toBe('unavailable');
|
||||
});
|
||||
|
||||
test('state 3: HEAD mismatch → changed', () => {
|
||||
_setGitHeadProbeForTests(() => 'def456');
|
||||
expect(probeSourceGitState('/tmp/repo', 'abc123')).toBe('changed');
|
||||
});
|
||||
|
||||
test('state 4: dirty tree with readable HEAD → changed (NOT unavailable)', () => {
|
||||
_setGitHeadProbeForTests(() => 'abc123');
|
||||
_setGitCleanProbeForTests(() => false);
|
||||
expect(probeSourceGitState('/tmp/repo', 'abc123', { requireCleanWorkingTree: true }))
|
||||
.toBe('changed');
|
||||
});
|
||||
|
||||
test('state 5: clean-probe ERROR with readable HEAD → changed (fail toward work)', () => {
|
||||
_setGitHeadProbeForTests(() => 'abc123');
|
||||
_setGitCleanProbeForTests(() => null);
|
||||
expect(probeSourceGitState('/tmp/repo', 'abc123', { requireCleanWorkingTree: true }))
|
||||
.toBe('changed');
|
||||
});
|
||||
|
||||
test('state 6: NULL inputs → changed, head probe never called (case-4 contract)', () => {
|
||||
let probeCalls = 0;
|
||||
_setGitHeadProbeForTests(() => { probeCalls++; return 'abc'; });
|
||||
expect(probeSourceGitState(null, 'abc')).toBe('changed');
|
||||
expect(probeSourceGitState('/tmp/repo', null)).toBe('changed');
|
||||
expect(probeSourceGitState('', '')).toBe('changed');
|
||||
expect(probeCalls).toBe(0);
|
||||
});
|
||||
|
||||
test('state 7: boolean façade parity — isSourceUnchangedSinceSync === (state is unchanged)', () => {
|
||||
_setGitHeadProbeForTests(() => 'abc123');
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
for (const [path, commit] of [
|
||||
['/tmp/repo', 'abc123'], // unchanged → true
|
||||
['/tmp/repo', 'other'], // changed → false
|
||||
] as const) {
|
||||
expect(isSourceUnchangedSinceSync(path, commit))
|
||||
.toBe(probeSourceGitState(path, commit) === 'unchanged');
|
||||
}
|
||||
_setGitHeadProbeForTests(() => null); // unavailable → false
|
||||
expect(isSourceUnchangedSinceSync('/tmp/gone', 'abc123'))
|
||||
.toBe(probeSourceGitState('/tmp/gone', 'abc123') === 'unchanged');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1580,3 +1580,174 @@ describe('BUG 4 — in-progress sync via live lock, not stale freshness', () =>
|
||||
expect(result.status).toBe('fail');
|
||||
});
|
||||
});
|
||||
|
||||
// ============================================================================
|
||||
// sync_freshness — clone-unavailable content-lag fallback (stateless deploys)
|
||||
// ============================================================================
|
||||
// A container restart (Docker on EB / K8s / Fly) wipes federated clones;
|
||||
// each one is only re-materialized when that source's next sync job runs.
|
||||
// Until then the LOCAL git short-circuit cannot probe HEAD at all. That is
|
||||
// not evidence of pending work, so instead of falling through to raw
|
||||
// wall-clock age (which no-op syncs never advance → false stale/FAIL for
|
||||
// every quiet source after a restart), the check borrows the REMOTE path's
|
||||
// newest_content_at lag (v0.41.32.0). Contracts:
|
||||
// F1: clone unavailable + content at/before last sync → healthy (lag 0).
|
||||
// F2: clone unavailable + content NEWER than last sync → still stale
|
||||
// (wall-clock) — real missed work is never masked.
|
||||
// F3: clone unavailable + NULL newest_content_at → wall-clock fallback
|
||||
// (pre-migration parity with git short-circuit case 5).
|
||||
// F4: chunker mismatch disables the fallback (D7 — a pending re-chunk is
|
||||
// never masked).
|
||||
// F5: a READABLE clone that failed the short-circuit (HEAD moved) keeps
|
||||
// wall-clock even when newest_content_at is old — the fallback is
|
||||
// scoped to 'unavailable' only.
|
||||
// ============================================================================
|
||||
describe('sync_freshness — clone-unavailable content-lag fallback', () => {
|
||||
function makeStubEngine(rows: any[]): any {
|
||||
return { executeRaw: async () => rows };
|
||||
}
|
||||
function agoMs(ms: number): Date { return new Date(Date.now() - ms); }
|
||||
const HOURS = 60 * 60 * 1000;
|
||||
let currentChunkerVersion: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
const { CHUNKER_VERSION } = await import('../src/core/chunkers/code.ts');
|
||||
currentChunkerVersion = String(CHUNKER_VERSION);
|
||||
_setGitHeadProbeForTests(null);
|
||||
_setGitCleanProbeForTests(null);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests(null);
|
||||
_setGitCleanProbeForTests(null);
|
||||
});
|
||||
|
||||
test('F1: quiet source, clone gone, content predates last sync → ok', async () => {
|
||||
const { checkSyncFreshness } = await import('../src/commands/doctor.ts');
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests(() => null); // clone not re-materialized yet
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
|
||||
const result = await checkSyncFreshness(makeStubEngine([
|
||||
{ id: 'quiet-docs', name: '', local_path: '/tmp/quiet-docs',
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'abc', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: agoMs(72 * HOURS) }, // content older than last sync
|
||||
]), { localOnly: true });
|
||||
|
||||
expect(result.status).toBe('ok');
|
||||
expect(result.details).toEqual({
|
||||
unchanged_count: 0, synced_recently_count: 1, stale_count: 0,
|
||||
});
|
||||
});
|
||||
|
||||
test('F2: clone gone but content NEWER than last sync → warn (real work not masked)', async () => {
|
||||
const { checkSyncFreshness } = await import('../src/commands/doctor.ts');
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests(() => null);
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
|
||||
const result = await checkSyncFreshness(makeStubEngine([
|
||||
{ id: 'missed-work', name: '', local_path: '/tmp/missed-work',
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'abc', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: agoMs(1 * HOURS) }, // content NEWER than last sync
|
||||
]), { localOnly: true });
|
||||
|
||||
expect(result.status).toBe('warn');
|
||||
expect(result.message).toMatch(/40h ago/);
|
||||
expect(result.details?.stale_count).toBe(1);
|
||||
});
|
||||
|
||||
test('F3: clone gone + NULL newest_content_at → wall-clock fallback (warn)', async () => {
|
||||
const { checkSyncFreshness } = await import('../src/commands/doctor.ts');
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests(() => null);
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
|
||||
const result = await checkSyncFreshness(makeStubEngine([
|
||||
{ id: 'pre-migration', name: '', local_path: '/tmp/pre-migration',
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'abc', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: null },
|
||||
]), { localOnly: true });
|
||||
|
||||
expect(result.status).toBe('warn');
|
||||
expect(result.details?.stale_count).toBe(1);
|
||||
});
|
||||
|
||||
test('F4: clone gone + chunker MISMATCH → fallback disabled, wall-clock warn', async () => {
|
||||
const { checkSyncFreshness } = await import('../src/commands/doctor.ts');
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests(() => null);
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
|
||||
const result = await checkSyncFreshness(makeStubEngine([
|
||||
{ id: 'needs-rechunk', name: '', local_path: '/tmp/needs-rechunk',
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'abc',
|
||||
chunker_version: '0', // STALE — re-chunk pending
|
||||
newest_content_at: agoMs(72 * HOURS) },
|
||||
]), { localOnly: true });
|
||||
|
||||
expect(result.status).toBe('warn');
|
||||
expect(result.details?.stale_count).toBe(1);
|
||||
});
|
||||
|
||||
test('F5: readable clone, HEAD moved → wall-clock even with old content', async () => {
|
||||
const { checkSyncFreshness } = await import('../src/commands/doctor.ts');
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests(() => 'NEW-HEAD'); // clone readable, real work
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
|
||||
const result = await checkSyncFreshness(makeStubEngine([
|
||||
{ id: 'has-commits', name: '', local_path: '/tmp/has-commits',
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'OLD-HEAD', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: agoMs(72 * HOURS) },
|
||||
]), { localOnly: true });
|
||||
|
||||
expect(result.status).toBe('warn');
|
||||
expect(result.message).toMatch(/40h ago/);
|
||||
expect(result.details?.stale_count).toBe(1);
|
||||
});
|
||||
|
||||
test('F6: three-bucket invariant holds across rescued + unchanged + stale', async () => {
|
||||
const { checkSyncFreshness } = await import('../src/commands/doctor.ts');
|
||||
const { _setGitHeadProbeForTests, _setGitCleanProbeForTests } =
|
||||
await import('../src/core/git-head.ts');
|
||||
_setGitHeadProbeForTests((path) => path === '/tmp/frozen' ? 'frozen-sha' : null);
|
||||
_setGitCleanProbeForTests(() => true);
|
||||
|
||||
const result = await checkSyncFreshness(makeStubEngine([
|
||||
{ id: 'frozen', name: '', local_path: '/tmp/frozen', // unchanged bucket
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'frozen-sha', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: agoMs(80 * HOURS) },
|
||||
{ id: 'rescued', name: '', local_path: '/tmp/rescued', // clone gone, quiet → healthy
|
||||
last_sync_at: agoMs(40 * HOURS),
|
||||
last_commit: 'abc', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: agoMs(80 * HOURS) },
|
||||
{ id: 'stale', name: '', local_path: '/tmp/stale', // clone gone, content newer → stale
|
||||
last_sync_at: agoMs(5 * 24 * HOURS),
|
||||
last_commit: 'def', chunker_version: currentChunkerVersion,
|
||||
newest_content_at: agoMs(1 * HOURS) },
|
||||
]), { localOnly: true });
|
||||
|
||||
expect(result.status).toBe('fail');
|
||||
expect(result.message).toContain(`'stale'`);
|
||||
expect(result.message).not.toContain(`'rescued'`);
|
||||
expect(result.details).toEqual({
|
||||
unchanged_count: 1, synced_recently_count: 1, stale_count: 1,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user