fix(workspace): batch cloud-workspace reconcile into one mutation#2256
fix(workspace): batch cloud-workspace reconcile into one mutation#2256andrewm4894 wants to merge 0 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99700e59eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| createdAt: timestamp, | ||
| updatedAt: timestamp, | ||
| })); | ||
| this.db.insert(workspaces).values(rows).run(); |
There was a problem hiding this comment.
workspaces.taskId is unique, but this batched insert has no conflict handling. Because reconcileCloudWorkspaces computes toCreate in a separate read step, a concurrent createWorkspace for any one task can insert first, causing this single values(rows).run() to fail and abort creation for all remaining tasks in the batch. The previous per-task Promise.allSettled path allowed other tasks to succeed even if one collided, so this introduces a regression under normal concurrent usage.
Useful? React with 👍 / 👎.
| if (result.created.length > 0) { | ||
| void queryClient.invalidateQueries( | ||
| trpcReact.workspace.getAll.pathFilter(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This only invalidates workspace.getAll when result.created.length > 0. If the DB already has those rows (for example from a concurrent creator or stale client cache), reconcile returns created: [] and the UI cache is never refreshed, so tasks can remain treated as missing and reconciliation can be retried unnecessarily. The previous flow invalidated after any successful create call, including no-op returns of existing workspaces.
Useful? React with 👍 / 👎.
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/code/src/main/services/workspace/service.ts:442-458
**No tests for the new reconcile method**
There are no test files for the workspace service (`services/workspace/service.test.ts` does not exist), and no new tests are added here. The deduplication logic — `findAll()` → build `existingTaskIds` set → `uniqueRequested` dedup → filter → batch insert — has several meaningful branches to cover: empty input, all-already-exist (idempotent), partial overlap, and full batch creation. These are exactly the cases that should be verified with parameterised tests, in line with the team's stated convention.
### Issue 2 of 2
apps/code/src/main/db/repositories/workspace-repository.ts:113
Defensive: use `onConflictDoNothing()` on the batch insert. `taskId` carries a `UNIQUE` constraint (schema.ts), so if `createCloudMany` is ever called with an ID that already exists — e.g., from a future caller that skips the service-layer dedup — the entire batch throws a SQLite constraint error and no rows are created. Adding the conflict clause makes the repository method safe to call in isolation and keeps the UNIQUE constraint as the source of truth.
```suggestion
this.db.insert(workspaces).values(rows).onConflictDoNothing().run();
```
Reviews (1): Last reviewed commit: "fix(workspace): batch cloud-workspace re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async reconcileCloudWorkspaces( | ||
| taskIds: string[], | ||
| ): Promise<ReconcileCloudWorkspacesOutput> { | ||
| if (taskIds.length === 0) return { created: [] }; | ||
|
|
||
| const existingTaskIds = new Set( | ||
| this.workspaceRepo.findAll().map((w) => w.taskId), | ||
| ); | ||
| const uniqueRequested = Array.from(new Set(taskIds)); | ||
| const toCreate = uniqueRequested.filter((id) => !existingTaskIds.has(id)); | ||
| if (toCreate.length === 0) return { created: [] }; | ||
|
|
||
| log.info( | ||
| `Reconciling ${toCreate.length} cloud workspaces (requested ${taskIds.length})`, | ||
| ); | ||
| this.workspaceRepo.createCloudMany(toCreate); | ||
| return { created: toCreate }; |
There was a problem hiding this comment.
There are no test files for the workspace service (services/workspace/service.test.ts does not exist), and no new tests are added here. The deduplication logic — findAll() → build existingTaskIds set → uniqueRequested dedup → filter → batch insert — has several meaningful branches to cover: empty input, all-already-exist (idempotent), partial overlap, and full batch creation. These are exactly the cases that should be verified with parameterised tests, in line with the team's stated convention.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/workspace/service.ts
Line: 442-458
Comment:
**No tests for the new reconcile method**
There are no test files for the workspace service (`services/workspace/service.test.ts` does not exist), and no new tests are added here. The deduplication logic — `findAll()` → build `existingTaskIds` set → `uniqueRequested` dedup → filter → batch insert — has several meaningful branches to cover: empty input, all-already-exist (idempotent), partial overlap, and full batch creation. These are exactly the cases that should be verified with parameterised tests, in line with the team's stated convention.
How can I resolve this? If you propose a fix, please make it concise.| createdAt: timestamp, | ||
| updatedAt: timestamp, | ||
| })); | ||
| this.db.insert(workspaces).values(rows).run(); |
There was a problem hiding this comment.
onConflictDoNothing() on the batch insert. taskId carries a UNIQUE constraint (schema.ts), so if createCloudMany is ever called with an ID that already exists — e.g., from a future caller that skips the service-layer dedup — the entire batch throws a SQLite constraint error and no rows are created. Adding the conflict clause makes the repository method safe to call in isolation and keeps the UNIQUE constraint as the source of truth.
| this.db.insert(workspaces).values(rows).run(); | |
| this.db.insert(workspaces).values(rows).onConflictDoNothing().run(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/db/repositories/workspace-repository.ts
Line: 113
Comment:
Defensive: use `onConflictDoNothing()` on the batch insert. `taskId` carries a `UNIQUE` constraint (schema.ts), so if `createCloudMany` is ever called with an ID that already exists — e.g., from a future caller that skips the service-layer dedup — the entire batch throws a SQLite constraint error and no rows are created. Adding the conflict clause makes the repository method safe to call in isolation and keeps the UNIQUE constraint as the source of truth.
```suggestion
this.db.insert(workspaces).values(rows).onConflictDoNothing().run();
```
How can I resolve this? If you propose a fix, please make it concise.
charlesvien
force-pushed
the
fix/batch-cloud-workspace-reconcile
branch
from
99700e5 to
08d0511
Compare
2 participants
Summary
Root-cause fix for the cloud-workspace storm part of #2251.
The reconcile effect in
MainLayout.tsxfires oneworkspace.createtRPC mutation per cloud task that has no local workspace row. With ~100 cloud tasks at boot, that's 100 simultaneous IPC roundtrips, each paying for tRPC validation + a sqlite write + aworkspace.getAllsubscription invalidation. Logs show the rate-limit cap saturating:This is the lurking systemic bug: every user who has accumulated many cloud tasks hits this on first boot (and again any time the local workspace rows get cleared — fresh install, DB reset, etc.).
Change
Add a new mutation
workspace.reconcileCloudWorkspaces({ taskIds })that:db.insert().values([...])callUpdate
MainLayout.tsxto call this once with all missing IDs instead ofPromise.allSettledover N parallelworkspaceApi.createcalls.One IPC. One sqlite roundtrip. One subscription invalidation.
Diff shape
db/repositories/workspace-repository.ts— newcreateCloudMany(taskIds[])db/repositories/workspace-repository.mock.ts— same on the mockservices/workspace/schemas.ts— new input/output schemas + typesservices/workspace/service.ts— newreconcileCloudWorkspaces(taskIds[])methodtrpc/routers/workspace.ts— wire up the mutationrenderer/features/workspace/hooks/useWorkspace.ts— addworkspaceApi.reconcileCloudWorkspacesrenderer/components/MainLayout.tsx— single batched call replaces the N-parallel patternTest plan
pnpm --filter code typecheckcleanpnpm --filter code test— 1348/1348 passingpnpm lintclean[ipc-rate] mutation workspace.create called 100 times in 2000msno longer appears inmain.log. Workspaces should still be reconciled (visible in the sidebar).createpath.Why this is separate from #2252
#2252 fixes the
getTaskPrStatusIPC storm (read side). This fixes theworkspace.createstorm (write side, on reconcile). Same systemic shape — N IPC calls per task on boot — different code path. They don't overlap.