Send become_coordinator on graceful coordinator handover - #178
Merged
Merged
Conversation
Graceful coordinator shutdown never sent a become_coordinator message, so only the crash-race path (each surviving peer independently racing to rebind the coordinator port) ever produced a new coordinator, despite the README documenting handoff to the longest-running peer as live behaviour. sendCoordinatorHandover picks the remaining peer with the earliest recorded startedAt as successor and sends it become_coordinator carrying every other remaining peer, matching the peerList shape handleBecomeCoordinator already expects. A no-op when this side isn't the coordinator or no other peers remain. Not yet wired into MeshStore.shutdown().
MeshStore.shutdown() stopped the stale-agent checker, dropped the hub gateway connection, and closed the transport, but never sent the become_coordinator handoff the README documents -- only the crash-race path (each surviving peer racing to rebind the coordinator port) ever actually produced a new coordinator. shutdown() now calls PeerLifecycle.sendCoordinatorHandover() after losing the gateway role but before the transport closes, since the handoff message rides the peer sessions shutdown() is about to tear down. The call is unconditional: sendCoordinatorHandover() is already a no-op when this side isn't the coordinator.
Array.prototype.filter() already returns a fresh array, so sorting it in place needs no further defensive spread.
Sending become_coordinator during a graceful shutdown (agent-comms#170) races the outgoing coordinator's own listening-socket close against the successor's rebind of the identical port: onBecomeCoordinator's dispatch in mesh-store.ts is fire-and-forget, so a bind failure there becomes an unhandled promise rejection that crashes the process, and the real mesh-e2e integration test reproduced this deterministically on every run once the sender actually started firing. becomeCoordinator now retries its bind a bounded number of times with a short delay whenever wireTransport.listen() rejects with EADDRINUSE, extracted as retryOnAddrInUse in its own bind-retry.ts (kept wire-mesh-transport.ts under the repo's max-lines cap). Retrying a plain server bind carries none of the risk the existing comment on MeshStore.init() documents against retrying a TLS connectToCoordinator (a Node TLS session-cache bug) -- this is ordinary bind-after-close backoff, and benefits the pre-existing crash-race becomeCoordinator call in init() equally, not just the new graceful path.
E2E_PORT was a fixed literal, so two concurrent vitest runs of this same file on the same machine (a real condition on a shared development host running several agent-comms worktrees at once) bind the identical port and one of them fails with EADDRINUSE. Picking a random port per process, in a band clear of every sibling integration test's own hardcoded port, avoids that collision. This is independent of, and layered on top of, becomeCoordinator's own bind-retry fix: that fix covers the real protocol-level race this test exists to exercise, this covers cross-process port contention on a shared machine.
Mearman
force-pushed
the
feat/coordinator-handover-sender
branch
from
September 17, 2026 17:03
c6d0ffb to
bd03d54
Compare
Mearman
marked this pull request as ready for review
September 17, 2026 17:05
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🎉 This PR is included in version 3.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
Closes #170
PeerLifecycle.handleBecomeCoordinator(the receiving side of abecome_coordinatorwire message) was real, wired, and unit-tested, but nothing insrc/ever sent that message -- only the crash-race path (each surviving peer independently racing to rebind the coordinator port) ever produced a new coordinator, despite the README documenting graceful handoff to the longest-running peer as live behaviour.PeerLifecycle.sendCoordinatorHandover()picks the remaining peer with the earliest recordedPeerInfo.startedAtas successor and sends itbecome_coordinatorcarrying every other remaining peer, matching thepeerListshapehandleBecomeCoordinatoralready expects. A no-op when this side isn't the coordinator or no other peers remain.MeshStore.shutdown()now calls it after losing the hub gateway role but before the transport closes, since the handoff message rides the peer sessions shutdown() is about to tear down.become_coordinatorhandler tries to rebind it, and that dispatch is fire-and-forget inmesh-store.ts, so a bind failure there was an unhandled rejection that crashed the process. Reproduced deterministically by the realmesh-e2e.integration.test.tson every run once the sender actually started firing (confirmed even with an isolated random port, ruling out cross-process contention as the cause). Fixed by retryingbecomeCoordinator's bind on EADDRINUSE with a short backoff (retryOnAddrInUse, extracted into its ownbind-retry.tsto keepwire-mesh-transport.tsunder the repo's max-lines cap) -- this also benefits the pre-existing crash-racebecomeCoordinatorcall ininit(), not just the new graceful path.mesh-e2e.integration.test.ts's own coordinator port instead of a fixed literal, since a shared dev machine can have more than one concurrent run of this same file.Test plan
pnpm typecheckpnpm lintpnpm test(full suite, 902 tests)mesh-e2e.integration.test.tsrun 5x in isolation to confirm the bind race is actually fixed, not just less likely