[PR #438] [MERGED] fix(docker): resume the running session instead of killing its container on re-launch #467

New issue

Closed

opened 2026-10-15 13:00:47 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 13:00:47 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/games-on-whales/wolf/pull/438
Author: @wolph
Created: 6/21/2026
Status: ✅ Merged
Merged: 6/23/2026
Merged by: @ABeltramo

Base: stable ← Head: fix/stale-container-monitor-loop

📝 Commits (1)

a28adc3 fix: resume an existing session instead of killing its container on re-launch

📊 Changes

3 files changed (+22 additions, -1 deletions)

View changed files

📝 src/moonlight-server/rest/endpoints.hpp (+15 -0)
📝 src/moonlight-server/runners/docker.cpp (+2 -1)
📝 tests/docker/testDocker.cpp (+5 -0)

📄 Description

Summary

Re-launching or reconnecting to an app a client is already running dropped the player back to a fresh launcher: the running container was destroyed and recreated, and the old runner's monitor thread was left polling the now-gone container, logging [CURL] error 404 - No such container.

(This started as an attempt at #364, but per discussion that issue looks unrelated — this is a distinct re-launch/reconnect bug that reproduces on its own.)

Root cause

session_id is derived from the client certificate (state/sessions.hpp), so the app container name "{app}_{session_id}" is stable across reconnects. launch() (rest/endpoints.hpp) does no dedup — it always starts a runner → RunDocker::run → DockerAPI::create(). With force_recreate_if_present defaulting to true, a name conflict makes create() remove the still-running container out from under the first runner. That first runner's monitor loop then dereferenced the now-empty get_by_id() optional (UB).

resume() is the correct re-init path: it reuses the session and carries over the wayland display + devices without starting a runner. launch() just never checked.

Changes

rest/endpoints.hpp — launch() delegates to resume() when the client already has a running session, so the container is reused (and the wayland display + devices carried over) instead of recreated. This is the actual fix.
runners/docker.cpp — guard the monitor loop's get_by_id(...)->status with value_or(Container{.status = EXITED}) so a container that has gone away is treated as not-running and the loop exits cleanly, rather than dereferencing std::nullopt. (This is the original change from the first round of review.)

tests/docker/testDocker.cpp adds a small regression asserting get_by_id() returns std::nullopt for a removed / non-existent container.

Reproduced + verified end-to-end

Reproduced the bug and verified the fix on a real Wolf instance (RTX 4090); a self-contained reproducer script and before/after output are in a comment below. Two /launch calls for the same app+client:

before: session container id changes (killed + recreated), already present, removing first + a No such container 404 loop;
after: same container id, no recreate, no 404 loop, launch() routes to resume(). GPU pipeline still comes up normally.

Notes

Changed files are clang-format (v18) clean. Layer 1 (launch()→resume()) is protocol-facing — happy to adjust the routing/policy if you'd prefer a different approach at the session layer.

(An earlier revision also added a DockerAPI::get_by_name + a reattach-to-running-container path in the runner as defense-in-depth; dropped it to keep this focused, since launch()→resume() already prevents the duplicate runner.)

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/games-on-whales/wolf/pull/438 **Author:** [@wolph](https://github.com/wolph) **Created:** 6/21/2026 **Status:** ✅ Merged **Merged:** 6/23/2026 **Merged by:** [@ABeltramo](https://github.com/ABeltramo) **Base:** `stable` ← **Head:** `fix/stale-container-monitor-loop` --- ### 📝 Commits (1) - [`a28adc3`](https://github.com/games-on-whales/wolf/commit/a28adc3f7ac075a65a742444132bcf1915db6ec7) fix: resume an existing session instead of killing its container on re-launch ### 📊 Changes **3 files changed** (+22 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/moonlight-server/rest/endpoints.hpp` (+15 -0) 📝 `src/moonlight-server/runners/docker.cpp` (+2 -1) 📝 `tests/docker/testDocker.cpp` (+5 -0) </details> ### 📄 Description ## Summary Re-launching or reconnecting to an app a client is **already running** dropped the player back to a fresh launcher: the running container was destroyed and recreated, and the old runner's monitor thread was left polling the now-gone container, logging `[CURL] error 404 - No such container`. (This started as an attempt at #364, but per discussion that issue looks unrelated — this is a distinct re-launch/reconnect bug that reproduces on its own.) ## Root cause `session_id` is derived from the client certificate (`state/sessions.hpp`), so the app container name `"{app}_{session_id}"` is **stable across reconnects**. `launch()` (`rest/endpoints.hpp`) does no dedup — it always starts a runner → `RunDocker::run` → `DockerAPI::create()`. With `force_recreate_if_present` defaulting to `true`, a name conflict makes `create()` remove the **still-running** container out from under the first runner. That first runner's monitor loop then dereferenced the now-empty `get_by_id()` optional (UB). `resume()` is the correct re-init path: it reuses the session and carries over the wayland display + devices without starting a runner. `launch()` just never checked. ## Changes 1. **`rest/endpoints.hpp`** — `launch()` delegates to `resume()` when the client already has a running session, so the container is reused (and the wayland display + devices carried over) instead of recreated. This is the actual fix. 2. **`runners/docker.cpp`** — guard the monitor loop's `get_by_id(...)->status` with `value_or(Container{.status = EXITED})` so a container that has gone away is treated as not-running and the loop exits cleanly, rather than dereferencing `std::nullopt`. (This is the original change from the first round of review.) `tests/docker/testDocker.cpp` adds a small regression asserting `get_by_id()` returns `std::nullopt` for a removed / non-existent container. ## Reproduced + verified end-to-end Reproduced the bug and verified the fix on a real Wolf instance (RTX 4090); a self-contained reproducer script and before/after output are in a comment below. Two `/launch` calls for the same app+client: - **before:** session container id changes (killed + recreated), `already present, removing first` + a `No such container` 404 loop; - **after:** same container id, no recreate, no 404 loop, `launch()` routes to `resume()`. GPU pipeline still comes up normally. ## Notes Changed files are `clang-format` (v18) clean. Layer 1 (`launch()`→`resume()`) is protocol-facing — happy to adjust the routing/policy if you'd prefer a different approach at the session layer. (An earlier revision also added a `DockerAPI::get_by_name` + a reattach-to-running-container path in the runner as defense-in-depth; dropped it to keep this focused, since `launch()`→`resume()` already prevents the duplicate runner.) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-15 13:00:47 +00:00