bitcoindevkit / bitcoindevkit/rust-electrum-client
`AllAttemptsErrored` drops the final (retry-exhausting) attempt's error
- Dominant language
- Rust
- Stars
- 89
- Forks
- 82
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 1
Description
**Crate:** `electrum-client`
**Version:** 0.25.0
**File:** `src/client.rs`, the `impl_inner_call!` macro
## Summary
When a call retries and finally gives up, `Error::AllAttemptsErrored(errors)` does not
include the error from the last attempt. In `impl_inner_call!`, `errors.push(e)` runs
*after* the `retries_exhausted` early-return, so on the attempt that exhausts the retry
budget the current error is never added to the vector. The returned `AllAttemptsErrored`
therefore always omits the last, and usually most diagnostic, failure.
It happens in both error arms of the macro:
- the operation-error arm (v0.25.0 lines 61-71): `return Err(AllAttemptsErrored(errors))`
at line 66 precedes `errors.push(e)` at line 71;
- the client re-creation (reconnect) arm (v0.25.0 lines 85-95): same ordering at
line 90 / line 95.
## Code (v0.25.0)
```rust
Err(e) => {
let failed_attempts = errors.len() + 1;
if retries_exhausted(failed_attempts, $self.config.retry()) {
warn!("call '{}' failed after {} attempts", stringify!($name), failed_attempts);
return Err(Error::AllAttemptsErrored(errors)); // <-- returns WITHOUT `e`
}
warn!("call '{}' failed with {}, retry: {}/{}", stringify!($name), e, failed_attempts, $self.config.retry());
errors.push(e); // <-- only reached when NOT exhausted
// ... reconnect ...
}
```
The reconnect arm (`Err(e)` from `ClientType::from_config` around line 85) has the same
shape: it returns `AllAttemptsErrored(errors)` at line 90 before pushing `e` at line 95.
## Symptom
With `retry = 1` (the default), the most common way to hit this is a reconnection whose
`server.version` handshake is rejected with an `Error::Protocol(...)`:
1. Attempt 1: the operation fails with a transient error (e.g. an IO error such as a
broken pipe because the server closed an idle connection). This error is pushed
(`errors == [io_error]`).
2. Reconnect: `ClientType::from_config(...)` runs `negotiate_protocol_version`, which sends
`server.version`. On a server that authenticates the handshake, this is rejected with
`Error::Protocol(...)` (in our case a JSON-RPC `-32004` "authentication required" from an
auth proxy). `failed_attempts = errors.len() + 1 = 2`, `retries_exhausted(2, 1)` is true,
so line 90 returns `AllAttemptsErrored([io_error])` -- the `Protocol` error is dropped.
The caller sees `AllAttemptsErrored` containing only the initial IO error, with no trace of
the `Protocol` error that actually caused the terminal failure.
Note that the first-attempt `Protocol`/`AlreadySubscribed` fast-path (lines 58-60) returns
those errors directly and is not affected. The problem is specifically a `Protocol` (or any
non-fast-path) error raised during the *reconnect* handshake, which flows through the
`Err(e)` arm and is swallowed on the retry-exhausting attempt.
## Impact
- The returned error is misleading: the vector never contains the error that ended the
retry loop.
- Downstream code cannot distinguish an authentication denial on reconnect from a plain
connection failure, so it cannot react (for example, invalidate and refresh an expired
OAuth token before retrying).
- The only workaround is to raise `retry` to `>= 2` purely so the terminal error lands in
`errors` on a non-final attempt and becomes visible. That is wasteful and non-obvious.
## Suggested fix
Push `e` onto `errors` *before* the `retries_exhausted` check in both arms, and compute
`failed_attempts` (and the warn messages) from `errors` afterwards, so the returned
`AllAttemptsErrored` always contains every attempt's error including the last:
```rust
Err(e) => {
errors.push(e);
let failed_attempts = errors.len();
if retries_exhausted(failed_attempts, $self.config.retry()) {
warn!("call '{}' failed after {} attempts", stringify!($name), failed_attempts);
return Err(Error::AllAttemptsErrored(errors)); // now includes the last error
}
warn!(
"call '{}' failed with {}, retry: {}/{}",
stringify!($name),
errors.last().expect("just pushed"),
failed_attempts,
$self.config.retry()
);
// ... reconnect ...
}
```
and the analogous change in the reconnect arm.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in src/client.rs at the impl_inner_call! macro and inspect both Err(e) arms: the operation-error path and the ClientType::from_config reconnect path. Exercise the retry=1 case described in the issue, checking that AllAttemptsErrored contains the terminal error from each path. Done means every retry attempt, including the retry-exhausting one, is represented in the returned error.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 82/100