🧾 feat: Persist Workspace Admission Across API Restarts - #302
Conversation
|
Head ef02127: durable workspace submission, fenced admission/reconciliation, retained results and cancellation. Legacy synchronous requests and worker concurrency are unchanged. Ready for exact-head review. |
|
Head 1bfe89c: fixes the four independent-review findings and legacy commit-timeout compatibility. Added reconnect/FIFO, reset fencing, route alias and replica-policy regressions. Ready for exact-head review. |
|
Head a945346: fences both serial and slot capacity acquisition against stale claims and cancellation, and verifies reservation ownership at enqueue. Added actual timeout/cancellation and delayed-write regressions. Earlier review findings remain fixed. Ready for exact-head review. |
|
Head 0a0fe93: fixes quarantine-first outcome retention by persisting durability in ownership receipts and sharing the normal/quarantine retention policy. Added recovery after a simulated two-minute outage and existing-result precedence regressions. All earlier findings remain fixed. Ready for exact-head review. |
|
Independent review completed with no findings at head 0a0fe93. All six prior findings are fixed: one P1 and five P2. Parent verification passed 228 focused tests, 62 Redis-backed tests, focused TypeScript, touched-line lint, service build and OpenAPI checks. Full-service TypeScript matches unchanged main diagnostics. CI remains blocked by the unchanged Node 20 edit timing test; the arm64 image build is pending. No deployment. |
|
@codex review the latest head, final review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0a0fe93c7d
ℹ️ 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".
| workspaceId: (registration.capabilities.workspaceLeaseSlots ?? 1) > 1 | ||
| ? workspaceAdmissionId(record.request.workspaceId, record.request.workspaceInstanceId, record.request.worktree) | ||
| : undefined, |
There was a problem hiding this comment.
When a request is accepted while the worker advertises one lease slot, this omits its workspace metadata. If the same incarnation re-registers with multiple slots before reconciliation, admission uses the current capacity and calls BridgeWorkspaceSlots.reserve, whose pending-queue scan rejects entries lacking workspace metadata. The request therefore remains queued until its deadline even when capacity is idle; persist the workspace admission ID regardless of the slot count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b5ea20. Durable entries always retain their workspace admission ID. Regressions cover both slot-change directions, FIFO order, duplicate submissions and independent roots.
| const controller = new AbortController(); | ||
| if (record.cancelRequested === true) controller.abort(); | ||
| settlement = await this.waitForSettlement(assignment, Date.parse(assignment.expiresAt), controller.signal) as unknown as CodeBridgeWorkspaceSettlement; | ||
| } |
There was a problem hiding this comment.
When DELETE marks an admitted read_file, list_files, or search_text request as cancelled, this passes an already-aborted signal into waitForSettlement, but that helper only writes the worker cancellation marker for mutating operations. It instead revokes the settlement deadline immediately, so the worker is never told to stop and the durable request becomes failed with ASSIGNMENT_EXPIRED rather than cancelled, even when the assignment had not started. Explicit durable cancellation should notify the worker for every operation and preserve a cancellation outcome unless a settlement already won.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b5ea20. Durable cancellation now notifies every workspace operation and drains settlement before closing. Read-only calls retain cancelled status without a settlement; winning results and unconfirmed mutation fences are preserved. Regressions cover unleased, claimed and acknowledged assignments in serial and concurrent modes.
|
Head 0b5ea20: fixes both Codex P2 findings. Durable FIFO entries preserve workspace metadata across slot renegotiation; admitted read-only cancellation notifies the worker and preserves result arbitration. Added 30 regression cases. Focused checks and a fresh independent exact-head review are running. |
|
Independent review completed with no findings at head 0b5ea20. Both Codex P2 findings are fixed and replied to. Parent checks: 258 focused tests, 92 Redis-backed tests, focused TypeScript, service build and touched-line lint passed. Full-service TypeScript matches unchanged main diagnostics. Reviewer protocol tests: 25 passed; durable Redis/HTTP tests were not independently rerun. CI: nine passed, unchanged Node 20 edit timing test failed, arm64 image build pending. No deployment. |
2 participants
Summary
Workspace admission currently loses its waiting invocation when an API process restarts. Clients also leave and rejoin FIFO admission when retrying synchronous requests.
Add an opt-in durable workspace-request API:
admittedtransition.The synchronous endpoint, worker protocol, worker concurrency and rate limits remain unchanged.
Client contract and rollout
Probe
GET /v1/workspace-tools/capabilities, then submit workspace-tool bodies toPOST /v1/workspace-tools/requestswithX-LibreChat-Workspace-Request-Id. Poll or cancel/v1/workspace-tools/requests/:requestId.Results and IDs are retained for 24 hours from acceptance. Reads do not consume results. Do not retry expired IDs or replay
ASSIGNMENT_EXPIRED. This covers workspace tools includingexecute_command, not programmatic/executejobs. Redis must survive the API restart.Deploy all Code API replicas first, then enable the companion LibreChat handoff. LibreChat owns single-consumer result claims and automatic wake-ups. Keep bridge policies identical behind each endpoint. Drain durable requests before changing policy or rolling back servers.
Verification
tsc --noEmit: blocked by an exact diagnostic match at unchangedmain; no new diagnostics.No deployment or live failure-rate measurement. The 70% reduction target requires the LibreChat companion and post-rollout measurements.
Review fixes
1bfe89c.a945346after an invariant sweep.0a0fe93. Ownership receipts carry durability and both settlement writers share the retention policy.0a0fe93c7d4ae1ae174b56863418577035ad7fb2. All six findings are fixed (one P1, five P2). The reviewer checked source; Redis/HTTP behavior was exercised by the parent verification, not independently rerun.Codex follow-up
Both Codex P2 findings fixed in
0b5ea20: always retain durable workspace admission metadata, and forward admitted cancellation for every operation. Winning results and unknown mutation fences remain unchanged. Added 30 regressions covering slot changes, lease states, serial/concurrent workers, and result/cancellation races.Independent review completed with no findings at
0b5ea20aa4bfa3e081de25a86a3984ff4f7048af. Both Codex P2 findings are fixed in that head and replied to in their threads. The reviewer passed 25 protocol tests; it did not independently run durable Redis/HTTP tests. Final-head local checks passed: 258 focused tests, 92 Redis-backed tests, focused TypeScript, service build and touched-line lint. Full-service TypeScript exactly matches unchanged main diagnostics. No deployment.Current CI
At
0b5ea20, nine checks passed, including service/API tests and Node 22/24. Node 20 failed the unchanged newline-dense edit timing assertion at 10.30 seconds against its 10-second bound. No code-package changes. The arm64 image build remains pending.Pushed and reviewed head:
0b5ea20aa4bfa3e081de25a86a3984ff4f7048af. No deployment or live failure-rate measurement.