MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(linux): acknowledge Play's scope hand-off before closing #2643

Closed

mooki000 wants to merge 1 commit from mooki000/fix/linux-play-scope-handoff into main

mooki000 commented

2026-11-12 03:58:56 +00:00

(Migrated from github.com)

What this is for: make Linux Builder Play wait for the game’s actual scope hand-off before closing, and start it directly if that hand-off fails. This follows #2540 and @toniosim’s analysis in #2529; thanks to @klikso for the original fix. What is measured: game-free checks and 26 transient-user-unit trials on Arch/Hyprland with systemd 262.1, comparing 9c5612f0 with the runtime code in 6ba5433d. The two injected failures start nothing on main and start the stand-in on this branch; ten ordinary launches work on each. Not measured: a real game, a graphical Play click, a rebuilt AppImage, or logout.

Refs #2529
Refs #2540

Problem

own_scope() proves that systemd-run … true works. play() then starts a different, asynchronous systemd-run and immediately returns to play_and_close(), which destroys the Builder window.

Two controlled cases reproduce the remaining gap:

The first scope trial genuinely succeeds, but the actual launch exits 72: Play neither starts the game nor falls back.
The actual runner takes another 250 ms before executing systemd-run: with the issue’s Type=simple, ExitType=main, KillMode=control-group service, the Builder exits and its service stops that runner before the hand-off.

The delays/failures are deliberate injections, not an estimate of their frequency on players’ desktops.

What changed

builder/linux/hookshot_builder.py: share the existing unified-cgroup reader, then poll the real child until it leaves the launcher’s cgroup and its descendants. Stop at the first proof; otherwise stop on child exit or a two-second bound.
The scoped command waits behind a small socket-backed shell gate. The parent sends permission only after that proof. On failure, closing the gate without permission prevents a late scope transfer from launching a second game; the runner is cancelled/reaped before the plain fallback. This also handles failure to execute the real runner.
The gate uses otherwise-discarded stdout and restores /dev/null before executing the existing diagnostics command. The game’s stdin/stderr socket arrangement is unchanged.
builder/linux/play-diagnostics.sh: include the fallback notice alongside the existing exit status in hookshot_startup.log, when that log is published after the game exits.
tests/test_linux_play_scope.py: injected clock, cgroups and process outcomes cover actual-call failure, a delayed hand-off, bounded fallback exactly once, first-proof return without a fixed delay, and exec failure. --source and --case allow each regression to be checked against main independently.
tests/test_linux_play_diagnostics.py: exercise publication of the fallback notice while preserving bounded stderr and the game’s own logging behavior.
readme.md: describe the bounded hand-off and the limitation of a plain fallback.

The original scope-selection policy and successful trial remain. Terminal launches, existing scopes, absent systemd, and an unavailable user manager retain their previous choice of launch path.

How it was tested

Arch/Hyprland, kernel 7.2.15-1-omarchy, systemd 262.1-1-arch, GCC 16.2.1 and Clang 23.1.3, 2026-11-11. Shared host; no window or GPU use, and no performance claim. The runs below were made on 6ba5433d over base 9c5612f0. The branch has since been rebased onto main de866017 as 80f16e6d; the launcher and its two tests are unchanged by the rebase, and both tests were run again there (pass), as were the five cases against main's launcher (each fails).

Game-free checks

All five new cases fail against main’s source and pass against the branch. Failures on main are the missing fallback/acknowledgment behavior, not a missing-helper assertion.
The full scope-choice/hand-off test passes.
The affected diagnostics fixture passes with GCC and Clang, including its compiled check that the game still writes its own log and the new fallback notice.
Linux-selected checks: 13 passed, 1 failed, 0 skipped. The failure is test_linux_docker_packages.py (PPA parser sees --no-update instead of the PPA).
Full Clang suite: 454 passed, 14 failed, 20 skipped. Each of those 14 checks was then run once on untouched main at 9c5612f0 and failed with the same error signature. This is not a green-suite claim; every check touching the launcher passes. The inherited failures are listed below and are not changed in this PR.
After those full-suite and real-unit runs, the regression fixture's fake SIGKILL was made independent of the host platform (Windows does not expose SIGKILL). The full scope test and all five individual cases were rerun both ways. No runtime, shell or documentation code changed after the real-unit/full-suite runs.

Full-suite failure list

test_wayland_libdecor_hint.py, test_display_startup_failure.py, test_texel_buffer_ownership.py, test_audio_aux.py, test_audio_biquad.py, test_dlss_checks.py, test_env_on.py, test_framegen_preset.py, test_hdr_encode.py, test_linux_docker_packages.py, test_mods_size_table.py, test_mouse_native.py, test_readback_part.py, test_rumble_menu.py.

