Repository navigation
supervisor: adopt only <id>, not swap dirs; a fresh crash record per install - #47
Merged
Merged
Conversation
scanInstalled adopted every subdirectory of the install root that held a valid manifest, without checking that the directory is named for the app. pilotctl's install leaves <id>.staging (from the manifest write to the swap) and <id>.previous (from the swap until it is retired) beside the live <id>, both holding a valid manifest for the same app. On a same-version rebuild the rescan swapped the new <id> in and then "rebuilt" the app again from .previous (and .staging): startOne overwrote appCancel[id], each supervisor's reapStale killed the other's process, and the app was suspended (a probe saw 9 restarts and 4 suspends in 12 s). At daemon start, Service.Start handed every scanned dir to run(), so a leftover .previous ran alongside the live app. A dir whose name is not its manifest's ID is now skipped, right after Validate and before any signature work. Each skipped dir is logged once, not on every 30 s rescan; a name that disappears is forgotten, so the next install's staging dir is logged again. Tests (zz8_install_dirs_test.go), both failing on ca6cce0: - TestRescanIgnoresStagingAndPreviousDirs: in-memory app pinned to sha A; on disk <id> (sha B), <id>.previous (sha A), <id>.staging. rescanForNew must return only <id>, and the next tick nothing. Before: "rescanForNew started [io.test.app io.test.app.previous io.test.app.staging]". - TestScanInstalled_SkipsDirNotNamedForItsApp: a .previous dir and an arbitrarily named copy are skipped and logged once over three scans. Before: "scanInstalled adopted [copy-of-test-app io.test.app io.test.app.previous]". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Crash records are keyed by app ID and recordCrash's suspended bit is sticky. rescanForNew cleared s.crashes[id] only for a same-version rebuild: the upgrade branch and the new-app tail kept it, and rescanForGone kept it when an app was uninstalled. So a v1 suspended for crash-looping made v2 report suspended and stop at its first exit, and an uninstall plus reinstall inherited the suspension. rescanForNew now deletes the record just before registering a new install or version, and rescanForGone deletes it with the install. The .suspended marker the old version left needs no change: superviseOne removes it when the new version's supervise goroutine starts. Tests (zz9_crash_record_test.go), both failing on ca6cce0: - TestRescanUpgradeClearsCrashRecord: v1 crash-loops past its budget, rescan to v2. isSuspended and Apps().Suspended are false, one recordCrash does not suspend, and superviseOne (pre-canceled context) removes v1's .suspended marker. Fails without the rescanForNew delete. - TestRescanReinstallClearsCrashRecord: suspend, remove the dir, rescanForGone, reinstall, rescanForNew. The record is gone after the uninstall and the reinstall is not suspended. Fails without the rescanForGone delete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
….previous An install interrupted between renaming <id> to <id>.previous and renaming <id>.staging to <id> leaves no <id>, so the supervisor adopts nothing and the app stops. The skip line now says the app is not installed and to run pilotctl appstore install <id>. Also fixes two comments: a garbled sentence about the crash record, and the crash-loop window, which an upgrade or reinstall now clears too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
Problems
Both were confirmed by probe tests on main (v1.0.5).
scanInstalledadopted every subdirectory with a valid manifest, including pilotctl's<id>.staging(manifest write → swap) and<id>.previous(swap → retire).appCancel[id]was overwritten, and the two killed each other's processes (reapStalematches the binary and socket paths). Every kill counted as a crash, and the app ended up suspended. A probe saw 9 restarts and 4 suspensions in 12 s..previousran beside the live app.appstore upgrade --allproduces same-version rebuilds.Fixes
scanInstalledskips a directory whose name is not its manifest's ID. Each skipped name is logged once, not on every 30 s rescan.rescanForNewclears the app's crash record when it (re)installs it, andrescanForGoneclears it on removal.Tests
Each test fails on main.
TestRescanIgnoresStagingAndPreviousDirsTestScanInstalled_SkipsDirNotNamedForItsAppTestRescanUpgradeClearsCrashRecordTestRescanReinstallClearsCrashRecordgo test -race ./...passes, and the new tests passed 20 runs in a row.Not changed:
go.modstays ongo 1.25.10. Dependabot #45 and #46 would raise it to 1.26.0, so they stay unmerged until after this ships.🤖 Generated with Claude Code