Skip to content

fix(broker): sweep stale attached sessions; replace pre-v3 broker in place on upgrade#47

Open
dehuaichendragonplus wants to merge 1 commit into
FunplayAI:mainfrom
dehuaichendragonplus:fix/broker-sweep-stale-sessions
Open

fix(broker): sweep stale attached sessions; replace pre-v3 broker in place on upgrade#47
dehuaichendragonplus wants to merge 1 commit into
FunplayAI:mainfrom
dehuaichendragonplus:fix/broker-sweep-stale-sessions

Conversation

@dehuaichendragonplus

Copy link
Copy Markdown
Contributor

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:

if (WaitingPull == null && !HasActiveRequestLocked() && AttachedSessions.Count == 0)
    // fast-fail: backend unavailable

With a dead session stuck in the set, AttachedSessions.Count never 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). AttachedSessions becomes session → last-seen ms (refreshed on attach and on every pull — the poll loop is a natural heartbeat). SweepLoop evicts sessions whose last-seen exceeds a staleness window. Two guards keep it safe:

  • It skips the session whose long-poll is currently parked in WaitingPull — that session is provably alive even though a parked poll doesn't refresh its timestamp.
  • Eviction reuses the exact detach reconciliation (extracted into RemoveSessionLocked), and that reconciliation now only fails still-queued (never-dispatched) requests when no backend remains (AttachedSessions.Count == 0 after 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 EnsureRunning happily 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, so EnsureRunning's existing upgrade-cleanup path shuts it down and launches a fresh one. TryReadState already 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), EnsureRunning shut the old broker down but then re-checked IsTcpPortOpen(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

  • No client-facing wire change. The attach/pull/push/redelivery protocol is unchanged; v3 is a broker-generation marker so upgrades replace the old broker in place.
  • EnsureRunning's port-free wait is gated on shutdownAccepted && existing.Port == port, so the existing port-change path and the foreign-process-on-port path are unaffected.
  • Additive: no tool signatures change.

Testing & verification

  • Full broker EditMode suite green: 15/15 (MCPBrokerTransportTests), including the existing detach / redelivery / port-change / upgrade-cleanup coverage — so the RemoveSessionLocked extraction and the port-free wait introduce no regression.
  • New tests:
    • 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 single EnsureRunning call (guards the port-free wait).
  • End-to-end upgrade validated manually: compiled the actual pre-v3 (v2) broker from source, launched it (confirmed /health reports protocol: 2 + X-Funplay-Broker: 2), then ran a single EnsureRunning with the v3 editor code — it shut the v2 broker down, waited for the port, and launched a v3 broker (new pid, /health now protocol: 3), returning true in ~0.6s.

Independence

Touches only Editor/MCP/Server/Broker/keepalive-broker.cs.txt, MCPBrokerProtocol.cs, MCPBrokerProcessManager.cs, and Tests/Editor/MCPBrokerTransportTests.cs. No overlap with the other open PRs; compiles and tests clean on current main.

Changelog

Added under ## Unreleased (Changed: protocol v3 + single-call replacement; Fixed: stale-session sweep).


🤖 Generated with Claude Code

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant