From f15163f727657f9162f1184b18340e9c283414cb Mon Sep 17 00:00:00 2001 From: 1alessio Date: Fri, 17 Jul 2026 01:22:21 +0200 Subject: [PATCH] fix(sync): normalize path separators + add mass-delete safety valve to full-sync reconcile (#2828) (#2836) On a Windows checkout, path.relative yields backslash-separated paths while a page's stored source_path can hold forward slashes (e.g. git-derived). The full-sync reconcile compared the two without normalizing separators, so every file-backed page looked stale and the reconcile deleted the entire source. - Normalize separators on BOTH sides of the membership test (shared .replace(/\/g, '/')), so pages written with either separator match on any OS. - Add a mass-delete safety valve: when a reconcile would sweep > 50% of the file-backed pages a strategy manages on a source with > 20 of them, skip the delete and surface a loud warning instead of silently wiping the brain. GBRAIN_ALLOW_MASS_RECONCILE=1 restores the old behavior. - Factor the decision into pure, exported helpers (planReconcileDeletes, massReconcileAllowed) and unit-test the separator matching, the valve threshold, and the env override without a live engine. Co-authored-by: Claude Fable 5 --- src/commands/sync.ts | 121 ++++++++++++++++++++--- test/sync-reconcile-mass-delete.test.ts | 124 ++++++++++++++++++++++++ 2 files changed, 233 insertions(+), 12 deletions(-) create mode 100644 test/sync-reconcile-mass-delete.test.ts diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 2f8d5e42a..ff245ef0a 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -3109,23 +3109,44 @@ async function performFullSync( // repo-relative (importFile uses `relative(dir, filePath)`), so relativize // to the same form before membership-testing — otherwise every page looks // stale and the reconcile would wrongly delete live pages. - const current = new Set( - collectSyncableFiles(repoPath, { strategy: opts.strategy ?? 'markdown' }) - .map(abs => relative(repoPath, abs)), - ); + // + // #2828: planReconcileDeletes ALSO normalizes path separators on both sides + // of the membership test. On a Windows checkout `path.relative` yields + // backslash paths while a stored source_path can hold git-derived forward + // slashes; without normalization every file-backed page mismatches, looks + // stale, and the reconcile wipes the whole source. + const currentFiles = collectSyncableFiles(repoPath, { strategy: opts.strategy ?? 'markdown' }) + .map(abs => relative(repoPath, abs)); const rows = await engine.executeRaw<{ slug: string; source_path: string | null }>( `SELECT slug, source_path FROM pages WHERE source_id = $1 AND source_path IS NOT NULL AND deleted_at IS NULL`, [sid], ); - const staleSlugs = rows - .filter(r => r.source_path != null - && isSyncable(r.source_path, reconcileSyncOpts) - && !current.has(r.source_path)) - .map(r => r.slug); - if (staleSlugs.length > 0) { + const plan = planReconcileDeletes( + rows, + currentFiles, + p => isSyncable(p, reconcileSyncOpts), + ); + if (plan.staleSlugs.length > 0 && plan.massDelete && !massReconcileAllowed()) { + // #2828 mass-delete safety valve: a reconcile that would sweep more than + // half of the pages this strategy manages, on a source with a non-trivial + // number of them, is almost always a path-comparison bug or the wrong repo + // path — NOT a genuine bulk deletion. Skip the delete and warn loudly + // instead of silently wiping the brain. + serr( + `\n WARNING: refusing to reconcile-delete ${plan.staleSlugs.length} of ` + + `${plan.reconcilableCount} file-backed page(s) for source '${sid}' ` + + `(> ${Math.round(MASS_RECONCILE_RATIO * 100)}% of them).\n` + + ` A full sync removes pages only when their backing file is gone. Deleting\n` + + ` this many at once almost always means the paths were compared wrong (e.g.\n` + + ` a path-separator mismatch) or the WRONG repo path was synced — not that\n` + + ` you actually deleted that many files. No pages were deleted.\n` + + ` If this bulk removal is genuinely intended, re-run with ` + + `GBRAIN_ALLOW_MASS_RECONCILE=1 to restore the old behavior.`, + ); + } else if (plan.staleSlugs.length > 0) { const deleteScopedOpts = { sourceId: sid }; - for (let i = 0; i < staleSlugs.length; i += DELETE_BATCH_SIZE) { - const batch = staleSlugs.slice(i, i + DELETE_BATCH_SIZE); + for (let i = 0; i < plan.staleSlugs.length; i += DELETE_BATCH_SIZE) { + const batch = plan.staleSlugs.slice(i, i + DELETE_BATCH_SIZE); try { const deleted = await engine.deletePages(batch, deleteScopedOpts); reconciledDeletes += deleted.length; @@ -3179,6 +3200,82 @@ async function performFullSync( }; } +/** + * #2828 full-sync reconcile safety-valve thresholds. A reconcile that would + * delete more than MASS_RECONCILE_RATIO of the file-backed pages a strategy + * manages, on a source that holds more than MASS_RECONCILE_MIN_PAGES of them, is + * treated as a suspected path-comparison bug rather than a real bulk deletion. + */ +export const MASS_RECONCILE_RATIO = 0.5; +export const MASS_RECONCILE_MIN_PAGES = 20; + +/** + * Normalize path separators so a page whose stored `source_path` was written + * with a different separator than the local OS's `path.relative` produces (e.g. + * git-derived forward-slash paths on a Windows checkout) still compares equal. + * Without this, on Windows every file-backed page looks stale and the reconcile + * wrongly deletes the whole source (#2828). + */ +function normalizeReconcilePath(p: string): string { + return p.replace(/\\/g, '/'); +} + +export interface ReconcilePlan { + /** Slugs whose backing file is genuinely gone; safe to reconcile-delete. */ + staleSlugs: string[]; + /** + * File-backed, in-strategy pages the reconcile can act on. This is the + * denominator for the mass-delete valve (the exact population at risk). + */ + reconcilableCount: number; + /** + * True when `staleSlugs` would sweep more than MASS_RECONCILE_RATIO of + * `reconcilableCount`, on a source with more than MASS_RECONCILE_MIN_PAGES of + * them — the mass-delete signal that trips the safety valve. + */ + massDelete: boolean; +} + +/** + * #2828: decide which file-backed pages a full-sync reconcile should delete, and + * whether that deletion is suspiciously large. Pure and exported so both the + * separator normalization and the mass-delete valve are unit-testable without a + * live engine or a Windows host. + * + * @param rows pages with a non-null `source_path` (deleted_at IS NULL). + * @param currentFiles repo-relative paths present in the working tree. + * @param isSyncablePath predicate excluding metafiles and the wrong strategy. + */ +export function planReconcileDeletes( + rows: ReadonlyArray<{ slug: string; source_path: string | null }>, + currentFiles: Iterable, + isSyncablePath: (p: string) => boolean, +): ReconcilePlan { + const current = new Set(); + for (const f of currentFiles) current.add(normalizeReconcilePath(f)); + const reconcilable = rows.filter( + r => r.source_path != null && isSyncablePath(r.source_path), + ); + const staleSlugs = reconcilable + .filter(r => !current.has(normalizeReconcilePath(r.source_path as string))) + .map(r => r.slug); + const massDelete = + reconcilable.length > MASS_RECONCILE_MIN_PAGES && + staleSlugs.length > reconcilable.length * MASS_RECONCILE_RATIO; + return { staleSlugs, reconcilableCount: reconcilable.length, massDelete }; +} + +/** + * #2828 escape hatch: `GBRAIN_ALLOW_MASS_RECONCILE=1` restores the pre-valve + * behavior for the rare intentional bulk removal. Env-only (an incident-time + * override), mirroring `resolveStallAbortSeconds`' pure, env-parameterized shape. + */ +export function massReconcileAllowed( + env: Record = process.env, +): boolean { + return env.GBRAIN_ALLOW_MASS_RECONCILE === '1'; +} + /** * Grace window (seconds) between the watchdog's SIGTERM and SIGKILL. SIGTERM * gives a responsive loop a clean shutdown; SIGKILL is the starvation backstop. diff --git a/test/sync-reconcile-mass-delete.test.ts b/test/sync-reconcile-mass-delete.test.ts new file mode 100644 index 000000000..a0fff89bc --- /dev/null +++ b/test/sync-reconcile-mass-delete.test.ts @@ -0,0 +1,124 @@ +/** + * Test: full-sync reconcile separator normalization + mass-delete safety valve + * (#2828 — Windows reconcile mass-delete). + * + * Pure-helper surface: `planReconcileDeletes` and `massReconcileAllowed` take + * plain inputs and read no engine / no ambient env (the env reader is + * parameterized), so these run without PGLite and without touching the shared + * `process.env` — parallel-loop safe by construction. + */ + +import { describe, test, expect } from 'bun:test'; +import { + planReconcileDeletes, + massReconcileAllowed, + MASS_RECONCILE_RATIO, + MASS_RECONCILE_MIN_PAGES, +} from '../src/commands/sync.ts'; + +/** Build stored page rows from a list of source_paths (slug = `slug-`). */ +function rows(paths: Array): Array<{ slug: string; source_path: string | null }> { + return paths.map((p, i) => ({ slug: `slug-${i}`, source_path: p })); +} + +/** + * Reconcile scenario: `total` file-backed pages, of which the first `stale` + * are absent from the working tree (deleted) and the rest are present. + */ +function scenario(total: number, stale: number) { + const stored = rows(Array.from({ length: total }, (_, i) => `p/${i}.md`)); + const present = stored.slice(stale).map((r) => r.source_path as string); + return planReconcileDeletes(stored, present, () => true); +} + +describe('planReconcileDeletes — separator normalization (#2828)', () => { + test('backslash working-tree paths match forward-slash stored source_path', () => { + // Windows `path.relative` yields backslashes; source_path was stored with + // forward slashes (e.g. git-derived). Both must compare equal. + const stored = rows(['topics/foo.md', 'topics/bar.md', 'notes/baz.md']); + const workingTree = ['topics\\foo.md', 'topics\\bar.md', 'notes\\baz.md']; + const plan = planReconcileDeletes(stored, workingTree, () => true); + expect(plan.staleSlugs).toEqual([]); + expect(plan.reconcilableCount).toBe(3); + expect(plan.massDelete).toBe(false); + }); + + test('forward-slash working-tree paths match backslash stored source_path', () => { + const stored = rows(['topics\\foo.md', 'topics\\bar.md']); + const workingTree = ['topics/foo.md', 'topics/bar.md']; + const plan = planReconcileDeletes(stored, workingTree, () => true); + expect(plan.staleSlugs).toEqual([]); + }); + + test('a genuinely removed file is the only stale slug, regardless of separator', () => { + const stored = rows(['topics/foo.md', 'topics/bar.md', 'topics/gone.md']); + const workingTree = ['topics\\foo.md', 'topics\\bar.md']; // gone.md deleted + const plan = planReconcileDeletes(stored, workingTree, () => true); + expect(plan.staleSlugs).toEqual(['slug-2']); + }); + + test('null source_path rows are never reconcilable (manual / put_page pages)', () => { + const stored = rows([null, 'x.md']); + const plan = planReconcileDeletes(stored, [], () => true); + expect(plan.reconcilableCount).toBe(1); + expect(plan.staleSlugs).toEqual(['slug-1']); + }); + + test('the strategy predicate excludes wrong-strategy pages from both stale set and denominator', () => { + const stored = rows(['a.md', 'b.md', 'code.ts', 'gone.md']); + const onlyMarkdown = (p: string) => p.endsWith('.md'); + const plan = planReconcileDeletes(stored, [], onlyMarkdown); // nothing present + expect(plan.reconcilableCount).toBe(3); // code.ts excluded + expect(plan.staleSlugs).toEqual(['slug-0', 'slug-1', 'slug-3']); + }); +}); + +describe('planReconcileDeletes — mass-delete safety valve (#2828)', () => { + test('trips when > 50% of a > 20-page source would be deleted', () => { + const plan = scenario(21, 11); // 11/21 ≈ 52% > 50%, and 21 > 20 + expect(plan.reconcilableCount).toBe(21); + expect(plan.staleSlugs.length).toBe(11); + expect(plan.massDelete).toBe(true); + }); + + test('holds at exactly 50% (threshold is strictly greater)', () => { + const plan = scenario(40, 20); // 20/40 == 50%, not > 50% + expect(plan.massDelete).toBe(false); + }); + + test('ignores small sources (<= 20 pages) even at 100% stale', () => { + expect(scenario(MASS_RECONCILE_MIN_PAGES, MASS_RECONCILE_MIN_PAGES).massDelete).toBe(false); + expect(scenario(15, 15).massDelete).toBe(false); + }); + + test('trips just past the min-pages boundary with a majority stale', () => { + const plan = scenario(21, 20); + expect(plan.massDelete).toBe(true); + }); + + test('thresholds are the documented constants', () => { + expect(MASS_RECONCILE_RATIO).toBe(0.5); + expect(MASS_RECONCILE_MIN_PAGES).toBe(20); + }); +}); + +describe('massReconcileAllowed — GBRAIN_ALLOW_MASS_RECONCILE escape hatch (#2828)', () => { + test('=1 restores the old behavior', () => { + expect(massReconcileAllowed({ GBRAIN_ALLOW_MASS_RECONCILE: '1' })).toBe(true); + }); + + test('unset or any other value keeps the valve active', () => { + expect(massReconcileAllowed({})).toBe(false); + expect(massReconcileAllowed({ GBRAIN_ALLOW_MASS_RECONCILE: '0' })).toBe(false); + expect(massReconcileAllowed({ GBRAIN_ALLOW_MASS_RECONCILE: 'true' })).toBe(false); + }); + + test('effective gate: the valve blocks the delete unless the override is set', () => { + const plan = scenario(21, 11); + // This mirrors the guard in performFullSync. + const blocked = plan.massDelete && !massReconcileAllowed({}); + const overridden = plan.massDelete && !massReconcileAllowed({ GBRAIN_ALLOW_MASS_RECONCILE: '1' }); + expect(blocked).toBe(true); // skip delete + loud warning + expect(overridden).toBe(false); // old behavior restored + }); +});