Repository navigation
fix(sentry): stop reporting remote connector tools/list refresh noise - #932
Conversation
tools/list timeouts and disconnects during snapshot refresh are expected home-connector lifecycle noise (same class as websocket closes in #916). Keep the soft-fail clear + ops console.warn; reserve Sentry for auth and handler bugs. Refs KODY-CLOUDFLARE-24.
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemote connector tool snapshot refreshes now classify failures centrally. Expected connector failures warn, clear cached tools, and persist state; malformed or unexpected responses propagate to Sentry. Tests cover both outcomes during ChangesRemote connector snapshot failure handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteConnectorSession
participant WebSocket
participant SessionState
participant Sentry
RemoteConnectorSession->>WebSocket: Request tools/list snapshot
WebSocket-->>RemoteConnectorSession: Return refresh result
alt Expected connector failure
RemoteConnectorSession->>SessionState: Clear tools and persist state
else Malformed or unexpected response
RemoteConnectorSession->>Sentry: Capture session error
end
Possibly related PRs
🚥 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 |
|
🔎 Preview deployed: https://kody-pr-932.kody-a99.workers.dev Worker: Mocks:
|
![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
🤖 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 `@packages/worker/src/remote-connector/session.ts`:
- Around line 506-510: Update refreshToolsSnapshot() and its callers, including
logToolsSnapshotRefreshFailure, to distinguish expected lifecycle or log-timeout
failures from protocol/runtime failures. Preserve warning-only handling for
expected failures, but rethrow or capture malformed tools/list responses and
other implementation errors through the existing Sentry path instead of
swallowing them or clearing tools.
🪄 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 Plus
Run ID: 06fc2ee4-a789-465a-9df6-fa9f136d242b
📒 Files selected for processing (2)
packages/worker/src/remote-connector/session.node.test.tspackages/worker/src/remote-connector/session.ts
Address review: only suppress expected lifecycle noise (timeout, disconnect, not connected, connector RPC error). Malformed tools/list payloads and other implementation errors still rethrow into the message-handler Sentry path and do not clear a still-valid tool cache.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/remote-connector/session.ts (1)
586-591: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate all
tools/listresult shapes before replacing the cache.
nullreaches Sentry, butresult: "bad"evaluatesresult.toolsasundefinedand persists[];{ tools: {} }persists an invalid cache. Validate that the result is a non-null object and that optionaltoolsis an array, then throw so malformed payloads preserve the valid cache and use the existing Sentry path. This is a remaining instance of the previously reported malformed-payload issue.Proposed fix
- const result = response.result as { - tools?: Array<RemoteConnectorSnapshot['tools'][number]> - } - this.stateSnapshot.tools = result.tools ?? [] + const result = response.result + if (result === null || typeof result !== 'object') { + throw new Error('Malformed tools/list result.') + } + const tools = (result as { tools?: unknown }).tools + if (tools !== undefined && !Array.isArray(tools)) { + throw new Error('Malformed tools/list tools.') + } + this.stateSnapshot.tools = + (tools as Array<RemoteConnectorSnapshot['tools'][number]> | undefined) ?? + []🤖 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 `@packages/worker/src/remote-connector/session.ts` around lines 586 - 591, Validate the tools/list payload before mutating state in the response handling flow: ensure result is a non-null object and, when present, result.tools is an array. Throw on malformed values so the existing Sentry path runs and the previously persisted valid cache remains unchanged; only assign stateSnapshot.tools and persist state after validation succeeds.
🤖 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.
Outside diff comments:
In `@packages/worker/src/remote-connector/session.ts`:
- Around line 586-591: Validate the tools/list payload before mutating state in
the response handling flow: ensure result is a non-null object and, when
present, result.tools is an array. Throw on malformed values so the existing
Sentry path runs and the previously persisted valid cache remains unchanged;
only assign stateSnapshot.tools and persist state after validation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fbdaa569-bb9a-4fbe-bc55-18d1c45cc7c0
📒 Files selected for processing (2)
packages/worker/src/remote-connector/session.node.test.tspackages/worker/src/remote-connector/session.ts
Reject non-object results and non-array tools so malformed payloads keep the prior tool cache and surface through the message-handler Sentry path.
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 (2)
packages/worker/src/remote-connector/session.ts (2)
572-576: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the cache when soft-fail persistence fails.
The handler clears
this.stateSnapshot.toolsbeforepersistState(). If that storage write fails, the outer Sentry path runs but the valid in-memory cache has already been lost, so subsequent reads return no tools until state is rehydrated.Suggested fix
+const previousTools = this.stateSnapshot.tools this.stateSnapshot.tools = [] console.warn(...) -await this.persistState() +try { + await this.persistState() +} catch (error) { + this.stateSnapshot.tools = previousTools + throw error +}🤖 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 `@packages/worker/src/remote-connector/session.ts` around lines 572 - 576, Update the soft-fail handler around this.stateSnapshot.tools and persistState() so it preserves the existing tools cache, restores it if persistState() fails, and then allows the existing outer Sentry error path to run. Keep the cache cleared only when persistence succeeds, using the handler’s current stateSnapshot and persistState symbols.
30-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClassify in-flight disconnects as expected failures.
handleDisconnect()rejects pending RPCs withRemote connector websocket closed ... before RPC response.at Line 183, but this classifier only recognizes named RPC errors plus timeout/not-connected messages. Atools/listrequest interrupted by a socket close will therefore still rethrow into Sentry instead of clearing and persisting tools as intended.Use a dedicated disconnect error name/type on the rejection path rather than relying only on message matching.
🤖 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 `@packages/worker/src/remote-connector/session.ts` around lines 30 - 46, The tools snapshot failure classifier must recognize in-flight disconnect rejections. Add a dedicated disconnect error name or type in handleDisconnect() where pending RPCs are rejected, then update isExpectedToolsSnapshotRefreshFailure() to classify that identity as expected while preserving the existing named-error and message checks.
🤖 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 `@packages/worker/src/remote-connector/session.ts`:
- Around line 586-589: Update the result validation in the tools/list handling
flow near the response.result check to reject arrays as well as null and
non-object values. Ensure array-valued results throw the existing “Malformed
tools/list result.” error before any tools defaulting or cache replacement
occurs.
---
Outside diff comments:
In `@packages/worker/src/remote-connector/session.ts`:
- Around line 572-576: Update the soft-fail handler around
this.stateSnapshot.tools and persistState() so it preserves the existing tools
cache, restores it if persistState() fails, and then allows the existing outer
Sentry error path to run. Keep the cache cleared only when persistence succeeds,
using the handler’s current stateSnapshot and persistState symbols.
- Around line 30-46: The tools snapshot failure classifier must recognize
in-flight disconnect rejections. Add a dedicated disconnect error name or type
in handleDisconnect() where pending RPCs are rejected, then update
isExpectedToolsSnapshotRefreshFailure() to classify that identity as expected
while preserving the existing named-error and message checks.
🪄 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 Plus
Run ID: dedcae88-7503-42bf-9bf1-712dcf1ddd95
📒 Files selected for processing (2)
packages/worker/src/remote-connector/session.node.test.tspackages/worker/src/remote-connector/session.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/remote-connector/session.node.test.ts
Reject array-valued tools/list results, and restore the in-memory tool cache if soft-fail persistence fails so storage bugs still reach Sentry without blanking later reads.
Summary
Sentry KODY-CLOUDFLARE-24 reported
Remote connector tools snapshot refresh failed.for thehomeconnector. The event context was:Timed out waiting for remote connector response to tools/list.That path already soft-fails (clears tools, persists, continues). The timeout is the same class of remote-connector lifecycle noise as websocket disconnects (#916): flaky home networks, slow connector processes, mid-RPC disconnects.
This PR stops opening Sentry issues for expected refresh failures (timeout / disconnect / not connected / connector RPC error) and keeps an ops
console.warninstead. Unexpected failures (malformedtools/listpayloads, storage bugs) still rethrow into the existing message-handler Sentry capture and do not clear a still-valid tool cache. Auth rejections still capture at error level.Test plan
tools/list_changedrefresh failure logs warn, does not call Sentry, clears toolstools/listresult still reports to Sentry and keeps cached toolsnpm run validategreenSystem recap — composes existing primitives (low risk)
Mode: recap · Base:
main@0bfcec47· Head:cursor/sentry-triage-kody-cloudflare-24-6962Classification: composes — observability wiring only; soft-fail behavior for expected snapshot refresh failures is unchanged.
Primitives touched
connector-ingressconsole.warn; unexpected still Sentryremote-connectorsSystem map
tools/list_changed (or hello) refresh fails → expected: soft-clear + warn; unexpected: rethrow to Sentry without clearing tools.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context.
Risk notes
toolsonly for expected refresh failures (pre-existing for those cases).Docs
No taxonomy or architecture doc changes required.
Summary by CodeRabbit
tools/list.tools/listresponses are still reported, without clearing the existing tools list.notifications/tools/list_changedrefresh failure and malformed snapshot response scenarios.