Skip to content

fix(download): never leave partial files behind - #165

Open
rominf wants to merge 2 commits into
mainfrom
fix/atomic-downloads-no-partial-leaks
Open

fix(download): never leave partial files behind#165
rominf wants to merge 2 commits into
mainfrom
fix/atomic-downloads-no-partial-leaks

Conversation

@rominf

@rominf rominf commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Downloads and atomic writes no longer leave partial files behind.

  • Stream downloads to a sibling .part file and rename into place only once the body is complete, removing the partial on every failure path.
  • Clean up the temp file on the failure paths of both write_file_atomically copies.
  • Preserve full file names in temp paths, so sdk.tar.gz no longer becomes sdk.tar.tmp-<id>.
  • Stream the SDK tarball instead of reading it fully into memory.

Root cause

Two separate defects, both reproduced against this tree.

write_file_atomically named its temp path.tmp-<unix_ms> and returned early on a write error without removing it. Because the name embeds a timestamp, each retry left a distinct orphan rather than reusing one. Filling a small filesystem and calling it twice produced two orphans, the first holding all the space that remained — so on a full disk, retrying made things worse:

attempt 0: ... No space left on device (os error 28)
  leftovers: [("artifact.tar.tmp-1785755921807", 67108864)]
attempt 1: ... No space left on device (os error 28)
  leftovers: [("artifact.tar.tmp-1785755921807", 67108864),
              ("artifact.tar.tmp-1785755921875", 0)]

download_file_to_path wrote straight to the final path with no temp and no cleanup, so an interrupted transfer left a truncated file exactly where callers look for a complete one. Serving a Content-Length of 1 MiB and then sending 16 bytes left those 16 bytes at the destination.

Tests

  • download_leaves_no_truncated_file_at_destination — a body that ends early leaves neither a destination file nor a partial.
  • stream_to_path_atomically_writes_complete_body — the happy path renames into place and keeps every extension.
  • temp_sibling_path_preserves_multi_dot_file_names.
  • write_file_atomically_cleans_up_temp_on_write_failure in both crates, marked #[ignore] because they fill /dev/shm to provoke ENOSPC and so are not safe to run concurrently. Run with cargo test -- --ignored.

The first two run by default and are portable. I ran the original failing reproduction against unpatched code first, so these are fails-before/passes-after rather than assertions written after the fact.

Notes

This is the generic fix in the shared download helper, so it covers every caller. #143 separately adds SHA-256 validation for the Lemonade archive specifically; the two are complementary and touch different files — this one stops a truncated file from ever landing at the destination, that one catches a bad file that got there another way.

Fixes #158

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rominf
rominf added this pull request to the merge queue Aug 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 6, 2026
rominf added 2 commits August 6, 2026 13:57
Interrupted downloads and failed atomic writes left large partial files
on disk that nothing ever removed. Because each temp name embeds a
timestamp, every retry leaked a distinct orphan — so on a full disk each
attempt consumed more of the little space that was left.

Stream downloads to a sibling .part file and rename into place only once
the body is complete, removing the partial on every failure path. This
also stops a truncated transfer from landing at the destination, where
the lemonade engine would accept it as a valid cache entry forever.

Clean up the temp file on the failure paths of both write_file_atomically
copies, and preserve full file names so sdk.tar.gz no longer becomes
sdk.tar.tmp-<id>.

The SDK tarball is now streamed rather than read fully into memory, which
removes a multi-gigabyte allocation on the install path.

Refs #158

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The two dashboard scenarios that open Observe send the `4` tab key straight
after launching the TUI, with no assertion in between. A key written into the
pseudo-terminal before the dashboard is reading input can be consumed by
whatever holds the terminal at that moment, and nothing ever retries it — so
the dashboard stays on Home and the scenario fails 30s later in an assertion
about a view it never left.

Send the key until the Observe chip is actually marked active, so the step
depends on the dashboard having acted on the key rather than on it having been
ready when the key was written.

Reproduced by pointing ROCM_CLI_BINARY at a wrapper that drains the terminal
before exec'ing the real binary: the scenario failed with exactly the CI
symptom before this change and passes after it.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf force-pushed the fix/atomic-downloads-no-partial-leaks branch from 8c1ec0c to 7204154 Compare August 7, 2026 07:53
@rominf

rominf commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Merge-queue ejection: root cause and fix

This PR was ejected from the merge queue when the blocking E2E tests job failed on dash-managed-service-metrics (merge-group run 31102672143, job 92621417125):

FAIL: 'dash-managed-service-metrics' was expected to pass on this host but FAILED — a regression.
Step panicked: TTFT metrics did not appear: timed out after 30s waiting for "50ms".

Root cause

The captured screen in that log shows the dashboard still on Home — the 4 keypress that should have switched to Observe never took effect, so the step waited 30s for metrics on a view it never reached.

The two scenarios that open Observe launch the TUI and send the tab key with no assertion in between, unlike the demo-data journeys which assert the home view first. A key written into the pseudo-terminal before the dashboard is reading input can be consumed by whatever holds the terminal at that moment, and nothing retries it. The step then fails much later, in an assertion about a view the dashboard never left — which is exactly the failure signature above.

This is a latent synchronisation defect in the E2E step, not in this PR's production code: nothing in the download/atomic-write change is reachable from rocm dash. It surfaced here rather than in this PR's own checks because those ran against the older base.

Fix

Send the tab key repeatedly until the Observe chip is actually marked active, so the step depends on the dashboard having acted on the key rather than on it having been ready when the key was written. The helper is restricted to idempotent keys (a tab jump, not a toggle).

Reproduction

Deterministic, by pointing the harness at a wrapper that drains the terminal before exec'ing the real binary — i.e. a dashboard that is not yet reading when the key arrives:

cat > /tmp/wrapper/rocm <<'SH'
#!/bin/sh
stty raw -echo min 0 time 20; dd bs=1 count=16 of=/dev/null 2>/dev/null; stty sane
exec /path/to/target/release/rocm "$@"
SH
ROCM_CLI_BINARY=/tmp/wrapper/rocm cargo xtask e2e -- -n "Observe displays metrics"

Before: 1 scenario (1 failed) with the CI symptom verbatim. After: 1 scenario (1 passed). The sibling scenario (dash-loading-service-status), which uses the same step, also passes under the wrapper.

Verified in addition: full cargo xtask e2e matches the base commit exactly (the two diagnose-* failures are pre-existing on this host, confirmed on ec2bcb32 itself), cargo fmt --all --check, both clippy invocations, cargo xtask manifest --check, and cargo test --workspace --all-targets (only two pre-existing proc_lifecycle failures local to this host).

Note: the guard is exercised by these two scenarios on every run, but the key-loss condition itself is only reproducible with the wrapper above, so there is no automated test that would fail without it.

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.

[Issue]: Interrupted downloads leave orphaned partial files that are never cleaned up

2 participants