Skip to content

Keep pooled SFTP connection on application-level SFTP errors - #73288

Open
stegololz wants to merge 1 commit into
apache:mainfrom
stegololz:fix/sftp-pool-error-classification
Open

stegololz wants to merge 1 commit into
apache:mainfrom
stegololz:fix/sftp-pool-error-classification

Conversation

@stegololz

Copy link
Copy Markdown
Contributor

SFTPClientPool.get_sftp_client yields the pooled client inside a bare
except Exception and marks the connection faulty for any exception the caller's
async with body raises, including SFTP status replies such as SFTPNoSuchFile
or SFTPPermissionDenied.

A status reply means the server answered over a live channel, so the connection is
still healthy. Dropping it costs a reconnect and re-auth each time, and since the
caller usually keeps iterating, one transient miss becomes self-sustaining.

Observed on a production task iterating ~36,000 files over one pool:

phase files SSH connections
before first miss 28,380 23
after first miss 7,598 9,244, with 7,598 re-auths

The files were not missing; they read back fine afterwards. Each failure also
logged a full traceback, producing 135-190 MB of task logs per attempt.

Only SFTPNoConnection, SFTPConnectionLost and SFTPBadMessage now count as
transport faults. Every other SFTPError returns the connection to the pool
intact. Non-SFTPError exceptions and CancelledError keep the previous
behaviour and still drop it. The exception always propagates unchanged.

The rule sits in a module-level _is_connection_faulty rather than inline in the
except block because it is a pure function of the exception type: one
parametrised test covers the whole SFTPError taxonomy with no pool, event loop
or mocked connection to stand up. It also keeps get_sftp_client reading as
acquire, classify, log, release, and puts the rule beside the
_TRANSPORT_SFTP_ERRORS tuple it consults.

The pool was introduced in #64465.

Tested with pytest providers/sftp/tests/unit/sftp/pools/test_sftp.py: 32 passed,
covering the classifier table, a miss returning the connection to the pool, five
consecutive misses reusing the same client without reconnecting, and the transport
and non-SFTPError cases still dropping it.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Opus 5) following the guidelines

An SFTP status reply such as SFTPNoSuchFile or SFTPPermissionDenied means the
server answered over a live channel, so the pooled connection behind it is
still usable. Treating every exception as a transport fault made one missing
file cost a full reconnect and re-auth, and because callers keep iterating the
cost compounds: a production task turned 7,598 misses into 9,244 SSH
connections and 135-190 MB of task logs per attempt.
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.

1 participant