fix(start-core): make install rollback crash-recoverable, and snapshot a stopped service - #3598
Open
helix-nine wants to merge 1 commit into
Open
fix(start-core): make install rollback crash-recoverable, and snapshot a stopped service#3598helix-nine wants to merge 1 commit into
helix-nine wants to merge 1 commit into
Conversation
…t a stopped service A user cancelled a long package update and lost that service's database. The rollback is what deletes data: `restore_volumes_from_install_backup` deleted the live volume root and then renamed the backup into place, so an interruption between the two left the service with neither — and because `handle_installed` and `Bind::pre_mount` recreate a missing volume dir as an empty one, the result was indistinguishable from a completed rollback. The next update then discarded the surviving backup as stale, which is where the data actually went. Restore is now a two-rename swap: move the live root to `<pkg>.restore-old`, rename the backup into place, drop the aside tree. Between the renames both copies exist, and `<pkg>.restore-old` is a marker that survives an empty-skeleton recreation, so `resolve_pending_restore` can finish any interruption point deterministically instead of guessing which tree is authoritative. It runs from `ensure_volume_root` — the funnel every path uses before it trusts or creates the live root — as well as from the snapshot, the boot sweep, and `remove_install_backup`, which now refuses to discard a backup while a restore is in flight. The snapshot no longer deletes the previous rollback point as its first act: it snapshots to `<pkg>.install-backup-tmp` and swaps, so the old backup is only dropped once its replacement exists. A rollback that fails is now fatal to the load and notifies the user, rather than being swallowed by `log_err` and then having its rollback point deleted by the `Ok` path that followed. A failed first-time install over pre-existing data keeps those volumes for the restore instead of deleting them first. The boot sweep is no longer readdir-order dependent: it resolves every pending restore before any branch inspects a backup, so the orphan-reaping arm can't delete a backup that an unvisited marker still depends on. Finally, `ServiceRef::quiesce` stops the service's main chain before the snapshot, leaving the package's uninit for `uninstall`. The snapshot already predated uninit and is atomic, so this is not about the snapshot's internal consistency: it means nothing is writing to the tree a later rollback will rename or delete, formats that aren't crash-safe on their own survive, and the rollback point is the state the user had when they pressed update. It writes nothing to the status — clearing `started` would let the actor race a restart back in, and setting `desired` to Stopped would leave the service stopped after a successful update — guards on `is_initialized` (an uninitialized container's `stop` throws), and is bounded, because proceeding un-quiesced beats hanging an update on a wedged daemon.
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.
Why
The rollback is the only thing in the stack that deletes a populated volume during an update, and it was doing it non-atomically:
Interrupt between those two and the service has neither copy. Worse, it doesn't look interrupted:
handle_installedandBind::pre_mountboth recreate a missingdata/<vol>as an empty dir, so the next boot sees a live root that exists, andrecover_and_sweep's "backup with no live volume" test — the one recovery we had — doesn't fire. The stranded backup then gets deleted by the next attempt'ssnapshot_volumes_for_install, whose first act was "remove any stale backup". That last step is where the data actually goes.What changed
Restore is a two-rename swap.
live → <pkg>.restore-old,backup → live, drop the aside tree. Between the renames both copies exist, and.restore-oldis a marker that survives the empty-skeleton recreation, so every interruption point maps to exactly one resolution:restore-old+backuprestore-old, nobackuprestore-old, nobackup, noliverestore-old+backup+ aliveholding real filesresolve_pending_restoreis called fromensure_volume_root— the funnel every path goes through before it trusts or creates the live root — plus the snapshot, the boot sweep, andremove_install_backup, which now refuses to discard a backup while a restore is in flight.The snapshot stops destroying the only surviving copy. It snapshots to
<pkg>.install-backup-tmpand swaps, so the previous rollback point is dropped only once its replacement exists.A failed rollback is fatal and visible. It was
.log_err()'d, and then theOkpath that followed calledremove_install_backup— so a restore that died mid-swap got its rollback point deleted by the code meant to tidy up after success. It now notifies and propagates.A failed first-time install over pre-existing data keeps that data. The
Installingarm rancleanup(soft=false)— an unconditional whole-root delete — before checking whether a backup existed. That arm is what a 0.3.x→0.4.0 package conversion runs under, sincev0_3_6_alpha_0resetspackageDatato{}and reinstalls.The boot sweep is no longer readdir-order dependent. The orphan-reaping arm could delete a backup that an unvisited
.restore-oldmarker still depended on. It now resolves every pending restore in a first pass. There's a test for exactly that.ServiceRef::quiescestops the main chain before the snapshot, leaving uninit touninstall.On the ordering, precisely
You asked for stop → snapshot → uninit. Snapshot-before-uninit was already true (
service_map.rs:336vs:376) and, being a CoW subvolume snapshot, uninit's writes can't reach it — so the genuinely new thing here is the quiesce, and I've kept the uninit ordering rather than claimed to have introduced it. What quiescing buys is not the snapshot's internal consistency (it's atomic, so a WAL store would replay fine) but: nothing is writing to the tree a later rollback will rename or delete, formats that aren't crash-safe on their own survive, and the rollback point is the state the user had when they pressed update.quiesce()deliberately writes nothing to the status. Settingdesired = Stoppedwould have left every successfully-updated service stopped (init()normalizesBackingUp/Restartingbut passesStoppedthrough), and clearingstartedwould let the actor's watch loop race a restart back in. It guards onis_initialized()because an uninitialized container'sstophandler throws rather than no-oping, and it's bounded at 5 min — proceeding un-quiesced beats hanging an update on a wedged daemon.Tests
11 new unit tests over the state machine, table-driven across {live present / skeleton / absent} × {backup} × {restore-old} × {owner installed / mid-flight / absent}.
recover_and_sweepalready took the volumes root as a parameter and the two protocol functions now do too, so these run on any filesystem with no btrfs, no root, and no fault injection — which matters, because the CI runner has neither CAP_SYS_ADMIN nor a btrfs workspace.cargo test -p start-core --lib: 556 passed.Deliberately not in this PR
start-os/v0.4.0.1is a cut tag, so I added a new## [0.4.0.2]CHANGELOG heading, but I left the manifest half alone — rootpackage.json, theCargo.tomllabel + lock,version/v0_4_0_2.rs+version/mod.rs, and the docs release links. Cutting the next OS version is your call, and doing it here would collide with anyone else doing the same. Say the word and I'll add it as a second commit.cancel_installis an abandon, not a cancel.UnixRpcClient::requesthas no cancellation, sotokio::select!drops the future while the package's migration keeps running; teardown then falls toDrop for PersistentContainer, which spawns a detacheddestroy(None)that sendsrpc::Exitwith a real target — i.e. the package's uninit runs concurrently with the detachedload(Undo)doing the renames. Two writers, one volume tree, no barrier. This is a plausible independent mechanism for the original incident and I think it wants its own change.Updating → Installed(old)DB flip still commits before the renames. The on-disk marker makes that recoverable, but arollback: boolonUpdatingState(honoured at boot instead of always loadingRetry) would also stop a crash mid-rollback from resuming the update the user cancelled.Bind::pre_mount);snapshot_subvolumeis non-recursive, so a nested subvolume would land in a backup as an empty dir.Test plan
cargo test -p start-core --lib— 556 pass.<pkg>.install-backupis gone afterwards.kill -9startd between the two renames (or drop a<pkg>.restore-oldby hand), reboot, and confirm the boot sweep finishes the restore rather than starting the service on empty data.