feat(start-os): offer to wait for a running backup before restarting or shutting down - #3755
Open
helix-nine wants to merge 7 commits into
Open
feat(start-os): offer to wait for a running backup before restarting or shutting down#3755helix-nine wants to merge 7 commits into
helix-nine wants to merge 7 commits into
Conversation
…or shutting down Powering the server off part-way through a backup can corrupt the backup of whichever service is being written at that moment, and nothing stopped it: Restart and Shutdown took effect immediately, and the case power button went straight to systemd-logind's poweroff. Restart or Shutdown during a backup now opens a prompt offering to wait for the backup to finish, which is also what happens if the user walks away — it counts down and takes that option for them. The choice is recorded server-side as serverInfo.statusInfo.deferredPowerAction, so it survives the browser: a task started at boot watches for the backup to clear and then carries the action out, an action bar in every connected UI says what is coming, and either the bar's Cancel or an immediate restart/shutdown clears it. The power button reaches the same place. While a backup runs, startd holds a systemd-logind 'handle-power-key' block inhibitor and reads the key itself from the devices udev tags 'power-switch' — logind's own set — so a press defers rather than powers off. It names 'handle-power-key' and not 'shutdown' deliberately: blocking 'shutdown' would also block the power-off StartOS asks systemd for at the end of its own teardown. Every step degrades to today's behavior on failure, and a beep acknowledges the press for whoever is standing at the server. Deferring is opt-in (--after-backup) so startos-shutdown.service and startos-restart.service, which cannot wait, are unchanged.
- power_key is Linux-only (libc::input_event, nix major/minor), so gate the module and its spawn on target_os = "linux"; it broke the apple-darwin build. - run_deferred_power_actions no longer returns after one action or one failure, so a later deferral is still honoured, and it backs off instead of spinning when the take mutation keeps failing. - Fold the backup check and the power action into one mutation, so a backup starting in between can no longer be interrupted. - Clear a stale backupProgress (and any deferred action) when startd starts without a reboot. A backup dies with the process that ran it, and a stale flag would otherwise leave the power key inhibited with nothing left to release it. - Hold the logind inhibitor only while the key is actually being read, so a server whose key StartOS cannot see keeps powering off as it does today rather than getting an inert button. - Route every in-app restart/shutdown through PowerService, so the restart-to-apply bar and the disk-repair restart stop bypassing the prompt. - Show one action bar at a time; they are position:fixed and were stacking. The deferred-power bar keeps its own gate, since the shared one is switched off for good on the first restart. - One timer drives the countdown label and the default it takes. - Mock clears the deferral on an immediate action, as the backend does. - Docs: the power button defers without asking, the CLI still interrupts unless asked not to, and shutting down on UPS battery wants "now".
… bar A second review round on the previous commit found that chaining the three action bars with @else if made the new one unreachable: statusInfo.restart is set on a hostname, language, kiosk or update change and is never cleared until the next boot, so it shadowed the deferred-power bar — the only place a queued restart or shutdown is shown, and the only way to cancel one. - Put the deferred-power bar first: it is transient and time-limited, the restart-reason bar is neither, and it is never gated off. - PowerService now reports whether the action was deferred, so the shared bar is only latched off once a restart is really under way. Dismissing the prompt used to hide the restart-to-apply bar for good with nothing having happened. - Give up the power key entirely when any one device stops being readable — there is no telling which device the firmware reports presses on, and an inhibitor outliving the reader is an inert button. - Re-defer rather than interrupt if a backup started since the deferred action was taken. - Reset the whole transient status, on the branch that skips init and before the RPC surface is serving. - Docs: ARCHITECTURE says the power-key handling is best-effort, the CLI reference lists --nowait alongside --after-backup, and a few comments now say what the code does.
…d's key codes Review round 1 on #3755. - The power key is now enumerated, inhibited and read once per backup rather than once per boot. Unplugging any keyboard used to end `read_power_key` for good — udev tags every key-capable device `power-switch`, so that is an ordinary event, not an error — leaving later backups unprotected with only a log line to say so. Per-backup setup also picks up a device plugged in since boot, and stops startd holding input devices open when nothing is at stake. - Match `KEY_POWER2` as well as `KEY_POWER`. logind handles both in one arm, and the inhibitor blocks both, so on hardware reporting 356 the button was inhibited and unread — inert for the whole backup. - A deferred action that fails to perform is put back rather than dropped, so it is either carried out or cancelled, never silently lost. - The backup prompt now carries the "will not come back online automatically" warning on the shutdown path. Skipping the usual confirm dropped it, and it is the one thing a user powering down needs to have read. - String 915 said "Powering down" but is shown on the restart path too; it now says "Interrupting it", in all five locales. - `STATUS_INFO_PTR` lived in two modules; `power.service`'s TSDoc claimed the emission reflected the server's decision when it reflects the user's choice; ARCHITECTURE.md restated the module's own rationale; a few comments narrated rather than explained. - surge-and-ups says the power button always waits, so on battery use the UI.
Review round 2 on #3755, plus one defect of my own found re-reading round 1. - Round 1's re-arm of a failed deferred action called `defer_or_begin`, which with the backup already over takes its other branch: it cleared the action and set `shuttingDown`/`restarting` without performing anything, leaving the header reading "Shutting down" until startd restarted. It writes the field directly now. - A failed logind `Inhibit` propagated out of `guard_backups` and ended the guard for the rest of the boot, while enumeration and read failures beside it only skipped one backup. A logind restart or a call timeout under backup load therefore left every later backup unguarded, silently. One backup's guard is now one fallible unit, so any failure costs that backup and no more — which is what the module doc already claimed. - The prompt's two buttons are sentences that do not shrink, and 25rem cannot hold them on one row in any locale — English overflows the dialog by 51px, German by 137px. The footer wraps now.
…n restart too Review round 3 on #3755. - Round 2 added `footer { flex-wrap: wrap }` to the prompt on the strength of a font measurement showing the two buttons overflowing a 25rem dialog. The measurement modelled a row that does not exist: Taiga already styles a selectorless dialog component's footer via `tui-dialog[data-appearance~=taiga]>ng-component>footer`, which sets `flex-wrap: wrap-reverse` at a specificity the component's own `footer[_ngcontent-*]` cannot reach. The footer always wrapped; the declaration never applied. Removed, along with the comment asserting otherwise. - `updateProgress` is written by an in-process task exactly like the four statuses beside it, so the boot-path reset now clears it too. A startd restart mid-download previously left it set with its writer dead, which `server.update` reads as "already updating" and the UI as a frozen progress bar — wedged until the box rebooted.
The review pass over this PR flagged its own weakest point: the branch that decides between recording a power action and performing it, and the take that clears it, had no test — and that is exactly where two of the defects found during review lived (a runner that exited after one action, and a re-arm that set shuttingDown without shutting anything down). Extracts the three db transitions from their mutate closures into free functions over DatabaseModel, following the pattern the version migrations already use, and drives them with json! fixtures. Seven tests, each verified to fail against a deliberately broken implementation: ignoring after_backup, taking without clearing, beginning without clearing a pending action, swapping the restart and shutdown arms, and making the power-key path begin an action rather than record one. The async wrappers and run_deferred_power_actions' loop still have no coverage; they need an RpcContext.
Member
no, we really should prevent this |
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
2 participants
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.
Closes #3608
Powering the server off part-way through a backup can corrupt the backup of whichever service is being written at that moment, and nothing stopped it:
RestartandShutdowntook effect immediately, and the case power button went straight to systemd-logind's poweroff.What it does now
Choosing
RestartorShutdownwhile a backup is running opens a prompt offering to wait for the backup instead — and waiting is what happens if the user walks away, since the prompt counts down and takes that option for them.The choice is recorded server-side as
serverInfo.statusInfo.deferredPowerAction, so it survives the browser: a task started at boot waits for the backup to clear and then carries the action out, an action bar in every connected UI says what is coming, and either that bar'sCancelor an immediate restart/shutdown clears it.The power button reaches the same place. While a backup runs,
startdholds a systemd-logindhandle-power-keyblock inhibitor and reads the key itself from the devices udev tagspower-switch— logind's own set — so a press defers rather than powers off, and a beep acknowledges it for whoever is standing at the server. It nameshandle-power-keyand notshutdowndeliberately: blockingshutdownwould also block the power-off StartOS asks systemd for at the end of its own teardown.Deferring is opt-in (
--after-backup), sostartos-shutdown.serviceandstartos-restart.service— which cannot wait — are unchanged.start-cli server cancel-deferred-powercalls a pending action off.Measured, not reasoned
The power-key half is the part that could not be settled by reading, so it was measured on a
startos-0.4.0VM (a VM, not physical hardware — the ACPI power button is synthesised through/dev/uinput):KEY_POWERpress, no inhibitorInhibit(handle-power-key, …, block)heldtype=1 code=116 value=1— logind does notEVIOCGRAB/run/udev/tags/power-switch/c13:64,c13:65, matchingdevice_id()sizeof(struct input_event)size-8tail parse read the press byte-for-byteInhibitover D-Bus as rooth 5) — the exactsssssignature of the zbus proxypower_key.rsis Linux-only (libc::input_event,nix::sys::stat::{major,minor}do not exist on darwin — checked by compiling both againstaarch64-apple-darwin), so the module and its spawn are#[cfg(target_os = "linux")]; the apple-darwin leg of the CI matrix is unaffected.Also run:
cargo test -p start-core(308 pass, incl. new tests for the event parse and the udev device-id),npm run check,npm run build:ui,npm run check:i18n,cargo fmt --check, prettier,make start-core-ts-bindings-check,make manpages-check.Review notes
The first commit is the feature; the two that follow are fixes from adversarial review passes over it, kept separate so the reasoning is legible. Worth a look:
shutting_down/restarting/backupProgressare now reset whenstartdrestarts within a boot, not only atinit. A backup dies with the process that ran it, and a stale flag would have left the power key inhibited with nothing left to release it. This also fixes a pre-existing wart where astartdcrash mid-teardown left the header reading "Shutting down" until reboot.PowerService— the restart-to-apply action bar and the disk-repair restart previously bypassed the new guard.position: fixedand were stacking. They are now one@if/@else ifchain with the deferred-power bar first, becausestatusInfo.restartis never cleared until reboot and would otherwise shadow it permanently.Three things review turned up that are adjacent and not fixed here, in case you want them:
.g-buttonsis inert on a selectorless dialog component's footer. Taiga stylestui-dialog[data-appearance~=taiga] > ng-component > footerat specificity (0,1,3), so all four of the class's declarations lose —gapandmargin-topincluded. It affectsprompt.component.tsandaction-confirm.component.tsas well as this PR's dialog; I kept the class for consistency with them rather than making this one the odd one out.action-confirm.component.ts's ownfooter { margin-top: 1.5rem }is overridden the same way.check-i18n.mjs's line regex cannot see(cond ? 'A' : 'B') | i18n, an idiom used at 15 sites across three apps, leaving five keys with no coverage today. The fix belongs in the script (or in typingENGLISHas a literal object sokeyofyields a real union), not in the call sites.assure_backing_upgates only onbackupProgress, so a backup can still be started after a power action is committed and be torn down by it. Pre-existing, and closing it means a new rejection path and error string.Two things I deliberately did not do, in case you disagree:
assure_backing_upstill accepts a new backup while the server is tearing down (it gates only onbackupProgress). That window is pre-existing and closing it means a new rejection path and error string, so it felt like a separate change.Docs, changelog (
0.4.0.2, untagged) and all five locale dictionaries ship with the code. The UI was verified by typecheck and a production build, not clicked through in a browser.