fix(server): isolate provider command lanes by thread - #7071
fix(server): isolate provider command lanes by thread#7071clintebbesen wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bf77599. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces significant concurrency changes by switching from a single worker queue to per-thread isolated lanes. There is also an unresolved High severity finding about a race condition in lane creation that could break the intended per-key FIFO ordering. You can customize Macroscope's approvability policy. Learn more. |
| const enqueue = (item: A): Effect.Effect<void> => | ||
| Effect.gen(function* () { | ||
| const key = keyOf(item); | ||
| const existing = entries.get(key); |
There was a problem hiding this comment.
🟠 High src/DrainableWorker.ts:65
Concurrent first enqueues for the same key create separate workers, so their items are processed concurrently instead of in per-key FIFO order; the later entries.set also hides the first lane from drain and idle cleanup. Serialize the keyed lookup, lane creation, and initial enqueue (or reserve the key before yielding) so only one lane can be created per key.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/shared/src/DrainableWorker.ts around line 65:
Concurrent first enqueues for the same key create separate workers, so their items are processed concurrently instead of in per-key FIFO order; the later `entries.set` also hides the first lane from `drain` and idle cleanup. Serialize the keyed lookup, lane creation, and initial enqueue (or reserve the key before yielding) so only one lane can be created per key.

Fixes #6517.
ProviderCommandReactor used one global DrainableWorker for lifecycle, turn, response, and stop commands. A provider start/restart that never resolves therefore prevented unrelated threads from starting and left their statuses blank or stuck on Connecting.
This change extends the existing DrainableWorker owner with keyed FIFO lanes. Commands for different threads now process independently; commands for the same thread retain order. Idle lanes remove themselves after draining so the reactor does not retain a queue and fiber for every historical thread.
Focused regression coverage proves independent progress for different keys and FIFO ordering for the same key. The existing ProviderCommandReactor test file still passes (47 tests); the shared worker tests pass (2 tests). Server/shared typechecks, targeted lint, formatting, and
done-check --base upstream/main --no-testspassed. Tests were run in the isolated worktree; no live T3 state was used.Scope disposition: this PR fixes the cross-thread scheduling lockout only. It does not duplicate the still-open interrupt responsiveness work in #6531, nor fold in the separately scoped lifecycle timeout/stale-session/restart-recovery work tracked by #6560, #4944, and #4584. The Codex framing defect remains separate in #5389 and fork coordination issue clintebbesen#1.
Verification environment: Codex harness, gpt-5.6-sol.
Note
Medium Risk
Changes orchestration concurrency for provider lifecycle/turn commands; same-thread ordering is preserved but cross-thread behavior is no longer globally serialized.
Overview
Fixes cross-thread lockout where a single global provider command queue let one thread’s stuck start/restart block every other thread’s lifecycle and turn handling.
Adds
makeKeyedDrainableWorkerin sharedDrainableWorker: one FIFO drainable worker per key, concurrent across keys, FIFO within a key, with idle lanes torn down after drain so historical threads don’t leak queues/fibers.ProviderCommandReactornow keys lanes onevent.payload.threadIdinstead ofmakeDrainableWorker.Regression test covers independent progress for different keys and FIFO for the same key.
Reviewed by Cursor Bugbot for commit 4822cc3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Isolate provider command processing into per-thread FIFO lanes
makeKeyedDrainableWorkerto DrainableWorker.ts, which maintains a per-keymakeDrainableWorkerlane, creating and cleaning up lanes dynamically as work arrives and drains.event.payload.threadId, so events on different threads are processed concurrently while preserving FIFO order within each thread.📊 Macroscope summarized 4822cc3. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.