diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 86f230cbf..8263328d9 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -2874,10 +2874,17 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise 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> | 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` 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: ``, + 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 `` sentinel a previous failing run recorded. + if (!reconcileFailed) succeededPaths.push(``); 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` (pin ancestry broken) and + // `` (#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 === '')) { 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 isSkippablePath(f.path)).length; serr( diff --git a/src/core/engine.ts b/src/core/engine.ts index 871e031b0..7c0e9d51d 100644 --- a/src/core/engine.ts +++ b/src/core/engine.ts @@ -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; + updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise; rewriteLinks(oldSlug: string, newSlug: string): Promise; /** diff --git a/src/core/pglite-engine.ts b/src/core/pglite-engine.ts index 94bed2de2..82a5471a9 100644 --- a/src/core/pglite-engine.ts +++ b/src/core/pglite-engine.ts @@ -5480,15 +5480,18 @@ export class PGLiteEngine implements BrainEngine { } // Sync - async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise { + async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise { 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 { diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index f116fe405..9ced64cce 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -5574,14 +5574,17 @@ export class PostgresEngine implements BrainEngine { } // Sync - async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise { + async updateSlug(oldSlug: string, newSlug: string, opts?: { sourceId?: string }): Promise { 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 { diff --git a/test/sync-rename-reconcile.serial.test.ts b/test/sync-rename-reconcile.serial.test.ts new file mode 100644 index 000000000..ff6e246f0 --- /dev/null +++ b/test/sync-rename-reconcile.serial.test.ts @@ -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 { + 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 { + 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 " — 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 + // `` 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 === '' && 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 === '' && f.state === 'open', + ); + expect(remaining).toHaveLength(0); + }); +});