feat(git-sync): remove the per-cycle delete cap; deletes apply + are logged every cycle
The delete cap (GIT_SYNC_MAX_DELETES_PER_CYCLE, default 5) was a defense-in-depth guard that SUPPRESSED a cycle's deletions when the planned count exceeded the limit. In practice it was a crutch over engine correctness that also blocked legitimate deletes: deleting a folder with many child pages is a normal action, and git-sync deletes are SOFT (Trash, reversible), so a blocking limit has little upside and real downside. There is also no user-facing surface to "confirm" a large delete from a background sync — the only channel is the operator log. So: drop the cap entirely. Deletes apply unconditionally; every cycle already logs its full push plan, per-action `delete: <pageId>` lines, and completion counts through the engine `log`, so what was deleted (and what was skipped) is always recorded. Engine correctness (the reconcile/layout/round-trip tests) is what prevents phantom deletions — not a blocking cap. Removed: orchestrator `resolveApplyClient` cap hook + `maxDeletes`, `getGitSyncMaxDeletesPerCycle`, the `GIT_SYNC_MAX_DELETES_PER_CYCLE` env/validation/.env.example, and the cap tests. (The engine's generic optional `resolveApplyClient` hook is left as an unused extension point.) server tsc clean, git-sync + environment jest 174. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -227,7 +227,3 @@ MCP_DOCMOST_PASSWORD=
|
|||||||
# after this deadline so it cannot hold the per-space lock forever.
|
# after this deadline so it cannot hold the per-space lock forever.
|
||||||
# GIT_SYNC_BACKEND_TIMEOUT_MS=120000
|
# GIT_SYNC_BACKEND_TIMEOUT_MS=120000
|
||||||
#
|
#
|
||||||
# Defense-in-depth absolute cap on soft-deletes applied per push cycle
|
|
||||||
# (default: 5). A non-convergent / phantom-absence cycle can never trash more
|
|
||||||
# than this many pages without an explicit override.
|
|
||||||
# GIT_SYNC_MAX_DELETES_PER_CYCLE=5
|
|
||||||
|
|||||||
@@ -15,27 +15,6 @@ describe('EnvironmentService', () => {
|
|||||||
expect(service).toBeDefined();
|
expect(service).toBeDefined();
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('getGitSyncMaxDeletesPerCycle', () => {
|
|
||||||
const withEnv = (value?: string) =>
|
|
||||||
new EnvironmentService({
|
|
||||||
get: (_key: string, fallback?: string) => value ?? fallback,
|
|
||||||
} as any);
|
|
||||||
|
|
||||||
it('defaults to 5 when unset', () => {
|
|
||||||
expect(withEnv().getGitSyncMaxDeletesPerCycle()).toBe(5);
|
|
||||||
});
|
|
||||||
|
|
||||||
it('parses a valid positive int', () => {
|
|
||||||
expect(withEnv('12').getGitSyncMaxDeletesPerCycle()).toBe(12);
|
|
||||||
});
|
|
||||||
|
|
||||||
it('falls back to 5 for non-positive or unparseable values', () => {
|
|
||||||
expect(withEnv('0').getGitSyncMaxDeletesPerCycle()).toBe(5);
|
|
||||||
expect(withEnv('-3').getGitSyncMaxDeletesPerCycle()).toBe(5);
|
|
||||||
expect(withEnv('not-a-number').getGitSyncMaxDeletesPerCycle()).toBe(5);
|
|
||||||
});
|
|
||||||
});
|
|
||||||
|
|
||||||
describe('getGitSyncPollIntervalMs', () => {
|
describe('getGitSyncPollIntervalMs', () => {
|
||||||
const withEnv = (value?: string) =>
|
const withEnv = (value?: string) =>
|
||||||
new EnvironmentService({
|
new EnvironmentService({
|
||||||
|
|||||||
@@ -399,20 +399,6 @@ export class EnvironmentService {
|
|||||||
return Number.isFinite(parsed) && parsed > 0 ? parsed : 2000;
|
return Number.isFinite(parsed) && parsed > 0 ? parsed : 2000;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Defense-in-depth absolute cap on how many pages a single push cycle may
|
|
||||||
* soft-delete (default 5). A non-convergent / phantom-absence cycle whose push
|
|
||||||
* plan would delete more than this is forced to skip deletions that cycle (the
|
|
||||||
* orchestrator logs a WARNING). A non-positive or unparseable value falls back
|
|
||||||
* to the default 5 so the cap can never be silently disabled.
|
|
||||||
*/
|
|
||||||
getGitSyncMaxDeletesPerCycle(): number {
|
|
||||||
const parsed = parseInt(
|
|
||||||
this.configService.get<string>('GIT_SYNC_MAX_DELETES_PER_CYCLE', '5'),
|
|
||||||
10,
|
|
||||||
);
|
|
||||||
return Number.isFinite(parsed) && parsed > 0 ? parsed : 5;
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The service user id git-sync writes are attributed to. Required when sync is
|
* The service user id git-sync writes are attributed to. Required when sync is
|
||||||
|
|||||||
@@ -209,12 +209,6 @@ export class EnvironmentVariables {
|
|||||||
@IsString()
|
@IsString()
|
||||||
GIT_SYNC_BACKEND_TIMEOUT_MS: string;
|
GIT_SYNC_BACKEND_TIMEOUT_MS: string;
|
||||||
|
|
||||||
// Defense-in-depth absolute cap on soft-deletes per push cycle (default 5): a
|
|
||||||
// non-convergent / phantom-absence cycle can never trash more than this many
|
|
||||||
// pages without an explicit override. Optional int (validated as a string env).
|
|
||||||
@IsOptional()
|
|
||||||
@IsString()
|
|
||||||
GIT_SYNC_MAX_DELETES_PER_CYCLE: string;
|
|
||||||
|
|
||||||
// Required when git-sync is enabled: the service user create/move/rename/delete
|
// Required when git-sync is enabled: the service user create/move/rename/delete
|
||||||
// are attributed to (issue #194 §7.2). Optional otherwise.
|
// are attributed to (issue #194 §7.2). Optional otherwise.
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
// Unit tests for the git-sync control plane. The engine's `runCycle`
|
// Unit tests for the git-sync control plane. The engine's `runCycle`
|
||||||
// (which owns the PULL->PUSH branch choreography) is mocked so we exercise ONLY
|
// (which owns the PULL->PUSH branch choreography) is mocked so we exercise ONLY
|
||||||
// the orchestrator's wiring: gating, the Redis leader lock + in-process mutex
|
// the orchestrator's wiring: gating, the Redis leader lock + in-process mutex
|
||||||
// (via SpaceLockService), the delete-cap POLICY it injects as `resolveApplyClient`,
|
// (via SpaceLockService),
|
||||||
// the remote-template substitution in the settings it hands the engine, the
|
// the remote-template substitution in the settings it hands the engine, the
|
||||||
// external-push ingest, and the idempotent interval lifecycle. The cycle
|
// external-push ingest, and the idempotent interval lifecycle. The cycle
|
||||||
// mechanics themselves are covered by the engine's own cycle round-trip spec.
|
// mechanics themselves are covered by the engine's own cycle round-trip spec.
|
||||||
@@ -48,7 +48,6 @@ interface BuildOptions {
|
|||||||
/** Env tunables (only the load-bearing ones are surfaced as overrides). */
|
/** Env tunables (only the load-bearing ones are surfaced as overrides). */
|
||||||
enabled?: boolean;
|
enabled?: boolean;
|
||||||
serviceUserId?: string | undefined;
|
serviceUserId?: string | undefined;
|
||||||
maxDeletes?: number;
|
|
||||||
remoteTemplate?: string | undefined;
|
remoteTemplate?: string | undefined;
|
||||||
dataDir?: string;
|
dataDir?: string;
|
||||||
pollIntervalMs?: number;
|
pollIntervalMs?: number;
|
||||||
@@ -73,7 +72,6 @@ interface Built {
|
|||||||
function build(opts: BuildOptions = {}): Built {
|
function build(opts: BuildOptions = {}): Built {
|
||||||
const {
|
const {
|
||||||
enabled = true,
|
enabled = true,
|
||||||
maxDeletes = 100,
|
|
||||||
remoteTemplate = undefined,
|
remoteTemplate = undefined,
|
||||||
dataDir = '/vaults',
|
dataDir = '/vaults',
|
||||||
pollIntervalMs = 15000,
|
pollIntervalMs = 15000,
|
||||||
@@ -87,7 +85,6 @@ function build(opts: BuildOptions = {}): Built {
|
|||||||
const env: Record<string, AnyMock> = {
|
const env: Record<string, AnyMock> = {
|
||||||
isGitSyncEnabled: jest.fn(() => enabled),
|
isGitSyncEnabled: jest.fn(() => enabled),
|
||||||
getGitSyncServiceUserId: jest.fn(() => serviceUserId),
|
getGitSyncServiceUserId: jest.fn(() => serviceUserId),
|
||||||
getGitSyncMaxDeletesPerCycle: jest.fn(() => maxDeletes),
|
|
||||||
getGitSyncRemoteTemplate: jest.fn(() => remoteTemplate),
|
getGitSyncRemoteTemplate: jest.fn(() => remoteTemplate),
|
||||||
getGitSyncDataDir: jest.fn(() => dataDir),
|
getGitSyncDataDir: jest.fn(() => dataDir),
|
||||||
getGitSyncPollIntervalMs: jest.fn(() => pollIntervalMs),
|
getGitSyncPollIntervalMs: jest.fn(() => pollIntervalMs),
|
||||||
@@ -177,15 +174,6 @@ function primeEngineHappyPath(): void {
|
|||||||
runCycleMock.mockResolvedValue(OK_CYCLE);
|
runCycleMock.mockResolvedValue(OK_CYCLE);
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Pull the `resolveApplyClient` hook out of the (single) runCycle call args. */
|
|
||||||
function lastResolveApplyClient(): (
|
|
||||||
plannedDeletes: number,
|
|
||||||
client: unknown,
|
|
||||||
) => unknown {
|
|
||||||
const calls = runCycleMock.mock.calls;
|
|
||||||
return calls[calls.length - 1][0].resolveApplyClient;
|
|
||||||
}
|
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
jest.clearAllMocks();
|
jest.clearAllMocks();
|
||||||
primeEngineHappyPath();
|
primeEngineHappyPath();
|
||||||
@@ -295,7 +283,6 @@ describe('GitSyncOrchestrator', () => {
|
|||||||
expect(deps.vault).toBe(built.vault);
|
expect(deps.vault).toBe(built.vault);
|
||||||
expect(deps.client).toBe(built.client);
|
expect(deps.client).toBe(built.client);
|
||||||
expect(deps.settings.vaultPath).toBe('/vaults/space-1');
|
expect(deps.settings.vaultPath).toBe('/vaults/space-1');
|
||||||
expect(typeof deps.resolveApplyClient).toBe('function');
|
|
||||||
// The bound datasource identity is the (workspace, service-user) pair.
|
// The bound datasource identity is the (workspace, service-user) pair.
|
||||||
expect(built.dataSource.bind).toHaveBeenCalledWith({
|
expect(built.dataSource.bind).toHaveBeenCalledWith({
|
||||||
workspaceId: 'ws-1',
|
workspaceId: 'ws-1',
|
||||||
@@ -390,57 +377,6 @@ describe('GitSyncOrchestrator', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('delete cap (anti-data-loss)', () => {
|
|
||||||
// The cap is now a POLICY the orchestrator injects as runCycle's
|
|
||||||
// `resolveApplyClient` hook; the engine calls it with the dry-run's planned
|
|
||||||
// delete count. We pull the hook out of the runCycle args and exercise it.
|
|
||||||
it('suppresses deletePage (by throwing) when planned deletes exceed the cap', async () => {
|
|
||||||
jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined);
|
|
||||||
const built = build({ maxDeletes: 5 });
|
|
||||||
await built.orchestrator.runOnce('space-1', 'ws-1');
|
|
||||||
|
|
||||||
const resolveApplyClient = lastResolveApplyClient();
|
|
||||||
const applyClient = resolveApplyClient(9, built.client) as any;
|
|
||||||
// deletePage is still a function (the engine calls it)...
|
|
||||||
expect(typeof applyClient.deletePage).toBe('function');
|
|
||||||
// ...but it THROWS, so the engine records a per-page failure and holds
|
|
||||||
// `last-pushed` (a resolving no-op would advance past the dropped deletes
|
|
||||||
// and never replay them — PR #119 review). The real deletePage is NOT hit.
|
|
||||||
await expect(applyClient.deletePage('p1')).rejects.toThrow(/suppress/i);
|
|
||||||
expect(built.client.deletePage).not.toHaveBeenCalled();
|
|
||||||
// Creates/updates pass through to the real client.
|
|
||||||
expect(applyClient.createPage).toBe(built.client.createPage);
|
|
||||||
});
|
|
||||||
|
|
||||||
it('fails safe: an Infinity planned-delete count (dry-run failed) is suppressed', async () => {
|
|
||||||
jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined);
|
|
||||||
const built = build({ maxDeletes: 5 });
|
|
||||||
await built.orchestrator.runOnce('space-1', 'ws-1');
|
|
||||||
|
|
||||||
// runCycle passes Number.POSITIVE_INFINITY when its dry-run planning threw;
|
|
||||||
// Infinity > cap → suppress (same throwing path).
|
|
||||||
const resolveApplyClient = lastResolveApplyClient();
|
|
||||||
const applyClient = resolveApplyClient(
|
|
||||||
Number.POSITIVE_INFINITY,
|
|
||||||
built.client,
|
|
||||||
) as any;
|
|
||||||
await expect(applyClient.deletePage('p1')).rejects.toThrow(/suppress/i);
|
|
||||||
expect(built.client.deletePage).not.toHaveBeenCalled();
|
|
||||||
});
|
|
||||||
|
|
||||||
it('passes through the original client when planned deletes are within the cap', async () => {
|
|
||||||
const built = build({ maxDeletes: 5 });
|
|
||||||
await built.orchestrator.runOnce('space-1', 'ws-1');
|
|
||||||
|
|
||||||
const resolveApplyClient = lastResolveApplyClient();
|
|
||||||
const applyClient = resolveApplyClient(3, built.client) as any;
|
|
||||||
// The ORIGINAL client is used (deletePage forwards to the real one).
|
|
||||||
expect(applyClient).toBe(built.client);
|
|
||||||
await applyClient.deletePage('p1');
|
|
||||||
expect(built.client.deletePage).toHaveBeenCalledWith('p1');
|
|
||||||
});
|
|
||||||
});
|
|
||||||
|
|
||||||
describe('remote template substitution', () => {
|
describe('remote template substitution', () => {
|
||||||
it('substitutes {spaceId} into the gitRemote settings handed to the engine', async () => {
|
it('substitutes {spaceId} into the gitRemote settings handed to the engine', async () => {
|
||||||
const built = build({ remoteTemplate: 'git@h:vault-{spaceId}.git' });
|
const built = build({ remoteTemplate: 'git@h:vault-{spaceId}.git' });
|
||||||
|
|||||||
@@ -253,8 +253,8 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy {
|
|||||||
* Drive ONE reconcile cycle for a space. The PULL->PUSH branch choreography
|
* Drive ONE reconcile cycle for a space. The PULL->PUSH branch choreography
|
||||||
* lives in the engine's `runCycle` (so it can never drift from the engine it
|
* lives in the engine's `runCycle` (so it can never drift from the engine it
|
||||||
* ships with); the orchestrator owns only the lock (its caller) and the
|
* ships with); the orchestrator owns only the lock (its caller) and the
|
||||||
* gitmost-specific delete-cap POLICY, injected here as the `resolveApplyClient`
|
* service binding. There is no delete cap — deletes apply unconditionally (they
|
||||||
* hook.
|
* are soft/reversible) and every cycle logs what it deleted via `log`.
|
||||||
*/
|
*/
|
||||||
private async driveCycle(
|
private async driveCycle(
|
||||||
spaceId: string,
|
spaceId: string,
|
||||||
@@ -266,7 +266,6 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy {
|
|||||||
const settings = await this.buildSettings(spaceId);
|
const settings = await this.buildSettings(spaceId);
|
||||||
const vault = await this.vaultRegistry.getVault(spaceId);
|
const vault = await this.vaultRegistry.getVault(spaceId);
|
||||||
const client = this.dataSource.bind({ workspaceId, userId: serviceUserId });
|
const client = this.dataSource.bind({ workspaceId, userId: serviceUserId });
|
||||||
const maxDeletes = this.environmentService.getGitSyncMaxDeletesPerCycle();
|
|
||||||
|
|
||||||
const result = await runCycle({
|
const result = await runCycle({
|
||||||
// Cooperative-abort signal from the per-space lock: if a heartbeat refresh
|
// Cooperative-abort signal from the per-space lock: if a heartbeat refresh
|
||||||
@@ -284,35 +283,13 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy {
|
|||||||
mkdir: (absDir) => mkdir(absDir, { recursive: true }).then(() => undefined),
|
mkdir: (absDir) => mkdir(absDir, { recursive: true }).then(() => undefined),
|
||||||
rm: (absPath) => rm(absPath, { force: true }),
|
rm: (absPath) => rm(absPath, { force: true }),
|
||||||
},
|
},
|
||||||
|
// Every cycle logs its full push plan + per-action lines + completion
|
||||||
|
// counts (created/updated/deleted/skipped/failures) through this `log`, so
|
||||||
|
// what was deleted (and what was not) is always recorded. There is no
|
||||||
|
// delete cap: deletes are soft (Trash, reversible), so a blocking limit
|
||||||
|
// only got in the way of legitimate deletes; engine correctness (covered by
|
||||||
|
// the reconcile/layout tests) is what prevents phantom deletions.
|
||||||
log: (line: string) => this.logger.log(`git-sync[${spaceId}] ${line}`),
|
log: (line: string) => this.logger.log(`git-sync[${spaceId}] ${line}`),
|
||||||
// DEFENSE-IN-DEPTH delete cap (gitmost-specific policy). A non-convergent
|
|
||||||
// vault (e.g. empty/duplicate titles -> colliding paths) can compute
|
|
||||||
// PHANTOM absence-deletions. When the push's planned delete count exceeds
|
|
||||||
// GIT_SYNC_MAX_DELETES_PER_CYCLE (or planning failed -> Infinity), suppress
|
|
||||||
// deletes by making deletePage THROW: the engine records each as a per-page
|
|
||||||
// failure, which keeps `refs/docmost/last-pushed` from advancing past the
|
|
||||||
// dropped-file commit, so the deletion is RETRIED next cycle rather than
|
|
||||||
// silently dropped (a no-op that resolved would advance the ref and a pull
|
|
||||||
// would then recreate the user's deleted files). See PR #119 review.
|
|
||||||
resolveApplyClient: (plannedDeletes, c) => {
|
|
||||||
if (plannedDeletes <= maxDeletes) return c;
|
|
||||||
this.logger.warn(
|
|
||||||
`git-sync[${spaceId}]: push delete count ${plannedDeletes} exceeds ` +
|
|
||||||
`GIT_SYNC_MAX_DELETES_PER_CYCLE=${maxDeletes}; suppressing deletions ` +
|
|
||||||
`this cycle (possible non-convergence / collision). Investigate vault ` +
|
|
||||||
`layout.`,
|
|
||||||
);
|
|
||||||
return {
|
|
||||||
...c,
|
|
||||||
deletePage: async () => {
|
|
||||||
throw new Error(
|
|
||||||
'git-sync: delete suppressed this cycle ' +
|
|
||||||
'(over GIT_SYNC_MAX_DELETES_PER_CYCLE) — refs intentionally held ' +
|
|
||||||
'so the deletion is retried, not dropped',
|
|
||||||
);
|
|
||||||
},
|
|
||||||
};
|
|
||||||
},
|
|
||||||
});
|
});
|
||||||
|
|
||||||
return { spaceId, ...result };
|
return { spaceId, ...result };
|
||||||
|
|||||||
Reference in New Issue
Block a user