Repository navigation
Conversation
The command-line client existed with no way to reach it from the interface most people will use. This adds the check, the notification and the install sequence, and the sequence is the interesting part. Replacing a desktop app underneath itself is the usual pattern, and it is not available here: the installer refuses maintenance while a transfer is in flight and never force-kills a running copy. So a running or paused job declines the update with the reason rather than queueing behind it or forcing it through. A paused job counts, because it is a partially written destination waiting to continue, and replacing the application under it is not a way to find out what happens. The queue is checked again immediately before the installer is launched, since a job can be started while the bytes are arriving. When nothing is running, the app closes itself so maintenance can proceed; that close is what makes the update possible, so it is announced rather than arriving as a side effect. The automatic check runs once a couple of seconds after the window opens and says nothing unless there is something to say, because an unrequested check that reports being up to date is noise. Help offers the same check with an answer either way, and Options turns the automatic one off. Both the network call and the signature probe run on a pool thread, so an unreachable network delays no paint and a failure lands in the status bar rather than a dialog. Writing the tests found an ordering fault worth keeping fixed: the check set the release before clearing its own busy flag, leaving a window in which a release was visible and prepare() still refused. In the app that is an update which announces itself and then quietly does nothing. The flag is now cleared before the result is published, and a test asserts that anything able to see a release can also start the download. Every step is injectable, so the whole sequence including each refusal is driven in tests without a network, a certificate or an installer.
34f8b75 to
65732f4
Compare
3e95b23 to
170f1a0
Compare
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed manual/automatic update checks, worker-to-GUI signaling, download progress, transfer refusal, and the install handoff. Three user-visible lifecycle defects are detailed inline: a failed check is reported as current, Cancel does not cancel, and the startup timer can suppress a manual result.
Validation: 18 targeted updater tests passed on this head and git diff --check passed. The injected raising-check case still emitted an uncaught worker traceback; a green test does not establish that the failed signal was delivered. Additional verification-path concerns inherited from #13 are included in the private updater notes rather than repeated publicly.
Three ways the update sequence told the user something that was not so. update.check() never raises and returns None for a failed fetch, a TLS error and an unparseable feed alike, and the controller read None as "you are the newest release". So a manual check reported that on the strength of a failed DNS lookup, and an automatic failure never reached the documented failure path at all. check_feed() is the form with somewhere to put the difference: None only when the feed answered and had nothing newer, FeedError otherwise. check() is unchanged for callers that have nowhere to put it. _run_check also had no except path, so an injected or future check that raised produced a bare QRunnable traceback and no signal - the window waited on a result that was never coming. The progress dialog offered Cancel and nothing was connected to it. Clicking it hid the dialog while the download and the signature check carried on, and the app then offered to install what the user had just declined. Cancellation is cooperative, because a blocking read loop on a pool thread has nowhere else to hand control back: the progress callback raises. What it guarantees is the part that was missing - the request is terminal, the incomplete download is removed, verification does not run, and ready is not emitted. Checked again after the download returns, so a file small enough to arrive between two callbacks cannot sail past the cancel. Only the file the release names is removed, since the download directory can be one the caller owns. Closing the dialog for a result that has arrived disconnects first, because QProgressDialog's own closeEvent emits canceled and that would be reported back as the user asking to stop. The announcement intent now belongs to the check that is running rather than to the window's last call. It was stamped before check_now reported whether a check had started, so a manual check requested during the first 2.5 seconds was downgraded by the launch timer firing behind it: the duplicate was refused, the intent was already overwritten, and the result the user asked for was handled silently. A refused duplicate now raises the intent and never lowers it, so it survives in both directions. Sixteen tests. Failed fetch, invalid feed and a genuine up-to-date answer as three separate outcomes at both the feed and the controller; a cancel during a blocked download and one arriving after the bytes have, each asserting the terminal signal, the absent installer and the removed file; a file beside it that is not removed; the refused-duplicate race in both directions and at the window; and the dialog's Cancel reaching the controller while closing it for a result does not.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Follow-up review of d2e4edc for failed checks, cancellation, announcement intent, and integration with the parent updater.
The three previously reported GUI lifecycle defects have targeted fixes: failure gets a failed signal, Cancel reaches the download callback and suppresses readiness, and a refused automatic check cannot downgrade a manual announcement. The feed-type guard introduces a separate incompatibility with the corrected release collection on #13, detailed inline.
Validation: all 34 updater-controller tests passed under headless Qt. A separate injected collection response reproduced FeedError from check_feed() and None from the compatibility check() wrapper. No installer or real download was executed.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
No new nonduplicate finding in this incremental diff. Previously private updater-verification concerns inherited from #13 remain outside the corrected GUI control flow.
Reviewed thread/controller ownership, feed outcomes, manual versus startup intent, cancellation, progress/state transitions and maintenance blocking around queued or active jobs. The earlier list-feed rejection, failure-reported-as-current, lost manual intent and cancellation findings are corrected in this head.
Validation: all 40 GUI updater-controller tests and seven added window/CLI outcome tests passed on the exact detached head. Network, signature and installer operations were injected. Current-head CI reports successful matrix/build/frozen checks; the diff check passes.
Limits: headless Qt without a signed published-release update, real UAC handoff or installation. Passing controller tests do not resolve the separate installer-verification issues already recorded privately.
Pinned scope: declared base 19c7be0735b097d76a4f047c6a9bf469b1d10f76, merge base 19c7be0735b097d76a4f047c6a9bf469b1d10f76, head dcae1f87aad9ff01f58a82831e885d033a9f3640. Existing discussion and current-head checks were reviewed; no threads were resolved and no formal approval or merge is being issued.
# Conflicts: # README.md # src/offloader/cli.py # src/offloader/update.py
# Conflicts: # README.md # tests/test_cli.py
Summary
The client from #13 had no way to reach it from the interface most people will use. This adds the check, the notification and the install sequence. The sequence is the interesting part.
Everything here works without a signing certificate or a published release, which is why it was the next thing buildable.
The sequence cannot be the usual one
Replacing a desktop app underneath itself is the standard pattern and it is not available here: the installer refuses maintenance while a transfer is in flight and never force-kills a running copy. So:
Restraint in the check
One check, a couple of seconds after the window opens, and it says nothing unless there is something to say: an unrequested check reporting "you are up to date" is noise. A found release gets a line in the header rather than a dialog stealing attention from a running offload. Help, Check for updates now answers either way; Options, Check for updates on launch turns the automatic one off (on by default — a packaged copy that never learns a fix exists is the worse default for a tool people trust with original media).
Both the network call and the signature probe run on a pool thread, so an unreachable network delays no paint and a failure lands in the status bar, not a modal.
An ordering fault the tests found
Worth flagging because it was a real bug, not a test artefact.
_run_checksetself._releasebefore clearing its own busy flag, leaving a window in which a release was visible andprepare()still refused. In the app that is an update which announces itself and then quietly does nothing, on a timing-dependent basis.The flag is now cleared before the result is published, and
test_a_visible_release_can_always_be_preparedasserts the invariant: anything able to see a release can also start the download.Design notes for review
QRunnable, for the same reasondrives._ScanTaskdoes it: a runnable's Python wrapper can be collected as soon asstart()returns.refusal()takes the queue's states rather than the controller, so the rule is a pure function and testable without a window.Test plan
tests/test_gui_updates.py— 18 tests: the refusal for running and paused, idle and queued-only allowed, one active job among many still refusing; newer release reported, up-to-date reported separately, a second check refused while one runs, a raising check not escaping the pool thread, the ordering invariant, download/verify/ready with the streamed digest passed to both sides, a failed verification leaving no installer the window could run, progress throttled but finishing, and install refusing without a verified fileruff check src tests scripts build/windowscleanoffloader-gui.exelaunched and stayed up for 6s, so the launch-time check does not break startupReview follow-ups (2026-10-12)
check_feed()and the updater'sfind_release()were two implementations of the same thing. One remains,find_release(), raisingFeedError(a subclass ofUpdateError) for every feed problem. A JSON list is the releases collection, an object withtag_nameis a single release, and any other object is an error body whose message is carried in the exception. The CLI exits 5 on a feed failure; the GUI's failed signal carries the message.