Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SFTPClientPool.get_sftp_clientyields the pooled client inside a bareexcept Exceptionand marks the connection faulty for any exception the caller'sasync withbody raises, including SFTP status replies such asSFTPNoSuchFileor
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:
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,SFTPConnectionLostandSFTPBadMessagenow count astransport faults. Every other
SFTPErrorreturns the connection to the poolintact. Non-
SFTPErrorexceptions andCancelledErrorkeep the previousbehaviour and still drop it. The exception always propagates unchanged.
The rule sits in a module-level
_is_connection_faultyrather than inline in theexceptblock because it is a pure function of the exception type: oneparametrised test covers the whole
SFTPErrortaxonomy with no pool, event loopor mocked connection to stand up. It also keeps
get_sftp_clientreading asacquire, classify, log, release, and puts the rule beside the
_TRANSPORT_SFTP_ERRORStuple 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-
SFTPErrorcases still dropping it.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines