From 61d9fe4ea69a1a7610a0efe1fe4c7202018f63ab Mon Sep 17 00:00:00 2001 From: vibesoftwarecoder Date: Wed, 9 Sep 2026 21:31:31 -0500 Subject: [PATCH] feat(api): answer 503, 409 and 404 instead of 400 for everything PR D of the #29 sequence, the last one. Ported from @Dani6ca-T's MultiSeat-Extended (18a733c, d0446ae) plus the SeatInfo contract change the issue assigned here. Every failure answered 400 Bad Request, which told callers their request was malformed when it was not: 503 the host is full -- seat limit reached, no port block free. The request was fine and the same one may succeed later. 400 tells a client to stop retrying, the opposite of the truth. 409 the request conflicts with state that exists -- account already has a seat, Windows account already exists, seat is in a status that forbids the operation. Retrying identically cannot work. NOT the same as 503, and both used to be 400. 404 the seat is not there. After PR C this also covers a seat torn down while the operation waited for the lifecycle gate: not a bad request, and never was. A plain InvalidOperationException still means 400, so this adds precision rather than reshuffling. All three new types derive from InvalidOperationException deliberately: every non-HTTP caller already catching that keeps catching these unchanged, and a test pins that promise. The 12 endpoint catch blocks route through one ApiErrors.ToResult so a new endpoint cannot quietly disagree; the one ArgumentException catch stays 400, because an unusable resolution really is a malformed request. SeatInfo gains ApolloIdentity, which closes the gap PR B left open. _instances is in-memory only, so after a service restart it is empty while the seat and its Apollo are both alive -- exactly when a kill previously had nothing but a bare PID and fell back to a process-name check. Stop now verifies against the identity the seat carries. Testing: 12 new tests, 538 passing, 0 failing. The stale-identity test needed a control to mean anything: the name-check fallback refuses to kill a "ping" process on its own, so with the default ApolloExePath it would have passed whether or not the identity branch existed. It now runs against a manager configured to target the victim, leaving the identity comparison as the only thing that can spare it -- verified by disabling that branch, which fails it with "Assert.False() Failure". PortAllocatorTests.Allocate_ThrowsWhenExhausted broke on the type change and was not merely loosened: Assert.Throws demands an exact type, so it now asserts CapacityExhaustedException AND that it is still assignable to InvalidOperationException, pinning the compatibility promise at the throw site. NOT included: dbb93c7's LaunchedProcessId. The field is a contract change that belongs here, but nothing would read it until the launched-app lifecycle work lands, and a contract field with no consumer is dead surface. It should travel with the behaviour that uses it. Ported from work by @Dani6ca-T. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw --- .../Accounts/AccountManager.cs | 4 +- src/MultiSeat.Service/Api/AccountEndpoints.cs | 6 +- src/MultiSeat.Service/Api/SeatEndpoints.cs | 22 ++-- src/MultiSeat.Service/HttpStatusExceptions.cs | 75 +++++++++++ src/MultiSeat.Service/Sessions/SeatManager.cs | 28 ++--- .../Streaming/ApolloManager.cs | 23 +++- .../Streaming/PortAllocator.cs | 2 +- src/MultiSeat.Shared/Models/SeatInfo.cs | 15 +++ .../Api/HttpStatusSemanticsTests.cs | 119 ++++++++++++++++++ .../Sessions/PortAllocatorTests.cs | 10 +- .../Streaming/ProcessIdentityTests.cs | 80 ++++++++++++ 11 files changed, 350 insertions(+), 34 deletions(-) create mode 100644 src/MultiSeat.Service/HttpStatusExceptions.cs create mode 100644 src/MultiSeat.Tests/Api/HttpStatusSemanticsTests.cs diff --git a/src/MultiSeat.Service/Accounts/AccountManager.cs b/src/MultiSeat.Service/Accounts/AccountManager.cs index a1c4d0a..a0a5ae4 100644 --- a/src/MultiSeat.Service/Accounts/AccountManager.cs +++ b/src/MultiSeat.Service/Accounts/AccountManager.cs @@ -110,7 +110,7 @@ public bool AccountExists(string username) => public AccountInfo CreateAccount(string username, string? password = null) { if (_managedAccounts.ContainsKey(username)) - throw new InvalidOperationException($"Account '{username}' already exists."); + throw new ResourceConflictException($"Account '{username}' already exists."); // Generate a strong random password if not provided password ??= GeneratePassword(); @@ -127,7 +127,7 @@ public AccountInfo CreateAccount(string username, string? password = null) var result = NetApi.NetUserAdd(null, 1, ref userInfo, out var paramErr); if (result == NetApi.NERR_UserExists) - throw new InvalidOperationException($"Windows account '{username}' already exists."); + throw new ResourceConflictException($"Windows account '{username}' already exists."); if (result != NetApi.NERR_Success) throw new InvalidOperationException( diff --git a/src/MultiSeat.Service/Api/AccountEndpoints.cs b/src/MultiSeat.Service/Api/AccountEndpoints.cs index 9ff3d5d..be782a7 100644 --- a/src/MultiSeat.Service/Api/AccountEndpoints.cs +++ b/src/MultiSeat.Service/Api/AccountEndpoints.cs @@ -23,7 +23,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -38,7 +38,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -53,7 +53,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); } diff --git a/src/MultiSeat.Service/Api/SeatEndpoints.cs b/src/MultiSeat.Service/Api/SeatEndpoints.cs index 0757769..de6d4b5 100644 --- a/src/MultiSeat.Service/Api/SeatEndpoints.cs +++ b/src/MultiSeat.Service/Api/SeatEndpoints.cs @@ -32,7 +32,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -50,7 +50,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -84,7 +84,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -100,7 +100,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -116,7 +116,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -131,7 +131,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -147,7 +147,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -162,7 +162,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -255,11 +255,13 @@ public static void Map(WebApplication app) } catch (ArgumentException ex) { + // An unusable resolution genuinely IS a malformed request, so 400 is right + // here and this one deliberately does not go through ApiErrors. return Results.BadRequest(new { error = ex.Message }); } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); @@ -344,7 +346,7 @@ public static void Map(WebApplication app) } catch (InvalidOperationException ex) { - return Results.BadRequest(new { error = ex.Message }); + return ApiErrors.ToResult(ex); } }); } diff --git a/src/MultiSeat.Service/HttpStatusExceptions.cs b/src/MultiSeat.Service/HttpStatusExceptions.cs new file mode 100644 index 0000000..c77fcb8 --- /dev/null +++ b/src/MultiSeat.Service/HttpStatusExceptions.cs @@ -0,0 +1,75 @@ +namespace MultiSeat.Service; + +/// +/// A request cannot currently be satisfied because server capacity is exhausted — the seat limit +/// is reached, or no port block is free. The API maps this to 503 Service Unavailable: the +/// condition is temporary and the same request may succeed later, unlike a conflict with a +/// specific existing resource. +/// +/// ⭐ All three types here derive from deliberately. Every +/// existing non-HTTP caller — worker autostart, the smoke script, tooling — already catches that, +/// and keeps catching these unchanged. The HTTP layer gets more precision without anything else +/// having to learn about it. +/// +internal sealed class CapacityExhaustedException : InvalidOperationException +{ + public CapacityExhaustedException(string message) : base(message) { } +} + +/// +/// A request conflicts with current server state — the account already has a live seat, the +/// Windows account already exists, or the seat is in a status that forbids the operation. The API +/// maps this to 409 Conflict: repeating the identical request cannot succeed until +/// something changes. +/// +/// ⚠️ Distinct from on purpose. Both used to surface as +/// 400, which told a caller "your request was malformed" when the request was fine and the server +/// was full — and gave no way to tell a retryable condition from a permanent one. +/// +internal sealed class ResourceConflictException : InvalidOperationException +{ + public ResourceConflictException(string message) : base(message) { } +} + +/// +/// The seat named by the request does not exist, or stopped existing while the operation waited +/// for the per-seat lifecycle gate. The API maps this to 404 Not Found. +/// +/// ⚠️ The second case is the one worth knowing about. After the PR C guards, a lifecycle +/// operation can be admitted through the gate only to find the seat torn down while it waited. +/// That is not a bad request and never was — the seat was real when the caller asked. +/// +internal sealed class SeatNotFoundException : InvalidOperationException +{ + public SeatNotFoundException(string message = "Seat not found.") : base(message) { } +} + +/// +/// Maps the exception types above onto HTTP results, so every endpoint answers the same way and a +/// new endpoint cannot quietly disagree with the others. +/// +internal static class ApiErrors +{ + /// + /// ⚠️ Order matters: all three types derive from , so + /// the specific cases must be tested before the fallback. A plain InvalidOperationException + /// still means 400 — a genuinely bad request, such as an unusable resolution. + /// + public static IResult ToResult(InvalidOperationException ex) => ex switch + { + SeatNotFoundException => Results.NotFound(new { error = ex.Message }), + ResourceConflictException => Results.Conflict(new { error = ex.Message }), + CapacityExhaustedException => Results.Json(new { error = ex.Message }, statusCode: 503), + _ => Results.BadRequest(new { error = ex.Message }) + }; + + /// The status would produce. Exists so tests can assert the + /// mapping without standing up the HTTP pipeline. + public static int StatusFor(InvalidOperationException ex) => ex switch + { + SeatNotFoundException => StatusCodes.Status404NotFound, + ResourceConflictException => StatusCodes.Status409Conflict, + CapacityExhaustedException => StatusCodes.Status503ServiceUnavailable, + _ => StatusCodes.Status400BadRequest + }; +} diff --git a/src/MultiSeat.Service/Sessions/SeatManager.cs b/src/MultiSeat.Service/Sessions/SeatManager.cs index 4c4b2f4..f638b3b 100644 --- a/src/MultiSeat.Service/Sessions/SeatManager.cs +++ b/src/MultiSeat.Service/Sessions/SeatManager.cs @@ -198,7 +198,7 @@ public async Task ProvisionSeatAsync(SeatRequest request, Cancellation // Count only live seats — Error/Idle entries hold no resources (their ports and // sessions were already released on failure) and must not block new provisioning. if (ActiveSeatCount >= _options.MaxSeats) - throw new InvalidOperationException($"Maximum seat count ({_options.MaxSeats}) reached."); + throw new CapacityExhaustedException($"Maximum seat count ({_options.MaxSeats}) reached."); if (!_accounts.AccountExists(request.AccountName)) throw new InvalidOperationException($"Account '{request.AccountName}' does not exist. Create it first via /api/accounts."); @@ -231,7 +231,7 @@ public async Task ProvisionSeatAsync(SeatRequest request, Cancellation // seat Guid, so two provisions of the same account hold different gates. This lock // covers only the ownership decision; it is released before any provisioning work. if (!TryRegisterSeat(_seats, _accountOwnershipLock, seat)) - throw new InvalidOperationException( + throw new ResourceConflictException( $"Account '{request.AccountName}' already has a seat — tear it down first."); await BroadcastState(seat); @@ -543,7 +543,7 @@ await File.WriteAllTextAsync(rustDeskConfig, public async Task LaunchAppInSeatAsync(Guid seatId, LaunchAppRequest request, CancellationToken ct) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Without the gate, a teardown could remove the seat — disconnecting and logging off its // session — between the status check and the process creation below, orphaning the app in @@ -552,10 +552,10 @@ public async Task LaunchAppInSeatAsync(Guid seatId, LaunchAppRequest request, Ca var seat = LiveSeatAfterGate(seatId, "app launch"); if (seat is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); if (seat.Status is not SeatStatus.Ready and not SeatStatus.Streaming) - throw new InvalidOperationException($"Seat is in {seat.Status} state — cannot launch apps."); + throw new ResourceConflictException($"Seat is in {seat.Status} state — cannot launch apps."); await _processInjector.LaunchInSessionAsync( seat.SessionId, seat.AccountName, @@ -688,7 +688,7 @@ public async Task GetSeatServicesAsync(Guid seatId, CancellationTo public async Task StopApollo(Guid seatId) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Mutates ApolloProcessId and the ApolloManager instance record. using var lease = await _lifecycleGate.AcquireAsync(seatId, CancellationToken.None); @@ -706,7 +706,7 @@ public async Task StopApollo(Guid seatId) public async Task StartApolloAsync(Guid seatId, CancellationToken ct) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Per-seat lifecycle gate: starts Apollo and mutates ApolloProcessId. using var lease = await _lifecycleGate.AcquireAsync(seatId, ct); @@ -735,7 +735,7 @@ public async Task StartApolloAsync(Guid seatId, CancellationToken ct) public async Task RestartApolloAsync(Guid seatId, CancellationToken ct) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Per-seat lifecycle gate: Stop + Start is one compound mutation, not two. using var lease = await _lifecycleGate.AcquireAsync(seatId, ct); @@ -901,7 +901,7 @@ public async Task ApplyDisplayIsolationAsync(SeatInfo seat, CancellationToken ct public async Task ResetAudioAsync(Guid seatId) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Nothing to reset under per-session audio: MultiSeat assigns no device, and the // session's Remote Audio endpoint lives and dies with the session itself. Re-assigning @@ -942,7 +942,7 @@ public async Task ResetAudioAsync(Guid seatId) public void ApplyAudioDefaults(Guid seatId) { var seat = GetSeat(seatId) - ?? throw new InvalidOperationException("Seat not found."); + ?? throw new SeatNotFoundException(); ApplyAudioDefaults(seat); } @@ -980,7 +980,7 @@ public async Task SetNvencPresetAsync(Guid seatId, NvencQualityPreset preset, SeatPresetStore presetStore, CancellationToken ct) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Per-seat lifecycle gate: KillForReconnect + Start mutate ApolloProcessId. using var lease = await _lifecycleGate.AcquireAsync(seatId, ct); @@ -1029,7 +1029,7 @@ public async Task SetResolutionAsync(Guid seatId, int width, int height, SeatPresetStore presetStore, CancellationToken ct) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // Per-seat lifecycle gate: rebuilds the session: SessionId, mstsc and ApolloProcessId all change. using var lease = await _lifecycleGate.AcquireAsync(seatId, ct); @@ -1092,7 +1092,7 @@ public async Task SetResolutionAsync(Guid seatId, int width, int height, public async Task ResetDisplayAsync(Guid seatId, CancellationToken ct) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); // A concurrent teardown between the destroy and create below — teardown releases the // display assignment and cleans the seat's Apollo config — would re-register a display @@ -1118,7 +1118,7 @@ public async Task ResetDisplayAsync(Guid seatId, CancellationToken ct) public async Task ResetControllerAsync(Guid seatId) { if (GetSeat(seatId) is null) - throw new InvalidOperationException("Seat not found."); + throw new SeatNotFoundException(); if (!_options.EnableViGEmController) { diff --git a/src/MultiSeat.Service/Streaming/ApolloManager.cs b/src/MultiSeat.Service/Streaming/ApolloManager.cs index 9bace15..5c43862 100644 --- a/src/MultiSeat.Service/Streaming/ApolloManager.cs +++ b/src/MultiSeat.Service/Streaming/ApolloManager.cs @@ -135,6 +135,12 @@ public async Task StartAsync(SeatInfo seat, CancellationToken ct) _instances[seat.Id] = instance; + // Also record it on the seat, which outlives this dictionary. _instances is in-memory + // only, so after a service restart it is empty while the seat and its Apollo are both + // still very much alive — and that is precisely when a kill would otherwise have nothing + // but a bare PID to go on. + seat.ApolloIdentity = instance.Identity; + _logger.LogInformation( "Seat {Id}: Apollo started (PID {Pid}) — Moonlight can connect on port {Port}", seat.Id, pid, seat.PortBase + 1); @@ -192,9 +198,20 @@ public void Stop(SeatInfo seat) return; } - // No identity available. This is the path after a service restart, where the instance - // record is gone and SeatInfo carries only a bare PID. Carrying the identity on SeatInfo - // would close it, but that is a contract change and belongs to PR D (issue #29). + // 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. + if (seat.ApolloIdentity is { } seatIdentity) + { + var outcome = TryKillIdentifiedProcess( + seatIdentity, $"stop for seat {seat.Id} (identity from seat)", waitMs: 5000); + _logger.LogInformation( + "Seat {Id}: Apollo stop via seat identity — {Outcome} (PID {Pid})", + seat.Id, outcome, seatIdentity.ProcessId); + return; + } + + // Neither source has an identity, which means the start time was unreadable at launch. + // Fall back to the name check — weaker, and the last resort rather than the default. if (seat.ApolloProcessId <= 0) return; KillUnidentifiedApollo(seat.Id, seat.ApolloProcessId, "stop", waitMs: 5000); } diff --git a/src/MultiSeat.Service/Streaming/PortAllocator.cs b/src/MultiSeat.Service/Streaming/PortAllocator.cs index 1c90ea2..fe52c7c 100644 --- a/src/MultiSeat.Service/Streaming/PortAllocator.cs +++ b/src/MultiSeat.Service/Streaming/PortAllocator.cs @@ -24,7 +24,7 @@ public int Allocate() lock (_lock) { if (_available.Count == 0) - throw new InvalidOperationException("No port blocks available. All seats occupied."); + throw new CapacityExhaustedException("No port blocks available. All seats occupied."); var port = _available.Min; _available.Remove(port); diff --git a/src/MultiSeat.Shared/Models/SeatInfo.cs b/src/MultiSeat.Shared/Models/SeatInfo.cs index 2f7ea3e..81aec06 100644 --- a/src/MultiSeat.Shared/Models/SeatInfo.cs +++ b/src/MultiSeat.Shared/Models/SeatInfo.cs @@ -35,6 +35,21 @@ public sealed class SeatInfo public int PortBase { get; set; } public int ApolloProcessId { get; set; } + /// + /// 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. + /// + /// 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. + /// + public ProcessIdentity? ApolloIdentity { get; set; } + // Emulator netplay — RetroArch host port for this seat (PortBase + offset; 0 = disabled). // Seats connect to each other over loopback at 127.0.0.1:. public int RetroArchNetplayPort { get; set; } diff --git a/src/MultiSeat.Tests/Api/HttpStatusSemanticsTests.cs b/src/MultiSeat.Tests/Api/HttpStatusSemanticsTests.cs new file mode 100644 index 0000000..baa5256 --- /dev/null +++ b/src/MultiSeat.Tests/Api/HttpStatusSemanticsTests.cs @@ -0,0 +1,119 @@ +using Microsoft.AspNetCore.Http; +using MultiSeat.Service; +using Xunit; + +namespace MultiSeat.Tests.Api; + +/// +/// PR D of the #29 sequence: API contract. Every failure below used to answer 400 Bad +/// Request, which told a caller their request was malformed when it was not. +/// +/// The three that were wrong, and why the distinction is worth carrying: +/// +/// - 503 — the host is full. The request was fine and the same one may succeed later. A +/// 400 tells a client to stop retrying, which is the opposite of the truth. +/// - 409 — the request conflicts with state that exists. Retrying identically cannot work +/// until something changes, so this is NOT the same as 503. +/// - 404 — the seat is not there. After the PR C guards this also covers a seat torn down +/// while the operation waited for the lifecycle gate: not a bad request, and never was. +/// +/// A plain still means 400, which is what keeps this an +/// improvement in precision rather than a reshuffle. +/// +/// Ported from @Dani6ca-T's MultiSeat-Extended (18a733c, d0446ae). +/// +public class HttpStatusSemanticsTests +{ + [Fact] + public void CapacityExhausted_Is503_NotBadRequest() + { + var ex = new CapacityExhaustedException("Maximum seat count (4) reached."); + + Assert.Equal(StatusCodes.Status503ServiceUnavailable, ApiErrors.StatusFor(ex)); + Assert.NotEqual(StatusCodes.Status400BadRequest, ApiErrors.StatusFor(ex)); + } + + [Fact] + public void ResourceConflict_Is409() + { + var ex = new ResourceConflictException("Account 'GuestA' already has a seat."); + + Assert.Equal(StatusCodes.Status409Conflict, ApiErrors.StatusFor(ex)); + } + + [Fact] + public void SeatNotFound_Is404() + { + Assert.Equal(StatusCodes.Status404NotFound, ApiErrors.StatusFor(new SeatNotFoundException())); + } + + [Fact] + public void APlainInvalidOperation_StaysBadRequest() + { + // The fallback has to keep working, or this change would silently reclassify every + // genuine bad request as something else. + Assert.Equal( + StatusCodes.Status400BadRequest, + ApiErrors.StatusFor(new InvalidOperationException("No active session."))); + } + + [Fact] + public void CapacityAndConflict_AreNotTheSameStatus() + { + // The whole point: "the host is full, try later" and "this conflicts with what exists" + // are different answers, and both used to be 400. + Assert.NotEqual( + ApiErrors.StatusFor(new CapacityExhaustedException("full")), + ApiErrors.StatusFor(new ResourceConflictException("conflict"))); + } + + // ── the compatibility guarantee that makes this safe ──────────────────────── + + [Theory] + [InlineData(typeof(CapacityExhaustedException))] + [InlineData(typeof(ResourceConflictException))] + [InlineData(typeof(SeatNotFoundException))] + public void EveryTypedException_IsStillAnInvalidOperationException(Type type) + { + // ⭐ Load-bearing. Non-HTTP callers — worker autostart, smoke scripts, tooling — catch + // InvalidOperationException and must keep catching these unchanged. If this ever fails, + // those callers stop handling errors they used to handle, silently. + Assert.True(typeof(InvalidOperationException).IsAssignableFrom(type), + $"{type.Name} must derive from InvalidOperationException."); + } + + [Fact] + public void TypedExceptions_PreserveTheirMessage() + { + const string message = "Maximum seat count (4) reached."; + Assert.Equal(message, new CapacityExhaustedException(message).Message); + Assert.Equal("Seat not found.", new SeatNotFoundException().Message); + } + + [Fact] + public void ToResult_AgreesWithStatusFor() + { + // StatusFor exists so tests can assert without an HTTP pipeline. If the two ever drift, + // every assertion above becomes decorative — so pin that they are the same switch. + foreach (InvalidOperationException ex in new InvalidOperationException[] + { + new CapacityExhaustedException("x"), + new ResourceConflictException("x"), + new SeatNotFoundException(), + new InvalidOperationException("x") + }) + { + var result = ApiErrors.ToResult(ex); + Assert.NotNull(result); + + var expected = ApiErrors.StatusFor(ex); + var actual = result switch + { + IStatusCodeHttpResult s => s.StatusCode, + _ => (int?)null + }; + + Assert.Equal(expected, actual); + } + } +} diff --git a/src/MultiSeat.Tests/Sessions/PortAllocatorTests.cs b/src/MultiSeat.Tests/Sessions/PortAllocatorTests.cs index 58aaccf..4d3beb3 100644 --- a/src/MultiSeat.Tests/Sessions/PortAllocatorTests.cs +++ b/src/MultiSeat.Tests/Sessions/PortAllocatorTests.cs @@ -1,3 +1,4 @@ +using MultiSeat.Service; using MultiSeat.Service.Streaming; using MultiSeat.Shared; using Xunit; @@ -27,7 +28,14 @@ public void Allocate_ThrowsWhenExhausted() for (int i = 0; i < Constants.MaxSeats; i++) allocator.Allocate(); - Assert.Throws(() => allocator.Allocate()); + // Running out of port blocks is a CAPACITY condition, which the API answers with 503 + // rather than 400 — the request was fine, the host is full (#29 PR D). + var ex = Assert.Throws(() => allocator.Allocate()); + + // ⭐ And it is still an InvalidOperationException, which is what lets every non-HTTP + // caller that already catches that keep working untouched. Assert.Throws demands an + // exact type, so this second assertion is the one that pins the compatibility promise. + Assert.IsAssignableFrom(ex); } [Fact] diff --git a/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs b/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs index 0cd76bc..a6bb79b 100644 --- a/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs +++ b/src/MultiSeat.Tests/Streaming/ProcessIdentityTests.cs @@ -38,6 +38,22 @@ public void Log(LogLevel l, EventId e, TState s, Exception? ex, configBuilder: null!, // untouched by the kill paths under test processInjector: null!); + /// + /// A manager whose configured Apollo executable IS the victim process. + /// + /// ⭐ This is what makes the stale-identity test mean anything. Stop falls back to a + /// process-NAME check when it has no identity, and with the default ApolloExePath + /// ("sunshine.exe") that check refuses to kill a "ping" process all by itself — so the test + /// would pass whether or not the identity branch existed. Pointing the config at ping.exe + /// makes the name check say YES, leaving the identity comparison as the only thing that can + /// still stop the kill. + /// + private static ApolloManager NewManagerTargetingVictim() => new( + new NoopLogger(), + Options.Create(new MultiSeatOptions { ApolloExePath = @"C:\Windows\System32\ping.exe" }), + configBuilder: null!, + processInjector: null!); + /// A real, long-lived child process to act on. Killed by the caller or by dispose. private static Process StartVictim() => Process.Start(new ProcessStartInfo("ping.exe", "127.0.0.1 -n 120") @@ -167,6 +183,70 @@ public void TryKillIdentifiedProcess_WhenAlreadyExited_ReportsAlreadyGone() Assert.NotEqual(ApolloKillOutcome.Killed, outcome); } + // ── PR D: the identity survives on the seat, so a restart does not lose it ── + + [Fact] + public void Stop_WithNoInstanceRecord_UsesTheIdentityCarriedOnTheSeat() + { + // The state after a service restart: _instances is empty, the seat and its Apollo are + // both still alive. Before SeatInfo.ApolloIdentity this fell back to a name check. + using var victim = StartVictim(); + try + { + var realStart = ApolloManager.GetProcessStartTime(victim.Id); + Assert.NotNull(realStart); + + var seat = new SeatInfo + { + Id = Guid.NewGuid(), + AccountName = "GuestTest", + ApolloProcessId = victim.Id, + ApolloIdentity = new ProcessIdentity(victim.Id, realStart!.Value) + }; + + NewManager().Stop(seat); + + victim.Refresh(); + Assert.True(victim.HasExited); + } + finally + { + if (!victim.HasExited) victim.Kill(entireProcessTree: true); + } + } + + [Fact] + public void Stop_WithAStaleIdentityOnTheSeat_LeavesTheProcessRunning() + { + // ⭐ The one that proves the seat's identity is VERIFIED rather than merely present. + // A recycled PID reaches here looking exactly like this. + using var victim = StartVictim(); + try + { + var realStart = ApolloManager.GetProcessStartTime(victim.Id); + Assert.NotNull(realStart); + + var seat = new SeatInfo + { + Id = Guid.NewGuid(), + AccountName = "GuestTest", + ApolloProcessId = victim.Id, + ApolloIdentity = new ProcessIdentity(victim.Id, realStart!.Value.AddSeconds(-30)) + }; + + // Configured so the name-check fallback WOULD kill this process. The only thing + // left that can spare it is the identity comparison. + NewManagerTargetingVictim().Stop(seat); + + victim.Refresh(); + Assert.False(victim.HasExited); + } + finally + { + if (!victim.HasExited) victim.Kill(entireProcessTree: true); + } + } + [Fact] public void TryKillIdentifiedProcess_NeverThrows() {