Conversation
4923607 to
e25716a
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| f"download_file must pass the extended-length form to s3transfer so the " | ||
| f"temp write inherits the prefix. Got: {fileobj[:60]}" | ||
| ) | ||
| assert len(fileobj) > WINDOWS_MAX_PATH_LENGTH, ( |
There was a problem hiding this comment.
The comment at L3396-3398 says the .f307214C suffix "is what TEMP_DOWNLOAD_ADDED_CHARS_LENGTH in _utils budgets for", but this test never exercises that budget. long_dir is grown until len(str(long_dir)) >= WINDOWS_MAX_PATH_LENGTH, so the destination is already past MAX_PATH on its own and _get_long_path_compatible_path would prefix it even if TEMP_DOWNLOAD_ADDED_CHARS_LENGTH were 0. This assertion pins that in place.
The case the constant actually exists for is the boundary one: a destination that is under 260 chars but whose +9-char s3transfer temp sibling is not. That is where a regression in the constant (or its removal) would produce the WinError 3 in #520, and it remains untested. Consider a second parametrisation with len(dest) + 9 >= 260 > len(dest) asserting the prefix is still applied.
There was a problem hiding this comment.
Good catch that the base test's comment overstated what it pins — the case you're asking for (a destination that is under MAX_PATH but whose +9-char temp sibling isn't) is exactly what test_download_file_prefix_gate_boundary, added in this same revision at line 3478, parametrizes at 251 and 259.
Updated the base test's fake_download comment in 423c1d6 to cross-reference the boundary test so the two are linked at the source. Happy to resolve.
| [ | ||
| # One below the gate: final=250, temp=259. Both fit within MAX_PATH plain, | ||
| # so the helper must not over-eagerly prefix. | ||
| (WINDOWS_MAX_PATH_LENGTH - TEMP_DOWNLOAD_ADDED_CHARS_LENGTH - 1, False), |
There was a problem hiding this comment.
This parametrization is the whole point of the new test: it pins the "+ TEMP_DOWNLOAD_ADDED_CHARS_LENGTH" term in _utils._get_long_path_compatible_path. But the two constants it computes the boundary from are imported from deadline.job_attachments.download (L62-63 of this file), and there they are independent literal re-definitions -- download.py:85-86 sets WINDOWS_MAX_PATH_LENGTH = 260 and TEMP_DOWNLOAD_ADDED_CHARS_LENGTH = 9 as fresh literals.
download.py does not import those names from ._utils (it only pulls _get_long_path_compatible_path and _is_relative_to), and nothing under src/ reads its copies. The gate at _utils.py:220-224 uses the _utils definitions at _utils.py:24 and _utils.py:30.
So the test computes its expected boundary from one pair of constants and asserts against behaviour driven by a different pair. Two consequences:
- If someone edits
_utils.TEMP_DOWNLOAD_ADDED_CHARS_LENGTH,below_gate_250fails with a message that reports the oldtemp_length, pointing away from the file that actually changed. - Worse for the pin: the obvious way to make that failure go away is to update the
download.pyshadow copy to match, at which point all three cases pass again for any value of the constant and the boundary is no longer pinned at all.
Importing both names from deadline.job_attachments._utils (which this file already imports from, for _get_long_path_compatible_path and WINDOWS_UNC_PATH_STRING_PREFIX) ties the parametrization to the constants the gate actually reads. Separately, the shadow copies at download.py:85-86 look dead -- if so, removing them and pointing this file plus the existing L2041/L2064 references at _utils removes the divergence entirely.
There was a problem hiding this comment.
Real trap — thanks. In 423c1d6 I moved the imports for WINDOWS_MAX_PATH_LENGTH and TEMP_DOWNLOAD_ADDED_CHARS_LENGTH from deadline.job_attachments.download to deadline.job_attachments._utils, so the test's boundary math now tracks the same constants the gate at _utils.py:220-224 actually reads. Pre-existing references at lines 2041/2064 pick up the _utils values automatically since they were already unqualified names.
Left the download.py:85-86 shadows in place: download is a public module and the non-underscore names are technically part of the public API surface, so removing them is a separate deprecation cycle rather than scope for a test-only PR. Worth filing for a follow-up though.
| # Reproduce s3transfer's real filesystem behaviour: temp next to fileobj | ||
| # with a `.<hex>` suffix, then os.replace onto the final name. | ||
| received_fileobjs.append(fileobj) | ||
| temp_path = fileobj + ".f307214C" |
There was a problem hiding this comment.
The new probe mocks get_s3_transfer_manager and substitutes fake_download, which puts it at odds with this file being a probe. Its module docstring (L5-10) draws the distinction explicitly: the unit tests "pin the string construction and nothing else," whereas here "every assertion here is a real file operation that either succeeds or raises OSError."
But fake_download is a hand-written imitation of s3transfer, not s3transfer. The load-bearing claim - that s3transfer writes to a <fileobj>.<8-hex> sibling and then renames - is asserted by this file rather than observed from it. So the probe provides no more assurance about s3transfer than test_download_file_extended_length_survives_boto3_write_then_rename already does, while costing a non-longPathAware CI leg to run. If s3transfer ever changed to stage the temp file somewhere other than next to the destination (a system temp dir, say), this probe and both new unit tests would keep passing while the real download broke on exactly the #520 path.
Two things that would restore the probe distinction:
- Derive the suffix instead of hardcoding
.f307214Cin three places (here,test_download.py:3400,test_download.py:3517).s3transfer.utils.OSUtils.get_temp_filenameis the source of truth for both the sibling placement and the 9-char length thatTEMP_DOWNLOAD_ADDED_CHARS_LENGTHmirrors. Calling it -OSUtils().get_temp_filename(fileobj)- makes the tests fail loudly if s3transfer moves the temp file, which is the failure mode a hardcoded string cannot detect. - Since
probe_local_long_path(L249-281) already exercises the write-.f307214C-then-rename mechanism against the real filesystem, the only genuinely new thing this probe adds over the unit test is runningdownload_fileunder a non-longPathAware interpreter. Worth stating that narrowly in the docstring rather than as an "end-to-end reproduction," which overstates what a mocked transfer manager can reproduce.
Also minor: get_s3_client is patched at L444-446 but never reached, since a non-None s3_client is passed positionally at L461 and download_file short-circuits at download.py:505-506. Same dead patch in both new unit tests.
There was a problem hiding this comment.
All three adopted in 423c1d6:
- Derived the temp name from
s3transfer.utils.OSUtils.get_temp_filenamein all three new fakes. If s3transfer ever moves the temp file or changes the suffix length, we now fail loudly rather than green-lie against a hard-coded literal. Leftprobe_local_long_path's hard-coded suffix alone since that probe isn't modeling s3transfer — it's directly exercising<file>.<9-char>write-then-rename against the prefix helper. - Removed the dead
get_s3_clientpatches from the two new tests and the new probe (s3_clientshort-circuit atdownload.py:505-506, as you note). Left the pre-existing patches at lines 3187/3255 alone. - Rewrote the probe docstring to name what it actually pins — the
download_filechain (destination prefix +.parent.mkdir+ boto3 handoff) under a non-longPathAwarehost — and to explicitly disclaim that the mock is not s3transfer, so drift there would show up in real download integ tests, not here.
| # The rename must have landed the file either way -- verifying the write-then- | ||
| # rename actually completes at each boundary point, not just that the string | ||
| # was constructed correctly. | ||
| final = _get_long_path_compatible_path(dest_path) |
There was a problem hiding this comment.
The final.is_file() filesystem assertions in both new tests (also L3450-3453) do not deterministically check anything, and the comments overstate them. The parametrize comment at L3471-3472 says the helper "must prefix here or the write fails," and L3560-3562 says these verify "the write-then-rename actually completes."
Whether a plain 260-char write fails is a property of the host, not the code: it needs both LongPathsEnabled and a non-longPathAware executable. Unit tests run under stock python.exe, which has declared longPathAware since CPython 3.6 (this PR spells that out at windows_long_path_probe.py:14). So on a runner with the registry setting on, a regression that dropped the + 9 would still write the 260-char temp successfully and final.is_file() would pass; on a runner with it off, the same regression fails. Either way the assertion that actually catches the regression is the actually_prefixed is expected_prefixed string check at L3552-3557 - which is deterministic and host-independent.
This is exactly the split the probe exists to handle, and it guards the precondition explicitly with --require-host-unaware (windows_long_path_probe.py:839-846), refusing to draw a conclusion if the aware interpreter was used. The unit suite has no equivalent guard and no way to add one.
The concrete cost of keeping the I/O here is that both tests build real 260+ char directory trees under tmp_path and then have to hand-roll teardown, because pytest cannot remove them (L3455-3458, L3565). That is real machinery, and test_download_file_prefix_gate_boundary additionally carries a pytest.skip at L3540-3544 that silently passes if tmp_path is long enough - so on a host with a long temp dir the boundary is not pinned at all and nothing reports that.
Suggest keeping the string assertions (they are the actual pin) and dropping the makedirs/is_file/rmtree from the unit tests, leaving filesystem verification to the probe where the host is controlled and asserted. If the I/O stays, the skip at L3540 should at least be a failure rather than a skip, since a skipped boundary test is indistinguishable from a passing one in CI output.
There was a problem hiding this comment.
Adopted the wording fixes; pushing back on the structural ask.
Softened "must prefix here or the write fails" to "must prefix here or the write fails on a non-longPathAware host" in the boundary parametrize comment (423c1d6). Also rewrote the .is_file() comment to say what the filesystem check does and doesn't pin.
Kept the filesystem I/O. Your central claim — that is_file() "does not deterministically check anything" — doesn't quite hold in the case that motivates these tests. A malformed \\?\ form (bad UNC segment, forward slashes surviving in the prefixed path, a broken join) fails open / os.replace on every host, including a fully longPathAware runner: Win32 normalization is suppressed once the prefix is applied and the path is handed to the filesystem verbatim. See probe_forward_slashes at windows_long_path_probe.py:279 for the specific failure mode this catches. Drop the I/O and both new tests degenerate into pure string assertions, which test_utils already has.
Left the pytest.skip as a skip: GitHub runners have short temp paths so the boundary is pinned where it matters, and a hard failure would penalize local devs with long temp dirs for an environmental condition rather than a regression. The skip is visible in CI counts, which was the underlying concern — happy to revisit if it ever goes silent in CI output.
…orker-agent#520 The failure trace in worker-agent#520 is s3transfer's write-`<hex>`-temp-then-rename pattern on a plain long destination under a non-longPathAware host -- the `.f307214C` suffix in the reporter's traceback is exactly this shape. aws-deadline#67 fixed the underlying prefix helper and aws-deadline#68 fixed the walk that feeds the snapshot side; the download side has been correct throughout, but no single test previously covered the exact shape end-to-end. If a future refactor dropped the `_get_long_path_compatible_path` call at `download.download_file:517`, standard Windows CI could stay green while non-longPathAware callers silently regressed. Add two tests that pin the shape: - `test_download_file_extended_length_survives_boto3_write_then_rename` (unit, Windows-only): mocks `transfer_manager.download` with the actual write-`.f307214C`-then-rename semantics on a >MAX_PATH destination and asserts (a) `fileobj` was extended-length prefixed on Windows, (b) the final file exists after the rename. - `probe_download_file_writes_to_long_local_path`: same wiring end-to-end under `python-nolpa.exe` (non-longPathAware, `LongPathsEnabled=1`), which is the configuration the reporter's failure actually surfaced under. Would go red on a refactor even if the standard Windows CI cells stayed green. Both pass at the current mainline; this is coverage hardening only, no production code change. Relates to: aws-deadline/deadline-cloud-worker-agent#520 Signed-off-by: Brian Axelson <86568017+baxeaz@users.noreply.github.com>
e25716a to
423c1d6
Compare
| # mirrors the 9-char length; `test_download_file_prefix_gate_boundary` pins | ||
| # that mirroring at the transition window. | ||
| received_fileobjs.append(fileobj) | ||
| temp_path = OSUtils().get_temp_filename(fileobj) |
There was a problem hiding this comment.
The OSUtils().get_temp_filename() call does not deliver the drift detection the comments claim for it. L3396-3401 says "if s3transfer ever moved the temp file elsewhere or changed the suffix length, the fake would go stale silently -- this call anchors us to the real API," and L3517-3518 repeats it ("a change there fails loudly rather than green-lying against a hard-coded literal").
Nothing in either test compares s3transfer's actual suffix length against TEMP_DOWNLOAD_ADDED_CHARS_LENGTH. The only assertions are on the prefix of fileobj (L3441/L3554) and on final.is_file(). Walk the drift scenario: suppose s3transfer bumped random_file_extension's default from 8 to 12 digits, so the real suffix becomes 13 chars and _utils.TEMP_DOWNLOAD_ADDED_CHARS_LENGTH = 9 is now an under-budget.
test_download_file_extended_length_survives_boto3_write_then_rename:long_diris grown past 260 on its own, so the gate fires regardless of the constant. The fake writes a 13-char-suffixed temp under the\\?\prefix, which succeeds. Green.test_download_file_prefix_gate_boundary: the parametrization computes its boundary from the stale constant, sobelow_gate_250asks forfinal = 260 - 9 - 1 = 250. The real temp is now 263 chars. The gate (which also reads the stale 9) declines to prefix,expected_prefixed=Falsematches, and the fake writes a 263-char plain temp. Under stockpython.exethat write succeeds, sofinal.is_file()passes too. Green — while production is now one bad path away from the WinError 3 in #520.
So both tests would keep passing across exactly the s3transfer change they claim to catch, because they consume get_temp_filename for its value rather than asserting anything about its shape. Compared to the hard-coded .f307214C this replaced, the behaviour is identical; the difference is only that the comments now assert a guarantee that is not there.
The anchor the comments describe is a one-line assertion on the delta, not a call site. Something like:
_probe = "C:\\dir\\scene.ma"
assert len(OSUtils().get_temp_filename(_probe)) - len(_probe) == TEMP_DOWNLOAD_ADDED_CHARS_LENGTHas a module-level check (or its own tiny test) does pin the mirroring, and it is the thing that turns red when s3transfer moves. Note it has to use a short basename: get_temp_filename truncates to _MAX_FILENAME_LEN - len(suffix) (255 - 9), so the delta is only +9 when len(basename) < 247 — worth encoding in the probe string rather than discovering later.
Separately, both this file and windows_long_path_probe.py:47 now import from s3transfer.utils, which is neither in pyproject.toml dependencies nor requirements-testing.txt — it arrives transitively via boto3, and OSUtils._MAX_FILENAME_LEN in particular is private. Worth a note if the suite is meant to survive a boto3 bump that repins s3transfer.
There was a problem hiding this comment.
Fair — the comments claimed drift detection that wasn't actually implemented. Fixed in c5df71d.
New unit test test_temp_download_added_chars_length_mirrors_s3transfer_suffix (test_download.py:3571-3596) does exactly what you suggested: probes with a short basename ("scene.ma") to sidestep OSUtils._MAX_FILENAME_LEN truncation, computes delta = len(OSUtils().get_temp_filename(probe)) - len(probe), and asserts delta == TEMP_DOWNLOAD_ADDED_CHARS_LENGTH. The assertion message names both the observed s3transfer suffix length and the current constant so a red test points at the file that actually needs updating.
Walked the drift scenario you described (s3transfer bumps random_file_extension to 12 hex digits): TEMP_DOWNLOAD_ADDED_CHARS_LENGTH=9 stays stale, real suffix is now 13 chars, delta == 13 != 9, the new test fails loudly. The two write-then-rename tests still pass regardless (as you noted, they're consuming the API for its value not its shape) but the delta test is now the tripwire that catches the drift.
On the deps note: added a paragraph to the delta test's docstring flagging that s3transfer.utils arrives transitively via boto3 and that this test is the surface where a repin surfaces first. OSUtils._MAX_FILENAME_LEN is not consulted in the test (still private), but noted in the docstring for anyone updating.
| surrounding ``download_file`` chain -- destination prefix, ``.parent.mkdir``, boto3 | ||
| handoff -- running on a host that cannot tolerate a plain long path. | ||
|
|
||
| The transfer manager is mocked, so this pins our contract with s3transfer (prefix the |
There was a problem hiding this comment.
This probe is the one place in the suite that could establish the download_file claim, and mocking the transfer manager removes the part that only a probe can do.
The file's module docstring sets the contract explicitly at L8-9: "Every assertion here is a real file operation that either succeeds or raises OSError," and L6-7 explains why — the unit tests "pin the string construction and nothing else. They cannot tell whether Windows actually accepts the result." With transfer_manager.download replaced by fake_download, the two checks here are (a) fileobj.startswith(WINDOWS_UNC_PATH_STRING_PREFIX) — a string assertion, and (b) final.is_file() — a file operation, but on a path the probe's own fake wrote. test_download_file_extended_length_survives_boto3_write_then_rename in test_download.py makes both of those same assertions against the same mock. So the probe entry costs a slot in the run and adds no coverage the unit suite lacks.
The distinctive capability of this file is the negative control, which probe_registry_alone_is_insufficient (L222-247) demonstrates: open the plain path, require OSError, then show the prefixed path succeeds. That is what converts "we passed a prefixed string" into "the unprefixed string would actually have failed here." This probe has no equivalent, and unlike its neighbour it also takes no host_aware argument and is registered unconditionally at L845-848 — so on the stock-python.exe leg it runs and passes while its own failure message (L469-472, "under a non-longPathAware host; would fail with WinError 3") describes a condition that does not hold.
Two changes would make it earn its slot:
- Take
host_awarelikeprobe_registry_alone_is_insufficientdoes, and when the host is not aware, assert the negative: calldownload_fileonce with_get_long_path_compatible_pathpatched to the identity (or drive the plain destination straight intoopen) and require the write to raise. Without that, nothing here distinguishes a correct gate from a removed one. - Consider dropping the transfer-manager mock and letting a real (moto-backed or pre-seeded) transfer run against the long destination. The docstring at L419-421 says drift "would show up in real download integration tests, not here" — but
test/integdoes not run on the non-longPathAware interpreter, so that deferral has no destination. Whether s3transfer's ownOSUtils.open/rename_filetolerate a\\?\path past MAX_PATH under a non-aware host is precisely the unverified link in the #520 chain, and this file is the only harness positioned to check it.
There was a problem hiding this comment.
Right — a probe that mocks the load-bearing bit isn't earning its slot. Fixed in c5df71d.
probe_download_file_writes_to_long_local_path now follows probe_registry_alone_is_insufficient's positive+negative shape:
- Takes
host_aware: booland skips on aware hosts with a message explaining the negative control is impossible there (nothing to demonstrate — the plain form works regardless of the prefix helper). - On non-aware hosts, opens the exact plain temp shape s3transfer would produce under a dropped prefix (
<dest>.f307214C) and requiresOSError. That's the tripwire that turns "we passed a prefixed string" into "the unprefixed string would have failed on this host." Positive case then proves the prefixed form succeeds. - Registered with
host_awareat the same call site as the sibling probe (windows_long_path_probe.py:905).
Docstring rewritten to name what the probe distinctively pins — the download_file chain (destination prefix + .parent.mkdir + boto3 handoff) under a genuinely non-longPathAware host, with the negative control up front — and to disclaim that the mock is not s3transfer, so drift there surfaces via test_temp_download_added_chars_length_mirrors_s3transfer_suffix, not here.
On dropping the mock entirely for a real (moto-backed) transfer against the long destination: agreed that would be the strongest version of this probe, but wanted to keep this PR scoped to pinning the shape #520 documents. Filed mentally as a follow-up if a customer report ever suggests s3transfer's own OSUtils.open/rename_file are the failing link.
…orker-agent#520 The failure trace in worker-agent#520 is s3transfer's write-`<hex>`-temp-then-rename pattern on a plain long destination under a non-longPathAware host -- the `.f307214C` suffix in the reporter's traceback is exactly this shape. aws-deadline#67 fixed the underlying prefix helper and aws-deadline#68 fixed the walk that feeds the snapshot side; the download side has been correct throughout, but no single test previously covered the exact shape end-to-end. If a future refactor dropped the `_get_long_path_compatible_path` call at `download.download_file:517`, standard Windows CI could stay green while non-longPathAware callers silently regressed. Add two tests that pin the shape: - `test_download_file_extended_length_survives_boto3_write_then_rename` (unit, Windows-only): mocks `transfer_manager.download` with the actual write-`.f307214C`-then-rename semantics on a >MAX_PATH destination and asserts (a) `fileobj` was extended-length prefixed on Windows, (b) the final file exists after the rename. - `probe_download_file_writes_to_long_local_path`: same wiring end-to-end under `python-nolpa.exe` (non-longPathAware, `LongPathsEnabled=1`), which is the configuration the reporter's failure actually surfaced under. Would go red on a refactor even if the standard Windows CI cells stayed green. Both pass at the current mainline; this is coverage hardening only, no production code change. Relates to: aws-deadline/deadline-cloud-worker-agent#520 Signed-off-by: Brian Axelson <86568017+baxeaz@users.noreply.github.com>
1 participant
Relates to: aws-deadline/deadline-cloud-worker-agent#520
What was the problem/requirement? (What/Why)
worker-agent#520 reports a
WinError 3on a long output path ending in.f307214C. That 9-character suffix is s3transfer's download-in-progresstemp name (boto3 writes to
<destination>.<8-hex>andos.replaces ontothe destination on completion), so the failing operation is a download to
a plain long path from a process that isn't
longPathAware. #67 fixed theunderlying
_get_long_path_compatible_pathhelper and #68 fixed the walk +fast-diff stat that feed the snapshot side. The download side has been
correct throughout —
download.download_fileprefixes its destination atline 517 before handing it to
transfer_manager.download— but no singletest previously covered the exact end-to-end shape.
That's a coverage gap, not a bug gap. If a future refactor dropped the
_get_long_path_compatible_pathcall atdownload_file:517("caller alreadyprefixes it, simplify"), standard Windows CI could stay green — the only tests
that would exercise the failure surface are
probe_local_long_path(whichtests the mechanism directly, not through
download_file) andtest_download_summary_paths_do_not_carry_the_unc_prefix(which checks thereturned path's prefix hygiene but writes directly to
fileobjin its mockrather than through the
.<hex>-temp-then-rename shape). Non-longPathAwarecallers could silently regress.
What was the solution? (How)
Add three tests that pin the shape, no production code changes:
test_download_file_extended_length_survives_boto3_write_then_rename(unit,Windows-only): mocks
transfer_manager.downloadwith the actualwrite-
.f307214C-then-rename semantics on a>MAX_PATHdestination, asserts(a) the
fileobjpassed to s3transfer was extended-length prefixed onWindows, (b) the final file exists at the destination after the rename.
test_download_file_prefix_gate_boundary(unit, Windows-only,parametrized): pins the
+ TEMP_DOWNLOAD_ADDED_CHARS_LENGTHarithmetic in_get_long_path_compatible_pathat three destination lengths in thetransition window — 250 (below the gate, must not prefix), 251 (right at
the gate, must prefix), and 259 (top of transition, must prefix). Catches
an off-by-one that dropped the
+ 9allowance, which would leave a 260-chartemp file written in plain form — exactly the #520 failure shape.
probe_download_file_writes_to_long_local_path(inscripted_tests/windows_long_path_probe.py): the same wiring end-to-endunder
python-nolpa.exe(non-longPathAware,LongPathsEnabled=1), whichis the configuration the reporter's failure surfaced under. Would go red on
a refactor of the same shape even if the standard Windows CI cells stayed
green.
What is the impact of this change?
None on production behavior — this is coverage hardening only. The new tests
all pass at the current mainline: worker-agent#520's failure shape is handled
correctly end-to-end today, and now can't quietly regress.
How was this change tested?
hatch run fmt,hatch run lint(including mypy): all clean.well-past-MAX_PATH case plus 3 boundary points at destination lengths
250, 251, and 259).
scripted_tests/windows_long_path_probe.py)under
python-nolpa.exewithLongPathsEnabled=1: 11 of 11registered probes passed (the 12th,
long UNC path, is skipped when--unc-rootis not supplied). The newprobe_download_file_writes_to_long_local_pathsucceeded on the firstrun — meaning the
download_filechain already handles the #520 shapecorrectly; this probe pins that behavior going forward.
downloadorasset_syncmodules?existing behavior in
download.download_file.Was this change documented?
they pin and why (cross-referencing worker-agent#520).
Does this PR introduce new dependencies?
Is this a breaking change?
No. Test-only PR; no production symbols changed.
Does this change impact security?
No new security boundary.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.