🤖 perf: scope workspace.activity.list to config-known workspaces and prune stale extensionMetadata entries - #3967
🤖 perf: scope workspace.activity.list to config-known workspaces and prune stale extensionMetadata entries#3967ThomasK33 wants to merge 74 commits into
Conversation
|
@codex review |
This comment has been minimized.
This comment has been minimized.
![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: ba3b3ad8d2
ℹ️ 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".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9cbca6cce
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
…une stale extensionMetadata entries - getActivityList() now iterates config-known workspace ids instead of every snapshot key on disk (fail-open fallback to the legacy union if config cannot be read); tombstone semantics for known workspaces are preserved. - remove() deletes the workspace's extensionMetadata.json entry after config deregistration (best-effort). - One-time lazy cleanup per process prunes entries whose workspace no longer exists; known-ids are fetched inside the file's serialized mutation for loss safety; survivors round-trip verbatim (upgrade/downgrade-safe). Fixes #3959
…ollback - pruneMissingWorkspaces now loads the file BEFORE fetching known ids (fresh config disk read inside the serialized mutation), so a workspace created concurrently by another backend process cannot be misclassified as stale. - Centralize entry cleanup as WorkspaceService.discardExtensionMetadataEntry; TaskService.rollbackFailedTaskCreate now prunes the entry a failed send's scheduled metadata writes could otherwise leak after deregistration.
loadConfigOrDefault swallows read/parse failures and resolves as the EMPTY default, which the destructive prune cannot distinguish from a truly empty config (it would wipe every extensionMetadata entry) and which would drop every live entry from the activity list. Thread throwOnError through getAllWorkspaceMetadata and use the strict read in both call sites, matching cleanupOrphanSessionDirs' existing internal guard. Regression test now corrupts config.json on disk (red-proofed against the unfixed code).
f38034b to
7d895ed
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d895ed2e1
ℹ️ 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".
A parseable config.json with a non-array projects value, non-pair entries, non-object project configs, or a non-array workspaces value was lenient- normalized into an empty/partial workspace view even under throwOnError, so destructive not-in-config decisions (extension-metadata prune, activity scoping) saw live workspaces as removed. Strict mode now validates the projects structure before normalization; lenient loads keep self-healing. Also hardens cleanupOrphanSessionDirs' existing strict read for free.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0f29f7579
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
…alization
Strict structural validation could not enumerate every entry the lenient
normalization discards (e.g. [null, {...}] pairs filtered for invalid
paths). Instead of narrowing validation further, destructive callers now
union a raw-scan superset of every workspace id in the persisted projects
subtree (readPersistedWorkspaceIdSuperset; over-collection is safe,
under-collection destroys live data) with the strict normalized view
(covers in-memory migration ids). Orphan session-dir cleanup gets the same
guarantee. Throws on unreadable/unparseable config, aborting both sweeps.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3f6254e25
ℹ️ 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".
|
@codex review |
|
@codex security review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c06fa7a147
ℹ️ 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".
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c4e943c6e
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9d70816fd
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
A generation-less stranded .recreated-* file could revert a newer local goal/status write: this build's mutation preserves recency and stamps writeGeneration, but the equal-recency tiebreak blindly preferred the generation-less side, resurrecting the stale copy indefinitely. Make writeGeneration a wall-clock epoch-ms stamp (monotonic per entry) so the stranded-leftover merge can compare a generation-carrying target against the stranded file's mtime (rename preserves it): a stamp that strictly postdates the stranded snapshot keeps the target; otherwise the order is unknowable and the generation-less side still wins to preserve downgraded-build writes.
|
@codex review |
|
@codex security review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e097ede94
ℹ️ 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".
…eteness, unique empty temp 1. Stranded-leftover recovery gathers adoption registration evidence strictly after the main read (pre-load pass now handles only generation-checked tombstone lifts), so a workspace deregistered during the probe awaits can no longer be resurrected on a stale positive. 2. Raw-visible→raw-invisible transitions count as removal only while the raw view is complete registration evidence: with id-less legacy entries present, the fresh (mid-list) or final (post-probe) authoritative enumeration must deny the id — an affirmation or a failed enumeration retains the entry instead of dropping a revived workspace and republishing the tombstone that evidence just cleared. 3. Empty-file quarantine recovery writes through a process-unique .empty-<pid>-<uuid>.tmp with finally-scoped cleanup, so concurrent recoveries can no longer truncate or unlink each other's in-flight temp (the shared fixed name could alias the canonical file).
|
@codex review |
|
@codex security review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9ddb97a2b
ℹ️ 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".
…ss entries An id removed before the authoritative enumeration and re-registered id-less right after it was dropped (and tombstoned) on the stale denial, even though the post-enumeration raw refresh proved the enumeration's completeness could have been invalidated. Both the mid-list and final-phase guards now run one bounded re-enumeration when the refreshed raw view reports id-less entries, so enumeration-backed removal arms always consult a denial that postdates that refresh; on failure they retain instead of trusting the stale denial set. Raw/snapshot views stay at their earlier reads because their staleness errs toward retention, never a wrong drop.
|
@codex review |
|
@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
workspace.activity.listno longer returns entries for workspaces that only exist as staleextensionMetadata.jsonkeys. The list is scoped to config-known workspaces, removed workspaces now drop their metadata entry at removal time, and a one-time lazy cleanup reclaims entries leaked by pre-existing deployments. On a synthetic 13,006-entry state this takes the bootstrap payload from 3.5 MB / 13,006 entries to 1.7 KB / 6 entries and shrinks the on-disk file from 5.85 MB to 2.9 KB.Fixes #3959.
Background
On a long-lived live deployment,
~/.mux/extensionMetadata.jsonheld 13,895 workspace entries while only 246 workspaces were live (1,513 in config incl. archived): entries for removed workspaces/sub-agents were never pruned.getActivityList()unioned every snapshot key with config workspaces and ran the per-workspace workflow-run bootstrap disk probe for all of them, producing a 13,901-entry / 2.89 MB response that took ~59 s server-side — delivered on the same ordered WebSocket the frontend boot gate waits on (a major contributor to a 131–162 s time-to-interactive). Full investigation: #3959.Implementation
Read-side scoping (
getActivityList): iterate config-known workspace ids (live + archived) instead of the union of all snapshot keys + in-memory caches. Stale ids no longer inflate the payload or trigger the per-idgetActiveWorkflowRunIdsdisk probe. The deliberate tombstone semantics are preserved: config-known ids still flow through the existing zero-count tombstone logic, so workflow/bash-monitor activity cleared while the renderer was disconnected still surfaces a clearing entry on reconnect — scoping only drops ids that are not in config at all. If the config read fails, the code fails open to the legacy unscoped union rather than dropping live entries.Write-side pruning:
WorkspaceService.remove()callsextensionMetadata.deleteWorkspace(workspaceId)(best-effort) right after config deregistration. Task removal and project removal both flow throughremove(), so one hook covers all removal paths.pruneStaleExtensionMetadataOnce, triggered from the activity bootstrap path) prunes entries whose workspace no longer exists in config. It is never a per-read scan: a boolean latch guarantees at most one pass per process.Loss safety of the cleanup:
pruneMissingWorkspaces(getKnownWorkspaceIds)invokes the known-ids callback inside the file's serialized mutation queue, before loading the file. All in-process writers go through the same queue, so any entry visible at load time was written by a mutation that completed earlier — and a workspace is registered in config before its first metadata write. A concurrently-created workspace therefore cannot lose its just-written entry. Entries belonging to existing workspaces are never touched (covered by tests).Upgrade↔downgrade safety
Addressing the repo rule head-on:
{ version: 1, workspaces: {...} }; older builds read the pruned file exactly as before.pruneMissingWorkspacesdeletes stale keys and round-trips surviving entries verbatim (no normalization), so unknown fields written by newer builds survive — verified by a dedicated test (preserves unrecognized fields on surviving entries).Validation
pruneMissingWorkspacestests (drop/keep, verbatim survivor round-trip, no-write on no-op); newWorkspaceService activity list scopingtests (stale ids dropped + lazily pruned exactly once, fail-open fallback when config is unreadable, removal-time prune); existing bash-monitor/workflow tombstone tests updated to register their workspaces in config, matching the new contract that tombstones are scoped to known ids.make static-checkgreen locally;bun test src/node/services/workspaceService.test.ts366/367 pass (the 1 failure,accepted history suppresses redelivery while wake-store reconciliation keeps failing, is a known local-env-dependent failure in this Coder workspace that also fails at origin/main; CI is green there).Dogfooding (before/after, synthetic 13k-stale-entry state)
Sandbox: fresh
XUM_ROOTwith aconfig.jsoncontaining 6 live workspaces and anextensionMetadata.jsonwith 13,006 entries (5.85 MB); server run vianode dist/cli/index.js server --no-auth— BEFORE = clean HEAD (bdc601e25) build in a probe worktree, AFTER = this branch, each against its own copy of the same root./orpc/workspace/activity/list)time mux api workspace activity list | wc -c(end-to-end, incl. ~0.4 s node CLI startup)extensionMetadata.jsonafter first listThe sandbox numbers are on an idle local NVMe box; the live deployment's ~59 s came from the same 13k-id fan-out on a busy server with 17 GB of session state, so the entry-count/payload reduction is the transferable result. Removal-time prune was also verified live:
POST /orpc/workspace/removeforlive-ws-05dropped the file to 5 entries.Review hardening (Codex rounds)
Codex review drove four loss-safety hardenings of the cleanup path, all covered by tests:
pruneMissingWorkspacesloads the metadata file before fetching known ids (fresh config disk read inside the serialized mutation), so a workspace created concurrently by another backend (XUM_ALLOW_MULTIPLE_INSTANCES) can't lose its just-written entry.TaskService.rollbackFailedTaskCreateprunes via the centralizeddiscardExtensionMetadataEntry(), closing the leak where a failed send's scheduled metadata write recreates the entry after deregistration.getAllWorkspaceMetadata({ throwOnError: true })+ strict structural validation inloadConfigOrDefault— a corrupted or structurally invalidconfig.json(non-arrayprojects, non-arrayworkspaces, …) throws instead of resolving as a lenient-normalized empty view, aborting the prune and sending the activity list to its fail-open unscoped fallback.projectssubtree (readPersistedWorkspaceIdSuperset; over-collection is safe, under-collection destroys live data) with the strict normalized view — entries that normalization discards (e.g.[null, {…}]pairs) keep their metadata.cleanupOrphanSessionDirs(pre-existing, same bug class) gets the same superset guarantee for session data.Risks
Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh