fix(deploy): keep manage after uninstall so manage purge stays reachable - #479
fix(deploy): keep manage after uninstall so manage purge stays reachable#479dormouse-bot wants to merge 2 commits into
Conversation
manage uninstall printed "use manage purge to delete those irreversibly" and then deleted $ROOT/bin, which is where manage itself lives. The command the banner pointed at no longer existed, so config/ and state/ could only be removed by hand or by reinstalling purely to get bin/manage back. Uninstall now removes bin/run-server rather than the whole bin directory, and says where manage still is. Purge, when bin/run-server is already gone, closes by printing the one command that clears what remains -- it cannot delete itself out from under the shell running it. Closes #473
Deploying mouseterm with
|
| Latest commit: |
ed8a59e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6dd515e9.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-issue-473.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Two findings, one of them the same class of bug this PR fixes.
purge's closing banner names a command that doesn't finish the job on Linux or macOS. It prints "this script is all that remains" and offers rm -rf "$ROOT", but on a real install the logs live outside $ROOT: the generated manage sets LOG_DIR="${XDG_STATE_HOME:-$HOME/.local/state}/dormouse-server/logs" on Linux and LOG_DIR="$HOME/Library/Logs/Dormouse Server" on macOS, while ROOT is ~/.local/share/dormouse-server / the install root. server.out.log and server.err.log survive the command the operator was told would clear everything — which is the shape of #473 itself, one directory over. Windows is unaffected: $LogDir = Join-Path $Root 'logs' there.
The verification in the PR body couldn't have caught it. Both manage scripts carry [ -d "$ROOT/logs" ] && LOG_DIR="$ROOT/logs" so a DORMOUSE_INSTALL_ROOT fixture — which is what the transcripts show — puts logs inside the root and makes the banner true. Worth re-running the fixture with the logs directory placed at the real path before calling this verified.
Inline suggestions below on both. If you'd rather keep the printed command to a single path, the alternative is to have uninstall (or purge) delete $LOG_DIR outright — logs aren't in the "preserved data" set the way config/ and state/ are, so there's no reason they have to outlive purge at all. Either way the prose needs the same edit: SELF_HOST.md says the command "clears whatever is left in the install root" and docs/specs/server.md says it "removes what is left", and both are only true today by the qualifier the second one omits.
The manage.cmd header comment in install-windows.ps1 is now false. In the $manageCmd here-string, the rem block justifying the trailing & exit says "manage uninstall" deletes bin -- this very file -- so that read fails. After this change uninstall deletes only bin\run-server.ps1, so nothing in manage ever removes manage.cmd and the documented failure mode is unreachable. That comment is outside the diff, so no inline suggestion — but it's the one place in the tree still asserting the behavior this PR removed, and it's load-bearing prose: it's the reason & exit is there at all, with a stated tradeoff (calling the .cmd from another batch script ends that script too) that now buys nothing.
Smaller: the new docs/specs/server.md paragraph lists what uninstall removes as "the releases, the pointers and bin/run-server" — run/ goes too, on all three platforms.
Everything else checks out. bin/ holds only run-server and manage (plus manage.cmd on Windows), so narrowing the delete strands nothing; cmd_rollback's [ -L "$ROOT/previous" ] guard and env_value's || return 1 mean the commands newly reachable on a torn-down install fail cleanly rather than under set -e; and the bin/run-server-absent gate keeps purge on a live install unchanged.
purge's closing banner offered `rm -rf "$ROOT"` and called it everything that remains, but on a real install the logs sit outside $ROOT: $XDG_STATE_HOME/dormouse-server on Linux, ~/Library/Logs/Dormouse Server on macOS. server.out.log and server.err.log survived the command the operator was told would clear the install -- the same shape as the bug this branch fixes, one directory over. Both banners now name the dormouse-owned log directory alongside the root. Windows is unaffected: $LogDir is inside $Root there. The earlier verification could not have caught it. Both manage scripts carry `[ -d "$ROOT/logs" ] && LOG_DIR="$ROOT/logs"`, so a DORMOUSE_INSTALL_ROOT-shaped fixture puts the logs inside the root and makes the old banner true. Re-ran the fixture with the logs at their real paths: at the previous head the two log files survive the printed command; here nothing dormouse-owned does, on either platform. Linux names LOG_ROOT (the dormouse-server directory) rather than its logs/ child, so the delete leaves no empty directory behind. On macOS LOG_DIR is already that level. Also: the manage.cmd header comment justified its trailing `& exit` with "manage uninstall deletes bin -- this very file", which this branch made false. Rewritten to say what is true now -- nothing in manage removes the file, and the guard is kept so the exit code does not depend on that staying so. And the spec's list of what uninstall removes omitted run/.
|
Both findings were real; fixed in ed8a59e, along with the The logs finding is the more serious one, and the review is right that my verification was structurally incapable of catching it. One deviation from the suggestion: on Linux the printed path is now On Verification — real-path logs, both platforms, plus the negativesMethod: extract the generated Linux, this head: Only the XDG directories, which must survive. macOS, same fixture: The space in the log directory name is quoted correctly and the command runs as printed. Baseline at 3e13275 (the previous head), identical fixture: Negatives, both platforms:
Windows: Lints: Docs
|
dormouse-bot
left a comment
There was a problem hiding this comment.
All three findings are fixed, and I re-derived each from the code rather than taking the reply at its word. On Linux LOG_ROOT is $XDG_STATE_HOME/dormouse-server, and that directory holds nothing but logs/ — the installer's only mkdir under XDG_STATE_HOME — so naming the parent is right and leaves ~/.local/state standing. On macOS $LOG_DIR is ~/Library/Logs/Dormouse Server, quoted correctly for the space, leaving ~/Library/Logs. Windows really is unaffected: $LogDir = Join-Path $Root 'logs'. run/ is in the spec's uninstall list and matches all three implementations. Threads resolved.
One observation, follow-up material rather than a blocker: nothing executes uninstall or purge. The Linux installer job in .github/workflows/ci.yml installs twice and then stops at bash -n "$root/bin/manage" — the generated script is parsed, never run — and deploy-lint.mjs is textual by design. So both the bug this PR fixes and the banner bug the last review caught sit in the one part of the tree with no executable coverage, which is why two rounds of hand-built fixtures were the only signal available.
Worth naming what a CI step would and wouldn't buy, because the obvious version is weaker than it looks. Running uninstall then purge against the existing test root would catch #473's shape directly — purge unreachable because bin is gone. It would not have caught the log-path bug: DORMOUSE_INSTALL_ROOT forces LOG_DIR="$INSTALL_ROOT/logs", so the generated manage takes the LOG_ROOT="$ROOT" branch and the banner's two paths collapse into one — the same blind spot the original transcripts had. Covering that half needs a fixture with HOME redirected and no $ROOT/logs. And cmd_uninstall refuses when stdin is not a terminal, so the step needs a pty (script -q -c ...), not a printf 'y\n' | pipe.
The change itself is correct as it stands.
manage uninstallprintedUse "manage purge" separately to delete those irreversibly.and then deleted$ROOT/bin— wheremanageitself lives — so the command it had just pointed at was gone by the time the banner was on screen.uninstallnow removesbin/run-serverinstead of the wholebindirectory, andpurgecloses by printing the one command that clears what is left — the install root and the log directory, which sits outside it on Linux and macOS. Verified by running the generatedmanagescripts against fake install roots (below); the bug reproduces onmainand does not on this branch.#473 left the fix as an operator-experience call and deliberately proposed none. You asked for the PR in #472, so this makes the call — preserve
manageacross uninstall, the second of the issue's three options — and is a single revertible commit if you want a different one.Why this option
The other two do not actually fix the reported thing. Reordering the message to "purge first, then uninstall" leaves anyone who already ran
uninstall— the common path — with no entry point, and inverts the natural order for everyone else. Advertisingpurgein the closing install banner is real but orthogonal: it improves discovery before teardown and changes nothing about reachability after it. Left out here so this stays one concern; happy to add it separately.Preserving
managegives the install root a statable invariant: uninstall removes the service and the code; what is left is your data plus the one tool that can delete it.config/andstate/were already preserved by design, sobin/managejoins them rather than being a new exception.purgedoes not delete itself. Self-deletion is the same class of problem this PR is fixing — on Windows in particular,cmd.exemay holdmanage.cmdopen for the duration, which I can't test from a Linux runner — so it prints therm -rf/Remove-Itemline instead. That line appears only whenbin/run-serveris already gone, so apurgeon a live install is unchanged.Side effect worth naming: the triage comment on #473 flagged that Windows'
Remove-Tree $binmay already be failing partially against that samecmd.exehandle, withInvoke-Uninstallreporting success regardless. That delete is gone now, so the hazard goes with it — but it was never confirmed on a Windows box, and this PR does not confirm it either.Verification
No automated coverage exists for this path —
DORMOUSE_INSTALL_TEST=1stops before systemd/launchd/Serve and never runsmanage uninstall, which is why the bug survived. So I extracted themanagescripts the installers generate and ran them against throwaway roots.The fixture has to put the logs at their real location. Both
managescripts carry[ -d "$ROOT/logs" ] && LOG_DIR="$ROOT/logs", so aDORMOUSE_INSTALL_ROOT-shaped root puts the logs inside it and makes a root-only teardown command look complete when it isn't — that is how the second bug fixed here got past the first pass. The transcripts below are from a fixture withHOMEpointed at a throwaway tree and no$ROOT/logs.Linux and macOS — full uninstall → purge, plus the baseline on
mainThe generated Linux
manage, run withROOT=$HOME/.local/share/dormouse-serverholdingbin/{manage,run-server},config/,state/,releases/,run/,current, and the logs at$XDG_STATE_HOME/dormouse-server/logs— outside the root, as on a real install:The command it names then works, and the command that prints finishes the job:
The macOS
manageproduces the LaunchAgent-worded equivalent with~/Library/Logs/Dormouse Serveras the second path — the space is quoted correctly and the command runs as printed, leaving~/Library/Logsand nothing of dormouse's.Baselines, same fixture. The original bug, using the
managegenerated byorigin/mainat01d1328c:And the logs bug, using the
managegenerated by this branch at3e132752:Negative cases, both platforms:
purgewithbin/run-serverstill present (a live install) printspurged.and stops there; and on aDORMOUSE_INSTALL_ROOT-shaped fixture, where the logs are inside the root, the two printed paths collapse andrm -rfhandles the duplicate.Windows — parse check plus a unit test of the changed branch
The generated
manage.ps1needsWindowsIdentityat load, so it cannot run end to end on a Linux runner. What I could do:[Parser]::ParseFileoninstall-windows.ps1and on the generatedmanage.ps1body — both clean.Invoke-Purgeand ran it underpwshwithRemove-Tree/Read-Hoststubs, with the onebin\run-server.ps1separator normalized for the Linux filesystem. Both branches behave: present →purged.and nothing more; absent → the teardown line with the root interpolated.Not verified on Windows: that
Invoke-Uninstall's[IO.File]::Delete($runServer)succeeds while the task is stopped, thecmd.exehandle question above, and that the& exitguard inmanage.cmdis still doing nothing harmful now that its stated cause is gone (see "Not included").deploy-lintremains the only automated signal for this file, asdocs/specs/server.mdsays.Lints:
spec-lint(24 specs),deploy-lint(27 checks) and its selftest,loopback-lint,xterm-lintall pass;bash -nclean on both shell installers and on both generatedmanagescripts.Not included
Removing the trailing
& exitfrommanage.cmd. Itsremblock justified it with "manage uninstalldeletes bin -- this very file", which this PR made false; the comment is rewritten to say so, but the& exitstays. Nothing inmanageremoves that file anymore, so the guard is unreachable — but the failure it prevents ismanage uninstallexiting 1 and looking broken, and I can't run a batch file from a Linux runner to confirm the removal is safe. That trade wants a Windows box, not an argument; happy to drop it if you'd rather.A
deploy-lintrule pinning the banner to the deletion would make this regression-proof, and #473 raises it. Left out because that lint is scoped to theFAIL IFclauses inSECURITY.md("Credentials at rest", "Network posture") and its header says so; a teardown-ordering rule would widen its charter. Worth doing if you want it there anyway — say the word.Specs
docs/specs/server.mdgained the teardown ordering as an invariant next to themanagesurface listing, since the reasonuninstallsparesbinis not visible from the code — including which paths the printed teardown command has to name on each platform, and why.SELF_HOST.mdcheckpoint 5 states the order explicitly, as #473 asked.