fix(builder): Cancel stops a Linux update while it downloads #2160

Merged

jarynclouatre merged 1 commit from fix/linux-update-cancel into main

2026-11-11 22:22:05 +00:00

jarynclouatre commented

2026-11-11 05:19:38 +00:00

(Migrated from github.com)

fetch_bytes and install in linux_update now take an optional stop event. With one, a part is read a megabyte at a time and the download ends with Cancelled at the next one, or in the wait between two tries, and a cancelled download gets none of the retries a failed one does. install looks at the event between parts and once more before the new folder takes the old one's place, and removes what it staged, as for any error. Without an event they read and wait as before. The builder's Cancel sets the event when no make_game.py is running, and the page then says "The update was cancelled. Your game folder is unchanged." with Back, the way a failed update does.

Once the new version is completely in place the new builder still starts; from there Cancel stops make_game.py as it always did.

Testing (Linux, Tk 8.6)

The real window on Xvfb, started with --update, with a 30 MB part served slowly by a patched opener. Before: Cancel changes nothing, the version is installed and the new builder is started. After: the page shows the cancelled message at once, the staging folder is gone and the installed version is untouched.
tests/test_linux_update_cancel.py (new, 13 tests) covers the read in pieces (same bytes, same size limit), no retry after a cancel, Cancel during the wait between tries, install cancelled in the first or the last part, an update nobody cancels, and the builder's own cancel and begin_update on a stand-in window. 12 fail on main.
tests/test_linux_update.py and tests/test_update_tracks.py pass.
Not tested: a real update from GitHub in the AppImage.

In the Linux builder, Cancel does nothing while an update is still downloading. The download carries on, the new builder starts and the update runs. Cancel only knew how to stop make_game.py, and during the download there is none yet. The Windows builder cancels its download. `fetch_bytes` and `install` in `linux_update` now take an optional stop event. With one, a part is read a megabyte at a time and the download ends with `Cancelled` at the next one, or in the wait between two tries, and a cancelled download gets none of the retries a failed one does. `install` looks at the event between parts and once more before the new folder takes the old one's place, and removes what it staged, as for any error. Without an event they read and wait as before. The builder's Cancel sets the event when no make_game.py is running, and the page then says "The update was cancelled. Your game folder is unchanged." with Back, the way a failed update does. Once the new version is completely in place the new builder still starts; from there Cancel stops make_game.py as it always did. **Testing (Linux, Tk 8.6)** - The real window on Xvfb, started with `--update`, with a 30 MB part served slowly by a patched opener. Before: Cancel changes nothing, the version is installed and the new builder is started. After: the page shows the cancelled message at once, the staging folder is gone and the installed version is untouched. - `tests/test_linux_update_cancel.py` (new, 13 tests) covers the read in pieces (same bytes, same size limit), no retry after a cancel, Cancel during the wait between tries, `install` cancelled in the first or the last part, an update nobody cancels, and the builder's own `cancel` and `begin_update` on a stand-in window. 12 fail on main. - `tests/test_linux_update.py` and `tests/test_update_tracks.py` pass. - Not tested: a real update from GitHub in the AppImage.

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 20:07:03 +00:00

Joey-Magnus (Migrated from github.com) left a comment

Nice fix, and well contained. Without a stop event, fetch_bytes, _read_at_most and install behave exactly as before. With one, a cancel is seen between 1 MB pieces, in the wait between tries, between parts, and once more right before the new folder replaces the old one, and the except Exception in install removes the staged folder. Cancelled is re-raised ahead of the generic handlers in both fetch_bytes paths, so a cancel never gets retried or falls through to the API download. On the builder side, Cancel only exists on the progress page, and a normal build sets self.proc in the same callback that shows the page, so the new elif self.proc is None branch can only run while an update's thread is working. I ran tests/test_linux_update_cancel.py (13 tests), tests/test_linux_update.py and tests/test_update_tracks.py at the PR's head, and all pass. CI is green, and it merges cleanly with main.

