bitcoindevkit / bitcoindevkit/rust-electrum-client

`AllAttemptsErrored` drops the final (retry-exhausting) attempt's error

Open Beginner friendly
#221 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.