fix(broker): sweep stale attached sessions; replace pre-v3 broker in place on upgrade#47
Open
dehuaichendragonplus wants to merge 1 commit into
Conversation
…place on upgrade A crashed or force-killed editor never sends a detach, so its session lingered in the broker's attached-session set forever, defeating the client-request fast-fail gate: new requests were queued and held up to the 300s hold deadline instead of failing fast once the only backend was gone. - Sessions carry a last-seen timestamp (refreshed on attach + every pull) and are swept after a staleness window (default = the 300s hold deadline so a domain-reload/compile gap never evicts a live-but-reloading session; FUNPLAY_BROKER_SESSION_STALE_MS override). The sweep skips the currently-parked-pull session and fails still-queued requests only when no backend remains. - Protocol bumped v2 -> v3 (no client-facing wire change) so a pre-v3 broker surviving a package upgrade is replaced in place; EnsureRunning waits for the port to free after shutting down its own broker so a same-port replacement completes in a single call. - EditMode tests for the stale-session sweep and the same-port in-place replacement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Why
The keepalive broker's attached-session set (
AttachedSessions) was only ever emptied by an explicit detach. A crashed or force-killed editor never sends one, so its session lingered in the set forever (until the broker process itself restarted).That defeats the fast-fail gate in
HandleClientRequest:With a dead session stuck in the set,
AttachedSessions.Countnever returns to 0, so every new client request that arrives after the editor is gone is queued and held up to the 300s hold deadline instead of failing fast — and this repeats for every subsequent request, indefinitely.What changed
1. Stale-session sweep (the fix).
AttachedSessionsbecomessession → last-seen ms(refreshed onattachand on everypull— the poll loop is a natural heartbeat).SweepLoopevicts sessions whose last-seen exceeds a staleness window. Two guards keep it safe:WaitingPull— that session is provably alive even though a parked poll doesn't refresh its timestamp.RemoveSessionLocked), and that reconciliation now only fails still-queued (never-dispatched) requests when no backend remains (AttachedSessions.Count == 0after removal). So a dead session swept while a healthy one is still connected leaves those requests queued for the healthy session instead of failing them.2. Staleness window default = the 300s hold deadline. A session silent longer than the longest a client request is ever held is definitely gone. Crucially this is ≥ any realistic domain-reload/compile gap, so a live-but-reloading editor (whose poll loop is briefly dead during the reload) is never swept mid-reload — preserving the broker's whole reason for existing. Overridable via
FUNPLAY_BROKER_SESSION_STALE_MS(used by tests to shorten the window).3. Deliver the fix on upgrade — protocol v2 → v3. The broker survives editor domain reloads by design, so after a package upgrade an old (pre-fix) broker keeps running and
EnsureRunninghappily reuses it (its health probe still passes), meaning the fix wouldn't take effect until the next natural broker restart. Bumping the protocol (no client-facing wire change) makes the old broker fail the new health probe, soEnsureRunning's existing upgrade-cleanup path shuts it down and launches a fresh one.TryReadStatealready tolerates stale-protocol pid records for exactly this cleanup.4. Single-call in-place replacement. When the replacement reuses the same port (which a protocol bump forces),
EnsureRunningshut the old broker down but then re-checkedIsTcpPortOpen(port)before the just-closed socket had released, and bailed with "port in use". It now waits for the port to free (only after shutting down its own recorded broker, same-port) so the replacement completes in a single call.Compatibility
EnsureRunning's port-free wait is gated onshutdownAccepted && existing.Port == port, so the existing port-change path and the foreign-process-on-port path are unaffected.Testing & verification
MCPBrokerTransportTests), including the existing detach / redelivery / port-change / upgrade-cleanup coverage — so theRemoveSessionLockedextraction and the port-free wait introduce no regression.BrokerTransport_StaleSessionIsSweptSoNewRequestsFailFast— a session that attached but never pulls/detaches (a crash) is swept, after which a new request fails fast instead of hanging. Fails (times out) without the sweep; passes with it.BrokerProcess_ReplacesRecordedBrokerOnSamePortInOneCall— a recorded broker that fails the identity probe is shut down and relaunched on the same port in a singleEnsureRunningcall (guards the port-free wait)./healthreportsprotocol: 2+X-Funplay-Broker: 2), then ran a singleEnsureRunningwith the v3 editor code — it shut the v2 broker down, waited for the port, and launched a v3 broker (new pid,/healthnowprotocol: 3), returningtruein ~0.6s.Independence
Touches only
Editor/MCP/Server/Broker/keepalive-broker.cs.txt,MCPBrokerProtocol.cs,MCPBrokerProcessManager.cs, andTests/Editor/MCPBrokerTransportTests.cs. No overlap with the other open PRs; compiles and tests clean on currentmain.Changelog
Added under
## Unreleased(Changed: protocol v3 + single-call replacement; Fixed: stale-session sweep).🤖 Generated with Claude Code