Two small things:

should fix builder/linux/hookshot_builder.py:979-981 with :1140-1141: the thread checks stop.is_set() and then queues do-build, but pump only drains the queue every 100 ms. If Cancel lands after the check and before pump handles do-build (most likely on the already-fetched path, where the thread does no download and finishes at once), start_make_update still runs. make_game.py then starts, and "Stopping the update" is overwritten by "Compiling what changed". The fix is a guard in the handler:
```
elif kind == "do-build":
    if self.stop_update.is_set():
        self.lines.put(("update-error", str(linux_update.Cancelled())))
    else:
        self.start_make_update(v)
```
minor builder/linux/linux_update.py:256-263: a cancel is noticed between reads, so on a connection that has stalled mid-read it only takes effect when the socket times out (60 s for a part, 120 s for the API path). The page says "Stopping the update" during that time, so it isn't wrong, just possibly slow. No change needed if you're fine with that; it might be worth a word in the PR description's "Not tested".

Nice fix, and well contained. Without a stop event, `fetch_bytes`, `_read_at_most` and `install` behave exactly as before. With one, a cancel is seen between 1 MB pieces, in the wait between tries, between parts, and once more right before the new folder replaces the old one, and the `except Exception` in `install` removes the staged folder. `Cancelled` is re-raised ahead of the generic handlers in both `fetch_bytes` paths, so a cancel never gets retried or falls through to the API download. On the builder side, Cancel only exists on the progress page, and a normal build sets `self.proc` in the same callback that shows the page, so the new `elif self.proc is None` branch can only run while an update's thread is working. I ran `tests/test_linux_update_cancel.py` (13 tests), `tests/test_linux_update.py` and `tests/test_update_tracks.py` at the PR's head, and all pass. CI is green, and it merges cleanly with main. Two small things: - **should fix** `builder/linux/hookshot_builder.py:979-981` with `:1140-1141`: the thread checks `stop.is_set()` and then queues `do-build`, but `pump` only drains the queue every 100 ms. If Cancel lands after the check and before `pump` handles `do-build` (most likely on the already-fetched path, where the thread does no download and finishes at once), `start_make_update` still runs. `make_game.py` then starts, and "Stopping the update" is overwritten by "Compiling what changed". The fix is a guard in the handler: ```python elif kind == "do-build": if self.stop_update.is_set(): self.lines.put(("update-error", str(linux_update.Cancelled()))) else: self.start_make_update(v) ``` - **minor** `builder/linux/linux_update.py:256-263`: a cancel is noticed between reads, so on a connection that has stalled mid-read it only takes effect when the socket times out (60 s for a part, 120 s for the API path). The page says "Stopping the update" during that time, so it isn't wrong, just possibly slow. No change needed if you're fine with that; it might be worth a word in the PR description's "Not tested".

Joey-Magnus (Migrated from github.com) reviewed

2026-11-11 21:23:28 +00:00

Joey-Magnus (Migrated from github.com) left a comment

It needs a rebase: it conflicts with #2406 in builder/linux/hookshot_builder.py around startup_manifest. Keep main's manifest.tag != target check alongside the new get=...stop argument.

The earlier should-fix is still open: a Cancel between the thread's stop.is_set() check and pump() handling do-build still starts make_game.py. A guard in the do-build handler (~line 1140) closes it. Everything else checks out.

It needs a rebase: it conflicts with #2406 in `builder/linux/hookshot_builder.py` around `startup_manifest`. Keep main's `manifest.tag != target` check alongside the new `get=...stop` argument. The earlier should-fix is still open: a Cancel between the thread's `stop.is_set()` check and `pump()` handling `do-build` still starts `make_game.py`. A guard in the `do-build` handler (~line 1140) closes it. Everything else checks out.

jarynclouatre commented

2026-11-11 21:50:51 +00:00

(Migrated from github.com)

