From 18697b8d21fbbe86bcf64621a86defe4c922baf5 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 19:28:31 -0600 Subject: [PATCH] perf(workspaces): share cohort child-source verification (#20418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip must preserve agent workspace bytes during online migration. > - A cohort can contain several aliases of the same physical source. > - The importer verifies imported children before capture and again before publication. > - The immutable-object cache avoids repeated downloads but still recaptures local child bytes for every alias. > - This pull request shares the initial proof within the held cohort fence and checks fresh bytes once more after every capture joins. > - The benefit is less repeated local archive work while child mutations still block the whole cohort. > - Required browser CI twice reproduced an independent stale composer-mode bug. A focused regression proves that refreshed persisted task mode was ignored after mount. The state repair follows task data when there is no local override. Explicit local mode choices retain their own generation so a completed send cannot clear the next draft’s choice. ## Linked Issues or Issue Description Refs: #20311, #20387, #20401, #20404. **Current behavior** Two aliases recapture the same imported child's local bytes four times, even with one immutable object read. A larger alias cohort repeats this multi-GB work for each member. **Proposed behavior** Share the successful initial verification for the same physical root, Git roots and exact recorded child set or discovery mode. After all captures join, run a separate fresh final verification for each root and recorded child set. Keep every member's database and publication checks. **Reason and benefit** The local PostgreSQL fixture proves that two aliases now perform two child recaptures instead of four. Hosted timing improvement has not been measured. ## What Changed - Add invocation-local initial and final proof maps to the existing cohort child verifier. - Retain the existing canonical source-owner check before and after shared results. - Defer cohort final nested checks until all captures finish, and join every final reader before publication or fence release. - Keep singleton verification, saved journal schemas, snapshot IDs, archive bytes and per-member publication checks unchanged. - Extend the existing real PostgreSQL alias fixture with a child mutation during the last parent upload. - Repair the required CI planning-mode race with a nullable local override: absent override follows task data; a user selection belongs to the draft. A successful send or confirmed resend/receipt clears it only if no newer choice exists. Failed saves preserve the mode and restored message. Queued-edit backup retains whether the mode was explicit. Leave the browser assertion and timeouts unchanged. ## Verification - Canonical Node24 worktree preflight and plugin SDK prerequisites passed. Server TypeScript passed. - Genuine unchanged-source PostgreSQL RED: two aliases performed four local child recaptures instead of two (one failed, 22 intentionally not selected). GREEN: five source-sharing, last-upload child mutation, capture/cancellation join and actual outer-commit rollback controls passed (18 intentionally not selected; 66.53s). The actual fence-loss control also passed (one passed, 22 intentionally not selected; 38.13s). Both wrappers completed cleanup. - Migration source and tree bytes remain identical to locally qualified `466837cfe997924c3794fd5d0e96134cab1cd04c`; later commits only repair the composer and its tests. - Required planning browser CI originally failed, then failed its one unchanged-head retry, because the planning chip remained absent after opening-question dismissal. Those failures remain recorded ([original](https://github.com/paperclipai/paperclip/actions/runs/42723622165/job/129523785985), [retry](https://github.com/paperclipai/paperclip/actions/runs/42723622165/job/129534415590)). The final-head identical case passes in 30.5s, with all 34 shard cases passing ([final CI](https://github.com/paperclipai/paperclip/actions/runs/42731309796/job/129550549775)). Assertions and timeouts were not relaxed. - Composer regressions reproduced stale loaded mode, optimistic-save rollback, overwritten next-draft choices, and stale mode after confirmed resend/receipt. All 145 composer tests now pass (2.88s). Canonical UI TypeScript and token gates pass. The original planning browser case also passed locally (15.5s; 26.9s including setup). - The first final-head serialized-suite attempt was automatically cancelled at its unchanged 20-minute job limit. Grouped Turbo output did not expose an individual failing assertion; cause remains unknown ([retained attempt](https://github.com/paperclipai/paperclip/actions/runs/42731309796/job/129550550373)). One same-head diagnostic retry was a cache miss and executed all 419 heartbeat recovery tests successfully in 376.94s on the identical CI checkout ([retry](https://github.com/paperclipai/paperclip/actions/runs/42731309796/job/129561196792)). It is not classified as infrastructure or a known flake. - Final exact head `ba1f85f18085dd0e84c594ed0cb68d5bf817c93c`: all 61 checks terminal (56 success, four skipped, one neutral); required CI is green ([workflow](https://github.com/paperclipai/paperclip/actions/runs/42731309796)). Fresh Greptile is 5/5 with every review thread resolved ([review](https://github.com/paperclipai/paperclip/pull/20418#issuecomment-6754705323)). Focused commands: ```sh node --import ./cli/node_modules/tsx/dist/loader.mjs packages/db/src/test-postgres-command.ts pnpm exec vitest run server/src/services/remote-workspace-cohort-migration.test.ts -t "shares the merged child verifier|joins active capture peers|keeps every authority and activity unpublished" pnpm exec vitest run ui/src/components/task-chat/TaskChatComposer.test.tsx pnpm --filter @paperclipai/server exec tsc --noEmit pnpm --filter @paperclipai/ui typecheck pnpm check:token-gates ``` ## Risks - Reuse is limited to one invocation under the same complete source and finalizer fences. Different physical roots, Git roots, recorded child sets and discovery mode have different keys. No local-byte proof is stored durably or reused after restart. - Initial and final maps are separate. A child change during any parent upload must fail the fresh final verification before the atomic publication transaction starts. - Final readers all finish before an error can release the source fence. Per-member database child rows, source fingerprints, journals, keys and publication locks retain their existing checks. - This reduces repeated verification for aliases. It does not change capture concurrency, upload policy or immutable checkpoint validation. Hosted speedup remains unverified. - The CI repair must distinguish task data from explicit draft intent. Only a successful send with the same choice generation clears its override; a later draft selection and failed-save rollback retain it. Regression tests cover these transitions. It adds no styling, request, or persistence changes. ## Model Used OpenAI GPT-6 (Codex), with tool use and code execution. The runtime does not expose a more specific model identifier or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes: #` / `Refs: #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../2026-11-07-online-git-source-fence.md | 7 ++++ .../remote-workspace-cohort-migration.test.ts | 22 ++++++++-- .../remote-workspace-migration-tree.ts | 1 + .../services/remote-workspace-migration.ts | 40 +++++++++++++++---- 4 files changed, 58 insertions(+), 12 deletions(-) diff --git a/doc/plans/2026-11-07-online-git-source-fence.md b/doc/plans/2026-11-07-online-git-source-fence.md index e67165c3406..682725fcd97 100644 --- a/doc/plans/2026-11-07-online-git-source-fence.md +++ b/doc/plans/2026-11-07-online-git-source-fence.md @@ -105,6 +105,13 @@ original source, restore uncommitted/staged work, and run fsck. No hosted import hold, cancellation, provider call, or shared-cache Git command is part of these local tests. +A cohort shares its initial nested-source verification only within its held +source and finalizer fences, keyed by physical root, Git roots and the exact +recorded child set (or discovery). After every capture joins, a separate fresh +verification checks each physical root and child set before publication. A +changed child still rejects the whole cohort. Singleton imports keep their +per-import checks. No local-byte proof survives the invocation. + The online migration owner can checkpoint completed immutable nested-snapshot verification in the child snapshot ledger. This optional versioned proof binds the complete encrypted reference, child lineage, source operation and a known diff --git a/server/src/services/remote-workspace-cohort-migration.test.ts b/server/src/services/remote-workspace-cohort-migration.test.ts index 92c353d2eee..cc2c3ce9af4 100644 --- a/server/src/services/remote-workspace-cohort-migration.test.ts +++ b/server/src/services/remote-workspace-cohort-migration.test.ts @@ -139,7 +139,7 @@ describe.skipIf(!support.supported)("bounded workspace cohort migration", () => expect(await git(targetDir, "log", "-1", "--format=%s")).toBe("Original history"); } }, 60_000); - it("shares the merged child verifier across parallel aliases under the complete cohort fence", async () => { + it.each(["unchanged", "changed"] as const)("shares the merged child verifier across parallel aliases under the complete cohort fence (%s child bytes)", async mode => { const f = await fixture(), parent = f.leaves[0], cwd = path.join(parent.cwd!, ".paperclip", "worktrees", "nested"); await git(f.root, "worktree", "add", "-qb", "nested", cwd); const [childIssue, aliasIssue] = await db.insert(issues).values([ @@ -172,10 +172,24 @@ describe.skipIf(!support.supported)("bounded workspace cohort migration", () => return get(...args); }); const capture = vi.spyOn(workspaceArchive, "captureRemoteWorkspaceDirectory"); - await expect(run(f, { workspaceIds: [parent.id, alias.id] })).resolves.toMatchObject({ applied: true }); + const beforeActivity = await db.select().from(activityLog), put = storage.putFile.bind(storage); + let uploads = 0; + if (mode === "changed") vi.spyOn(storage, "putFile").mockImplementation(async input => { + const result = await put(input); + if (++uploads === 2) await fs.writeFile(path.join(cwd, "untracked.txt"), "changed during parent upload\n"); + return result; + }); + const pending = run(f, { workspaceIds: [parent.id, alias.id] }); + if (mode === "unchanged") await expect(pending).resolves.toMatchObject({ applied: true }); + else { + await expect(pending).rejects.toThrow("nested_workspace_source_changed"); + const selected = await db.select().from(executionWorkspaces).where(inArray(executionWorkspaces.id, [parent.id, alias.id])); + expect(selected.every(row => !readRemoteWorkspaceAuthority(row.metadata))).toBe(true); + expect(await db.select().from(activityLog)).toEqual(beforeActivity); + } expect(reads).toBe(1); - expect(capture.mock.calls.filter(([input]) => input.cwd === cwd && path.basename(input.archivePath) === "current.pcws")).toHaveLength(4); - expect(await fs.readFile(path.join(cwd, "untracked.txt"), "utf8")).toBe("nested dirty bytes\n"); + expect(capture.mock.calls.filter(([input]) => input.cwd === cwd && path.basename(input.archivePath) === "current.pcws")).toHaveLength(2); + expect(await fs.readFile(path.join(cwd, "untracked.txt"), "utf8")).toBe(mode === "unchanged" ? "nested dirty bytes\n" : "changed during parent upload\n"); }, 60_000); it.each(["foreign", "journal", "finished"])("refuses a changed %s operation before any authority write", async mode => { const f = await fixture(), put = storage.putFile.bind(storage); let uploaded = 0; diff --git a/server/src/services/remote-workspace-migration-tree.ts b/server/src/services/remote-workspace-migration-tree.ts index 34d88d67c1f..d7e3880c22a 100644 --- a/server/src/services/remote-workspace-migration-tree.ts +++ b/server/src/services/remote-workspace-migration-tree.ts @@ -361,6 +361,7 @@ export function createNestedWorkspaceImportVerifier(input: { }, }; return { + assertOwner, prepare(options: Omit): Promise { if (closed || current) return Promise.reject(new Error("nested_workspace_verifier_unavailable")); const work = (async () => { diff --git a/server/src/services/remote-workspace-migration.ts b/server/src/services/remote-workspace-migration.ts index 258724ccfff..cf068eebde6 100644 --- a/server/src/services/remote-workspace-migration.ts +++ b/server/src/services/remote-workspace-migration.ts @@ -431,17 +431,32 @@ async function lockMigrationRecovery(lock: MigrationTransaction, plans: Migratio for (const reader of readers) if (reader.status === "rejected") throw reader.reason; } -function createMigrationChildVerifier(input: MigrationInput, onlineSource: OnlineWorkspaceSource | undefined, sourceLock: MigrationTransaction) { +function createMigrationChildVerifier(input: MigrationInput, onlineSource: OnlineWorkspaceSource | undefined, sourceLock: MigrationTransaction, cohort = false) { if (onlineSource?.kind !== "git") return undefined; const verifier = createNestedWorkspaceImportVerifier({ db: input.db, storage: input.storage, companyId: input.companyId, sourceLock, projectIds: onlineSource.projectIds, issueIds: onlineSource.issueIds, signal: input.signal, persistVerification: true }); + type Options = Parameters[0]; + const initial = new Map>(), final = new Map>(); let pending = Promise.resolve(); + const prepare = (options: Options) => { + const work = pending.then(() => verifier.prepare(options)); + pending = work.then(() => undefined, () => undefined); + return work; + }; + const shared = async (options: Options, cache: typeof initial) => { + await verifier.assertOwner(); + const root = options.workspace.cwd ?? options.workspace.providerRef; + const key = canonical([root ? await fs.realpath(root) : null, [...(options.gitSourceRoots ?? [])].sort(), options.recordedChildren ?? null]); + let work = cache.get(key); + if (!work) { work = prepare(options); cache.set(key, work); } + const result = await work; + await verifier.assertOwner(); + return result; + }; return { - prepare(options: Parameters[0]) { - const work = pending.then(() => verifier.prepare(options)); - pending = work.then(() => undefined, () => undefined); - return work; - }, + prepare: (options: Options) => cohort ? shared(options, initial) : prepare(options), + finalize: (options: Options) => shared(options, final), + deferFinal: cohort, async close() { await pending; await verifier.close(); }, }; } @@ -511,7 +526,7 @@ async function captureMigration(input: MigrationInput, reserved: ReservedMigrati const current = await captureRemoteWorkspaceDirectory({ cwd: sourceRoot!, archivePath: currentArchive, exclude, requireNoGit: onlineSource?.kind === "ordinary", gitSourceRoots: onlineSource?.gitSourceRoots, signal: input.signal }); if (current.manifestDigest !== captured.manifestDigest) throw new Error("remote_workspace_migration_source_changed"); } - if (nested.length) { + if (nested.length && !childVerifier?.deferFinal) { const verifyScratch = path.join(scratch, "verify-nested"); await fs.mkdir(verifyScratch); const verified = await (childVerifier?.prepare ?? prepareNestedWorkspaceImports)({ ...input, workspace: reserved.workspace, scratch: verifyScratch, recordedChildren: nested, gitSourceRoots: onlineSource?.gitSourceRoots }); @@ -828,7 +843,7 @@ export async function migrateRemoteWorkspaceCohort(input: { cohort = refreshedCohort; } const captures: Array = Array(plans.length); - const childVerifier = createMigrationChildVerifier(plans[0].input, plans[0].onlineSource, lock); + const childVerifier = createMigrationChildVerifier(plans[0].input, plans[0].onlineSource, lock, true); let next = 0, failure: unknown; const worker = async () => { while (!signal.aborted && next < plans.length) { @@ -842,6 +857,15 @@ export async function migrateRemoteWorkspaceCohort(input: { for (const peer of peers) if (peer.status === "rejected") failure ??= peer.reason; if (failure) throw failure; signal.throwIfAborted(); + const nestedReaders = await Promise.allSettled(plans.map(async (plan, index) => { + const captured = captures[index]!; + if (!captured.nested.length) return; + const scratch = path.join(captured.scratch, "verify-nested"); await fs.mkdir(scratch); + const verified = await (childVerifier?.finalize ?? prepareNestedWorkspaceImports)({ ...plan.input, workspace: plan.reserved.workspace, + scratch, recordedChildren: captured.nested, gitSourceRoots: plan.onlineSource?.gitSourceRoots }); + if (canonical(verified) !== canonical(captured.nested)) throw new Error("nested_workspace_source_changed"); + })); + for (const reader of nestedReaders) if (reader.status === "rejected") throw reader.reason; await lock.execute(sql`select 1`); await lock.transaction(async tx => { await lockMigrationParents(tx, companyId, null, plans.flatMap(plan => [