Skip to content

fix: preserve SFTP targets after stat failures - #147

Draft
robjarawan wants to merge 2 commits into
fix/sender-dry-run-local-dirfrom
fix/sftp-stat-recovery
Draft

fix: preserve SFTP targets after stat failures#147
robjarawan wants to merge 2 commits into
fix/sender-dry-run-local-dirfrom
fix/sftp-stat-recovery

Conversation

@robjarawan

Copy link
Copy Markdown
Owner

Closes #59.

I changed Sftp.put() so it creates a multipart destination only when the remote stat() reports that the file does not exist. An I/O error, permission error or timeout now reaches Sender.send() as a failed attempt without opening the destination.

I reproduced the original corruption through Sender.send() and real Paramiko SFTP traffic over a local socket pair. The destination starts as AAAABBBBCCCC, and the sender tries to replace bytes 4–7 with xxxx. On the baseline, each metadata failure opens the existing file with wb, changes it to four zero bytes plus xxxx, and returns success. With this change the original bytes remain untouched, the attempt returns failure, and a second send succeeds after the temporary error clears.

Check #138 head 84f3f2377 Proposed runtime fix f810224c4
I/O, permission and one-second timeout; buffered and pipelined modes 6 failed with target corruption All pass; target unchanged, then recovery succeeds
Healthy existing and genuinely missing target controls 4 passed 4 passed
New SFTP regression 6 failed, 4 passed 10 passed
SFTP, sender and absolute-path focused suite Not rerun separately 14 passed
Broader unit suite, excluding Azure emulator tests Existing #138 evidence 352 passed, 1 skipped
In-memory syntax compile Not applicable 2 changed files compiled
python -m pytest -q \
  tests/sarracenia/transfer/sftp_stat_recovery_test.py \
  tests/sarracenia/transfer/sftp_test.py \
  tests/sarracenia/flow/sender_test.py \
  tests/sarracenia/flowcb/accept/sftp_absolute_test.py

The regression starts an in-process Paramiko SFTP server over socket.socketpair(). It exposes no port and uses only temporary files and synthetic fixture authentication. The timeout case delays the first STAT reply while the parsed sender config has timeout 1. The test verifies complete destination bytes, the file-open calls, the sender result and the retry after recovery.

This is stacked on #138 because that PR fixes the separate healthy middle-block truncation and gives this regression a correct success control. The runtime change here is one focused commit, f810224c4. I also carried the fork-only CI commit from #140 so opening this draft does not run the live-feed dynamic_flow jobs. Once #138 and #140 are resolved, this can be rebased or retargeted to development without changing the SFTP fix.

The old upstream proposal MetPX#1739 closed without merging because it lacked a running caller/protocol reproduction. This PR stays in my fork and remains draft pending the applicable flow matrix and independent review.

@robjarawan

Copy link
Copy Markdown
Owner Author

I ran the fork CI and checked the failures instead of treating the red matrix as an SFTP regression.

  • Both push and PR maintenance matrices passed on Ubuntu 22.04 and 24.04.
  • Twelve of 26 flow jobs passed. The fork-only dynamic_flow jobs did not run.
  • Twelve of the 14 failed jobs never started a flow. In every one, Launchpad returned HTTP Error 504, apt could not locate metpx-sr3, and sr_insects reported No Sarra C package available. Cannot test. The same setup sequence is present on PR fix: dispatch AMQP acknowledgements to the owning connection #146 before this SFTP branch.
  • Two jobs reached their assertions. The MQTT flakey_broker run failed four asynchronous count/comparison checks; PR fix: dispatch AMQP acknowledgements to the owning connection #146 has the same failure class. Its SFTP subscriber and poll-count checks passed. The AMQP restart_server run had a 22-file AMQP copy mismatch; its SFTP subscriber and all SFTP poll-count checks passed, and the Ubuntu 22.04 sibling passed.

Neither completed product-level failure reports a remote stat() failure or target corruption. The changed branch only differs when multipart SFTP stat() raises something other than ENOENT, so these checks do not contradict the direct regression proof. I am leaving the PR as a draft because the overall flow matrix is red even though the changed path, local broader suite and maintenance matrices pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant