Restart bridge on failure - #5
Conversation
Review failedKANRI_PI_MODEL did not match an available model: opencode-go/kimi-k3 Walkthrough by kanri |
📝 WalkthroughWalkthroughThe bridge classifies opaque and stale SDK failures, preserves agent IDs for resume attempts, handles timeout recovery, schedules guarded restarts, and exposes classifiers for tests. ChangesOpaque SDK failure recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentSDK
participant LocalBridge
participant AgentCache
participant BridgeProcess
participant Server
participant HealthEndpoint
AgentSDK-->>LocalBridge: opaque or stale run failure
LocalBridge->>AgentCache: evict and preserve agent ID
LocalBridge->>AgentSDK: resume agent or create replacement
LocalBridge->>LocalBridge: count failures
LocalBridge->>BridgeProcess: close and exit at threshold
BridgeProcess-->>Server: unexpected exit
Server->>BridgeProcess: respawn after delay
Server->>HealthEndpoint: wait for /health
HealthEndpoint-->>Server: health result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/cursor-sdk-local-agent-bridge.mjs (3)
2691-2701: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCounter only resets on auth errors, not on other non-opaque failures.
consecutiveOpaqueFailuresaccumulates across any opaque failure and is only reset bynoteSdkRunSuccess()or an authentication error. A retryable-but-not-opaque failure (e.g. capacity/rate-limit) in between two opaque failures doesn't reset the count, so genuinely non-consecutive opaque incidents can still add up toward the restart threshold. Low impact with the default threshold of1, but undermines the "consecutive" semantics ifCURSOR_SDK_BRIDGE_OPAQUE_FAILURE_RESTARTis raised.♻️ Proposed fix
function noteSdkRunFailure(error) { if (isOpaqueSDKRunFailure(error)) { consecutiveOpaqueFailures += 1 if (consecutiveOpaqueFailures < opaqueFailureRestartThreshold) return scheduleBridgeRestart(...) return } - if (isAuthenticationSDKError(error)) consecutiveOpaqueFailures = 0 + consecutiveOpaqueFailures = 0 }🤖 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 `@scripts/cursor-sdk-local-agent-bridge.mjs` around lines 2691 - 2701, Update noteSdkRunFailure so every non-opaque SDK failure resets consecutiveOpaqueFailures, while preserving the existing opaque-failure increment and restart logic. Keep authentication handling consistent with this reset behavior so only uninterrupted opaque failures contribute toward opaqueFailureRestartThreshold.
2675-2711: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for
noteSdkRunFailure/scheduleBridgeRestartthreshold and restart-trigger logic.Only
isOpaqueSDKRunFailureis unit tested. The threshold accumulation, single-shotbridgeRestartScheduledguard, and reset-on-success/auth behavior innoteSdkRunFailure/scheduleBridgeRestartare core to this PR's objective and currently untested.🤖 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 `@scripts/cursor-sdk-local-agent-bridge.mjs` around lines 2675 - 2711, Add tests covering noteSdkRunFailure and scheduleBridgeRestart: verify opaque failures accumulate without restarting below opaqueFailureRestartThreshold, trigger exactly one restart at the threshold despite subsequent failures, and do not schedule outside the main module. Also verify noteSdkRunSuccess and authentication failures reset consecutiveOpaqueFailures, while preserving existing non-opaque failure behavior.
2703-2711: 🧹 Nitpick | 🔵 TrivialRestart blast radius: any consecutive-opaque threshold hit kills all cached agents, not just the failing one.
closeAndExit(1)tears down the whole bridge process, so every other in-flight/cached agent session (unrelatedcacheKeys) is dropped along with the one that actually failed. With the default threshold of1, a single opaque failure on any session restarts the bridge for everyone. This is likely intentional per the PR's goal, but worth confirming this trade-off is acceptable for concurrent multi-session usage, and consider logging a metric/count for how often this fires in practice.🤖 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 `@scripts/cursor-sdk-local-agent-bridge.mjs` around lines 2703 - 2711, Update scheduleBridgeRestart and its callers to avoid terminating the entire bridge when a consecutive-opaque threshold is reached for one cacheKey; isolate recovery to the failing agent session while preserving normal restart behavior for unrecoverable bridge-wide failures. If bridge-wide termination remains required, add a metric or counter and include it in the existing restart logging to quantify these events.scripts/__tests__/cursor-sdk-local-agent-bridge.test.mjs (1)
67-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the three main branches; consider adding the remaining classifier branches.
Not exercised: the
error.code && error.code !== "cursor_sdk_error"early-return, and the case wheresummary.codeis present whilemessage/statusdon't otherwise disqualify it. Since this classifier gates bridge restarts, a bit more branch coverage would add confidence.🤖 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 `@scripts/__tests__/cursor-sdk-local-agent-bridge.test.mjs` around lines 67 - 96, The classifier tests around isOpaqueSDKRunFailure should cover the remaining branches: add an error with a non-cursor_sdk_error code to verify the early false return, and add an otherwise-eligible error whose summary.code is populated while message and status do not disqualify it to verify the expected classification.
🤖 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 `@scripts/cursor-sdk-local-agent-bridge.mjs`:
- Around line 253-269: Update the final non-retryable error path in the run
catch handling to evict the cached agent before noteSdkRunFailure(error) and
rethrowing. Apply this when shouldRetry is false, preserving the existing resume
behavior based on isOpaqueSDKRunFailure(error), so exhausted retries or
already-emitted events cannot leave a broken agent in agentCache.
---
Nitpick comments:
In `@scripts/__tests__/cursor-sdk-local-agent-bridge.test.mjs`:
- Around line 67-96: The classifier tests around isOpaqueSDKRunFailure should
cover the remaining branches: add an error with a non-cursor_sdk_error code to
verify the early false return, and add an otherwise-eligible error whose
summary.code is populated while message and status do not disqualify it to
verify the expected classification.
In `@scripts/cursor-sdk-local-agent-bridge.mjs`:
- Around line 2691-2701: Update noteSdkRunFailure so every non-opaque SDK
failure resets consecutiveOpaqueFailures, while preserving the existing
opaque-failure increment and restart logic. Keep authentication handling
consistent with this reset behavior so only uninterrupted opaque failures
contribute toward opaqueFailureRestartThreshold.
- Around line 2675-2711: Add tests covering noteSdkRunFailure and
scheduleBridgeRestart: verify opaque failures accumulate without restarting
below opaqueFailureRestartThreshold, trigger exactly one restart at the
threshold despite subsequent failures, and do not schedule outside the main
module. Also verify noteSdkRunSuccess and authentication failures reset
consecutiveOpaqueFailures, while preserving existing non-opaque failure
behavior.
- Around line 2703-2711: Update scheduleBridgeRestart and its callers to avoid
terminating the entire bridge when a consecutive-opaque threshold is reached for
one cacheKey; isolate recovery to the failing agent session while preserving
normal restart behavior for unrecoverable bridge-wide failures. If bridge-wide
termination remains required, add a metric or counter and include it in the
existing restart logging to quantify these events.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a262da6d-34d9-4f61-938e-ce1c64f8f7f9
📒 Files selected for processing (3)
scripts/__tests__/cursor-sdk-local-agent-bridge.test.mjsscripts/cursor-sdk-local-agent-bridge.mjsserver.ts
Format stale-auth tests for oxfmt and reset opaque-failure counters on any non-opaque error so exhausted retries cannot reuse broken cached agents.
1 participant
https://forum.cursor.com/t/cursor-sdk-1-0-22-local-agent-returns-bare-status-error-after-idle-process-restart-fixes-it-not-quota/164866
What this PR does
This PR adds recovery logic for the Cursor SDK local-agent 'bare status=error' stuck state. It classifies opaque retryable SDK errors and stale exchanged-token auth failures, preserves the last agent ID so subsequent requests try
Agent.resumebefore falling back toAgent.create, and triggers a configurable bridge process restart after repeated opaque failures. The server now waits for the bridge/healthendpoint to be healthy before considering a restart successful.Summary by CodeRabbit