feat: Add Bounded Concurrent BYOM Workspace Leases - #167
Conversation
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. |
|
@codex review Please review the current PR head e7abf6d. Confirm this exact reviewed commit. This is the second and final requested cycle. The update fixes the legacy heartbeat regression, bounds cleanup handoff waits on cancellation/deadline, shortens aggregate lease expiry after slot release, and covers allocation failures before native dispatch. Real Redis + HTTP + native SRT verification passed concurrent commands and executor-death isolation. Please focus on lifecycle ownership and mixed-version safety. |
danny-avila
changed the title
|
Self-review follow-up pushed as e3348dc: enforce the local Code API replica ceiling again at dispatch/lease consumption, test drain-before-capacity-change and successful local-cleanup handoff, and remove unrelated formatting churn. Focused service tests (12), worker handoff/negotiation tests (5), and typechecks pass. The real Redis + HTTP + native SRT rerun passed two concurrent commands (~1.53s), a 12-command queued burst with same-root exclusion, injected pre-assignment lease transport recovery, and executor-death quarantine isolation. Two review requests have already been made, so I am not triggering a third. The in-progress requested review targets e7abf6d, not this follow-up 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: e7abf6d603
ℹ️ 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 Please review the exact current head 4882def and confirm the reviewed commit. The review cap has been lifted. Previous findings are addressed; especially scrutinize independent post-settlement quarantine, receipt lifetime/ownership, reset replay protection, concurrent cleanup and mixed-version behavior. Focused tests, live Redis/HTTP/native-SRT fault injection and CI tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4882def260
ℹ️ 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 Please review the current PR head 760ab7f. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. This round makes result finalization plus local cleanup a two-phase root release, bounds local cleanup, retries ownership receipts while isolating lane-local transport failures, and keeps reset-only registration unready. Focused transition/failure-injection tests and real Redis/HTTP/native-SRT canaries pass. |
|
Additional live verification on unchanged head 760ab7f: actual Redis + HTTP bridge + native SRT passed a post-unlink cleanup hang (five-second production bound), retaining the affected root fence while a subsequent healthy-root command succeeded. Independent one-second commands overlapped (~1.52 seconds total), and a 12-command burst preserved same-root exclusion. Process-death and post-unlink exception canaries also passed. All CI checks are now green. These were isolated local tests on an ephemeral HTTP port and Redis Unix socket, not deployment acceptance tests. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 760ab7f3ff
ℹ️ 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 Please review the current PR head 51540fe. Confirm this exact reviewed commit and ignore findings applying only to earlier heads. Both latest P1s are fixed: every advertised slot keeps polling after lane-local receipt failures, and identical rejected-settlement retries finish pending root finalization. Regressions cover every quarantine response lost after server commit, sustained transport outage without re-executing the failed command, and finalization failure after the dispatcher exits. 17 focused service tests and both typechecks pass. Latest main (#168 scratch hardening) is merged; real Redis/HTTP/native-SRT verification passes with all quarantine responses dropped, same-root serialization, parallel roots, and healthy-root continuation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51540fe8fe
ℹ️ 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 Please review exact current head 6ebad00 and confirm the reviewed commit. All four latest findings are addressed: mapped guards gate serial readiness (including fallback negotiation); clean-rejection receipt failures use lane-local quarantine handling; known-clean executor failures retire the child without command replay; Compose forwards slot ceilings to both API consumers. Focused tests (13 package, 7 service), Compose render checks, typechecks, and the real Redis/HTTP/native-SRT lost-quarantine-response canary pass. Please audit remaining transition/rollout invariants, ignoring findings that apply only to old heads. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
1 participant
Summary
I added opt-in bounded concurrency for independent BYOM native workspace roots, while retaining serial compatibility and workspace-specific failure containment.
--workspace id=pathregistration, reject overlapping roots, and bound the persistent native executor cache.This change is confined to Code API and
@librechat/code. LibreChat's workspace selection work is independent; callers must preserve the registered workspace IDs. Concurrent Docker/runtime sessions and same-root file-level parallelism are not enabled.Change Type
Testing
Test Configuration
Set
CODEAPI_BRIDGE_MAX_WORKSPACE_LEASE_SLOTS=2on all API replicas and start the updated worker with--workspace-lease-slots 2, a primary directory, and an additional non-overlapping--workspace second=/path/to/second. Defaults remain serial. Existing pairing, approvals, and sandbox network configuration remain required.Checklist