From 06de7277191a5f0b84ed48c301fd1468ede5eb92 Mon Sep 17 00:00:00 2001 From: vibesoftwarecoder Date: Thu, 10 Sep 2026 05:23:55 -0500 Subject: [PATCH] fix(streaming): refresh the process identity when Apollo restarts 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 Claude-Session: https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw --- .../Streaming/ApolloManager.cs | 47 +++++++++++++-- src/MultiSeat.Shared/Models/SeatInfo.cs | 25 +++++--- .../Streaming/ProcessIdentityTests.cs | 58 +++++++++++++++++++ 3 files changed, 118 insertions(+), 12 deletions(-) diff --git a/src/MultiSeat.Service/Streaming/ApolloManager.cs b/src/MultiSeat.Service/Streaming/ApolloManager.cs index 5c43862..646b246 100644 --- a/src/MultiSeat.Service/Streaming/ApolloManager.cs +++ b/src/MultiSeat.Service/Streaming/ApolloManager.cs @@ -176,7 +176,12 @@ public void KillForReconnect(SeatInfo seat) } // Reset restart count — a sleep reconnect is not a crash - _instances[seat.Id] = instance with { ProcessId = 0, RestartCount = 0 }; + // Clear the identity along with the PID. Leaving it behind describes a process that has + // just been killed, and the record would then claim an identity it does not have. It + // fails closed rather than dangerously — a stale identity matches nothing — but a record + // that contradicts itself is exactly what made the restart path wrong. + _instances[seat.Id] = instance with { ProcessId = 0, RestartCount = 0, Identity = null }; + seat.ApolloIdentity = null; } /// @@ -198,8 +203,18 @@ public void Stop(SeatInfo seat) return; } - // The instance record is gone — the normal state after a service restart. The seat itself - // still carries the identity, so this stays a verified kill rather than a hopeful one. + // Second source for the identity, when the instance record has none. + // + // ⚠️ CORRECTION. This branch was originally justified by "after a service restart + // _instances is empty while the seat is still alive". That cannot happen: seats are held + // in memory only, with no persistence and no restore, so _seats and _instances are + // populated together and lost together. A restarted service has no SeatInfo to call this + // with in the first place. + // + // It is kept because it is cheap, correct, and the honest fallback if either store ever + // gains independent lifetime — not because the scenario above occurs today. The two are + // now written together at every site (start, restart, reconnect-kill), so they cannot + // disagree. if (seat.ApolloIdentity is { } seatIdentity) { var outcome = TryKillIdentifiedProcess( @@ -250,16 +265,40 @@ public async Task RestartAsync(SeatInfo seat, CancellationToken ct) if (pid > 0) { + // ⛔ The identity MUST be re-read here. Carrying `prev`'s forward alongside a new + // ProcessId produces a record that contradicts itself, and both readers then fail in + // dangerous directions: + // + // IsAlive compares the new ProcessId against the OLD identity's, so a perfectly + // healthy restarted Apollo reports DEAD — and SessionHealthCheck restarts + // it again, forever, until MaxRestartAttempts. + // Stop kills using the OLD identity, finds that PID long gone, reports + // AlreadyGone, and never touches the Apollo that is actually running — + // leaking it on every teardown that follows a restart. + // + // Found by runtime-testing a real provision/teardown cycle; no unit test caught it. + var restartedAt = GetProcessStartTime(pid); + if (restartedAt is null) + { + _logger.LogWarning( + "Seat {Id}: Apollo restarted (PID {Pid}) but its start time could not be " + + "read — no PID-reuse protection for this instance", seat.Id, pid); + } + + ProcessIdentity? identity = restartedAt is { } t ? new ProcessIdentity(pid, t) : null; + _instances[seat.Id] = prev with { ProcessId = pid, StartedAt = DateTimeOffset.UtcNow, RestartCount = prev.RestartCount + 1, SessionId = seat.SessionId, - AccountName = seat.AccountName + AccountName = seat.AccountName, + Identity = identity }; seat.ApolloProcessId = pid; + seat.ApolloIdentity = identity; _logger.LogInformation( "Seat {Id}: Apollo restarted (PID {Pid})", seat.Id, pid); } diff --git a/src/MultiSeat.Shared/Models/SeatInfo.cs b/src/MultiSeat.Shared/Models/SeatInfo.cs index 81aec06..ab62975 100644 --- a/src/MultiSeat.Shared/Models/SeatInfo.cs +++ b/src/MultiSeat.Shared/Models/SeatInfo.cs @@ -38,15 +38,24 @@ public sealed class SeatInfo /// /// The identity — PID plus the OS-reported start time — of the Apollo this seat launched. /// - /// ⭐ This is what makes a kill safe when ApolloManager's in-memory instance record is - /// gone, which is exactly the state after a service restart. Without it the only survivor is - /// , a bare number Windows is free to have handed to something - /// else in the meantime, and terminating on that alone can kill an unrelated process tree. - /// PR B narrowed that path to a process-name check; carrying the identity here closes it. + /// It is the seat's own record of which process it owns, so a client can see it and a kill + /// has a second source to verify against. alone is a bare + /// number Windows is free to have handed to something else, and terminating on that can kill + /// an unrelated process tree. /// - /// Null when the start time could not be read at launch. ⛔ Never populate it with a - /// substitute timestamp: an identity carrying a made-up time can compare equal to a recycled - /// PID by coincidence, which is worse than having no identity at all. + /// ⚠️ This was first justified as covering "the instance record is gone after a service + /// restart, but the seat survives". That is NOT true — seats are in-memory only, with no + /// persistence and no restore, so the seat and the instance record are lost together. The + /// field is genuinely useful; that particular argument for it was wrong. + /// + /// ⛔ It must be rewritten wherever is, and cleared wherever + /// that is cleared. A restart that advanced the PID while leaving this pointing at the dead + /// process made IsAlive report a healthy Apollo as dead and made teardown leak the + /// live one. + /// + /// Null when the start time could not be read. ⛔ Never populate it with a substitute + /// timestamp: an identity carrying a made-up time can compare equal to a recycled PID by + /// coincidence, which is worse than having no identity at all. /// public ProcessIdentity? ApolloIdentity { get; set; } diff --git a/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs b/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs index a6bb79b..a52227d 100644 --- a/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs +++ b/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs @@ -247,6 +247,64 @@ public void Stop_WithAStaleIdentityOnTheSeat_LeavesTheProcessRunning() } } + // ── the record must never contradict itself ───────────────────────────────── + + [Fact] + public void AnInstanceWhoseIdentityNamesADifferentPid_ReportsDead() + { + // This is the shape RestartAsync used to produce: a NEW ProcessId carried alongside the + // PREVIOUS identity. Both readers then fail, in opposite and dangerous directions — + // IsAlive calls a healthy Apollo dead (restart loop), and Stop kills the old PID and + // leaks the live one. The test documents WHY the two fields must be written together. + using var victim = StartVictim(); + try + { + var realStart = ApolloManager.GetProcessStartTime(victim.Id); + Assert.NotNull(realStart); + + var contradictory = new ApolloInstance( + SeatId: Guid.NewGuid(), + ProcessId: victim.Id, // the live process + ConfigPath: "x", SessionId: 2, AccountName: "GuestTest", + StartedAt: DateTimeOffset.UtcNow, RestartCount: 1, + Identity: new ProcessIdentity(victim.Id + 1, realStart!.Value)); // a different one + + Assert.False(contradictory.IsAlive); // healthy process, reported dead + + // The same record with a consistent identity reports the truth. + var consistent = contradictory with + { + Identity = new ProcessIdentity(victim.Id, realStart.Value) + }; + Assert.True(consistent.IsAlive); + } + finally + { + if (!victim.HasExited) victim.Kill(entireProcessTree: true); + } + } + + [Fact] + public void AnInstanceWithNoIdentity_FallsBackToPidExistence() + { + // Start time unreadable at launch. Reporting dead here would make SessionHealthCheck + // restart a seat whose Apollo is running fine, so the fallback must say alive. + using var victim = StartVictim(); + try + { + var noIdentity = new ApolloInstance( + SeatId: Guid.NewGuid(), ProcessId: victim.Id, + ConfigPath: "x", SessionId: 2, AccountName: "GuestTest", + StartedAt: DateTimeOffset.UtcNow, RestartCount: 0, Identity: null); + + Assert.True(noIdentity.IsAlive); + } + finally + { + if (!victim.HasExited) victim.Kill(entireProcessTree: true); + } + } + [Fact] public void TryKillIdentifiedProcess_NeverThrows() {