Repository navigation
fix(proxy): close restartless observability gaps - #1223
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe proxy updates offline log analysis, request-log durability, environment-triggered rolling replacement, Anthropic retry telemetry, supervisor detection, and ChangesProxy observability and reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProxyRuntimeConfigStore
participant SocketWorker
participant RollingWorkerSupervisor
participant RollingProxyServer
ProxyRuntimeConfigStore->>SocketWorker: report environmentChanged
SocketWorker->>RollingWorkerSupervisor: send replacement-requested(environment)
RollingWorkerSupervisor->>RollingProxyServer: notify active generation
RollingProxyServer->>RollingWorkerSupervisor: start same-version replacement
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
🤖 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 hardens the proxy’s restartless worker lifecycle and observability pipeline, focusing on safer rolling worker replacement triggers, more durable request/attempt logging during shutdown, and richer offline log analysis output for operators.
Changes:
- Add “same-version” rolling worker replacements on
.env(non-routing) changes, with debouncing/serialization and safer cleanup. - Improve request/attempt observability: attempt logging on success paths, flush admitted request/body logs on shutdown, and surface unattributed persisted counters in status.
- Enhance
proxy analyzeto report per-stream coverage windows and body-artifact data quality without reading artifacts.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/proxyUpdaterFallback.test.ts | Asserts status output includes an unattributed internal account row when persisted counters can’t be attributed. |
| test/proxyRollingWorkerHandoff.test.ts | Adds coverage for worker-requested same-version replacements (environment change signal). |
| test/proxyReliabilityHardening.test.ts | Covers SSE preflight after OAuth refresh and validates attempt logging is emitted for success paths. |
| test/proxyObservabilityFoundation.test.ts | Verifies request-log flushing and log cleanup behavior (preserve current-day metadata, evict older artifacts). |
| test/proxyConfigHotReload.test.ts | Ensures env-only changes are detected and published as replacement-worthy (environmentChanged). |
| test/proxyAnalysis.test.ts | Adds analysis checks for per-stream observation windows and body-artifact presence/quality metrics. |
| test/continuous-test-suite-bugfixes.ts | Tightens supervisor PID validation expectations to require neurolink proxy start. |
| src/lib/types/proxy.ts | Extends proxy analysis/status types (debug stream, data-quality metrics, replacement-request message, env-change metadata). |
| src/lib/server/routes/claudeProxyRoutes.ts | Logs attempt outcomes on success paths and preflights immediate SSE errors before client commit (especially post-refresh). |
| src/lib/proxy/runtimeConfig.ts | Hashes env file content to detect env-only changes and reports environmentChanged on reload. |
| src/lib/proxy/rollingWorkerSupervisor.ts | Forwards active-worker replacement requests to the rolling server via onReplacementRequested. |
| src/lib/proxy/rollingWorkerProtocol.ts | Validates the new proxy-worker:replacement-requested status message shape. |
| src/lib/proxy/rollingProxyServer.ts | Debounces/serializes worker-requested replacements and hardens listener/supervisor cleanup paths. |
| src/lib/proxy/requestLogger.ts | Tracks pending log writes for flushing, and preserves current-day metadata during cleanup. |
| src/lib/proxy/proxyAnalysis.ts | Adds debug-stream ingestion, stream observation windows, artifact presence checks, and more accurate “coverage” semantics. |
| src/cli/commands/proxyAnalyze.ts | Prints new debug-file counts, per-stream observation windows, and body-artifact data quality summary. |
| src/cli/commands/proxy.ts | Tightens supervisor process matching, flushes request logs on shutdown, improves stale status handling, and adds unattributed stats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4b5c013 to
3b536e1
Compare
🤖 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/server/routes/claudeProxyRoutes.ts (1)
4311-4361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead stream branch from
handleAnthropicSuccessfulRetryResponse. It is only called from the!body.streampath, so theif (body.stream && retryResp.body)block and its stream-handling code are unreachable and misleading.🤖 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/server/routes/claudeProxyRoutes.ts` around lines 4311 - 4361, Remove the unreachable stream-handling branch from handleAnthropicSuccessfulRetryResponse, including the body.stream/retryResp.body condition and its associated streaming logic. Keep this helper focused on the non-stream retry path used by the response construction shown, preserving its existing non-stream response behavior.src/cli/commands/proxy.ts (1)
3671-3695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
StatusStats["accounts"]for the live/statuscast
This inline account shape is duplicated and already stale: it hard-codescooling: booleanand omitsstatus, while the sharedStatusStats["accounts"]entry already models both. Reuse the canonical type here to avoid drift.🤖 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 3671 - 3695, Update the live /status response type cast in the statusData flow to reuse the canonical StatusStats["accounts"] type for stats.accounts instead of the duplicated inline account shape. Preserve the existing stats fields while removing the stale inline account definition, including its hard-coded cooling property, so the shared type remains authoritative.
🤖 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.
Nitpick comments:
In `@src/cli/commands/proxy.ts`:
- Around line 3671-3695: Update the live /status response type cast in the
statusData flow to reuse the canonical StatusStats["accounts"] type for
stats.accounts instead of the duplicated inline account shape. Preserve the
existing stats fields while removing the stale inline account definition,
including its hard-coded cooling property, so the shared type remains
authoritative.
In `@src/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 4311-4361: Remove the unreachable stream-handling branch from
handleAnthropicSuccessfulRetryResponse, including the body.stream/retryResp.body
condition and its associated streaming logic. Keep this helper focused on the
non-stream retry path used by the response construction shown, preserving its
existing non-stream response behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 317cf5f6-899c-4116-9d0b-128dfecabde8
📒 Files selected for processing (17)
src/cli/commands/proxy.tssrc/cli/commands/proxyAnalyze.tssrc/lib/proxy/proxyAnalysis.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/rollingProxyServer.tssrc/lib/proxy/rollingWorkerProtocol.tssrc/lib/proxy/rollingWorkerSupervisor.tssrc/lib/proxy/runtimeConfig.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tstest/continuous-test-suite-bugfixes.tstest/proxyAnalysis.test.tstest/proxyConfigHotReload.test.tstest/proxyObservabilityFoundation.test.tstest/proxyReliabilityHardening.test.tstest/proxyRollingWorkerHandoff.test.tstest/proxyUpdaterFallback.test.ts
3b536e1 to
a34bd18
Compare
|
Addressed both CodeRabbit nitpicks in a34bd18: removed the unreachable auth-retry streaming branch after SSE preflight unification, and reused the canonical StatusStats type for live status parsing. The focused and full proxy tests, typecheck, and complete pre-commit pipeline pass. |
🤖 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the 17 changed files in this proxy observability/lifecycle PR. The changes are well-scoped and the test coverage is thorough, but I found 3 MAJOR issues that should be addressed before merge, plus 3 MINOR suggestions.
Blocking issues (MAJOR)
-
src/lib/server/routes/claudeProxyRoutes.ts—handleAnthropicSuccessfulRetryResponselogslogAttempt(retryResp.status)withoutattemptDurationMs, while the streaming retry path and main success paths do include it. This leaves a real observability gap in the "JSON, streaming, and auth-retry flows" consistency the PR targets. -
src/lib/proxy/rollingProxyServer.ts— TherequestedReplacementRunningguard is set inside the asyncsetTimeoutcallback, not before it. Between the timer firing and the flag being set, a concurrentreplace()call can observerequestedReplacementRunning === false, skip awaiting, and invokesupervisor.replace()concurrently. The supervisor serializes the underlying spawn, but the rolling-server state (requestedReplacementPromise,requestedReplacementRunning) ends up tracking the wrong replacement. -
src/lib/proxy/requestLogger.ts—flushRequestLogs()can hang indefinitely if a trackedappendFile/writeFilepromise never settles (stuck NFS, broken pipe, etc.). Thewhileloop keeps awaitingPromise.allSettled([...pendingLogOperations])without progress. The shutdown wrapper times out, but the underlying hung promise remains and can prevent clean process exit.
Non-blocking suggestions (MINOR)
rollingProxyServer.ts:clearTimeoutdoes not remove a timer callback that has already been queued by the event loop, creating another race window between scheduled and explicit replacements.proxyAnalysis.ts: the artifact path-traversal guard is reasonable but could be more robust;writeFailuresis incremented whenbodyPathis absent, which may represent an intentional redaction-only capture rather than a write failure.runtimeConfig.ts:environmentChangedis only computed after the early-return branch; consider computing it earlier so achanged:falsereload result can still report it.
Existing review comments
The two prior Copilot comments (lifecycle log matcher, StatusStats.accounts[].cooling required) appear resolved in the latest commit and are not re-raised.
Next steps
Please address the three MAJOR issues above. The replacement-serialization race and the flush hang are the highest-risk for production reliability.
a34bd18 to
5b8c23b
Compare
|
Fresh read-only live evidence exposed one additional handoff gap: the active v10.3.0 worker lost four socket transfers, the supervisor recovered to generation 2, but the 5-second acknowledgement threshold killed the worker and successful recovery cleared the root-cause field. Commit 5b8c23b raises the production acknowledgement deadline to 30 seconds, logs exact transfer/exit details (including code/signal), retains lastFailure across successful recovery, and keeps ambiguous transferred sockets non-replayable. Revalidated: 175 proxy tests, 234 bugfix tests, typecheck, ESLint, CLI build, full pre-commit, and all performance budgets. |
🤖 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.
Actionable comments posted: 1
🤖 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/server/routes/claudeProxyRoutes.ts`:
- Around line 4029-4031: Update the non-streaming retry path around
handleAnthropicAuthRetry so its logAttempt call includes the retry attempt
duration. Modify retryLogAttempt to preserve extra.attemptDurationMs when
provided, falling back to elapsed time from retryAttemptStartedAt only when
absent, and retain the attempt number. Ensure this helper remains restricted to
non-streaming retries, either by renaming it to reflect that scope or asserting
!body.stream.
🪄 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: 21170c3b-4ca0-4601-aec7-b4cfc3fbbf89
📒 Files selected for processing (18)
src/cli/commands/proxy.tssrc/cli/commands/proxyAnalyze.tssrc/lib/proxy/proxyAnalysis.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/rollingProxyServer.tssrc/lib/proxy/rollingWorkerProcess.tssrc/lib/proxy/rollingWorkerProtocol.tssrc/lib/proxy/rollingWorkerSupervisor.tssrc/lib/proxy/runtimeConfig.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tstest/continuous-test-suite-bugfixes.tstest/proxyAnalysis.test.tstest/proxyConfigHotReload.test.tstest/proxyObservabilityFoundation.test.tstest/proxyReliabilityHardening.test.tstest/proxyRollingWorkerHandoff.test.tstest/proxyUpdaterFallback.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- test/proxyConfigHotReload.test.ts
- test/proxyUpdaterFallback.test.ts
- test/proxyRollingWorkerHandoff.test.ts
- src/lib/proxy/rollingWorkerProtocol.ts
- src/lib/proxy/runtimeConfig.ts
- src/cli/commands/proxyAnalyze.ts
- test/proxyObservabilityFoundation.test.ts
- src/lib/types/proxy.ts
- src/lib/proxy/rollingProxyServer.ts
- src/lib/proxy/requestLogger.ts
- src/cli/commands/proxy.ts
- test/proxyReliabilityHardening.test.ts
- test/proxyAnalysis.test.ts
- src/lib/proxy/proxyAnalysis.ts
5b8c23b to
637e098
Compare
🤖 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 |
637e098 to
b6b404f
Compare
🤖 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/proxy/runtimeConfig.ts (1)
337-340: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winenvFileHash hashes raw file bytes, not parsed vars — cosmetic
.envedits trigger real worker replacement.
envFileHashis derived from the raw file content (envFileContent) rather than the parsed key/value pairs. A whitespace-only or comment-only edit to.envwill therefore still flip the hash, setenvironmentChanged: true, and (per the downstream consumer inproxy.ts) trigger an actual rolling worker replacement even though no effective environment variable changed.Consider hashing a normalized, sorted representation of the parsed env vars (e.g.
JSON.stringify(Object.entries(parse(envFileContent)).sort())) instead of the raw content, so only semantically meaningful changes trigger a replacement.♻️ Proposed fix
- const envFileHash = createHash("sha256") - .update(envFilePresent ? envFileContent : "[missing]") - .digest("hex") - .slice(0, 16); + const envFileHash = createHash("sha256") + .update( + envFilePresent + ? JSON.stringify( + Object.entries(effectiveEnv).sort(([a], [b]) => a.localeCompare(b)), + ) + : "[missing]", + ) + .digest("hex") + .slice(0, 16);🤖 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/runtimeConfig.ts` around lines 337 - 340, Update the envFileHash calculation in the runtime configuration flow to hash a normalized, deterministically sorted representation of the parsed environment variables rather than raw envFileContent. Reuse the existing env parsing logic, ensuring whitespace-only and comment-only edits produce the same hash while actual key/value changes still update environmentChanged.
🤖 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 106-144: In scheduleRequestedReplacement, move the
requestedReplacementPending = false assignment to after the closing or
inactive-worker guard. Preserve the flag when the timer fires without an active
worker so queueReplacement’s recovery/self-heal path can retry the pending
environment replacement.
---
Nitpick comments:
In `@src/lib/proxy/runtimeConfig.ts`:
- Around line 337-340: Update the envFileHash calculation in the runtime
configuration flow to hash a normalized, deterministically sorted representation
of the parsed environment variables rather than raw envFileContent. Reuse the
existing env parsing logic, ensuring whitespace-only and comment-only edits
produce the same hash while actual key/value changes still update
environmentChanged.
🪄 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: b2726221-d9e9-4d43-af64-0b03afe8c7ce
📒 Files selected for processing (18)
src/cli/commands/proxy.tssrc/cli/commands/proxyAnalyze.tssrc/lib/proxy/proxyAnalysis.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/rollingProxyServer.tssrc/lib/proxy/rollingWorkerProcess.tssrc/lib/proxy/rollingWorkerProtocol.tssrc/lib/proxy/rollingWorkerSupervisor.tssrc/lib/proxy/runtimeConfig.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tstest/continuous-test-suite-bugfixes.tstest/proxyAnalysis.test.tstest/proxyConfigHotReload.test.tstest/proxyObservabilityFoundation.test.tstest/proxyReliabilityHardening.test.tstest/proxyRollingWorkerHandoff.test.tstest/proxyUpdaterFallback.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/lib/proxy/rollingWorkerProtocol.ts
- src/cli/commands/proxyAnalyze.ts
- test/proxyConfigHotReload.test.ts
- src/lib/types/proxy.ts
- test/proxyObservabilityFoundation.test.ts
- test/continuous-test-suite-bugfixes.ts
- test/proxyAnalysis.test.ts
- src/lib/proxy/rollingWorkerSupervisor.ts
- src/lib/proxy/requestLogger.ts
- src/lib/server/routes/claudeProxyRoutes.ts
- test/proxyReliabilityHardening.test.ts
- src/cli/commands/proxy.ts
b6b404f to
34c61af
Compare
🤖 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 |
34c61af to
2412be2
Compare
|
Live evidence captured after the released 10.3.0 worker rotated:
Commit 2412be2 guards both parent and worker copies of pre-commit transferred sockets. A client reset now removes/destroys the pending descriptor instead of terminating either process. Coverage includes a deterministic pending-socket The same commit also preserves an environment-triggered replacement if the active worker exits during the debounce window, and hashes parsed/sorted env values so comment- or whitespace-only edits do not roll a worker. Validation: proxy suites 181/181, bugfix suite 234/234, typecheck, ESLint, CLI/package/browser builds, publint, security validation, and all proxy performance budgets. No live model request was sent and no additional live restart was performed during this investigation. |
🤖 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the proxy observability/lifecycle changes in this PR. The previously raised blocking issues have been addressed in the latest commits:
- Non-stream OAuth retry duration:
handleAnthropicSuccessfulNonStreamRetryResponsenow logsattemptDurationMs, andretryLogAttemptpreserves an explicitly passed duration. - Rolling replacement serialization:
rollingProxyServer.tsuses a sharedqueueReplacementserializer and defers clearingrequestedReplacementPendinguntil after the active-worker guard. - Request-log flush deadline:
flushRequestLogsnow enforces a bounded deadline, drops timed-out tracked operations, and uses abortablewriteFilecalls. - Artifact path containment:
proxyAnalysis.tsusespath.relativeplus absolute/parent checks instead of a simple prefix match. - Env-hash normalization:
runtimeConfig.tshashes parsed, sorted env values so comment/whitespace edits do not trigger worker replacement.
No new blocking issues were found in this run. The test coverage is thorough (rolling handoff, observability foundation, reliability hardening, config hot-reload, and analysis suites), and the changes stay within the proxy subsystem without affecting the public SDK API.
Files reviewed: 20 changed files (focused on src/cli/commands/proxy.ts, src/lib/proxy/*, src/lib/server/routes/claudeProxyRoutes.ts, src/lib/types/proxy.ts, and test updates).
New issues raised this run: 0
Approving.
2412be2 to
c347f31
Compare
🤖 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.
🧹 Nitpick comments (1)
src/lib/proxy/rollingWorkerProcess.ts (1)
143-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ErrorFactoryfor the transfer-exit error.This new lifecycle error bypasses the repository’s typed-error contract. Use an existing lifecycle factory or add a dedicated factory case so callers can reliably classify the failure.
🤖 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/rollingWorkerProcess.ts` around lines 143 - 149, Update the child.once("exit") handler to create the socket-transfer failure through ErrorFactory instead of directly constructing Error. Reuse an existing lifecycle factory case when applicable; otherwise add a dedicated factory case that preserves the exit code and signal context and allows callers to classify the error reliably.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/lib/proxy/rollingWorkerProcess.ts`:
- Around line 143-149: Update the child.once("exit") handler to create the
socket-transfer failure through ErrorFactory instead of directly constructing
Error. Reuse an existing lifecycle factory case when applicable; otherwise add a
dedicated factory case that preserves the exit code and signal context and
allows callers to classify the error reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8fed8fd-e82e-442b-b796-89a052896221
📒 Files selected for processing (20)
src/cli/commands/proxy.tssrc/cli/commands/proxyAnalyze.tssrc/lib/proxy/proxyAnalysis.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/rollingProxyServer.tssrc/lib/proxy/rollingWorkerProcess.tssrc/lib/proxy/rollingWorkerProtocol.tssrc/lib/proxy/rollingWorkerSupervisor.tssrc/lib/proxy/runtimeConfig.tssrc/lib/proxy/socketWorkerRuntime.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tstest/continuous-test-suite-bugfixes.tstest/fixtures/proxySocketWorker.cjstest/proxyAnalysis.test.tstest/proxyConfigHotReload.test.tstest/proxyObservabilityFoundation.test.tstest/proxyReliabilityHardening.test.tstest/proxyRollingWorkerHandoff.test.tstest/proxyUpdaterFallback.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- test/proxyObservabilityFoundation.test.ts
- src/lib/proxy/rollingWorkerProtocol.ts
- test/proxyUpdaterFallback.test.ts
- test/continuous-test-suite-bugfixes.ts
- src/lib/types/proxy.ts
- src/lib/proxy/rollingProxyServer.ts
- test/proxyAnalysis.test.ts
- src/lib/proxy/runtimeConfig.ts
- src/lib/proxy/proxyAnalysis.ts
- src/cli/commands/proxy.ts
- src/lib/proxy/requestLogger.ts
- test/proxyReliabilityHardening.test.ts
- src/lib/server/routes/claudeProxyRoutes.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the proxy restartless-observability changes. The PR addresses real gaps, but I found 1 CRITICAL and 5 MAJOR new issues that need to be fixed before merge.
New issues raised
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 1 | src/lib/proxy/rollingWorkerProcess.ts |
| 5 | src/lib/proxy/requestLogger.ts, src/lib/proxy/proxyAnalysis.ts, src/lib/proxy/rollingProxyServer.ts, src/lib/server/routes/claudeProxyRoutes.ts, src/lib/proxy/socketWorkerRuntime.ts |
|
| 💡 MINOR | 0 | — |
| 💬 SUGGESTION | 0 | — |
Blocking reasons
- CRITICAL correctness bug in socket handoff (
rollingWorkerProcess.ts): the supervisor can sendsocket-cancelto a worker after the worker has already accepted and committed the socket, which may destroy a live connection. - ≥3 MAJOR issues: shutdown log-flush can drop newly-admitted operations; the offline analyzer's path containment is bypassable via symlinks; recovery backoff state is not reset on successful explicit replacements; pending socket event listeners leak after commit.
Next steps
Please address the inline comments and re-request review. Focus first on the CRITICAL socket-cancel race and the MAJOR shutdown-flush / path-traversal issues, as those have the highest operational/security impact. The existing resolved review threads were not re-raised.
c347f31 to
eb3feca
Compare
🤖 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 |
|
Final review update on eb3feca (rebased onto current origin/release v10.4.2):
Verification on the final rebased commit:
All review threads are answered and resolved. Fresh CI and Yama re-review are running. |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the proxy restartless-observability changes in PR #1223. This is a final verification pass over the latest commit (eb3feca4).
Previously raised blocking issues — verified fixed
- CRITICAL socket-cancel race (
src/lib/proxy/rollingWorkerProcess.ts):socket-cancelis now sent only while!pending.accepted, so a late parent timeout cannot destroy a socket the worker already owns. - MAJOR worker-side listener leak (
src/lib/proxy/socketWorkerRuntime.ts): pending pre-commiterror/closelisteners are now removed on commit/cancel/drain/shutdown, while unrelated server listeners are preserved. - MAJOR shutdown log-flush accounting (
src/lib/proxy/requestLogger.ts):flushRequestLogsnow snapshots admitted operations and only settles/removes that snapshot; operations admitted mid-flush remain queued for the next flush instead of being silently dropped. - MAJOR offline analyzer path traversal (
src/lib/proxy/proxyAnalysis.ts): artifact paths now reject NUL bytes and..components lexically, then comparerealpath-resolved paths against the canonical bodies root so symlink escapes are excluded. - MAJOR replacement serialization (
src/lib/proxy/rollingProxyServer.ts): environment, explicit, and recovery replacements share thequeueReplacementserializer; pending timer callbacks carry a monotonic schedule token and are invalidated by explicit replacement/close. - MAJOR non-stream OAuth retry duration (
src/lib/server/routes/claudeProxyRoutes.ts):handleAnthropicSuccessfulNonStreamRetryResponselogsattemptDurationMs, andretryLogAttemptpreserves an explicitly passed duration. - MINOR env-hash normalization (
src/lib/proxy/runtimeConfig.ts):envFileHashnow hashes parsed, sorted key/value pairs so comment/whitespace edits do not trigger worker replacement.
New issues raised this run
0 — no new blocking or major issues found in the current commit.
Verification performed
- Re-read the changed source files at
refs/pull/1223/head. - Cross-checked the fixes against the resolved review threads.
- Confirmed test coverage additions in
test/proxyRollingWorkerHandoff.test.ts,test/proxyObservabilityFoundation.test.ts,test/proxyReliabilityHardening.test.ts,test/proxyAnalysis.test.ts, andtest/proxyConfigHotReload.test.ts.
The changes stay within the proxy subsystem, do not touch the public SDK API, and the previously identified blocking concerns are addressed. Approving.
|
🎉 This PR is included in version 10.4.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
.envchanges while preserving the public listener and draining existing streamsECONNRESETcannot crash the supervisor or worker.envrewritesproxy analyzeValidation
pnpm exec vitest run test/proxyAnalysis.test.ts test/proxyConfigHotReload.test.ts test/proxyObservabilityFoundation.test.ts test/proxyReliabilityHardening.test.ts test/proxyReplay.test.ts test/proxyRollingWorkerHandoff.test.ts test/proxyUpdaterFallback.test.ts test/proxyUsageStatsPersistence.test.ts(181 passed)pnpm run test:bugfixes(234 passed)pnpm run typecheckpnpm run build:clipnpm run proxy:performance(all budgets passed; 0 dropped handoff requests; active stream preserved)proxy analyze --helpsmoke testLive safety
No repository test sent model traffic or used the live proxy as a development target.
Summary by CodeRabbit
environmentChanged./statusnow improves serving-state details and per-account attribution, including an “unattributed” entry.