Skip to content

Kick: a close that tells the peer why and bounds the wait - #244

Merged
aaylward merged 7 commits into
mainfrom
claude/registry-kick
Oct 13, 2026
Merged

aaylward merged 7 commits into
mainfrom
claude/registry-kick

Conversation

@aaylward

Copy link
Copy Markdown
Collaborator

What

ADR-0022 amendment. SessionRegistry::Close(id) was documented as the kick for a silent partition. On Beast it isn't one: its close is the graceful handshake, bounded only by request_timeout_seconds (30s). games_hub hit this when a phone that dropped off the network kept its player out of a chess game.

  • WebSocket::Kick(CloseReason, grace): sends the close frame with an application code and reason, then tears the connection down after at most grace. On Beast, grace becomes the handshake timeout for that one close. A kick that finds Close's handshake already running closes the socket under it.
  • WebSocket::PeerCloseReason(): the close frame the peer ended the session with. A kicked client can then see e.g. 4001 "superseded" and stop redialing.
  • EventStreamHandle::Kick and SessionRegistry::Kick(id, why, grace) wrap it. Beast, the pair, the dialed client and JsonRpcStreamSocket implement both members. The base defaults (Kick is Close(), no peer reason) keep other implementors compiling. Bad codes or reasons fail fast (ADR-0009).
  • Fix: a handle's Close/Kick no longer deadlocks when the transport completes the parked receive inline (the pair does). The stream was being destroyed inside the call, waiting on the pin that call held. Handles from AsyncEventStream now keep the socket alive by ownership instead. Borrowed sockets are still pinned.

The registry's Close doc no longer claims the silent-partition case.

Not covered: any hub-side change. MoonBase bumps the pin and adopts Kick in its own PR.

Testing

  • Tests written first and watched red:
    • Beast: silent peer vs. Kick, plus the Close control that pins why it exists; the code reaching a raw peer and the opal client; a normal close reading 1000; a kick mid-close.
    • A contract-suite case (all four transports): a kick ends a parked receive without the peer.
    • Pair code/reason semantics and the death tests.
    • The registry kick.
    • The handle inline-teardown deadlock, for both Close and Kick.
    • JSON-RPC delegation.
    • A consumer acceptance test.
  • Hand mutations: 7 of 7 killed. They covered the grace, the peer capture, the mid-close kick, the frame's code, the owner path, the JSON-RPC delegation, and the pair's first-close-wins guard.
  • Locally:
    • bazel test //runtime/...: 38/38.
    • New timing tests at --runs_per_test=15.
    • Consumer websocket_acceptance_test and websocket_contract_consumer_test pass.
    • clang-format clean; clang-tidy quiet on the three changed sources.
  • On CI only: sanitizers, the -fno-exceptions gate, the rest of the consumer module, codegen/goldens (untouched), and fuzz.

Checklist

  • Tests added/updated for the change
  • bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally (runtime suite only; codegen untouched)
  • Formatting clean (clang-format; no BUILD or Java changes)
  • Architectural decisions recorded as an ADR (ADR-0022 amendment)

Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

lockfiles (resolution freshness) failed for a reason outside this PR's diff. .bazelversion is 9.x, so CI now resolves Bazel 9.3.0. That release resolves apple_support 2.5.2 and rules_cc 0.2.15, which neither checked-in lockfile records. The MODULE.bazel files are untouched here, and main would fail the same check today.

9069d77 regenerates both lockfiles under 9.3.0. Bazel module overrides drop modules out of the lockfile, so I didn't use any. The one override was a repository override: the sandbox blocks the rules_perl source archive, which an extension needs, and a repository override doesn't change what the lockfile records. Under 9.3.0, bazel mod deps --lockfile_mode=error fails on the old locks with CI's exact error and passes on the new ones, root and consumer.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

The review panel ran against d8605c0 with four lenses (correctness, concurrency, tests/docs, altitude). 92ebe13 carries the fixes.

Fixed:

  • The Beast kick has its own timer now. It closes the socket grace after the first kick. Before, the kick shortened Beast's handshake timeout, and that bound didn't hold in three cases:
    • a keep-alive or pong frame holding Beast's write lock;
    • a protocol-error close deferred behind a wedged write;
    • a second kick, which cut the first kick's frame.
      A second kick now changes nothing (ASecondKickKeepsTheFirstKicksDeadline, ASecondKickLeavesTheFirstKicksFrameAlone).
  • Handles hold owned sockets by reference for every operation, not just Close/Kick. A handle Send/SendAsync to a peer that answers by closing hit the same inline-teardown deadlock (AHandleSend[Async]ThePeerAnswersByClosingReturns, red before the fix). Pins are now for borrowed sockets only.
  • Tests:
    • The inline-teardown helper drops its own reference to the socket and checks what the peer reads.
    • New: a client-side kick, and a registry Kick over Beast with a silent peer, which proves grace is forwarded.
    • Hardening: client receives are bounded, a session that never arrives aborts instead of hanging, the timing stamp moves after Socket(), and the failure path aborts instead of detaching.
  • Docs: the ResumeOrAdd comment, the registry class comment, the ADR's claim that every implementation overrides the new methods, and the Kick contract's wording for a session already closing.

Kept as is:

  • grace stays a per-call argument. The altitude lens suggested a transport option instead. The caller is the one who knows how long its admission retry waits, and the pair and the base default ignore grace either way.
  • A kick that lands behind an already-requested Close sends 1000, not its own code. The contract says so: the requested frame stands.
  • Pre-existing, not touched: the defaulted AsyncEventStream move-assignment ends the old view after replacing socket_. Separate from this change.

Mutation checks: 9 of 9 killed, including the two new guards. The new timing tests pass 15 times out of 15. bazel test //runtime/...: 38/38.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

31ea055 fixes the move-assignment bug the panel found, which my last comment had left out of scope. It was pre-existing.

The defaulted move-assignments of EventStream and AsyncEventStream assigned members in declaration order. So the socket was released before the view_ assignment ended the old session through a raw pointer to it. Reordering the members can't help, because destruction needs view_ last. Both classes now have an explicit move-assignment that ends the old view first.

MoveAssignmentEndsTheOldSessionWhileItStillOwnsTheSocket covers both classes without committing the use-after-free: a fake socket records, when it's closed, whether the stream still owned it. Before the fix it saw 1 owner (only the test's reference); now it sees 2. The suites that use streams pass with cached results disabled.


Generated by Claude Code

Comment thread runtime/include/opal/eventstream/event_stream.h
@aaylward
aaylward merged commit 46eaba7 into main Oct 13, 2026
16 checks passed
@aaylward
aaylward deleted the claude/registry-kick branch October 13, 2026 23:31
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.

2 participants