Skip to content

Remove HTTP-era leftovers from the WebSocket coordinator - #475

Closed
hermabr wants to merge 2 commits into
t3code/migrate-http-to-websocketsfrom
simplify-ws-protocol
Closed

Remove HTTP-era leftovers from the WebSocket coordinator#475
hermabr wants to merge 2 commits into
t3code/migrate-http-to-websocketsfrom
simplify-ws-protocol

Conversation

@hermabr

@hermabr hermabr commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Stacked on #470. Follow-up simplification: the transport migrated to WebSockets, but three HTTP-era concepts survived in the new code. This deletes them (−94 production lines net, plus one fewer race and two fewer protocol concepts).

Changes

  1. Blocking lease_job on a single Condition. "wait" existed because HTTP couldn't block a request. lease_job now blocks until a job is available or the run is done (Job | None), the lock/wake pair merges into one threading.Condition notified by mutators inside the locked section, and the server's 1s wake poll disappears — along with the missed-wakeup race between lease_job releasing the lock and the server thread acquiring wake. Tests that asserted "wait" now assert via count_satisfiable_jobs (same predicate, non-blocking).
  2. Drop redundant message envelopes. The worker→server channel is strictly sequential: one hello, then exactly one result per assignment — there is never a point where the server could receive either. Hellos validate directly with HelloMessage.model_validate_json, result frames are a bare JobResult (already discriminated on status; the adapter moves from execute.py to protocol.py), and ResultMessage, worker_message_adapter, and HelloMessage.kind go away. AssignMessage | StopMessage stays — that direction is a genuine union.
  3. lease_job no longer releases prior leases. Release-on-re-lease was load-bearing under HTTP (a crashed worker could simply request again). Over the socket the server thread enforces the sequence — it never re-leases while results are outstanding — so connection teardown (worker_lost) is now the single release path, and lease_job is side-effect-free on other state.
  4. Fold client.py into worker_loop. One caller, one connect() + one send(), and an always-true isinstance assert.

Explicitly considered and rejected: an explicit protocol state machine (the thread-per-connection blocking design already encodes the states in control flow), in-process execution for the local backend, and event-driven pool scaling.

Testing

  • pytest tests/test_worker_execution_coordinator.py tests/test_worker_subprocess.py tests/test_slurm_backend.py: 127 passed.
  • Full suite: 609 passed; the 3 failures in test_furu_locking_contention.py reproduce identically without this change (deadline-sensitive tests on AFS) and are unrelated.
  • ruff check, ruff format, ty check: clean.

lease_job now blocks on the coordinator's condition until a job is
available or the run is done, deleting the "wait" token and the server's
1s wake poll (and with it a missed-wakeup race). The lock and wake
primitives merge into a single Condition notified by mutators.

The strictly sequential worker channel drops its envelopes: workers send
a bare JobResult instead of ResultMessage, hellos validate directly, and
worker_message_adapter goes away. lease_job also stops releasing prior
leases per request - connection teardown (worker_lost) is the single
release path. client.py folds into its only caller, worker_loop.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hermabr commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@hermabr
hermabr marked this pull request as ready for review August 1, 2026 23:14
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