Repository navigation
feat(proxy): add zero-downtime rolling workers - #1209
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a macOS rolling proxy supervisor with generation-based socket workers, stable-listener handoff, recovery and rollback handling, expanded CLI status/state integration, cross-process tests, and a rolling performance benchmark. ChangesRolling proxy worker lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProxyGuard
participant LaunchdSupervisor
participant RollingProxyServer
participant RollingWorkerSupervisor
participant SocketWorker
ProxyGuard->>LaunchdSupervisor: request rolling activation
LaunchdSupervisor->>RollingProxyServer: replace worker version
RollingProxyServer->>RollingWorkerSupervisor: start candidate
RollingWorkerSupervisor->>SocketWorker: send activation and sockets
SocketWorker-->>RollingWorkerSupervisor: report ready, activated, and drained
RollingWorkerSupervisor-->>RollingProxyServer: publish rolling snapshot
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@coderabbitai review Stacking note: PR #1208 is still open and this PR is based on its single commit. The repository policy computes the count against |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/proxy.ts`:
- Around line 4495-4496: Update the uninstall cleanup flow around
clearProxyState and clearProxySupervisorState so supervisor state is not deleted
until launchd shutdown is confirmed. Handle both the missing-plist path and
swallowed launchctl unload failures by failing the uninstall or verifying and
terminating the saved supervisor PID before clearing state.
- Around line 3839-3873: Update the rollback flow surrounding
resolveGlobalInstaller and validateInstalledVersion so that after reinstalling
runningVersion and calling writeTrampoline, it explicitly replaces the active
supervisor generation with runningVersion and waits for that generation to pass
health validation. Ensure rollback completion is reported only after the
supervisor targets and successfully serves runningVersion, including when the
prior replacement is still in flight.
In `@src/lib/proxy/rollingProxyServer.ts`:
- Around line 42-55: Update the explicit replacement flow around
supervisor.replace to cancel any pending recoveryTimer before replacing the
worker, then revalidate the desired version/state before allowing recovery
scheduling or launch to continue. Ensure the recovery timer callback and
replacement path cannot race, and prevent scheduleRecovery from starting another
generation after the requested worker is active. Preserve recoveryFailures
handling for genuine recovery failures.
In `@src/lib/proxy/rollingWorkerSupervisor.ts`:
- Around line 95-112: Replace the generic errors at all three lifecycle failure
sites with typed errors created through ErrorFactory: update replace in
src/lib/proxy/rollingWorkerSupervisor.ts lines 95-112 for closed,
invalid-version, and conflicting-replacement failures; update the worker-spawn
failure in src/lib/proxy/rollingWorkerProcess.ts lines 74-76; and update the
listener-startup failure in src/lib/proxy/rollingProxyServer.ts lines 112-116.
Preserve the existing messages and rejection behavior while using the
appropriate ErrorFactory classifications.
- Around line 440-447: Update flushQueuedSockets to capture this.active in a
local worker reference after the active check and use that same reference for
every transferSocket call while draining queuedSockets. Preserve the existing
early return and timeout cleanup, avoiding repeated reads of this.active after
sendSocket may synchronously clear it.
In `@src/lib/proxy/socketWorkerRuntime.ts`:
- Around line 259-279: The socket worker’s drain handling currently destroys
accepted-but-uncommitted sockets before their commit arrives. Update the message
handling around pendingSockets to track graceful-drain state, reject only
sockets offered after the drain request, and defer runtime.drain() until
existing pending sockets are committed or canceled; preserve normal
commit/cancel behavior and add a delayed-ack regression test covering this
sequence.
In `@test/fixtures/proxySocketWorker.cjs`:
- Around line 76-79: Update the message handler registered by
process.on("message") to destroy the transferred socket before returning when
message.generation does not match generation. Preserve the existing early-return
behavior after ensuring the mismatched-generation socket handle is closed.
In `@test/proxyRollingWorkerHandoff.test.ts`:
- Around line 558-575: Make the “replays readiness emitted before a supervisor
subscribes” test deterministic by adding an explicit synchronization signal from
the worker fixture confirming readiness was emitted, instead of relying on the
100ms sleep. Await that signal before calling handle.onMessage, while preserving
the existing timeout and readiness assertion.
- Around line 142-144: Ensure the integration tests in proxyRollingWorkerHandoff
register spawned workers, servers, and listeners in a cleanup stack as they are
created, then have afterEach close or kill every registered resource even when
assertions or timeouts fail. Preserve the existing successful cleanup behavior
while making teardown unconditional alongside vi.useRealTimers().
In `@tools/testing/proxyRollingHandoffBenchmark.ts`:
- Around line 102-119: Move the startRollingProxyServer initialization into the
try-protected scope so failures during setup reach cleanup, and update the
finally cleanup around rolling.close and direct.close to invoke both
independently with Promise.allSettled. Ensure a rejection from either close does
not prevent the other server from being closed.
- Around line 53-75: Update the request flow around the outgoing request and
response listeners to add a request timeout and reject when the response closes
or is aborted before completing. Preserve the existing end-based metrics
resolution, while ensuring timeout and premature response termination settle the
Promise as failures instead of allowing the benchmark to hang.
- Around line 124-171: Update the benchmark’s CPU and heap measurement around
the rolling path in proxyRollingHandoffBenchmark so it includes metrics from
test/fixtures/proxySocketWorker.cjs, rather than sampling only the parent
process. Aggregate worker CPU and heap usage through IPC, or execute both paths
under a shared process model, and use the combined values when calculating
cpuMicrosPerRequest and heapGrowthBytes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ab924d1-9aeb-4c71-a1fa-51713e31a323
📒 Files selected for processing (14)
package.jsonsrc/cli/commands/proxy.tssrc/lib/proxy/rollingProxyServer.tssrc/lib/proxy/rollingWorkerProcess.tssrc/lib/proxy/rollingWorkerProtocol.tssrc/lib/proxy/rollingWorkerSupervisor.tssrc/lib/proxy/socketWorkerRuntime.tssrc/lib/types/cli.tssrc/lib/types/proxy.tstest/continuous-test-suite-bugfixes.tstest/fixtures/proxySocketWorker.cjstest/proxyReliabilityHardening.test.tstest/proxyRollingWorkerHandoff.test.tstools/testing/proxyRollingHandoffBenchmark.ts
murdore
force-pushed
the
feat/proxy-rolling-worker-handoff
branch
from
July 19, 2026 04:58
099b06f to
c4b46de
Compare
murdore
force-pushed
the
feat/proxy-rolling-worker-handoff
branch
from
July 19, 2026 09:40
c4b46de to
96bdbd2
Compare
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR introduces a zero-downtime “rolling worker” architecture for the proxy: a stable supervisor keeps the public TCP listener bound while swapping serving worker processes via IPC socket transfer + activation handshake, including recovery/rollback support and richer status visibility.
Changes:
- Added rolling supervisor/worker IPC protocol, socket-worker runtime, and stable rolling proxy listener with recovery backoff.
- Integrated rolling supervision into the proxy CLI (launchd supervisor mode, updater activation via SIGUSR2, supervisor/worker status reporting, safer uninstall).
- Added extensive tests/fixtures plus a new rolling handoff performance benchmark and CI wiring.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/testing/proxyRollingHandoffBenchmark.ts | Adds benchmark covering concurrent handoff + stream preservation and overhead budgets. |
| test/proxyRollingWorkerHandoff.test.ts | Adds unit/integration coverage for supervisor lifecycle, socket transfer semantics, draining, and recovery. |
| test/proxyReliabilityHardening.test.ts | Extends invariants to ensure rolling-supervisor plumbing is present in launchd lifecycle source. |
| test/fixtures/proxySocketWorker.cjs | Adds IPC socket-worker fixture used by tests/benchmarks, including metrics logging and commit/cancel handshake. |
| test/continuous-test-suite-bugfixes.ts | Updates regression suite expectations for new activation/validation flow and logging changes. |
| src/lib/utils/errorHandling.ts | Introduces a typed error code/factory helper for rolling worker lifecycle failures. |
| src/lib/types/proxy.ts | Adds protocol + runtime + supervisor/server types for rolling worker handoff. |
| src/lib/types/cli.ts | Adds persisted supervisor state types and records supervisor PID in proxy state. |
| src/lib/proxy/socketWorkerRuntime.ts | Implements accepting transferred sockets into an HTTP server and the worker-side IPC adapter. |
| src/lib/proxy/rollingWorkerSupervisor.ts | Implements worker generation management, activation handshake, socket queueing, drain/shutdown, and failure recording. |
| src/lib/proxy/rollingWorkerProtocol.ts | Defines env flags and type guards for IPC messages. |
| src/lib/proxy/rollingWorkerProcess.ts | Implements spawning IPC workers and the socket offer/accept/commit/cancel handshake. |
| src/lib/proxy/rollingProxyServer.ts | Implements stable listener + recovery loop around the RollingWorkerSupervisor. |
| src/cli/commands/proxy.ts | Integrates supervisor mode, rolling activation, rollback activation, status output enhancements, and uninstall hardening. |
| package.json | Adds rolling benchmark script and includes it in the proxy performance aggregate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3113
to
+3117
| status.mode = state | ||
| ? state.passthrough | ||
| ? "passthrough" | ||
| : "full" | ||
| : null; |
Comment on lines
+213
to
+217
| const args = execFileSync("ps", ["-p", String(pid), "-o", "args="], { | ||
| encoding: "utf-8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }); | ||
| return /neurolink/i.test(args); |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/commands/proxy.ts (1)
1637-1639: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winLog line remains injectable despite
sanitizeForLog.sanitizeForLog(per its definition) only truncates and masks secret patterns — it does not strip\r/\n. Sincemodelis client-controlled (request body) andc.req.pathis unsanitized here, a crafted value can inject forged log lines (CWE-117).🛡️ Strip control characters from interpolated request-derived values
- const logModel = sanitizeForLog(String(model)); + const logModel = sanitizeForLog(String(model)).replace(/[\r\n]+/g, " "); logger.always( - `[proxy] ${c.req.method} ${c.req.path} → model=${logModel} ${stream} tools=${toolCount}`, + `[proxy] ${c.req.method} ${c.req.path.replace(/[\r\n]+/g, " ")} → model=${logModel} ${stream} tools=${toolCount}`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/proxy.ts` around lines 1637 - 1639, Update the logging call in the proxy command around logModel to remove carriage returns, newlines, and other control characters from every request-derived interpolated value, including model and c.req.path, before passing the message to logger.always. Preserve the existing sanitizeForLog behavior while ensuring crafted request values cannot create additional log lines.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/proxy/rollingProxyServer.ts`:
- Around line 120-127: Update the address-shape failure branch in the rolling
proxy listener setup to handle supervisor.close() before rethrowing the
ErrorFactory.proxyWorkerLifecycle error, matching the awaited cleanup behavior
of the nearby catch path and preventing a discarded rejection.
---
Outside diff comments:
In `@src/cli/commands/proxy.ts`:
- Around line 1637-1639: Update the logging call in the proxy command around
logModel to remove carriage returns, newlines, and other control characters from
every request-derived interpolated value, including model and c.req.path, before
passing the message to logger.always. Preserve the existing sanitizeForLog
behavior while ensuring crafted request values cannot create additional log
lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52f2f46b-fd7f-4513-9051-d7cb86ac217a
📒 Files selected for processing (15)
package.jsonsrc/cli/commands/proxy.tssrc/lib/proxy/rollingProxyServer.tssrc/lib/proxy/rollingWorkerProcess.tssrc/lib/proxy/rollingWorkerProtocol.tssrc/lib/proxy/rollingWorkerSupervisor.tssrc/lib/proxy/socketWorkerRuntime.tssrc/lib/types/cli.tssrc/lib/types/proxy.tssrc/lib/utils/errorHandling.tstest/continuous-test-suite-bugfixes.tstest/fixtures/proxySocketWorker.cjstest/proxyReliabilityHardening.test.tstest/proxyRollingWorkerHandoff.test.tstools/testing/proxyRollingHandoffBenchmark.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/proxy/rollingWorkerProtocol.ts
- package.json
- src/lib/types/cli.ts
- test/proxyReliabilityHardening.test.ts
- test/fixtures/proxySocketWorker.cjs
- src/lib/types/proxy.ts
- src/lib/proxy/socketWorkerRuntime.ts
- test/proxyRollingWorkerHandoff.test.ts
- test/continuous-test-suite-bugfixes.ts
- tools/testing/proxyRollingHandoffBenchmark.ts
- src/lib/proxy/rollingWorkerSupervisor.ts
Comment on lines
+120
to
+127
| const address = listener.address(); | ||
| if (!address || typeof address === "string") { | ||
| listener.close(); | ||
| void supervisor.close(); | ||
| throw ErrorFactory.proxyWorkerLifecycle( | ||
| "rolling proxy listener did not expose a TCP address", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled rejection risk on the address-shape failure path.
listener.close() is fire-and-forget and void supervisor.close(); discards its promise without a .catch, unlike the .catch block at lines 104-107 which properly awaits supervisor.close() before rethrowing. If supervisor.close() ever rejects here, it becomes an unhandled rejection on top of the thrown ErrorFactory error.
🛡️ Proposed fix
if (!address || typeof address === "string") {
- listener.close();
- void supervisor.close();
+ await new Promise<void>((resolve) => listener.close(() => resolve()));
+ await supervisor.close().catch(() => {});
throw ErrorFactory.proxyWorkerLifecycle(
"rolling proxy listener did not expose a TCP address",
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const address = listener.address(); | |
| if (!address || typeof address === "string") { | |
| listener.close(); | |
| void supervisor.close(); | |
| throw ErrorFactory.proxyWorkerLifecycle( | |
| "rolling proxy listener did not expose a TCP address", | |
| ); | |
| } | |
| const address = listener.address(); | |
| if (!address || typeof address === "string") { | |
| await new Promise<void>((resolve) => listener.close(() => resolve())); | |
| await supervisor.close().catch(() => {}); | |
| throw ErrorFactory.proxyWorkerLifecycle( | |
| "rolling proxy listener did not expose a TCP address", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/proxy/rollingProxyServer.ts` around lines 120 - 127, Update the
address-shape failure branch in the rolling proxy listener setup to handle
supervisor.close() before rethrowing the ErrorFactory.proxyWorkerLifecycle
error, matching the awaited cleanup behavior of the nearby catch path and
preventing a discarded rejection.
Contributor
|
🎉 This PR is included in version 9.94.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Jul 19, 2026
This was referenced Aug 2, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Safety and rollout
Verification
Fault coverage
Summary by CodeRabbit
New Features
Bug Fixes
Tests