fix: propagate update failures without duplicate execution - #343
ragnarlotus wants to merge 14 commits into
Conversation
|
Thanks for the work on #343. I ran a fairly deep review against the current The core idea of the PR looks good: mutating commands should run exactly once and the original failure status must be propagated. However, I found a few things that I think should be addressed before we rebase this onto the later 1. Interactive TTY semanticsThis is the most important point.
Input still works under a PTY and
So commands which change behaviour based on stdout/stderr TTY detection can behave differently. Interactive mode is the normal Ultimate Updater mode and must retain real terminal semantics. Headless/non-interactive mode is optional. Please adjust this so the new error-capture mechanism does not change the terminal contract for interactive package/update commands, and add a PTY regression test for it. 2.
|
|
I want to clarify one consequence of the TTY requirement before changing this part of the PR. The capture layer was implemented this way deliberately because the goal is not only to propagate a non-zero exit code. When a mutating operation fails, we need to retain the result of that exact execution:
This avoids the previous situation where a mutating command could effectively be executed again just to obtain its error output, potentially reporting a different result from the original failure. The difficulty with preserving the interactive TTY semantics is specifically with shell functions. A shell function may need to remain in the current shell because it can depend on or modify its state. If it is moved into another PTY/session for capture, its execution environment changes. If it remains directly attached to the real terminal, preserving those TTY semantics, then its stdout/stderr cannot also be transparently intercepted and retained in the same way. The exit code can still be propagated, but in that case the updater may no longer have the actual diagnostic output from the failed execution and would only know that the function returned a failure. That is the trade-off I wanted to make explicit. We implemented it this way because preserving the real failure information is important for error propagation and will also be useful for later work around package handling, recovery, logging and structured execution results. I am happy to follow whichever behaviour you want for upstream. I just want to make sure that, if we prioritise preserving the current interactive TTY semantics here, we do so knowing that exact failure-output propagation for those cases may no longer be guaranteed. |
|
Thanks for clarifying the trade-off. I agree that retaining the exact stdout/stderr of the original failing execution is valuable, especially for headless automation, logging and later structured recovery work. For Ultimate Updater we still want to prioritise the existing terminal contract in interactive mode, because that mode is intentionally used for human-driven updates where package managers or maintainer scripts may ask questions and expect a real terminal. The user must be able to answer those prompts reliably. This is not just cosmetic output behaviour; interactive updates are a first-class operating mode for us. At the same time, we also need the opposite behaviour for scheduled runs: in non-interactive/headless mode there is no human present, so exact capture of the original stdout/stderr and exit status is more important than preserving TTY semantics. So the upstream direction we want is:
I would prefer this split over introducing another pseudo-terminal/session layer in #343. The latter would make an already fairly large error-propagation PR more complex and harder to validate. So yes: please keep the single-execution/error-propagation goal, but branch the execution strategy according to the effective interactive/headless mode. The other review points still stand unchanged: scope the strict-mode change in Once those points are addressed against the current base, we can do the final rebase after #341/#340/#339 and run the integration/regression pass. |
|
One important clarification on why I made the error-propagation changes in the first place: The real problem I was trying to solve was not I hit this several times while updating real LXCs. Typical examples were Community Scripts/helper updates failing because the container could not complete the build/update step (for example because runtime/build resources were insufficient), or a nested helper/script returning a real error. The package-manager part could still have completed, the helper failed afterwards, but the failure was swallowed or lost and Ultimate Updater could continue to the end and report the target/update as successful even though the application itself had not actually been updated. That is why the change was important to me: a failed mutating update step must not later become The intended guarantee was therefore:
The resource-preflight work I planned next was complementary to this: preflight tries to avoid known resource-related failures before running the helper, while this PR makes sure that if an update still fails for resource reasons (or any other real error), that failure cannot be silently turned into success. So I am fine with keeping |
|
Thanks, that clarification makes the original motivation much clearer — and I agree with the core requirement. The important invariant for #343 should be:
I also agree that I checked the current PR head as well, and there are still a few implementation points to align before the final rebase:
So: the requirement is now fully aligned, but the PR is still CHANGES REQUIRED before rebase. Once these points are addressed against the current base, we can keep the planned sequence: merge #341 → #340 → #339, then do one final rebase of #343 and run the full integration/regression pass. Thanks again — the false-success problem you are fixing here is absolutely a real one, and I want to keep that guarantee intact while avoiding unrelated behaviour changes in the same PR. |
|
Thanks, that makes sense. I’m reviewing each of the remaining points one by one before changing the implementation, mainly to understand the capabilities and limitations of each approach and make sure the final behaviour stays consistent with the error-propagation guarantees we now agree on. I’ll use that review to decide the cleanest approach for each point, and I’ll raise anything that still has a meaningful trade-off before changing the code. |
|
I have now addressed the The approach is deliberately narrow:
This keeps the invariant we agreed on without changing the failure policy of Pi-hole, ioBroker, Pterodactyl, OctoPrint, Docker, etc. The existing So the intended distinction is now explicit in the code:
I am continuing to review the remaining points individually before changing them. |
|
I have now addressed the two remaining review points that do not conflict with the error-propagation guarantees:
|
|
I have been looking more closely at the interactive/headless distinction, because I think there is one important conceptual point we should clarify before changing this part. My concern is that we may currently be treating two different things as equivalent:
In practice, those are not necessarily the same thing. I have been running Ultimate Updater manually from a console for months. According to the current mode detection, those runs are therefore considered interactive. However, the actual guest update process has never behaved interactively for me in the usual sense of asking what to do during package/application updates. That also seems consistent with the updater itself: package managers are generally called with automatic/non-confirming options ( This distinction matters because the original problem behind #343 happened precisely in this kind of manually launched run. For example, a Community Scripts/helper update could require more resources than the guest currently had. The helper could fail, but Ultimate Updater did not stop and ask me how to proceed or offer to change resources. The failure was effectively lost and the target was later presented as successfully updated. So, in that real case: Preserving the fact that a real terminal exists would not by itself have helped with that failure. What mattered was preserving the failed execution, its exit status and enough diagnostic information to prevent it being interpreted as success. I completely understand the reason for not changing terminal semantics for commands which genuinely need a terminal. A maintainer script or another exceptional command may inspect whether stdout/stderr are connected directly to a terminal and alter its behaviour accordingly. What I am questioning is whether the presence of a terminal should automatically mean that all update commands must give up exact failure capture. It may be more accurate to distinguish: rather than treating: as a single condition. There is also a simpler case where I do not think there should be any ambiguity: executions which are not launched from a user console. For cron, WebUI jobs, scheduled execution or another service-driven invocation, the caller should explicitly mark the run as non-interactive. That seems cleaner than trying to infer intent from file descriptors later. The project already has mechanisms in this direction ( Conceptually: For manually launched runs, perhaps the remaining question is therefore narrower: Which concrete update operations in Ultimate Updater actually need direct terminal semantics? If only specific commands need that property, it may be preferable to preserve direct terminal execution for those commands rather than sacrificing exact failure capture for every command merely because the top-level updater was started from a console. The invariant I want to preserve from #343 is still the same: And where technically possible, retaining the exact output of that failed execution remains valuable for diagnostics, logging, future recovery logic and structured/WebUI results. |
|
Thanks — I think this is exactly the distinction we needed to make explicit. For Ultimate Updater, the upstream product decision is still to keep the normal/manual execution path mostly interactive by default. The reason is not that every package operation normally asks a question. Most of them do not. The problem is that we cannot know at the start of a run whether a later package, maintainer script or configuration transition will suddenly require human input. We have already seen a concrete example of this in practice: an That is why we deliberately chose this policy: So I agree with your conceptual point that "a terminal exists" and "this specific operation will definitely ask a question" are not the same thing. But for the normal path we intentionally cannot classify commands ahead of time as safely non-interactive, because whether a prompt appears can depend on the current package state, distro version, maintainer scripts, repository changes, local config files, and third-party application updaters. Trying to whitelist only the operations that currently need direct TTY semantics would therefore make the safety of the normal path depend on assumptions we cannot reliably maintain. In other words: normal mode is not "interactive because every command needs interaction"; it is potentially interactive because we cannot safely rule interaction out before the run. For this PR that means I am fine with the interactive path losing exact internal stdout/stderr capture where necessary, as long as:
For headless mode, exact capture remains the desired behaviour because there is intentionally no human present to answer anything. So please keep the implementation split along that policy rather than trying to infer, per package command, whether interaction will be needed. |
|
I want to clarify one point about If a user script is configured to run as part of a target update, I think its result has to contribute to the final target result. Otherwise Ultimate Updater can report a successful update even though the configured update procedure did not complete successfully. A concrete example from my environment is Caddy: I have custom logic around the wildcard reverse-proxy/SSL setup. If that custom update step fails, the container may still be running and the base packages may have updated correctly, but an essential service can be left non-functional because the reverse proxy / SSL configuration was not applied correctly. In that situation, reporting the target as successfully updated would be a false success just as much as when a package/helper update fails. Conceptually I see the configured update as: If any required part of that chain fails, the target is not fully updated. This also matters for the later hardening work that #343 is intended to make possible. The roadmap after error propagation includes resource preflight, package semantics, health/recovery, storage safety, logging and structured/WebUI execution results. In particular, recovery/rollback only works correctly if the updater can trust the target result. For example: If the user-script failure is swallowed and the target is marked SUCCESS: So I am completely fine reducing the unrelated behavioural changes you pointed out (sorting, hidden/temp/backup filtering, Alpine The one part I do want to preserve is the semantic guarantee that a configured user script is part of the update result: it runs once, its real exit status is preserved, and a real failure propagates outward so the target cannot be reported as successfully updated. That is important on its own today, and it is also a prerequisite for the later recovery/logging/structured-result work to be trustworthy. |
|
I want to push this point one step further before we change the implementation, because the concrete behaviour of Ultimate Updater matters here more than the generic behaviour of I have been using Ultimate Updater manually from a console for months, and in normal update runs it has never asked me for any package/update decision. That by itself is anecdotal, but the current code also explains why: the normal update paths are intentionally written to auto-manage package decisions. For Debian/Ubuntu-family updates, Ultimate Updater does not just run: It defines and uses: DPKG_OPTIONS=(
-o Dpkg::Options::=--force-confdef
-o Dpkg::Options::=--force-confold
)and those options are included in the normal host/LXC/VM upgrade paths together with Likewise, the other package managers are already called with their non-confirming forms ( So I agree with the generic statement that There are also places where Ultimate Updater does know that an operation is genuinely interactive. The Debian distribution-upgrade flow, for example, explicitly calls That suggests to me that the important distinction is not simply: but rather: This is also why the original implementation in #343 tried to keep the exact stdout/stderr from the same failed execution. The real failures I was trying to solve were not cases where Ultimate Updater stopped and asked me what to do. They were the opposite: helpers/application updates failed without asking anything, the failure was swallowed or lost, and the target was later reported as successful. For example, a Community Scripts/helper update can fail because the guest does not have enough runtime/build resources. In my real runs Ultimate Updater never asked whether resources should be increased; it simply continued. That exact failure information is important because later hardening work can use it for diagnostics, resource preflight, logging, health/recovery and possibly rollback decisions. There is another relevant case: Community Scripts is deliberately executed with stdin detached ( The same applies conceptually to configured user scripts unless they are explicitly designed to be interactive. A user script may be essential to the final service state (for example, applying the Caddy wildcard/reverse-proxy configuration). If it fails, the service may be materially broken even though the guest itself is still running, so the updater needs the real failure result rather than only a generic indication that something failed. I therefore think the execution policy should be explicit rather than inferred only from the existence of a TTY. My preferred approach would be a configuration policy, something along the lines of: (name only illustrative) Meaning: That would describe the actual product behaviour much more directly than equating "manual launch" with "interactive update". A second, simpler mechanism can still exist at invocation level: The project already has pieces of this ( I am not claiming that no third-party script can ever call For #343, the invariant I do not want to lose is still: If upstream wants manual runs to remain potentially interactive by default, I think an explicit configuration switch for auto-managed vs operator-interactive execution would be a cleaner long-term contract than using "a console exists" as the policy decision. |
Summary
Harden update error propagation so mutating commands are executed exactly once, their combined output is retained, and their real exit status reaches the existing updater error/final-status model.
This is intentionally limited to error propagation. It does not include later work around resource preflight, APT policy, health/recovery, storage, logging, or WebUI behavior.
Problem
Several update paths currently use the pattern
command || { ... $(command 2>&1) ...; }. When the first execution fails, the command is executed again to capture its output. For package upgrades and other mutating operations this can:Changes
Single-execution runtime
tee, stores the same combined output for diagnostics, and preserves the original command exit code.ERRORmodel and return the same status to their caller.update.shfor installations wheretarget-runtime.shis not yet present during an update.Host / LXC / VM propagation
pveupdate, package upgrade and cleanup steps through the single-execution runtime.fstrim, distribution-upgrade probes/commands and user-script failures instead of silently continuing.ERROR()return the captured error status while retaining the existing aggregateUPDATE_FAILUREbehavior.Extra updates
update-extras.shso an enabled extra updater cannot fail silently.</dev/nullso a controlling terminal cannot stop the nested helper through terminal ioctls.Compatibility with current
developThe work branch was created directly from current
developat:f84fb17992137db84a7db48c94b3b2c3a1f11f9f(fix: repair SSH guest package count commands)Existing current-
developbehavior was retained, including internal target selection, post-update status capture, script-only mode, current package-count helpers, check-only finalization, and Community Scripts stdin detachment.Regression coverage
tests/test-update-step-runtime.shto verify that a failing mutating command:ERRORonce;tests/test-community-scripts-update.shto require exact helper failure propagation and single execution while retaining the PTY/stdin-detachment regression case.developalready contains those portions of the earlier hardening.Scope
Files changed:
target-runtime.shupdate.shupdate-extras.shtests/test-community-scripts-update.shtests/test-update-step-runtime.shNo real package update is required by the regression tests.