fix(retryProvider): Don't mask quorum failures with a ReferenceError - #1483
Conversation
When quorum is not reached and no fallback providers remain, the early-exit throwQuorumError() called getMismatchedProviders(), which closed over quorumResult - a const declared only after the fallback results are counted. That access lands in the temporal dead zone, so instead of the intended 'Not enough providers agreed to meet quorum' error the call site crashes with a ReferenceError, and the diagnostic warn log is skipped, hiding which providers disagreed. Pass the quorum result into getMismatchedProviders explicitly and skip the mismatch computation on the early-exit path, where no quorum result exists to compare against. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review! No inline comments to address, so no further changes from my side. CI (Builds/Lint/Test) is still running — I'll leave merging to you once it's green. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b24805fde1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…stack
RetryProvider.send and the Solana QuorumFallback factory accumulate
per-provider failures via `err.stack || err.toString()`. For ethers
transport errors (SERVER_ERROR, etc.), `err.stack` and the stringified
form embed the failed RPC URL with its API key in cleartext, plus a
multi-line ethers stack trace. That accumulator then gets concatenated
into the aggregate "Not enough providers succeeded" error message and
into the `logQuorumMismatchOrFailureDetails` warn log — both reach
Slack/Datadog and leak the key.
Introduce `summarizeProviderError(err)` which produces a single-line,
log-safe summary. Preference order:
1. Parsed JSON-RPC error message (e.g. "execution reverted: ERC20:
burn amount exceeds balance"), drilling into `err.error` if
necessary — this is what callers actually want.
2. `err.reason` — ethers' short, URL-free reason string.
3. `err.message` for non-ethers errors (no `err.code` present).
4. `err.code` (e.g. "SERVER_ERROR", "NETWORK_ERROR") as a last resort.
`err.message` is deliberately skipped for ethers-shaped errors because
that's exactly where transport-layer errors splice in the failed URL.
The original error remains reachable via `error.cause` on the aggregate
thrown by `createSendErrorWithMessage`, so forensic detail isn't lost.
Wire it into both error-collection sites in `retryProvider.ts` and both
in `solana/quorumFallbackRpcFactory.ts`. No behavior change beyond the
shape of the recorded error string.
Rebase notes (onto #1483's branch): master's Solana path meanwhile
introduced formatRpcError(), which preserves the SolanaError JSON-RPC
code in the wrap message. Keep formatRpcError() at the Solana call
sites and replace only its non-Solana fallback (err.stack ||
err.toString()) with summarizeProviderError(), so both intents compose:
code preservation for SolanaErrors, no URL/API-key leakage for
everything else.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the Codex P2 finding in 77d1105: the no-fallback quorum-failure path now computes the most frequent result from the successful providers' responses (via a new @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 883e5439ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mismatchedProviders = quorum | ||
| ? getMismatchedProviders(quorum.quorumResult, [...values, ...quorum.fallbackValues]) | ||
| : []; |
There was a problem hiding this comment.
When quorum requires all configured providers and those providers return different valid results, this early-exit path passes no quorum candidate and hard-codes mismatchedProviders to [], so the warn log cannot identify any diverging RPC even though the call is failing because the successful providers disagreed. Compute the most frequent successful response on this no-fallback path and compare the successful providers against it before logging, matching the later fallback path’s diagnostics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — this PR was squash-merged just before the follow-up commit landed on the branch, so master still has the empty-mismatchedProviders early-exit path. Re-opened the fix as #1484: the no-fallback path now computes the most frequent successful response via a findQuorumCandidate helper (shared with the fallback path) and measures mismatches against it, with a test asserting the diverging provider is flagged.
|
Codex's re-flag is valid, but for a timing reason: #1483 was squash-merged at 16:02 UTC with only the original commit — the diagnostics follow-up (77d1105) reached the branch right after the merge, so it never landed on master, and Codex reviewed the merge commit. I've re-opened the fix as #1484 (#1484), cherry-picked cleanly onto master: the no-fallback quorum-failure path now computes the most frequent successful response via a shared |
Picks up across-protocol/sdk#1483 (quorum failures throw the intended error instead of a ReferenceError, fixing the recurring zion-across-proposer crash) and across-protocol/sdk#1441 (log-safe per-provider error summaries, no RPC URL/API-key leakage). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Picks up two RetryProvider fixes shipping together in 4.4.12: - across-protocol/sdk#1483 — quorum failures with no fallback providers left now throw the intended "Not enough providers agreed to meet quorum" error instead of crashing with `ReferenceError: Cannot access 'quorumResult' before initialization`. This is the error that has been killing `zion-across-proposer` runs (5 consecutive runs on 2026-07-15, previously 2026-06-22) and other dataworker/monitor bots since April. - across-protocol/sdk#1441 — per-provider failures are summarized into single-line, log-safe messages instead of `err.stack`, keeping RPC URLs (and embedded API keys) out of aggregate error messages and logs. Co-authored-by: droplet-rl <284132418+droplet-rl@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Motivation
When
RetryProvider.sendfails to reach quorum and no fallback providers remain, the early-exitthrowQuorumError()call crashes with:getMismatchedProviderscloses overquorumResult, aconstthat is only declared ~45 lines later (after fallback results are counted). The early-exit path invokes it inside the temporal dead zone, so instead of the intended "Not enough providers agreed to meet quorum" error the process dies with aReferenceError— and since the crash happens beforelogQuorumMismatchOrFailureDetails, the diagnostic warn (which providers mismatched, on which method) is lost too.The pattern was introduced in #728. In production it has been crashing the Across dataworker/proposer and other bots whenever a provider failure exhausts the fallback list and the surviving providers disagree (most recently
zion-across-proposer, 5 consecutive runs on 2026-07-15).Changes
getMismatchedProvidersnow takes the quorum result as an explicit parameter instead of closing over the not-yet-declaredquorumResult.throwQuorumErrortakes an optional{ quorumResult, fallbackValues }: the post-count call site passes it; the early-exit call site has no quorum result, so mismatch computation is skipped there (there is nothing to compare against — the thrown error already lists the disagreeing providers).test/providers/retryProvider.test.tscovering: the no-fallback disagreement path (throws the quorum error, not aReferenceError), the fallback-consumed-by-failure path, and the quorum-reached-via-fallback happy path.🤖 Generated with Claude Code