fix(streaming): refresh the process identity when Apollo restarts - #42
Merged
Merged
Conversation
Found by runtime-testing a real provision/restart/teardown cycle. No unit test caught it, and it was introduced by #39. RestartAsync set a new ProcessId but carried the PREVIOUS identity forward, so the instance record contradicted itself. Both readers then failed, in opposite and dangerous directions: IsAlive compared the new ProcessId against the OLD identity's, so a healthy restarted Apollo reported DEAD. SessionHealthCheck would restart it again on that reading, and keep doing so to MaxRestartAttempts. Stop killed using the OLD identity, found that PID long gone, reported AlreadyGone, and never touched the Apollo actually running -- leaking it on every teardown that followed a restart. RestartAsync now re-reads the start time for the new PID and writes the identity to both the instance record and the seat. KillForReconnect clears the identity along with the PID, in both places, rather than leaving one that describes a process it just killed. Verified end to end on the reference host, not only in tests: provision pid=28072 identity.pid=28072 apollo/restart pid=26976 identity.pid=26976 (was 28072 before the fix) teardown 26976 confirmed dead, 0 seats, no stray sunshine Also corrects a false justification from #41. SeatInfo.ApolloIdentity was argued for as covering "the instance record is gone after a service restart while the seat survives". That cannot happen -- seats are in-memory only, with no persistence and no restore, so _seats and _instances are populated together and lost together. The field is genuinely useful and is correctly populated; that particular argument for it was wrong, and the comments now say so instead of repeating it. The fallback branch is kept as the honest second source now that both are written together at every site. 2 new tests, 540 passing. They pin the invariant -- a record whose identity names a different PID reports its live process dead -- with a consistent record as the control, so the hazard is documented rather than merely fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw
Merged
vibesoftwarecoder
added a commit
that referenced
this pull request
Sep 10, 2026
version.txt 0.6.2 -> 0.6.3, plus the hand-written notes the release workflow reads from docs/release-notes/<version>.md. No code changes -- everything shipping already merged in #39, #40, #41 and #42. This is the first release whose changes were exercised on real hardware before shipping rather than only in CI. A seat was provisioned, paired, streamed to a Moonlight client on another machine, restarted mid-life and torn down; the client received the seat's own desktop with working input; hevc_nvenc opened inside the seat's RDP session; teardown after the restart terminated the running Apollo instead of leaking it; and a standalone Apollo streamed throughout without being touched. The notes name the two known limitations that remain -- seats capture the RDP surface rather than a dedicated virtual display, and a streaming seat still reports Ready (#43) -- along with the MoonlightVibe seat-discovery gap and its workaround. Overstating a release is worse than understating one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by runtime-testing a real provision/restart/teardown cycle on the reference host. No unit test caught it, and 538 were passing. It was introduced by #39.
The bug
RestartAsyncset a newProcessIdbut carried the previous identity forward, so the instance record contradicted itself. Both readers then failed, in opposite and dangerous directions:IsAliveProcessIdagainst the old identity's, so a healthy restarted Apollo reported DEAD — andSessionHealthCheckrestarts on that reading, again and again, toMaxRestartAttemptsStopAlreadyGone, and never touched the Apollo actually running — leaking it on every teardown after a restartThe fix
RestartAsyncre-reads the start time for the new PID and writes the identity to both the instance record and the seat.KillForReconnectclears the identity along with the PID in both places, rather than leaving one that describes a process it just killed.Verified end to end, not only in tests
The host also had no leftover RDP session afterwards, and the standalone console Apollo was untouched throughout (
Skipping non-MultiSeat Apollo PID 10988).A correction to #41
SeatInfo.ApolloIdentitywas argued for as covering "the instance record is gone after a service restart while the seat survives". That cannot happen. Seats are in-memory only — no persistence, no restore — so_seatsand_instancesare populated together and lost together. A restarted service has noSeatInfoto callStopwith in the first place.The field is genuinely useful and correctly populated. That particular argument for it was wrong, and the comments now say so rather than repeating it. The fallback branch is kept as an honest second source, now that both are written together at every site.
Testing
2 new tests, 540 passing, 0 failing.
They pin the invariant rather than just fixing it: a record whose identity names a different PID reports its live process dead, with a consistent record over the same process as the control. That pairing is what makes the assertion mean something — a test that only checked the fixed case would pass on a record that was never broken.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw