Retry transient SSH/SFTP errors when fetching remote benchmark files

[kei-nan]

Summary

fetch_file_from_remote_setup() opened a single pysftp connection with no retry, so any transient network blip on an otherwise-healthy remote host was treated as fatal for the whole run.

Root-caused this against a real failure: RediSearch benchmark CI run where memtier_benchmark on the remote client exited normally (code 0, 0 errors), and 0.35s later a fresh SFTP connection opened to fetch the results file failed instantly with paramiko.ssh_exception.SSHException: Error reading SSH protocol banner. That single failure cascaded into every other queued benchmark in the 68-test matrix failing too.

Checked AWS-side to rule out real causes before treating this as transient:

• CloudTrail: the client instance's own TerminateInstances event (issued by Terraform teardown after the failure) shows previousState: running — AWS's control plane still considered the instance healthy 16s after the SSH failure. Not a spot reclaim, not an AWS-initiated termination.
• CloudWatch StatusCheckFailed_System/StatusCheckFailed_Instance: both 0 throughout — no hypervisor/host-level fault.
• CloudWatch CPUUtilization/NetworkOut on the client: idle (~0.7% CPU) at the time — no resource starvation.

That leaves an ordinary, low-probability transient connection blip as the only remaining explanation — the kind of thing that's normally invisible because clients retry. This one wasn't retried anywhere.

Change

• fetch_file_from_remote_setup() now retries with bounded exponential backoff (SSH_FETCH_MAX_RETRIES, default 3) on connection-level exceptions (paramiko.SSHException, EOFError, ConnectionError, TimeoutError, OSError), matching the backoff pattern already used for keyspace-check retries in run/common.py.
• Non-connection errors (e.g. a genuinely missing remote file) still fail immediately — no pointless retrying.

Test plan

• Added tests/test_remote_fetch_retry.py: succeeds first try / retries-then-succeeds / gives up after max retries and re-raises / does not retry non-transient errors.
• pytest tests/test_remote_fetch_retry.py tests/test_remote.py — 21 passed, 3 skipped (pre-existing, unrelated RTS-port skips).
• black + flake8 clean on changed files.

[github-actions]

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

Root-cause write-up in the description is the good kind: CloudTrail/CloudWatch ruled out the real causes before landing on "transient," and the 2**attempt + "Retrying in {} seconds..." shape does match the keyspace-check backoff in run/common.py:1103-1105, so the "mirrors the existing pattern" claim checks out. Four points, most important first.

1. OSError in TRANSIENT_SSH_ERRORS swallows the exact case the description says fails fast. FileNotFoundError is a subclass of OSError, and paramiko raises IOError(errno.ENOENT, ...) on SFTP_NO_SUCH_FILE, which Python resolves to FileNotFoundError. So a genuinely missing remote file now goes through the full retry loop rather than failing immediately. This isn't hypothetical — run_remote/remote_failures.py:56 has a dedicated except FileNotFoundError around this call ("Unable to fetch remote file"), which is the pre-existing evidence that a missing file surfaces here as an OSError subclass. That path is the failure path (failed_remote_run_artifact_store runs after a benchmark exits non-zero), i.e. precisely where the results file most often legitimately doesn't exist. test_fetch_does_not_retry_on_non_transient_error doesn't catch this because ValueError isn't an OSError; if you re-point that test at FileNotFoundError it should fail today. Narrowing to (paramiko.SSHException, EOFError, ConnectionError, TimeoutError) and dropping bare OSError would keep the intended coverage — ConnectionError/TimeoutError are already OSError subclasses, so nothing transient is lost by removing the parent. Worth doing before merge, I think.

   Same shape one level down: paramiko.AuthenticationException and BadHostKeyException subclass SSHException, so a bad key or wrong username also retries 4× now. Less costly than the missing-file case, but also not transient.

2. The wall-clock arithmetic on the failure path. With the default max_retries=3 that's up to 4 attempts and 1 + 2 + 4 = 7s of sleeping per call, on top of 4 SSH handshakes — and pysftp.Connection is constructed with no timeout, so each attempt can also sit on paramiko's default banner timeout before the backoff even starts. For the 68-test matrix in your own repro, if the tests fail with no results file written, that's on the order of 7-8 minutes of pure backoff added across the matrix on a run that's already failing (and more if the host is genuinely gone rather than just missing a file). Fixing point 1 removes most of this, but it may still be worth bounding the total retry budget or passing a connect timeout, given this runs inside CI benchmark loops.

3. The connection isn't closed when srv.get raises. srv.close() only runs on the success path, so a transient failure during the transfer (as opposed to at connect time) leaks the connection on every retry. with pysftp.Connection(...) as srv: inside the try would handle it, or a try/finally. Related: all four tests drive side_effect on pysftp.Connection, so they only exercise connect-time failures — a conn.get.side_effect = paramiko.SSHException(...) case would cover both the retry-on-transfer-failure path and the close.

4. Minor/process: the version bump to 0.12.39 is bundled into this PR, whereas bumps here are normally their own PR (Bumping version from 0.12.37 to 0.12.38 #564, Bumping version from 0.12.33 to 0.12.34 #546). AGENTS.md also notes a self-authored bump can't be self-approved on master, so folding it in here means this PR carries the release too. Not a correctness issue, just flagging in case that's unintentional.

Happy to send the FileNotFoundError and get-raises test cases as a PR against this branch if that's easier than writing them again. Points 2-4 I'd take or leave; point 1 I do think is worth a fix, since as written the retry loop changes behavior on the one path that already had explicit missing-file handling. Good catch on the underlying bug either way — this is a real gap and it should land.

(For what it's worth on the review state: this reads as comments rather than a block — the fix for point 1 is a one-line change to the exception tuple.)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.93%. Comparing base (4f9e334) to head (3e30594).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
+ Coverage   48.74%   48.93%   +0.18%     
==========================================
  Files          74       74              
  Lines        9022     9035      +13     
==========================================
+ Hits         4398     4421      +23     
+ Misses       4624     4614      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.