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() {