Real transient units: every trial

The real imported Builder module’s Builder.play_and_close() was called without constructing Tk; only the object’s destroy was replaced. The executable was an authored heartbeat script with a start counter, not a mock of the launch code.

Trial	Main	This branch
Actual call exits 72 after a successful probe	No child	One plain child; fallback logged
Actual call delayed 250 ms	No child	One scoped child after launcher collection
Actual call delayed 2.5 s, beyond the bound	One scoped child after its delay	One plain child; no later second start; fallback logged
Ordinary 0	One scoped child	One scoped child
Ordinary 1	One scoped child	One scoped child
Ordinary 2	One scoped child	One scoped child
Ordinary 3	One scoped child	One scoped child
Ordinary 4	One scoped child	One scoped child
Ordinary 5	One scoped child	One scoped child
Ordinary 6	One scoped child	One scoped child
Ordinary 7	One scoped child	One scoped child
Ordinary 8	One scoped child	One scoped child
Ordinary 9	One scoped child	One scoped child

The delayed/ordinary trials use ExitType=main to model the reported launcher lifetime. The exit-72 and timeout trials use ExitType=cgroup to isolate fallback: a plain child cannot escape a main-lifetime service, and this change does not claim otherwise. These properties model the issue, not every GNOME/KDE/COSMIC release’s defaults.

The fallback’s actual startup log:

injected actual-scope launch failure

Hookshot exited with status 0
Hookshot: systemd scope hand-off failed; started without a separate scope.

All 48 named transient services/scopes were collected, all stand-ins and shims removed, and the final all-state unit listing for the test prefix was empty. No persistent unit, desktop file, configuration change, or user game-file change was made.

Not changed here

Logout policy is unchanged. In the earlier review, the generated scope had empty PartOf/BindsTo; stopping an isolated mock session boundary stopped a deliberately bound control scope but left that scope alive. Inference, not a real logout result: a lingering user manager could leave it running unless the game or desktop independently exits it. This patch does not add session dependencies.

The existing stale-game closer is Windows-only; this patch does not add a Linux closer. It also does not change the game’s response to SIGTERM.

Not tested

A real desktop’s Play click; a GNOME, KDE or COSMIC session; actual logout; Flatpak confinement; a rebuilt AppImage; Windows or a real game/GPU run. The stand-in tests do not establish how the real game responds to display loss.

Risks

A user manager taking more than two seconds to transfer the real runner falls back to the previous direct-launch behavior. On a main-lifetime launcher service that direct child can still be stopped with the launcher.
Cgroup visibility is necessary for acknowledgment. If it disappears during launch, the attempt is cancelled and falls back rather than assuming success.
The new gate must not change diagnostics descriptors or permit a late second launch; the socket/compiled-logging checks, timeout case, and real start counters cover those boundaries.

Overlaps

The overlap check found #2547 adds its Add to Steam paragraph at the same README insertion point; the hunk was read. This is a documentation-ordering conflict, not a shared launcher implementation change. #1759, #1872, #1958, #2317, #2373, #2430 and #2465 share a changed code file and merge cleanly according to the overlap check. This follows, rather than replaces, merged #2540.

Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]