Thanks. Rebased on main (6f4f28ac) and done in f62186e6:

The conflict with #2406: main's manifest.tag != target check stays, right after the startup_manifest(..., get=...stop) call.
The window's do-build handler checks the stop event, so a Cancel that comes after the thread's last check and before the window takes the line ends as cancelled ("The update was cancelled. Your game folder is unchanged.") and does not start make_game.py. A new test covers that order.
test_linux_downgrade.py (from main) builds its window by hand, so it now gives it the stop event too; and the cancel tests' stub gained what main's begin_update now asks for (game_version, older_version, older_ok).

Tested on Linux: test_linux_update_cancel.py (14 tests), test_linux_downgrade.py, test_linux_update.py, test_linux_update_skip.py and test_linux_saves_move.py pass.

Thanks. Rebased on main (6f4f28ac) and done in f62186e6: - The conflict with #2406: main's `manifest.tag != target` check stays, right after the `startup_manifest(..., get=...stop)` call. - The window's `do-build` handler checks the stop event, so a Cancel that comes after the thread's last check and before the window takes the line ends as cancelled ("The update was cancelled. Your game folder is unchanged.") and does not start `make_game.py`. A new test covers that order. - `test_linux_downgrade.py` (from main) builds its window by hand, so it now gives it the stop event too; and the cancel tests' stub gained what main's `begin_update` now asks for (`game_version`, `older_version`, `older_ok`). Tested on Linux: `test_linux_update_cancel.py` (14 tests), `test_linux_downgrade.py`, `test_linux_update.py`, `test_linux_update_skip.py` and `test_linux_saves_move.py` pass.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 22:17:46 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good to me. Checked at f62186e6.

The open point from Joey-Magnus's review is fixed: the do-build handler in pump now looks at the stop event, so a Cancel that lands after the thread's last check ends as cancelled and does not start make_game.py. The test for that order is there too. The rebase keeps main's manifest.tag != target check right after the startup_manifest(..., get=...) call, and the get override is only used when HOOKSHOT_LINUX_LIST is unset, so it does not change the override path.

I also checked that elif self.proc is None can only be reached during an update's download: self.proc is reset after a build ends, and the Cancel button only exists on the progress page. Cancelled is re-raised ahead of the generic handlers in both fetch_bytes paths, so no retries and no fall-through to the API download, and install removes the staged folder through its existing except Exception. Without an event, the reads and waits are as before.

Still open, and not worth blocking on: Joey-Magnus's minor note that a stalled socket only ends at its timeout (60 s / 120 s), so "Stopping the update" can stay up for that long. A word in the description would cover it.

CI is green (Linux and Windows Python checks, AppImage, Windows Vulkan build); the Linux-only test is skipped on Windows through RUN_ALL_SKIP.

Looks good to me. Checked at f62186e6. The open point from Joey-Magnus's review is fixed: the `do-build` handler in `pump` now looks at the stop event, so a Cancel that lands after the thread's last check ends as cancelled and does not start make_game.py. The test for that order is there too. The rebase keeps main's `manifest.tag != target` check right after the `startup_manifest(..., get=...)` call, and the `get` override is only used when `HOOKSHOT_LINUX_LIST` is unset, so it does not change the override path. I also checked that `elif self.proc is None` can only be reached during an update's download: `self.proc` is reset after a build ends, and the Cancel button only exists on the progress page. `Cancelled` is re-raised ahead of the generic handlers in both `fetch_bytes` paths, so no retries and no fall-through to the API download, and `install` removes the staged folder through its existing `except Exception`. Without an event, the reads and waits are as before. Still open, and not worth blocking on: Joey-Magnus's minor note that a stalled socket only ends at its timeout (60 s / 120 s), so "Stopping the update" can stay up for that long. A word in the description would cover it. CI is green (Linux and Windows Python checks, AppImage, Windows Vulkan build); the Linux-only test is skipped on Windows through `RUN_ALL_SKIP`.