fix(sync): reconcile the stale old row when a rename falls back to add (#3056) (#3479)

master's rename loop swallows updateSlug failures with an empty catch
("treat as add"), and updateSlug returns void — so a zero-row UPDATE
(old slug absent) and a thrown collision are both invisible. Either way
the run falls through to importFile at the new path while the old row
stays behind live: slug occupied, 0 chunks after the next embed pass,
page count unchanged. A rename that didn't rename, with no trace.

The fix reconciles the duplicate:

- updateSlug returns the number of rows moved in both engines (a
  zero-row UPDATE does not throw; the count is the only way to see it).
- When the cheap rename didn't move a row AND the destination
  demonstrably materialized — imported, or an errorless skip AT the new
  slug (NOT an identity-dedup skip against the old row, which would mean
  nothing landed and deleting the old row would destroy the only copy) —
  the stale row is located positively by source_path = from and deleted.
  No source_path match → nothing is deleted (code-strategy imports don't
  populate source_path and fall back safely to leaving the row).
- A failed reconcile delete records a <rename:…> sentinel: the failure
  gate hard-blocks the bookmark, the auto-skip valve can never
  chronic-skip it (which would bank the duplicate permanently after a
  multi-run outage), and the rename is not checkpointed — the next run
  retries the same diff and clears the sentinel on convergence.

Co-authored-by: Time Attakc <89218912+time-attack@users.noreply.github.com>
This commit is contained in:
Masa
2026-07-28 19:32:55 -07:00
committed by GitHub
co-authored by Time Attakc
parent 85286a556c
commit e72d93fdb5
5 changed files with 377 additions and 9 deletions
+81 -4
View File
@@ -2874,10 +2874,17 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
: await resolveSlugByPathOrSourcePath(engine, from, undefined);
// The new path doesn't yet have a row, so resolve from path only.
const newSlug = resolveSlugForPath(to);
// #3056: the cheap rename is OBSERVED, not assumed. A zero-row UPDATE
// doesn't throw, and a thrown collision used to be swallowed by an
// empty catch — both fell through to importFile, which created/updated
// the row at the new path while the old row stayed behind live. Both
// shapes now fall through to the reconcile below.
let renameApplied = false;
try {
await engine.updateSlug(oldSlug, newSlug, renameOpts);
renameApplied = (await engine.updateSlug(oldSlug, newSlug, renameOpts)) > 0;
} catch {
// Slug doesn't exist or collision, treat as add
// Destination slug occupied or invalid — treat as add; the reconcile
// below removes the stale old row once the destination materialized.
}
// Reimport at new path (picks up content changes). Wrapped to match the
// deletes/adds loops: a malformed renamed file is recorded to failedFiles
@@ -2890,9 +2897,11 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
// NAV-1 TOCTOU: refuse a destination that realpath-resolves outside the
// repo (committed symlink pointing out).
const filePath = join(gitContextRoot, to);
let importResult: Awaited<ReturnType<typeof importFile>> | undefined;
if (existsSync(filePath) && isPathSafe(filePath, gitContextRoot)) {
try {
const result = await importFile(engine, filePath, to, { noEmbed, sourceId: opts.sourceId, activePack: syncActivePack });
importResult = result;
if (result.status === 'imported') chunksCreated += result.chunks;
else if (result.status === 'skipped' && (result as { error?: string }).error) {
failedFiles.push({ path: to, error: String((result as { error?: string }).error) });
@@ -2901,9 +2910,68 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
failedFiles.push({ path: to, error: e instanceof Error ? e.message : String(e) });
}
}
// #3056 reconcile: the rename fell back to add semantics, so the row
// that still represents the OLD path is the stale half of the rename
// (git reported the old path gone; a plain delete of that path would
// remove this row). Two safety rails, both from the #3252 review:
//
// 1. Delete only after the destination demonstrably materialized —
// `imported`, or an errorless `skipped` AT the new slug. Identity
// dedup can skip against the OLD row (result.slug === oldSlug),
// in which case nothing landed at newSlug and deleting the old
// row would destroy the only copy.
// 2. Locate the stale row POSITIVELY by `source_path = from`, never
// by the oldSlug guess — after a collision, a path-derived
// fallback slug could name an unrelated (e.g. manually curated)
// row. No source_path match → nothing is deleted (this also means
// code-strategy imports, which don't populate source_path, fall
// back safely to leaving the old row rather than guessing).
//
// A failed delete records a `<rename:…>` SENTINEL (not an ordinary
// path failure): the gate hard-blocks the bookmark, and — unlike a
// plain path row — the auto-skip valve can never chronic-skip it after
// N attempts, which would advance the bookmark and make a transient
// delete outage a permanent duplicate. The sentinel clears through the
// ordinary success path once the rename converges on a later run.
let reconcileFailed = false;
if (!renameApplied && importResult !== undefined) {
const destMaterialized = importResult.status === 'imported' ||
(importResult.status === 'skipped' && !importResult.error && importResult.slug === newSlug);
if (destMaterialized) {
try {
const staleMap = await engine.resolveSlugsByPaths([from], { sourceId: opts.sourceId ?? DEFAULT_SOURCE_ID });
const staleSlug = staleMap.get(from);
if (staleSlug !== undefined && staleSlug !== newSlug) {
await engine.deletePage(staleSlug, renameOpts);
deletedSlugs.add(staleSlug); // never hand a deleted slug to auto-embed
serr(` [sync] rename reconciled: removed stale row ${staleSlug} (${from} -> ${to} fell back to add).`);
} else if (staleSlug === undefined) {
serr(` [sync] rename fallback: no row has source_path ${from}; stale row (if any) left in place.`);
}
} catch (e: unknown) {
reconcileFailed = true;
failedFiles.push({
path: `<rename:${to}>`,
error: `rename reconcile failed (stale row for ${from} not removed): ` +
`${e instanceof Error ? e.message : String(e)}`,
});
}
} else {
serr(
` [sync] rename fallback: ${from} -> ${to} did not materialize at ${newSlug} ` +
`(import ${importResult.status}); old row left in place.`,
);
}
}
// Converged (cheap rename, clean reconcile, or nothing to reconcile):
// clear any `<rename:…>` sentinel a previous failing run recorded.
if (!reconcileFailed) succeededPaths.push(`<rename:${to}>`);
pagesAffected.push(newSlug);
deletedSlugs.delete(newSlug); // #1284: rename landed on a previously-deleted slug → embeddable again
await markCompleted(to);
// A failed reconcile must NOT checkpoint: banking `to` would make the
// resume filter skip this rename on the retry run, turning a transient
// delete failure into a permanent duplicate — the exact bug being fixed.
if (!reconcileFailed) await markCompleted(to);
progress.tick(1, newSlug);
}
progress.finish();
@@ -3362,7 +3430,10 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
if (!gate.advanced) {
const codeBreakdown = formatCodeBreakdown(failedFiles);
if (gate.sentinelBlocked) {
// Two sentinel classes block here: `<head>` (pin ancestry broken) and
// `<rename:…>` (#3056 — a rename-reconcile delete failed and advancing
// would permanently bank the duplicate). Pick the message by which fired.
if (gate.sentinelBlocked && failedFiles.some(f => f.path === '<head>')) {
serr(
`\nSync blocked: repository history changed during sync (force-push / reset).\n` +
`${codeBreakdown}\n\n` +
@@ -3370,6 +3441,12 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
`a commit that doesn't match the indexed tree. Re-run sync to re-pin against ` +
`current HEAD.`,
);
} else if (gate.sentinelBlocked) {
serr(
`\nSync blocked: a rename left a stale duplicate that could not be removed:\n` +
`${codeBreakdown}\n\n` +
`The next 'gbrain sync' retries the reconcile from the same diff.`,
);
} else {
const fileFailCount = failedFiles.filter(f => isSkippablePath(f.path)).length;
serr(
+6 -1
View File
@@ -1951,8 +1951,13 @@ export interface BrainEngine {
* preserved via stable page_id). `opts.sourceId` scopes the UPDATE — without
* it, the bare `WHERE slug = old` matches every row across every source and
* would either rename them all OR violate the (source_id, slug) UNIQUE.
*
* Returns the number of rows moved. 0 means the old slug had no row in the
* scoped source — an UPDATE that matches nothing does NOT throw, so callers
* that need to know whether the rename actually happened (the sync rename
* path, #3056) must check the return value rather than rely on the catch.
*/
updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise<void>;
updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise<number>;
rewriteLinks(oldSlug: string, newSlug: string): Promise<void>;
/**
+5 -2
View File
@@ -5480,15 +5480,18 @@ export class PGLiteEngine implements BrainEngine {
}
// Sync
async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise<void> {
async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise<number> {
newSlug = validateSlug(newSlug);
const sourceId = opts?.sourceId ?? 'default';
// Source-qualify so a rename in source A doesn't sweep up same-slug rows
// in sources B/C/D (mirrors postgres-engine.ts).
await this.db.query(
const result = await this.db.query(
`UPDATE pages SET slug = $1, updated_at = now() WHERE slug = $2 AND source_id = $3`,
[newSlug, oldSlug, sourceId]
);
// #3056: rows moved — a zero-row UPDATE does not throw, so the count is
// the only way callers can see the no-op.
return result.affectedRows ?? 0;
}
async rewriteLinks(_oldSlug: string, _newSlug: string): Promise<void> {
+5 -2
View File
@@ -5574,14 +5574,17 @@ export class PostgresEngine implements BrainEngine {
}
// Sync
async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise<void> {
async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise<number> {
newSlug = validateSlug(newSlug);
const sql = this.sql;
const sourceId = opts?.sourceId ?? 'default';
// Source-qualify so a rename in source A doesn't sweep up same-slug rows
// in sources B/C/D (which would either rename them all OR fail the
// (source_id, slug) UNIQUE if the new slug already exists in another source).
await sql`UPDATE pages SET slug = ${newSlug}, updated_at = now() WHERE slug = ${oldSlug} AND source_id = ${sourceId}`;
const result = await sql`UPDATE pages SET slug = ${newSlug}, updated_at = now() WHERE slug = ${oldSlug} AND source_id = ${sourceId}`;
// #3056: rows moved — a zero-row UPDATE does not throw, so the count is
// the only way callers can see the no-op.
return result.count ?? 0;
}
async rewriteLinks(_oldSlug: string, _newSlug: string): Promise<void> {
+280
View File
@@ -0,0 +1,280 @@
/**
* #3056 — sync rename path: a failed `updateSlug` must not leave a live
* duplicate of the renamed page behind.
*
* Before the fix, the rename loop swallowed `updateSlug` failures with an
* empty catch ("treat as add") and could not see a zero-row UPDATE at all
* (updateSlug returned void). The run then fell through to importFile,
* which created/updated the row at the new path — while the old row stayed
* behind, live, with its slug occupied. Nothing was logged, no counter
* moved, and the duplicate was permanent.
*
* The fix reconciles: when the cheap rename didn't move a row AND the
* destination demonstrably materialized, the stale old row is located
* positively by `source_path = from` and deleted. Two safety rails:
*
* - dedup-skip protection: identity dedup can skip the import against
* the OLD row, in which case nothing landed at the destination and
* deleting the old row would destroy the only copy — no reconcile.
* - no slug-guess deletes: the stale row is found by source_path only;
* an unrelated row that happens to sit at the guessed slug survives.
*
* A failed reconcile delete lands in failedFiles so the existing failure
* gate blocks the bookmark and the next run retries the same rename diff.
*/
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from 'bun:test';
import { execSync } from 'node:child_process';
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { PGLiteEngine } from '../src/core/pglite-engine.ts';
import { resetPgliteState } from './helpers/reset-pglite.ts';
let engine: PGLiteEngine;
const repos: string[] = [];
// Serial-file requirement: blocked runs write real rows to the sync-failure
// ledger under the gbrain home — isolate it per test so the operator's
// actual ledger is never touched (GBRAIN_HOME is the isolation lever;
// process.env.HOME does not redirect Bun's os.homedir()).
let tmpHome: string;
const originalGbrainHome = process.env.GBRAIN_HOME;
beforeAll(async () => {
engine = new PGLiteEngine();
await engine.connect({});
await engine.initSchema();
});
afterAll(async () => {
await engine.disconnect();
});
beforeEach(async () => {
tmpHome = mkdtempSync(join(tmpdir(), 'gbrain-3056-home-'));
process.env.GBRAIN_HOME = tmpHome;
await resetPgliteState(engine);
});
afterEach(() => {
if (originalGbrainHome !== undefined) process.env.GBRAIN_HOME = originalGbrainHome;
else delete process.env.GBRAIN_HOME;
try { rmSync(tmpHome, { recursive: true, force: true }); } catch { /* ignore */ }
while (repos.length) {
const d = repos.pop();
if (d) rmSync(d, { recursive: true, force: true });
}
});
function personMd(title: string, body: string): string {
return ['---', 'type: person', `title: ${title}`, '---', '', body].join('\n');
}
/** Create a temp git repo seeded with the given files + an initial commit. */
function mkRepo(files: Record<string, string>): string {
const dir = mkdtempSync(join(tmpdir(), 'gbrain-3056-'));
repos.push(dir);
execSync('git init', { cwd: dir, stdio: 'pipe' });
execSync('git config user.email "test@test.com"', { cwd: dir, stdio: 'pipe' });
execSync('git config user.name "Test"', { cwd: dir, stdio: 'pipe' });
for (const [rel, content] of Object.entries(files)) {
mkdirSync(join(dir, rel, '..'), { recursive: true });
writeFileSync(join(dir, rel), content);
}
execSync('git add -A && git commit -m "initial"', { cwd: dir, stdio: 'pipe' });
return dir;
}
const SYNC_OPTS = { noPull: true, noEmbed: true, noExtract: true, sourceId: 'default' } as const;
async function countPages(): Promise<number> {
const rows = await engine.executeRaw<{ n: number | string }>(
`SELECT count(*)::int AS n FROM pages WHERE source_id = 'default'`,
);
return Number(rows[0]?.n ?? 0);
}
describe('updateSlug engine contract (#3056)', () => {
test('returns 1 when the old slug row is moved', async () => {
await engine.putPage('people/old', {
type: 'person', title: 'Old', compiled_truth: 'body',
}, { sourceId: 'default' });
const moved = await engine.updateSlug('people/old', 'people/new', { sourceId: 'default' });
expect(moved).toBe(1);
expect(await engine.getPage('people/new')).not.toBeNull();
});
test('returns 0 when the old slug has no row (the silent no-op case)', async () => {
const moved = await engine.updateSlug('people/ghost', 'people/new', { sourceId: 'default' });
expect(moved).toBe(0);
});
});
describe('#3056: rename fallback reconciles the stale old row', () => {
test('collision: destination slug occupied → stale old row deleted after import lands', async () => {
const { performSync } = await import('../src/commands/sync.ts');
const repo = mkRepo({ 'people/carol.md': personMd('Carol', 'Carol is a person.') });
await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
expect(await engine.getPage('people/carol')).not.toBeNull();
// A pre-existing row already occupies the rename destination, so
// updateSlug throws (source_id, slug) UNIQUE and the loop falls back.
await engine.putPage('people/dana', {
type: 'person', title: 'Dana (stale)', compiled_truth: 'occupies the destination slug',
}, { sourceId: 'default' });
execSync('git mv people/carol.md people/dana.md', { cwd: repo, stdio: 'pipe' });
execSync('git commit -m "rename carol to dana"', { cwd: repo, stdio: 'pipe' });
const result = await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
expect(result.status).toBe('synced');
// The destination carries the renamed file's content...
const dana = await engine.getPage('people/dana');
expect(dana).not.toBeNull();
expect(dana!.compiled_truth).toContain('Carol is a person.');
// ...and the stale old row is gone — no live duplicate.
expect(await engine.getPage('people/carol')).toBeNull();
expect(await countPages()).toBe(1);
});
test('dedup-skip against the old row must NOT reconcile: the only copy survives', async () => {
const { performSync } = await import('../src/commands/sync.ts');
// frontmatter.id gives identity dedup a handle: the import at the new
// path can skip as "identical to <old row>" — in which case NOTHING
// landed at the destination and deleting the old row would destroy the
// only copy of the content.
const md = ['---', 'type: person', 'title: Carol', 'id: ext-3056', '---', '', 'Carol is a person.'].join('\n');
const repo = mkRepo({ 'people/carol.md': md });
await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
expect(await engine.getPage('people/carol')).not.toBeNull();
// Destination occupied → updateSlug throws → fallback path.
await engine.putPage('people/dana', {
type: 'person', title: 'Dana (stale)', compiled_truth: 'occupies the destination slug',
}, { sourceId: 'default' });
execSync('git mv people/carol.md people/dana.md', { cwd: repo, stdio: 'pipe' });
execSync('git commit -m "rename carol to dana"', { cwd: repo, stdio: 'pipe' });
await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
// The import skipped against the OLD row (identity dedup), so the
// destination never materialized with the renamed content — the
// reconcile must not have deleted the old row, which still holds the
// only copy.
const carol = await engine.getPage('people/carol');
expect(carol).not.toBeNull();
expect(carol!.compiled_truth).toContain('Carol is a person.');
});
test('reconcile never deletes by slug guess: unrelated manual row survives', async () => {
const { performSync } = await import('../src/commands/sync.ts');
const repo = mkRepo({ 'people/carol.md': personMd('Carol', 'Carol is a person.') });
await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
// The file's real row drifts to a divergent slug with no source_path
// (unlocatable), and an UNRELATED manually-curated page happens to sit
// at the path-derived slug a naive reconcile would guess.
await engine.executeRaw(
`UPDATE pages SET slug = 'people/carol-divergent', source_path = NULL
WHERE source_id = 'default' AND slug = 'people/carol'`,
);
await engine.putPage('people/carol', {
type: 'person', title: 'Manual Carol', compiled_truth: 'hand-authored, not from the file',
}, { sourceId: 'default' });
// Destination occupied → updateSlug throws UNIQUE → fallback path.
await engine.putPage('people/dana', {
type: 'person', title: 'Dana (stale)', compiled_truth: 'occupies the destination slug',
}, { sourceId: 'default' });
execSync('git mv people/carol.md people/dana.md', { cwd: repo, stdio: 'pipe' });
execSync('git commit -m "rename carol to dana"', { cwd: repo, stdio: 'pipe' });
const result = await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
expect(result.status).toBe('synced');
// The destination materialized with the file's content...
const dana = await engine.getPage('people/dana');
expect(dana).not.toBeNull();
expect(dana!.compiled_truth).toContain('Carol is a person.');
// ...but no row had source_path = from, so the reconcile deleted
// NOTHING: the unrelated manual row at the guessed slug survives.
const manual = await engine.getPage('people/carol');
expect(manual).not.toBeNull();
expect(manual!.compiled_truth).toContain('hand-authored');
});
test('happy path: clean git mv rename keeps page_id and touches nothing else', async () => {
const { performSync } = await import('../src/commands/sync.ts');
const repo = mkRepo({ 'people/carol.md': personMd('Carol', 'Carol is a person.') });
await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
const before = await engine.getPage('people/carol');
expect(before).not.toBeNull();
execSync('git mv people/carol.md people/dana.md', { cwd: repo, stdio: 'pipe' });
execSync('git commit -m "rename carol to dana"', { cwd: repo, stdio: 'pipe' });
const result = await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
expect(result.status).toBe('synced');
const after = await engine.getPage('people/dana');
expect(after).not.toBeNull();
expect(after!.id).toBe(before!.id); // cheap-path rename preserved the row
expect(await engine.getPage('people/carol')).toBeNull();
expect(await countPages()).toBe(1);
});
test('reconcile failure blocks the bookmark and the next run retries to convergence', async () => {
const { performSync } = await import('../src/commands/sync.ts');
const repo = mkRepo({ 'people/carol.md': personMd('Carol', 'Carol is a person.') });
await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
await engine.putPage('people/dana', {
type: 'person', title: 'Dana (stale)', compiled_truth: 'occupies the destination slug',
}, { sourceId: 'default' });
execSync('git mv people/carol.md people/dana.md', { cwd: repo, stdio: 'pipe' });
execSync('git commit -m "rename carol to dana"', { cwd: repo, stdio: 'pipe' });
// Inject a transient failure into the reconcile delete.
const origDelete = engine.deletePage.bind(engine);
engine.deletePage = async () => { throw new Error('injected transient delete failure'); };
let blocked;
try {
blocked = await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
} finally {
engine.deletePage = origDelete;
}
// The failed reconcile is not checkpointed past: the run blocks and the
// stale duplicate is still visible. The failure is recorded as a
// `<rename:…>` SENTINEL, which the auto-skip valve can never
// chronic-skip — an outage lasting longer than the threshold must not
// quietly bank the duplicate.
expect(blocked.status).toBe('blocked_by_failures');
expect(blocked.failedFiles).toBe(1);
expect(await engine.getPage('people/carol')).not.toBeNull();
const { loadSyncFailures } = await import('../src/core/sync-failure-ledger.ts');
const openSentinels = loadSyncFailures().filter(
f => f.path === '<rename:people/dana.md>' && f.state === 'open',
);
expect(openSentinels).toHaveLength(1);
// Next run (failure gone) retries the same rename diff and converges.
const result = await performSync(engine, { repoPath: repo, ...SYNC_OPTS });
expect(result.status).toBe('synced');
expect(await engine.getPage('people/carol')).toBeNull();
const dana = await engine.getPage('people/dana');
expect(dana).not.toBeNull();
expect(dana!.compiled_truth).toContain('Carol is a person.');
expect(await countPages()).toBe(1);
// The convergence also clears the sentinel row — doctor must not keep
// warning about a rename that has since reconciled.
const remaining = loadSyncFailures().filter(
f => f.path === '<rename:people/dana.md>' && f.state === 'open',
);
expect(remaining).toHaveLength(0);
});
});