**What this is for:** make Linux Builder Play wait for the game’s actual scope hand-off before closing, and start it directly if that hand-off fails. This follows #2540 and @toniosim’s analysis in #2529; thanks to @klikso for the original fix. **What is measured:** game-free checks and 26 transient-user-unit trials on Arch/Hyprland with systemd 262.1, comparing `9c5612f0` with the runtime code in `6ba5433d`. The two injected failures start nothing on main and start the stand-in on this branch; ten ordinary launches work on each. **Not measured:** a real game, a graphical Play click, a rebuilt AppImage, or logout. Refs #2529 Refs #2540 ## Problem `own_scope()` proves that `systemd-run … true` works. `play()` then starts a *different*, asynchronous `systemd-run` and immediately returns to `play_and_close()`, which destroys the Builder window. Two controlled cases reproduce the remaining gap: - The first scope trial genuinely succeeds, but the actual launch exits 72: Play neither starts the game nor falls back. - The actual runner takes another 250 ms before executing systemd-run: with the issue’s `Type=simple`, `ExitType=main`, `KillMode=control-group` service, the Builder exits and its service stops that runner before the hand-off. The delays/failures are deliberate injections, not an estimate of their frequency on players’ desktops. ## What changed - `builder/linux/hookshot_builder.py`: share the existing unified-cgroup reader, then poll the *real* child until it leaves the launcher’s cgroup and its descendants. Stop at the first proof; otherwise stop on child exit or a two-second bound. - The scoped command waits behind a small socket-backed shell gate. The parent sends permission only after that proof. On failure, closing the gate without permission prevents a late scope transfer from launching a second game; the runner is cancelled/reaped before the plain fallback. This also handles failure to execute the real runner. - The gate uses otherwise-discarded stdout and restores `/dev/null` before executing the existing diagnostics command. The game’s stdin/stderr socket arrangement is unchanged. - `builder/linux/play-diagnostics.sh`: include the fallback notice alongside the existing exit status in `hookshot_startup.log`, when that log is published after the game exits. - `tests/test_linux_play_scope.py`: injected clock, cgroups and process outcomes cover actual-call failure, a delayed hand-off, bounded fallback exactly once, first-proof return without a fixed delay, and exec failure. `--source` and `--case` allow each regression to be checked against main independently. - `tests/test_linux_play_diagnostics.py`: exercise publication of the fallback notice while preserving bounded stderr and the game’s own logging behavior. - `readme.md`: describe the bounded hand-off and the limitation of a plain fallback. The original scope-selection policy and successful trial remain. Terminal launches, existing scopes, absent systemd, and an unavailable user manager retain their previous choice of launch path. ## How it was tested Arch/Hyprland, kernel `7.2.15-1-omarchy`, systemd `262.1-1-arch`, GCC 16.2.1 and Clang 23.1.3, 2026-11-11. Shared host; no window or GPU use, and no performance claim. The runs below were made on `6ba5433d` over base `9c5612f0`. The branch has since been rebased onto main `de866017` as `80f16e6d`; the launcher and its two tests are unchanged by the rebase, and both tests were run again there (pass), as were the five cases against main's launcher (each fails). ### Game-free checks - All five new cases fail against main’s source and pass against the branch. Failures on main are the missing fallback/acknowledgment behavior, not a missing-helper assertion. - The full scope-choice/hand-off test passes. - The affected diagnostics fixture passes with GCC and Clang, including its compiled check that the game still writes its own log and the new fallback notice. - Linux-selected checks: **13 passed, 1 failed, 0 skipped**. The failure is `test_linux_docker_packages.py` (PPA parser sees `--no-update` instead of the PPA). - Full Clang suite: **454 passed, 14 failed, 20 skipped**. Each of those 14 checks was then run once on untouched main at `9c5612f0` and failed with the same error signature. This is not a green-suite claim; every check touching the launcher passes. The inherited failures are listed below and are not changed in this PR. - After those full-suite and real-unit runs, the regression fixture's fake SIGKILL was made independent of the host platform (Windows does not expose SIGKILL). The full scope test and all five individual cases were rerun both ways. No runtime, shell or documentation code changed after the real-unit/full-suite runs. <details> <summary>Full-suite failure list</summary> `test_wayland_libdecor_hint.py`, `test_display_startup_failure.py`, `test_texel_buffer_ownership.py`, `test_audio_aux.py`, `test_audio_biquad.py`, `test_dlss_checks.py`, `test_env_on.py`, `test_framegen_preset.py`, `test_hdr_encode.py`, `test_linux_docker_packages.py`, `test_mods_size_table.py`, `test_mouse_native.py`, `test_readback_part.py`, `test_rumble_menu.py`. </details> ### Real transient units: every trial The real imported Builder module’s `Builder.play_and_close()` was called without constructing Tk; only the object’s `destroy` was replaced. The executable was an authored heartbeat script with a start counter, not a mock of the launch code. | Trial | Main | This branch | |---|---|---| | Actual call exits 72 after a successful probe | No child | One plain child; fallback logged | | Actual call delayed 250 ms | No child | One scoped child after launcher collection | | Actual call delayed 2.5 s, beyond the bound | One scoped child after its delay | One plain child; no later second start; fallback logged | | Ordinary 0 | One scoped child | One scoped child | | Ordinary 1 | One scoped child | One scoped child | | Ordinary 2 | One scoped child | One scoped child | | Ordinary 3 | One scoped child | One scoped child | | Ordinary 4 | One scoped child | One scoped child | | Ordinary 5 | One scoped child | One scoped child | | Ordinary 6 | One scoped child | One scoped child | | Ordinary 7 | One scoped child | One scoped child | | Ordinary 8 | One scoped child | One scoped child | | Ordinary 9 | One scoped child | One scoped child | The delayed/ordinary trials use `ExitType=main` to model the reported launcher lifetime. The exit-72 and timeout trials use `ExitType=cgroup` to isolate fallback: a plain child cannot escape a main-lifetime service, and this change does not claim otherwise. These properties model the issue, not every GNOME/KDE/COSMIC release’s defaults. The fallback’s actual startup log: ```text injected actual-scope launch failure Hookshot exited with status 0 Hookshot: systemd scope hand-off failed; started without a separate scope. ``` All 48 named transient services/scopes were collected, all stand-ins and shims removed, and the final all-state unit listing for the test prefix was empty. No persistent unit, desktop file, configuration change, or user game-file change was made. ## Not changed here Logout policy is unchanged. In the earlier review, the generated scope had empty `PartOf`/`BindsTo`; stopping an isolated mock session boundary stopped a deliberately bound control scope but left that scope alive. **Inference, not a real logout result:** a lingering user manager could leave it running unless the game or desktop independently exits it. This patch does not add session dependencies. The existing stale-game closer is Windows-only; this patch does not add a Linux closer. It also does not change the game’s response to SIGTERM. ## Not tested A real desktop’s Play click; a GNOME, KDE or COSMIC session; actual logout; Flatpak confinement; a rebuilt AppImage; Windows or a real game/GPU run. The stand-in tests do not establish how the real game responds to display loss. ## Risks - A user manager taking more than two seconds to transfer the real runner falls back to the previous direct-launch behavior. On a main-lifetime launcher service that direct child can still be stopped with the launcher. - Cgroup visibility is necessary for acknowledgment. If it disappears during launch, the attempt is cancelled and falls back rather than assuming success. - The new gate must not change diagnostics descriptors or permit a late second launch; the socket/compiled-logging checks, timeout case, and real start counters cover those boundaries. ## Overlaps The overlap check found #2547 adds its Add to Steam paragraph at the same README insertion point; the hunk was read. This is a documentation-ordering conflict, not a shared launcher implementation change. #1759, #1872, #1958, #2317, #2373, #2430 and #2465 share a changed code file and merge cleanly according to the overlap check. This follows, rather than replaces, merged #2540. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <[email protected]>

