diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index aeb59eff..5ce2f386 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -1274,8 +1274,18 @@ export class PageService { pageId: string, userId: string, workspaceId: string, + // Optional provenance. A git-sync-driven soft-delete stamps + // `lastUpdatedSource = 'git-sync'` so the change-listener loop-guard skips + // its own write (mirrors the create/update/move provenance branches above). + provenance?: AuthProvenanceData, ): Promise { - await this.pageRepo.removePage(pageId, userId, workspaceId); + const isGitSync = provenance?.actor === 'git-sync'; + await this.pageRepo.removePage( + pageId, + userId, + workspaceId, + isGitSync ? 'git-sync' : undefined, + ); } private async parseProsemirrorContent( diff --git a/apps/server/src/database/repos/page/page.repo.ts b/apps/server/src/database/repos/page/page.repo.ts index 45cb57ab..00fef86a 100644 --- a/apps/server/src/database/repos/page/page.repo.ts +++ b/apps/server/src/database/repos/page/page.repo.ts @@ -298,6 +298,11 @@ export class PageRepo { pageId: string, deletedById: string, workspaceId: string, + // Optional provenance marker. When the soft-delete is driven by an automated + // data plane (e.g. git-sync), stamp `lastUpdatedSource` so the change-listener + // loop-guard recognizes it as its own write and does not schedule an echo + // cycle. Omitted for ordinary user deletes (column keeps its prior value). + lastUpdatedSource?: string, ): Promise { const currentDate = new Date(); @@ -348,6 +353,7 @@ export class PageRepo { .set({ deletedById: deletedById, deletedAt: currentDate, + ...(lastUpdatedSource ? { lastUpdatedSource } : {}), }) .where('id', 'in', pageIds) .where('deletedAt', 'is', null) @@ -378,7 +384,14 @@ export class PageRepo { } } - async restorePage(pageId: string, workspaceId: string): Promise { + async restorePage( + pageId: string, + workspaceId: string, + // See removePage: stamp `lastUpdatedSource` for automated (git-sync) restores + // so the change-listener loop-guard skips the echo cycle. Omitted for + // ordinary user restores. + lastUpdatedSource?: string, + ): Promise { // First, check if the page being restored has a deleted parent const pageToRestore = await this.db .selectFrom('pages') @@ -429,7 +442,12 @@ export class PageRepo { // On restore, disarm the death timer: pulling a note out of trash means // "keep it". Otherwise a deadline now in the past would re-trash it on the // next cleanup sweep. - .set({ deletedById: null, deletedAt: null, temporaryExpiresAt: null }) + .set({ + deletedById: null, + deletedAt: null, + temporaryExpiresAt: null, + ...(lastUpdatedSource ? { lastUpdatedSource } : {}), + }) .where('id', 'in', pageIds) .execute(); diff --git a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts index a1a89608..f10f96c4 100644 --- a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts +++ b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts @@ -303,7 +303,7 @@ describe('GitSyncOrchestrator', () => { }); describe('delete cap (anti-data-loss)', () => { - it('neutralizes deletePage on the apply client when planned deletes exceed the cap', async () => { + it('suppresses deletePage on the apply client (by throwing) when planned deletes exceed the cap', async () => { jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); const built = build({ maxDeletes: 5 }); // Dry-run plans 9 deletes (over the cap of 5); apply still runs. @@ -315,20 +315,22 @@ describe('GitSyncOrchestrator', () => { expect(res.ran).toBe(true); expect(runPushMock).toHaveBeenCalledTimes(2); - // The second runPush (real apply, dryRun:false) got a neutralized client. + // The second runPush (real apply, dryRun:false) got a suppressed client. const [applyDeps, applyOpts] = runPushMock.mock.calls[1]; expect(applyOpts).toEqual({ dryRun: false }); const applyClient = applyDeps.makeClient(); - // deletePage is still a function (the engine may call it)... + // deletePage is still a function (the engine calls it)... expect(typeof applyClient.deletePage).toBe('function'); - await applyClient.deletePage('p1'); - // ...but it is a NO-OP: the underlying real deletePage was NOT invoked. + // ...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: a throwing dry-run still suppresses deletes and does not throw', async () => { + it('fails safe: a throwing dry-run still suppresses deletes and does not throw the cycle', async () => { jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); const built = build({ maxDeletes: 5 }); runPushMock @@ -340,7 +342,8 @@ describe('GitSyncOrchestrator', () => { expect(res.ran).toBe(true); const [applyDeps] = runPushMock.mock.calls[1]; const applyClient = applyDeps.makeClient(); - await applyClient.deletePage('p1'); + // Suppressed via throw (same fail-safe path as the over-cap case). + await expect(applyClient.deletePage('p1')).rejects.toThrow(/suppress/i); expect(built.client.deletePage).not.toHaveBeenCalled(); }); diff --git a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts index f4476a4f..e0823fef 100644 --- a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts +++ b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts @@ -473,11 +473,26 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { ); } - // When over the cap, neutralize deletes by wrapping the client's deletePage - // into a no-op (every other op is forwarded). The dry-run already committed - // the working tree to `main`, so the apply re-diffs and converges normally. + // When over the cap, suppress deletes by making deletePage THROW (every + // other op is forwarded). A throw is recorded by the engine as a per-page + // `failure`, which (a) keeps `refs/docmost/last-pushed` from advancing past + // the commit that dropped the files, and (b) makes the next cycle re-diff + // from the un-advanced ref and re-plan the same deletes — so a transient + // over-cap is retried rather than silently dropped forever. (A no-op that + // resolved would let the engine count `deleted++` with no failure, advance + // the ref, and never replay the deletions — a pull would then recreate the + // user's deleted files. See PR #119 review.) const applyClient = suppressDeletes - ? { ...client, deletePage: async () => undefined } + ? { + ...client, + 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', + ); + }, + } : client; const pushResult = await runPush( diff --git a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts index be341a39..542fa6fa 100644 --- a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts +++ b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts @@ -292,10 +292,13 @@ describe('GitmostDataSourceService', () => { it('uses the soft-delete path (removePage), not a force delete', async () => { const { service, mocks } = build(); await service.bind(CTX).deletePage('p1'); + // Passes git-sync provenance so the soft-delete stamps + // lastUpdatedSource='git-sync' (loop-guard, PR #119 review). expect(mocks.pageService.removePage).toHaveBeenCalledWith( 'p1', 'svc-user', 'ws-1', + { actor: 'git-sync', aiChatId: null }, ); // No forceDelete on the service surface used here. expect((mocks.pageService as any).forceDelete).toBeUndefined(); @@ -358,7 +361,12 @@ describe('GitmostDataSourceService', () => { it('restores via the repo restore path scoped to the workspace', async () => { const { service, mocks } = build(); await service.bind(CTX).restorePage('p1'); - expect(mocks.pageRepo.restorePage).toHaveBeenCalledWith('p1', 'ws-1'); + // Stamps lastUpdatedSource='git-sync' on restore (loop-guard, PR #119). + expect(mocks.pageRepo.restorePage).toHaveBeenCalledWith( + 'p1', + 'ws-1', + 'git-sync', + ); }); }); }); diff --git a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts index c19a00bc..1b6652d2 100644 --- a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts +++ b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts @@ -232,7 +232,12 @@ export class GitmostDataSourceService { ctx: GitSyncBindContext, pageId: string, ): Promise { - await this.pageService.removePage(pageId, ctx.userId, ctx.workspaceId); + await this.pageService.removePage( + pageId, + ctx.userId, + ctx.workspaceId, + GIT_SYNC_PROVENANCE, + ); return { id: pageId }; } @@ -368,7 +373,13 @@ export class GitmostDataSourceService { ctx: GitSyncBindContext, pageId: string, ): Promise { - await this.pageRepo.restorePage(pageId, ctx.workspaceId); + // Stamp git-sync provenance so the change-listener loop-guard skips the + // PAGE_RESTORED echo (mirrors deletePage / create / update / move). + await this.pageRepo.restorePage( + pageId, + ctx.workspaceId, + GIT_SYNC_PROVENANCE.actor, + ); return { id: pageId }; }