Skip to content

fix(arcup): check disk space in TMP_DIR, not just BIN_DIR - #304

Open
batuhankocyigit wants to merge 2 commits into
circlefin:mainfrom
batuhankocyigit:fix/arcup-tmp-dir-disk-space
Open

fix(arcup): check disk space in TMP_DIR, not just BIN_DIR#304
batuhankocyigit wants to merge 2 commits into
circlefin:mainfrom
batuhankocyigit:fix/arcup-tmp-dir-disk-space

Conversation

@batuhankocyigit

Copy link
Copy Markdown

Summary

main() in arcup/arcup calls check_disk_space exactly once, on BIN_DIR:

mkdir -p "$BIN_DIR"
check_disk_space "$BIN_DIR"

But the release archive is downloaded into TMP_DIR (mktemp -d, usually
/tmp) and also extracted there before the binaries are copied to
BIN_DIR. So TMP_DIR briefly needs to hold both the compressed archive
and its extracted contents (~2x the final install size), and it is never
checked at all.

TMP_DIR and BIN_DIR are frequently different filesystems — e.g. /tmp
is a small tmpfs in many containers/CI images while BIN_DIR
($ARC_DIR/bin, under $HOME) sits on a much larger disk. In that case the
BIN_DIR check passes even though there isn't enough room in TMP_DIR, and
the user gets an opaque curl/tar failure instead of the intended
friendly Insufficient disk space error.

Fix

Add a check_disk_space call for TMP_DIR as well, with a higher
threshold (1000MB vs. the 500MB default) since it temporarily holds both
the archive and its extracted contents.

check_disk_space "$BIN_DIR"
check_disk_space "$TMP_DIR" 1000

Testing

  • Added test_check_disk_space_rejects_insufficient: direct unit test that
    check_disk_space errors with Insufficient disk space when the
    required size exceeds what's available.
  • Added test_main_checks_tmp_dir_disk_space: end-to-end regression test.
    Points TMPDIR at a directory whose fake df output reports ~1MB free
    while BIN_DIR reports effectively unlimited space, runs the real
    arcup binary via arcup -i 1.2.3, and asserts it fails with the
    Insufficient disk space message (proving the failure comes from the new
    TMP_DIR check and not the pre-existing BIN_DIR check).
  • Ran the full suite locally: all 27 assertions pass (25 pre-existing + 2
    new).
$ bash arcup/test_arcup.sh
...
ok - install_binary rejects symlink
ok - check_disk_space rejects insufficient space
ok - main rejects insufficient TMP_DIR disk space

No functional changes outside arcup/arcup and arcup/test_arcup.sh.

@osr21

osr21 commented Sep 1, 2026

Copy link
Copy Markdown

Verified this independently at ec3a656 — the premise, the placement, and both tests check out, and running your e2e test against main surfaced a second, related defect worth a maintainer's eye. Two review points at the end.

Confirmed on main:

  • check_disk_space has exactly one call site (BIN_DIR, line 753), while the archive is downloaded (line 775) and extracted (line 818) into TMP_DIR before install_binary copies out — so the ~2x transient footprint claim is structurally right. TMP_DIR is assigned at line 50 (mktemp -d), so it's live at the new check's position, and check_disk_space already takes the optional MB argument (default 500), so 1000 slots into the existing signature without touching the function.
  • Full suite: 27/27 on Linux, and the regression test does fail on main — with main's arcup and this branch's test file, main() sails past the fake 1MB TMP_DIR and dies much later with a misleading error.
  • Merge-tested against the open arcup queue: applying fix(arcup): a release is newer than its own pre-release #300, fix(arcup): accept a valid checksum written in uppercase hex #302, and fix(arcup): accept checksum files without a trailing newline #243 in sequence and then this PR merges clean at each step — the main() hunk and end-of-file test additions stay disjoint from all of them.

Review point 1 — the 1000MB constant is ~5.4x the measured need. I downloaded and extracted the real v0.8.0 x86_64-unknown-linux-gnu release: 52MB archive + 133MB extracted = 185MB peak in TMP_DIR. Requiring 1000MB free means a machine with, say, a 512MB tmpfs /tmp — a common container default — that installs fine today gets hard-refused after this PR. That converts a false negative (no check) into false positives (spurious refusals), which is the worse failure mode for an installer. 250–300MB keeps comfortable headroom; even the plain 500 default is 2.7x measured peak. Worth sizing to the data rather than doubling on instinct.

Review point 2 (observed, not this PR's fault) — on curl 8.14.x, download_file's error handling is silently defeated. Running your e2e test against main on a box with curl 8.14.1, the failure message wasn't download_error's friendly output — it was Checksum file is empty: …. Cause: curl 8.14.0/8.14.1 return exit 0 on a 404 when --retry is combined with --fail (upstream curl/curl#17554, fixed in later releases; --retry 0 or no retry exits 22 correctly). Since CURL_RETRY_ARGS includes --retry 3, if ! curl_with_headers …; then download_error never fires on those curl versions, and a failed download cascades into whichever downstream step trips first. Everything still fails closed — checksum verification refuses the install — but the "opaque error instead of the friendly one" problem this PR fixes for disk space exists for downloads too, on a curl line that shipped in current distros/nixpkgs. A [[ -s "$output_path" ]] check after the curl in download_file would immunize it regardless of curl version. Happy to see that spun off separately; flagging it here since this PR's own test is what exposed it.

The recurring gap: no ARCUP_INSTALLER_VERSION bump. The script header requires an increment for any modification and the self-update check depends on it. Note the coordination problem already queued: #300, #243, and #262 each claim 0.2.0 → 0.2.1, so whichever subset lands, later ones renumber at land time — but the bump should be present in the diff.

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.

2 participants