Skip to content

Revert "Continue to retry failed requests when offline (#8623)"#8739

Merged
mcmire merged 1 commit intomainfrom
revert-8623
May 7, 2026
Merged

Revert "Continue to retry failed requests when offline (#8623)"#8739
mcmire merged 1 commit intomainfrom
revert-8623

Conversation

@mcmire
Copy link
Copy Markdown
Contributor

@mcmire mcmire commented May 7, 2026

Explanation

This reverts commit 0caca27.

This PR is faulty, because although it does restore retries for failed requests, it also causes onDegraded and onBreak to be fired for RpcServiceChain, and hence causes NetworkController:rpcEndpointChainDegraded and NetworkController:rpcEndpointChainUnavailable to be fired as well.

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Changes RpcService retry/circuit-breaker behavior based on offline status, which can affect request reliability and when network degraded/unavailable events fire. Risk is mitigated by expanded unit/integration tests for offline scenarios.

Overview
Reverts the previous behavior that continued retrying RPC requests while the user is offline by making RpcService's retry filter short-circuit when isOffline() is true.

This prevents offline connection failures from triggering retries, circuit breaks, and downstream NetworkController retry/degraded/unavailable event emissions, with updated/added tests covering offline unavailable and degraded scenarios and updated ESLint suppression metadata.

Reviewed by Cursor Bugbot for commit 263e1ca. Bugbot is set up for automated code reviews on this repo. Configure here.

This reverts commit 0caca27.

This PR is faulty, because although it does restore retries for failed
requests, it also causes `onDegraded` and `onBreak` to be fired for
`RpcServiceChain`, and hence causes
`NetworkController:rpcEndpointChainDegraded` and
`NetworkController:rpcEndpointChainUnavailable` to be fired as well.
@mcmire mcmire temporarily deployed to default-branch May 7, 2026 19:19 — with GitHub Actions Inactive
@mcmire mcmire marked this pull request as ready for review May 7, 2026 19:26
@mcmire mcmire requested review from a team as code owners May 7, 2026 19:26
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 263e1ca. Configure here.

// This prevents degraded/break callbacks from being triggered
if (isOffline()) {
return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circuit breaker still fires onBreak when offline

Medium Severity

The comment on line 366 claims skipping retries when offline "prevents degraded/break callbacks from being triggered," but this is only partially true. The retryFilterPolicy returning false prevents onDegraded (since onGiveUp won't fire), but the circuit breaker is a separate inner policy that still counts each individual request failure via handleWhen(isServiceFailure). After DEFAULT_MAX_CONSECUTIVE_FAILURES (15) separate failed requests while offline, the circuit will break and onBreak will fire — and the !this.#isOffline() guard that previously suppressed this callback was removed. With block tracker polling roughly every 20 seconds, this threshold is reachable after ~5 minutes offline, potentially causing the same NetworkController:rpcEndpointUnavailable events the PR aims to prevent.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 263e1ca. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we definitely need to investigate this, but this comment was present before the commit we are reverting.

@mcmire mcmire added this pull request to the merge queue May 7, 2026
Merged via the queue into main with commit c856b75 May 7, 2026
371 of 372 checks passed
@mcmire mcmire deleted the revert-8623 branch May 7, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants