feat(api): answer 503, 409 and 404 instead of 400 for everything - #41
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw
This was referenced Sep 10, 2026
vibesoftwarecoder
added a commit
that referenced
this pull request
Sep 10, 2026
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.
PR D of the #29 sequence — the last one.
Ported from work by @Dani6ca-T in
MultiSeat-Extended(18a733c,d0446ae), plus theSeatInfocontract change #29 assigned here.Everything answered 400
503 and 409 are not interchangeable, and both used to be 400. "The host is full, try later" and "this conflicts with what already exists" are different answers — one is retryable, the other cannot succeed until state changes. A 400 told the caller their request was malformed in both cases, which was never true.
The 404 case matters more after PR C: a lifecycle operation can now be admitted through the gate only to find the seat torn down while it waited. That is not a bad request, and never was.
A plain
InvalidOperationExceptionstill means 400, so this adds precision rather than reshuffling.Compatibility is the load-bearing part
All three new types derive from
InvalidOperationExceptiondeliberately. Every non-HTTP caller — worker autostart, smoke scripts, tooling — already catches that and keeps catching these unchanged. There is a test asserting exactly that, because if it ever stopped being true those callers would silently stop handling errors they used to handle.The 12 endpoint catch blocks route through one
ApiErrors.ToResult, so a new endpoint cannot quietly disagree with the others. The singleArgumentExceptioncatch deliberately does not — an unusable resolution really is a malformed request.Closing the gap PR B left open
SeatInfogainsApolloIdentity._instancesis 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.Stopnow verifies against the identity the seat carries.Testing
12 new tests. 538 passing, 0 failing, 17 skipped (pre-existing hardware gates).
The stale-identity test needed a control to mean anything. The name-check fallback refuses to kill a
pingprocess on its own, so with the defaultApolloExePathit 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 withAssert.False() Failure.An existing test broke, and that was the contract working.
Assert.Throws<T>demands an exact type, soPortAllocatorTests.Allocate_ThrowsWhenExhaustedfailed on the new derived type. It was not merely loosened: it now assertsCapacityExhaustedExceptionand that it is still assignable toInvalidOperationException, pinning the compatibility promise at the throw site rather than only in the new test file.Deliberately not included
dbb93c7'sLaunchedProcessId. The field is a contract change that belongs in this PR, 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 — alongside526202f, which PR C also left out for depending on theProcessTrackingsubsystem this port scoped away.Sequence complete
A (#27) · B (#39) · C (#40) · D (this one). With it, #29's four-PR plan is done.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SQvL62WkT8xDWXqyjFCGDw