Skip to content

Serialize server election with a flock so only one server spawns - #15

Open
agigante80 wants to merge 1 commit into
yilunzhang:mainfrom
agigante80:fix/election-race
Open

Serialize server election with a flock so only one server spawns#15
agigante80 wants to merge 1 commit into
yilunzhang:mainfrom
agigante80:fix/election-race

Conversation

@agigante80

Copy link
Copy Markdown

Closes #14.

Problem

The server election is meant to be bind()-atomic but isn't. spawn.py sets SO_REUSEADDR on the client's bind socket, and SO_REUSEADDR permits a second bind() on the same port while neither socket has called listen() yet. So two sessions connecting at the same moment both bind() and both spawn a server:

  1. Server A writes its pidfile, listen()s, and serves.
  2. Server B writes the pidfile too — overwriting it with B's pid — then its listen() raises EADDRINUSE; its cleanup (_unlink_own_identity) sees B's own pid and deletes it, wiping the live server A's identity.
  3. Both clients then fail verify_server_identity and print server identity check failed … refusing to connect.

This is what made the four two-listener integration tests fail (test_two_clients_exchange_messages, test_send_routes_via_control, test_broadcast, test_list_shows_all_agents).

Fix

Serialize the election with a per-endpoint advisory flock (server.<port>.election.lock). ensure_server_running acquires it via a non-blocking poll that short-circuits on is_server_up (so it never wedges on a stuck holder); the holder re-checks is_server_up, then binds + spawns + waits; losers observe the server come up and return. Only one server ever spawns.

Both prior invariants are preserved:

  • server.py still writes identity before listen() (closes the different race where a client's TCP probe succeeds before the pidfile exists);
  • SO_REUSEADDR stays (needed for fast rebind after a SIGKILLed server whose connections sit in TIME_WAIT) — it's safe now that the flock guarantees a single binder.

A dead lock holder releases the flock automatically (kernel-released fd lock), so there's no deadlock risk.

Tests

  • The four two-listener tests go green — verified stable across repeated runs (20/20).
  • New deterministic concurrency test: N threads calling ensure_server_running at once → exactly one server pidfile, identity-verified, all callers see it up (8/8 across runs).

Full suite: 200 passed, 0 failed (previously 3–4 failing from this race).

Notes

Independent of the label/security PRs (branches off main), so it can land standalone.

…ses fork #5)

The election was meant to be bind()-atomic but wasn't: with SO_REUSEADDR set,
two clients racing to start a server could both bind() the port before either
listened, so both spawned a server. The loser's listen() then got EADDRINUSE,
and its cleanup deleted the pidfile — wiping the winning server's identity —
so both clients failed verify_server_identity and refused to connect. This is
what made the four two-listener integration tests fail.

Fix: serialize the election with a per-endpoint advisory flock
(server.<port>.election.lock). ensure_server_running acquires it (non-blocking
poll that short-circuits on is_server_up and never wedges on a stuck holder),
the holder re-checks then binds + spawns + waits, and losers observe the server
come up and return. Only one server ever spawns. Both prior invariants are
preserved: server.py still writes identity before listen(), and SO_REUSEADDR
stays (needed for fast rebind after a SIGKILLed server whose connections sit
in TIME_WAIT).

Tests: the four two-listener tests go green (verified across repeated runs),
plus a new deterministic concurrency test (N threads → exactly one server).
Full suite now 200 passed, 0 failed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WwNCo3qLamBCzhMVAHGuH9
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.

Race: two simultaneous clients both spawn a server, wiping identity

1 participant