mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
bb3376e3b0
commit
f15163f727
+109
-12
@@ -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<string>,
|
||||
isSyncablePath: (p: string) => boolean,
|
||||
): ReconcilePlan {
|
||||
const current = new Set<string>();
|
||||
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<string, string | undefined> = 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.
|
||||
|
||||
@@ -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-<index>`). */
|
||||
function rows(paths: Array<string | null>): 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
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user