github-sync commented

2026-11-12 14:43:45 +00:00

Owner

@Joey-Magnus approved on GitHub · 2026-11-12 14:33 UTC · original

I merged this into current main and ran tests/test_linux_play_scope.py and tests/test_linux_play_diagnostics.py (both pass), plus run_all.py linux builder play. The only failure there is test_portable_local_builder, which is Windows-only and fails the same way on main when forced. CI is green.

I traced the hand-off. own_scope uses systemd-run --user --scope, which moves its own PID into the new scope before it execs. So wait_scope (builder/linux/hookshot_builder.py:186) polling /proc/<runner pid>/cgroup for a cgroup outside the launcher's is the right proof. The gate in start_scoped (:203) only lets the shell exec after "go\n". On timeout or failure, the closed socket plus os.killpg (:224) and the reap happen before the plain Popen, so a late scope transfer can't start a second game. The diagnostics script keeps its stdin/stderr sockets, and stdout is put back to /dev/null before the exec.

Two things for the record, neither blocking. Every Play from a service-launched launcher can now wait up to 2 s, though in practice it returns at the first proof. And the fallback still dies with a ExitType=main service, as the readme now says. A real GNOME/KDE Play click would still be good to have.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2643#pullrequestreview-5876226528) I merged this into current main and ran tests/test_linux_play_scope.py and tests/test_linux_play_diagnostics.py (both pass), plus `run_all.py linux builder play`. The only failure there is test_portable_local_builder, which is Windows-only and fails the same way on main when forced. CI is green. I traced the hand-off. `own_scope` uses `systemd-run --user --scope`, which moves its own PID into the new scope before it execs. So `wait_scope` (builder/linux/hookshot_builder.py:186) polling `/proc/<runner pid>/cgroup` for a cgroup outside the launcher's is the right proof. The gate in `start_scoped` (:203) only lets the shell `exec` after "go\n". On timeout or failure, the closed socket plus `os.killpg` (:224) and the reap happen before the plain `Popen`, so a late scope transfer can't start a second game. The diagnostics script keeps its stdin/stderr sockets, and stdout is put back to /dev/null before the exec. Two things for the record, neither blocking. Every Play from a service-launched launcher can now wait up to 2 s, though in practice it returns at the first proof. And the fallback still dies with a `ExitType=main` service, as the readme now says. A real GNOME/KDE Play click would still be good to have.

github-sync referenced this pull request from a commit

2026-11-12 17:57:17 +00:00

fix(linux): acknowledge Play's scope hand-off before closing (#2643)

github-sync referenced this pull request

2026-11-12 17:58:03 +00:00

docs: rewrite the docs: player docs in docs/user/, developer docs in docs/dev/, the readme as a front page #2561

github-sync closed this pull request

2026-11-12 17:58:18 +00:00

Commenting is not possible because the repository is archived.

No reviewers