Skip to content

fix(flow): resolve sender path and inflight staging - #1773

Open
robjarawan wants to merge 1 commit into
MetPX:developmentfrom
robjarawan:fix/sender-dry-run-local-dir
Open

fix(flow): resolve sender path and inflight staging#1773
robjarawan wants to merge 1 commit into
MetPX:developmentfrom
robjarawan:fix/sender-dry-run-local-dir

Conversation

@robjarawan

Copy link
Copy Markdown
Contributor

When running sender with dry_run enabled, Flow.send skipped changing directory to local_dir. Relative path existence checks against local_file failed when the daemon working directory differed from local_dir, preventing test executions from completing successfully. Furthermore, staging directories were prepared during fileOp events, partitioned block transfers triggered invalid renames of non-existent temporary paths, short final blocks were rejected during inplace transfers, and non-string inflight values caused unhandled exceptions.

Fixes: #1772
Refs: robjarawan#138

What this adds

Flow.send resolves local_path using os.path.realpath and verifies that os.path.commonpath([base_dir, local_path]) == base_dir. Any attempted path traversal outside base_dir logs an error and returns -1.

Flow.send preserves the original process working directory file descriptor prior to directory modification and restores it in a finally: block across live and dry run modes.

Inflight options enforce string validation. Directory staging paths starting with / or ending in / take precedence over file suffixes. Absolute staging paths disable accelerated transfer and are rejected on object storage backends. Staging directory creation and rename capability checks are deferred to the payload transfer section so fileOp events (remove, rename, directory, link) bypass staging operations.

Partitioned block transfers bypass staging and temporary rename steps. Inplace block transfers extract offsets and lengths from msg['blocks'] manifests, take precedence over inflight is None, and use msg['size'] as the expected written length for short final blocks. In sarracenia/transfer/sftp.py, file truncation during partial block writes is removed so out-of-order block assembly succeeds.

Written byte counts returned by transfer backends are validated against expected_length before executing renames or writing success report codes.

Blast radius and rollback

This change modifies transfer staging, directory switching, and write validation logic in sarracenia/flow/init.py and sarracenia/transfer/sftp.py.

Destructive operations:

  • Staging directory preparation creates remote directories using cd_forced when configured with inflight directory paths.
  • Temporary files are renamed to final destination paths upon payload transfer completion.
  • Files and directories are removed during remove fileOp events.

If transfer validation fails, files remain in temporary staging paths without replacing existing target files. Rollback is accomplished by reverting the commit on the branch. Reviewers should focus on the staging directory preparation placement in sarracenia/flow/init.py lines 2735-2755 and block transfer precedence in lines 2820-2850.

How to verify

Execute the flow test suite:

python3 -m pytest tests/sarracenia/flow/

Verify formatting and line length:

git diff origin/development..HEAD | pycodestyle --diff --max-line-length=119

Evidence

Platform results
============================= test session starts ==============================
platform linux -- Python 3.10.12, pytest-9.0.2, pluggy-1.6.0
rootdir: /net/local/home/jarawanr/src/sarracenia/tests
configfile: pytest.ini
plugins: mock-3.15.1, depends-1.0.1, anyio-4.13.0
collected 13 items

tests/sarracenia/flow/__flow___test.py ....                              [ 30%]
tests/sarracenia/flow/callback_dispatch_test.py ........                 [ 92%]
tests/sarracenia/flow/metrics_leak_test.py .                             [100%]

======================== 13 passed, 2 warnings in 1.65s ========================

@robjarawan robjarawan added the bug Something isn't working label Sep 5, 2026
@robjarawan robjarawan self-assigned this Sep 5, 2026
When running sender with dry_run enabled, Flow.send skipped changing
to local_dir, causing relative path checks against local_file to fail
when the working directory differed from local_dir. Furthermore,
inflight modes allowed directory mutations during fileOp events,
performed renames on partitioned block transfers, mishandled short
final inplace blocks, permitted path traversal, and failed to restore
the caller working directory.

Resolve local_path with realpath and enforce containment under baseDir.
Defer inflight staging and rename checks to payload transfers. Support
absolute and subfolder staging paths while disabling accelerated
transfers for absolute paths. Ensure block transfers bypass temporary
rename, prioritize inplace block offsets over inflight=None, accept
short final blocks, validate write byte counts, and restore working
directory.

Fixes: MetPX#1772
Refs: #138
@robjarawan

robjarawan commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

I gave this PR a thorough review. Two early concerns did not hold up on verification (local_offset is always 0 on senders, so the old code KeyError'd; over-length tolerance is required by real FTP drivers), so I am dropping those.

My sign-off: mergeable with two small conditions, both one-line comments. At flow/__init__.py:3012, the rename-fail return 0 is retryable by contract, so do not escalate it to -1, just note the retry intent. At :2560, note the parent-dir-only realpath resolve still blocks dir and .. escapes via the commonpath check at :2567.

One explicit drop: do not restore the blanket rfp.truncate() in sftp.py put. Verification showed it fires per block and re-breaks the out-of-order assembly this PR fixes, and transfer/__init__.py:397-398 already covers the short final. The shrink-overwrite tail case stays open as a follow-up, gated on final-block-only truncation.

Riskiest remaining item: flow/__init__.py:3000 returns 0 on persistent short writes with only a 0.01s sleep path elsewhere, so a stuck writer hot-retries with no poison cap. Worth a follow-up issue, not a blocker.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flow.send dry_run fails on files in local_dir and unbound len_written

1 participant