Skip to content

Fix an update leaving Maestro unable to replace maestro-server - #329

Open
RBilly wants to merge 1 commit into
mainfrom
maestro/rich-nebula-74
Open

Fix an update leaving Maestro unable to replace maestro-server#329
RBilly wants to merge 1 commit into
mainfrom
maestro/rich-nebula-74

Conversation

@RBilly

@RBilly RBilly commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

After updating on Windows, Maestro could land in a state where no local session would start — reported as "the update becomes unusable because the replacement of the maestro-server failed, the service is still running while trying to replace the file".

What was happening

deployment_version() is <app version>-protocol-<n>, so every update invalidates the cached server binary and ensure_cached_binary re-downloads it. download_server_binary finished with a bare rename(tmp, dest) onto %APPDATA%\com.maestro.app\bin\maestro-server-windows-x86_64.exe.

Windows will not let the image of a running process be overwritten. Any surviving maestro-server.exe made that rename fail with ERROR_ACCESS_DENIED, and the error was fatal with no retry — download_server_binaryensure_cached_binaryensure_local_serveropen_local_transport. The stale binary stayed in place, so relaunching just repeated the failure, and the .download-tmp leaked on every attempt.

A server survived quitting because nothing stopped it. kill_on_drop(true) only fires when the Child is dropped inside the runtime, and handle.exit(0) dropped neither ACP map. release_active_project_lock does clear connection_servers — its comment even claims that covers "leaving the project or quitting" — but the quit path never called it that way. An installer-forced kill or a crash leaves the same orphan with no chance to exit at all.

The fix

Make the swap survive a locked destination. Renaming a running image is permitted on Windows even though overwriting and deleting are not, so install_binary_at moves the old binary aside to <name>.old-<nanos> and lets the new one take its name — the same trick install_local_link already used for ~/.local/bin a hundred lines earlier. Deleting the aside copy only succeeds once its process exits, so sweep_stale_binaries reclaims it on a later run. Failure paths restore the old binary rather than leaving nothing at the path, and clean up the partial download.

Stop orphaning servers on quit. stop_connection_servers clears acp.sessions and acp.connection_servers in the CloseRequested handler, so kill_on_drop fires.

A graceful stdin-EOF shutdown was considered and does not work here: the shared reader task holds its own writer_tx clone until it sees EOF, so the pipe never closes and the wait would always time out. Clearing the maps is what the leave-a-project path has always done.

Testing

cargo test -p maestro --lib — 402 passed. cargo clippy --workspace --all-targets -- -D warnings clean.

Three new tests in deploy.rs. The Windows one reproduces the real failure rather than approximating it: it copies ping.exe into place as the destination and spawns it so a live process holds that image, asserts the plain rename fails so the test proves something, then asserts install_binary_at succeeds anyway, leaves exactly one .old- sibling, and that the sweep reclaims it once the holder is killed.

Not yet verified by a manual GUI run that no maestro-server.exe survives a normal quit; that half is verified by inspection against the release_active_project_lock path it mirrors.

Release Notes:

  • Fixed an update on Windows leaving Maestro unable to start sessions when a leftover maestro-server process still held the old binary

🤖 Generated with Claude Code

`deployment_version()` folds in the app version, so every update invalidates
the cached server binary and `download_server_binary` re-downloads it. It
finished with a bare rename onto the cached path, and Windows will not let the
image of a running process be overwritten — so a surviving maestro-server.exe
made that fail with ERROR_ACCESS_DENIED. Nothing retried it and the stale
binary stayed in place, so the updated app could not start a single local ACP
session, and relaunching only repeated the failure.

Renaming a running image *is* permitted, so `install_binary_at` moves the old
binary aside to `<name>.old-<nanos>` and lets the new one take its name, the
same trick `install_local_link` already used for ~/.local/bin. Deleting the
aside copy only works once its process exits, so `sweep_stale_binaries`
reclaims it on a later run.

A server survived in the first place because quitting never stopped one:
`kill_on_drop(true)` fires when the `Child` is dropped inside the runtime, and
`handle.exit(0)` dropped neither ACP map. Clearing both on close is what
`release_active_project_lock` already does when the user leaves a project —
its comment claimed to cover quitting too, but nothing called it that way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant