Repository navigation
feat(update): show download progress and abort only stalled downloads - #15382
ealeksandrov wants to merge 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
roboomp
left a comment
There was a problem hiding this comment.
P2 — terminal progress addresses #9499, but replacing the 15-minute total deadline with an unbounded transfer and 60-second stall timeout needs maintainer sign-off for full and patch downloads.
Should-fix: add external contributor credit and the PR link to the new packages/coding-agent/CHANGELOG.md:9 entry.
Verification: targeted update tests could not start in this checkout because src/export/html/tool-views.generated.js is missing. Maintainer: is the stall-only timeout policy acceptable? Thanks @ealeksandrov.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a37c12a20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
What
downloadVerifiedBinary(used for both full and patch downloads) now:42.1MB / 180.3MB (23%), 1.2MB/s, 1m50s left. Non-TTY output is unchanged.Download stalled: no data received for 60 seconds). A slow but steady connection can now finish.The stale-temp sweep used to rely on that 15-minute deadline: a
.new/.patchtemp older than it could not belong to a live download. Without a total limit that no longer holds, and on Windows a file's mtime may not change until its writer closes it. The sweep now also spares any temp whose updater pid (already in the file name) is still running, viaprocmgr.isPidRunning. The 15-minute age rule still applies to temps whose updater has exited, and to legacy names.Why
On my network
omp updateprintedDownloading omp-darwin-arm64…and nothing else for a long time, then in some cases failed withTimed out downloading release binary after 15 minutes. It's both annoying to never see any progress and wait for a very long timeout when target download is completely inaccessible.Fixes #9499. Supersedes #9570, which no longer applies to
mainafter the build-service/patch rewrite of the update path.Testing
bun run build), copied it to a scratch directory first onPATH, and ranomp update --force(the branch reports the current release, 18.9.1). In a terminal the progress line updated in place through 197.7MB, then the build verified, installed and passed the version check. With output piped, the run printed the same lines as before this change and no progress.fetch: when the server stops sending halfway, the download aborts after about 62 seconds with the stall message and removes the partial file.bun test test/update-cli.test.ts: two fake-timer tests replace the old body-timeout test. A download receiving one byte every 20 seconds for minutes completes; a body that stops sending aborts after 60 seconds and leaves no file. Removing thestallTimer.refresh()call makes the first test fail. The sweep test now uses a pid that has exited for its orphaned temps (the old hard-coded4242could be a live process) and checks that an old temp owned by a running process survives. One unrelated test (overrides per-tool release age during actual mise upgrade resolution) fails on unmodifiedmainin my environment too, because the local mise cannot resolve thegithub:tool.bun checkpasses (TypeScript and Rust).bun checkpasses