Repository navigation
fix(aqua): keep the lock-time artifact download out of TEMP - #12064
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe Aqua backend stores lock-time provenance downloads under the configured cache directory and validates Windows path limits. A Windows end-to-end test compares lockfiles generated with normal and excessively long ChangesLock temporary-directory independence
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change keeps lock-time artifact verification in mise’s cache instead of TEMP, preventing lockfile contents from varying with the machine’s TEMP path. No actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant LockWorkflow
participant AquaBackend
participant CacheDirectory
participant Downloader
LockWorkflow->>AquaBackend: Verify lock-time provenance
AquaBackend->>CacheDirectory: Create lock-provenance scratch directory
AquaBackend->>Downloader: Download verification artifacts
Downloader-->>AquaBackend: Return artifacts
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR moves Aqua lock-time provenance downloads from the process temporary directory into a sweepable cache directory and adds Windows path-length diagnostics.
Confidence Score: 4/5The PR should not merge until deep MISE_CACHE_DIR values no longer cause lock-time verification to remove provenance from generated lockfiles. The move fixes TEMP dependence, but the independently configurable cache root remains part of every scratch path; when Windows rejects a generated path, the error branch still clears provenance and writes a machine-dependent lockfile. The author’s reply says the issue was fixed by adding a diagnostic, but the current code provides the concrete residual counterexample: it still treats the path failure as a warning and assigns provenance to None. Files Needing Attention: src/backend/aqua.rs Important Files Changed
Reviews (6): Last reviewed commit: "fix(aqua): report the scratch length aft..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ddee104 to
6ddbd3f
Compare
|
Could you put this scratch directory under a dedicated cache path such as These artifacts are disposable and are not reused by installation. AI-assisted — Tool: Codex; model: OpenAI/GPT-5; version: unavailable. |
6ddbd3f to
4685094
Compare
|
Done in You are right about the orphan, and the part I had not thought through is that the two directories are swept by different things. I moved this to It is now The unit test moved with it: it pinned the scratch under |
4685094 to
35b5aca
Compare
`mise lock` verifies provenance for the current platform by downloading the
release artifact and checking it cryptographically. That download went to
`tempfile::tempdir()`, i.e. under TEMP, which makes the generated lockfile depend
on where the machine points TEMP.
On Windows it does so visibly. The download lands at
`<TEMP>\.tmpXXXXXX\<artifact filename>`, and once that reaches MAX_PATH it fails:
mise WARN lock-time provenance verification failed for jqlang/jq,
will be verified at install time: (os error 3)
`mise lock` still exits 0, but the `provenance` line for the current platform is
gone from mise.lock. Measured with jq 1.8.2 (`jq-windows-amd64.exe`, so the
artifact path is TEMP + 35), varying only the length of TEMP:
TEMP=210 -> path 245, provenance recorded
TEMP=230 -> path 265, provenance dropped
TEMP=250 -> path 285, provenance dropped
The threshold moves with the artifact's filename, so it differs per tool.
Nothing else in mise is affected: install, reshim, doctor, cache clear, ls and
upgrade --dry-run all succeed with TEMP at 250. This is the only path that puts a
release artifact in TEMP — install already downloads to `MISE_DOWNLOADS_DIR`.
Point the lock-time download at `MISE_DOWNLOADS_DIR` too. It is still a `TempDir`
removed on drop, so this changes where the scratch lives, not how long.
The consequences were mild and are unchanged by this: with no `provenance` in the
lockfile, `has_lockfile_integrity` is false, so install runs the real verification
rather than trusting the lockfile, and the line is restored. What was actually
wrong is that a committed artifact's contents varied by machine.
Follow-up to review. The scratch directory went under MISE_DOWNLOADS_DIR because that is where install puts the same artifact — but this copy is verified and thrown away, never reused, and `TempDir` cleanup is best-effort. A crash, an abort or a failed delete strands a randomly named `.tmpXXXXXX` at the root of a persistent directory that nothing sweeps: not `mise cache clear`, not the per-tool download cleanup. `MISE_CACHE_DIR/lock-provenance` keeps the download independent of TEMP, which is what this PR is for, while leaving any orphan somewhere ordinary cache clearing collects it. Added as its own commit rather than folded into the first, so the review already on this branch still applies to what it reviewed.
…ot fit
Follow-up to Greptile's review, which is right and whose earlier version I had
pushed back on for a reason that does not transfer here.
Moving the scratch out of TEMP removed one environment variable from the answer,
not the arithmetic. MISE_CACHE_DIR is configurable too, and the longest path in
this operation is not the artifact but the download's own sidecar,
`.{filename}.mise-part.json`, written beside it by http::PartialDownload. Past
MAX_PATH that comes back as `os error 3`, the caller warns and clears provenance,
and the lockfile loses the entry for this platform.
The defence used against the same argument about MISE_DOWNLOADS_DIR was that
install writes a longer path under the same root and therefore fails first,
loudly. That does not hold for the cache: the two roots are independent.
Measured on 2026.8.6 with MISE_CACHE_DIR at 200 characters — `mise install` and
`mise lock` both exit 0 and provenance is recorded, so nothing else in mise
fails at that depth to hint at why.
So check the length before downloading and say what to change. This does not
turn `mise lock` into a failure: the caller already catches this and degrades to
install-time verification, so what changes is that the reason is legible and
names MISE_CACHE_DIR instead of being a bare OS error code.
Its own commit again, so the review already on this branch still applies to what
it reviewed.
35b5aca to
8a60147
Compare
…ing before The previous commit checked one path — the artifact's `.mise-part.json` sidecar — before downloading. That is not enough, and Greptile was right to say so: the same scratch directory later receives a checksum file, a provenance or signature file, and each takes its name from a URL. A check on the first file passes while a later, longer one does not, so the guard could be satisfied and verification still fail with the lockfile quietly losing provenance. Enumerating those files is the wrong shape. Annotate the failure instead: when lock-time verification fails, append the scratch directory's length and name the variable to shorten. That covers every file written there, including any added later, because it reacts to what happened rather than predicting what will. This still does not make `mise lock` fail — the caller already catches the error, warns, and degrades to install-time verification. What changes is that the warning carries a reason. Also drops the local MAX_PATH: jdx#12058 merged, so `file::MAX_PATH` is the shared one, as its body said it would be.
8a60147 to
83faea0
Compare
mise lockverifies provenance for the current platform by downloading the release artifactand checking it cryptographically. That download goes to
tempfile::tempdir()— underTEMP— which makes the generated lockfile depend on where the machine pointsTEMP.Found while sweeping Windows behaviour under a long
TEMPon 2026.8.6 windows-x64.What happens
<TEMP>\.tmpXXXXXX\<artifact filename>is the download target. Once that path reachesMAX_PATHthe download fails:mise lockstill exits 0, and the lockfile is still written — but theprovenanceline forthe current platform is gone from it. Same project, same command, only the length of
TEMPdiffers:
[tools.jq."platforms.windows-x64"] checksum = "sha256:a6fc67fe…" url = "https://github.com/jqlang/jq/releases/download/jq-1.8.2/jq-windows-amd64.exe" url_api = "https://api.github.com/repos/jqlang/jq/releases/assets/453012788" -provenance = "github-attestations"The cross-platform entries are unaffected: lock-time verification only runs for the current
platform, so the others keep the provenance they got from registry metadata.
Measurement
jq-windows-amd64.exeis 20 characters and the scratch directory adds\.tmpXXXXXX\, so the artifact path isTEMP + 32. The thresholdwas predicted from that and then measured — Windows 11 26200,
LongPathsEnabled=0:TEMPlengthprovenancein mise.lockThe threshold moves with the artifact's filename, so it is different for every tool.
Nothing else in mise is affected. With
TEMPat 250,install,reshim,doctor --json,cache clear,lsandupgrade --dry-runall exit 0. This is the onlypath that puts a release artifact in
TEMP.Why this is the fix
mise already owns a configurable place for exactly this file: install downloads the artifact
to
MISE_DOWNLOADS_DIRviatv.download_path(), under a per-tool path its own cleanup owns. The lock-time download is the same artifactfrom the same URL, and it was the only one going somewhere else. Pointing it at
MISE_CACHE_DIR/lock-provenancetakes the lockfile'scontents out of the environment's hands.
It is still a
TempDirremoved on drop — this changes where the scratch lives, not howlong. Reusing the downloaded artifact at install time would be a real improvement (today it
is fetched twice) but is a behaviour change, so it is deliberately left out.
How bad was it
Mild, and this PR does not change that part — measured, not assumed:
provenancein the lockfile,has_lockfile_integrityis false, so install takesthe
verify_provenancebranch rather than trusting the lockfile. Verified end to end:installing from the degraded lockfile logs
verify GitHub artifact attestations→GitHub attestations verified.So there is no verification gap. What was actually wrong is that a committed artifact's
contents varied by machine — a lockfile written on a box with a deep
TEMPdiffers by oneline from the same lockfile written anywhere else, and the difference bounces back on the
next install.
Tests
src/backend/aqua.rs) —lock_time_download_dir()returns a directory underMISE_CACHE_DIR/lock-provenance. Cheap, no network, and it is the property the fix is about.e2e-win/lock_temp_independence.Tests.ps1— runsmise locktwice, once with anordinary
TEMPand once with a 230-character one, and requires the two lockfiles to beidentical. The control run is also asserted to contain
provenance, so the comparisoncannot pass vacuously if lock ever stopped recording it at all.
Notes
cargo fmt --allonly.cargo testandwindows-e2eare left toCI.
e2e/lockfile/*tests drivemise lock --platform "$PLATFORM"; this change moveswhere the scratch directory is created and nothing about what is written.
Summary by CodeRabbit
Bug Fixes
mise lockreliability when temporary-directory paths are unusually long on Windows.Tests
Follow-up after review
The scratch first went under
MISE_DOWNLOADS_DIR, reasoning only about where install puts the sameartifact. jdx pointed out the part that reasoning missed: the two directories are swept by different
things. Install's copy lands under a per-tool path its own cleanup owns; this one is a
.tmpXXXXXXat the root, owned by nothing —
TempDircovers the ordinary path and neithermise cache clearnor the per-tool download cleanup covers the rest, so a crash or a failed delete would strand it
indefinitely.
It is now
MISE_CACHE_DIR/lock-provenance, in its own commit so the earlier review still applies towhat it reviewed. The unit test moved with it.