cli: reuse a fully-staged server download after an interrupted update - #331752
Open
srikanthananthula (srikanthananthula63053) wants to merge 1 commit into
Conversation
Fixes microsoft#331690 DownloadCache::create() unconditionally deleted its .staging directory before every download. If the process was killed after a download+ extract finished populating .staging but before the atomic rename into the final commit-keyed directory (e.g. VS Code closed right as a server update finished), the next launch discarded that already-valid download and re-fetched it from the update service, wasting bandwidth on a patch that was already available locally. Write a completion marker into the staging directory right after the do_create closure succeeds, before the rename is attempted. On the next call, a marker's presence means the staged content is already complete and can be renamed into place directly instead of being wiped and re-downloaded; a staging directory without the marker is still treated as partial and discarded, unchanged from before. The marker is stripped from the final directory after rename so callers that enumerate it don't see it. No call site of create() needed to change.
Copilot started reviewing on behalf of
srikanthananthula (srikanthananthula63053)
August 20, 2026 05:57
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Adds resumable finalization for completed CLI/server downloads interrupted before staging is renamed.
Changes:
- Adds a staging completion marker.
- Reuses completed staging directories.
- Adds cache recovery unit tests.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The marker is staging-only bookkeeping; strip it from the final | ||
| // location so callers that enumerate the returned directory (e.g. | ||
| // looking for the single extracted CLI binary) don't see it. | ||
| let _ = std::fs::remove_file(target_dir.join(STAGING_COMPLETE_MARKER)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #331690.
DownloadCache::create()(used by the Rust CLI's server/CLI installer atcli/src/tunnels/code_server.rs,cli/src/commands/serve_web.rs, andcli/src/tunnels/agent_host.rs) unconditionally deleted its.stagingdirectory before every download. If the process was killed after a download+extract fully finished populating.stagingbut before the atomic rename into the final commit-keyed directory (e.g. VS Code closed right as a server update finished downloading), the next launch discarded that already-complete download and re-fetched it fromupdate.code.visualstudio.com, wasting bandwidth on a patch that was already available locally — exactly the scenario described in the issue.Root cause
In
DownloadCache::create():<name>.stagingdirectory.<name>.stagingis renamed into the final<name>directory.<name>doesn't exist yet, any leftover<name>.stagingwas unconditionally wiped before starting a fresh download — with no way to tell "this is a partial download from a connection failure" apart from "this is a complete download that just never got renamed because the process was killed a moment too early".Fix
Write a small completion marker file (
.complete) into the staging directory immediately after thedo_createclosure (the download+extract+verify logic) succeeds, before attempting the rename. On the nextcreate()call:do_createentirely and go straight to the rename, avoiding any network request.The marker is stripped from the final directory after a successful rename, so callers that enumerate the returned directory (e.g. the CLI-binary installer, which picks the first entry via
read_dir) never see it.This is fully contained in
DownloadCache— none of its 4 call sites needed to change.Test plan
cli/src/download_cache.rsunit tests:test_existing_valid_target_is_reused_without_download— an already-installed commit is returned without invokingdo_create.test_missing_patch_triggers_download— nothing cached →do_createruns and its output lands at the final path.test_incomplete_staging_download_triggers_fresh_download— a staging dir without the completion marker (simulating a connection drop mid-download) is discarded anddo_createruns again, with stale partial content not surviving into the result.test_interrupted_update_restart_reuses_completed_staging_without_redownload— the core regression test: a staging dir with the completion marker (simulating a kill right before the final rename) is reused withdo_createnever invoked, and the marker itself doesn't leak into the final directory.