fix(workspace): re-read the catalog on every v2 registry operation - #1706
Conversation
- drop the in-memory write cache in the v2 registry: every list/get/ mutation is a fresh read-modify-write of workspaces.json under the op mutex, so v1 writers (touchWorkspaceRegistry) are never clobbered by a stale snapshot; the session-index merge stays once per process behind a flag (Codex P1) - move the shared workspaces.json helper from services/workspace (the upper facade) to session/store, so the rpc runtime no longer imports back into services/ (Codex P2)
commit: |
![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: 05397ba1b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| private cache: Map<string, Workspace> | undefined; | ||
| private deletedIds: Set<string> | undefined; | ||
| /** Whether the once-per-process session-index sync already ran. */ |
There was a problem hiding this comment.
packages/agent-core-v2/AGENTS.md requires comments in this package to live solely in the top-of-file /** */ block and never beside functions, methods, or statements. This new inline field comment violates that convention (as do the new helper docblocks below), so please move any necessary rationale into the file header or let the merged name carry it.
Useful? React with 👍 / 👎.
1 participant
Related Issue
Follow-up to #1701 (merged) — addresses the two Codex review findings on that PR.
Problem
Two issues in #1701's implementation, flagged by Codex's automated review:
workspaces.jsononce into an in-memory cache and wrote that snapshot back wholesale on every mutation. When a v1 process (the TUI's newtouchWorkspaceRegistrypath) updated the same file between the v2 load and a later v2 create/update/delete, the v2 write clobbered the v1-added workspace entries and deletion tombstones — breaking the cross-engine sync the change was meant to provide.workspaces.jsonhelper lived inservices/workspace, butservices/is agent-core's upper facade: the rpc runtime must not import back into it, andcore-impl.tsdid exactly that.What changed
list/get/createOrTouch/update/deleteis now a fresh read-modify-write ofworkspaces.jsonunder the existing promise-chain mutex — the same per-op read model v1 already uses on both sides (WorkspaceRegistryServiceandtouchWorkspaceRegistry). The session-index merge stays once per process behind a flag, still inside the mutex, so it cannot interleave with a mutation. The cross-process lost-update window shrinks from the process lifetime to a single read-modify-write, with the next startup merge as the healer.services/workspace/workspaceRegistryFile.tstosession/store/workspace-registry-file.ts(same layer assession-index.ts, kebab-case per directory convention) and updated all importers; theservices/facade no longer re-exports it.createOrTouch(entries + tombstones),delete(tombstones accumulate), andupdate(operates on current file state, misses externally removed ids).Checklist
gen-changesetsskill, or this PR needs no changeset (amends fix(workspace): sync workspace catalog across v1/v2 and soft-delete #1701's unreleased behavior; that PR's changeset already describes the final user-visible change).gen-docsskill, or this PR needs no doc update.