(7/7) feat(session): launch the supervisor topology; delete the single-process chassis#1519
(7/7) feat(session): launch the supervisor topology; delete the single-process chassis#1519guapisolo wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an opt-in multi-process session server supervisor (SessionServerSupervisor) that spawns and monitors multiple worker processes and a router process to shard sessions, controlled by the new --session-server-workers argument. The review feedback highlights several critical reliability and resource management improvements: ensuring the supervisor is explicitly shut down in RolloutManager.dispose() to prevent port-binding conflicts, handling process spawning failures gracefully to avoid orphan processes, unregistering from atexit to prevent memory leaks, and making the supervisor's shutdown method thread-safe using a lock.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
2e99b9d to
31c6638
Compare
9e9dd8c to
a1b8548
Compare
31c6638 to
506e3da
Compare
a1b8548 to
1c688b8
Compare
506e3da to
e4dfc23
Compare
1c688b8 to
9e0fc91
Compare
e4dfc23 to
2e7fdc6
Compare
9e0fc91 to
18e6d8b
Compare
2e7fdc6 to
756bdbb
Compare
18e6d8b to
17c5f1d
Compare
756bdbb to
2076ac7
Compare
17c5f1d to
1bb4956
Compare
2076ac7 to
2e76cb4
Compare
1bb4956 to
fceaa54
Compare
2e76cb4 to
9668f79
Compare
fceaa54 to
e05b81a
Compare
9668f79 to
948ef7a
Compare
e05b81a to
dcedb28
Compare
948ef7a to
4ccc465
Compare
165ffc1 to
3c1d6e0
Compare
4ccc465 to
b7b352c
Compare
3c1d6e0 to
45f0e76
Compare
b7b352c to
3421e64
Compare
bc6f752 to
736dd29
Compare
736dd29 to
2fac59c
Compare
69b53e8 to
8d6973a
Compare
|
A successful CI run https://github.com/radixark/miles/actions/runs/28771003169 |
2fac59c to
e87eab4
Compare
a7bfd2e to
4805a46
Compare
…ose() dispose() released the metric checker and health monitors but left the supervisor's worker/router processes and their bound port to the atexit hook, which only fires when the actor process exits — an actor disposed and reused would still hold the port. shutdown() is idempotent and thread-safe, so the atexit backstop stays harmless. Addresses the Gemini review comment on #1519. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ee3eeeb to
e755326
Compare
…ess chassis start_session_server now always builds and starts a SessionServerSupervisor — one topology for every worker count; --session-server-workers=1 (the new flag's default) is simply N=1, not a separate code path. It returns the supervisor, and rollout_manager calls check() at the start of BOTH generate() and eval(), so a dead worker fails the rollout loudly instead of letting requests to its hash shard hang. sessions.py and server.py are deleted. Two chassis duplicate the route surface (the same six routes defined twice) and the proxy stack (SessionServer.do_proxy vs the worker's ProxyBackend, held identical only by equivalence tests), and every future behavior change pays that tax twice. The old workers=1 server was already a spawned child; the delta is one extra tokenizer-free router child and a per-request IPC hop measured in µs against LLM-turn latencies. Doc and header wording that referenced the deleted chassis is updated to the one-chassis reality. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rationale (fail-fast on a dead worker's hash shard, called from both generate() and eval()) lives in the doc-dev spec and supervisor.check()'s docstring; the inline restatement was redundant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ling knob
The old help text described the pre-switchover behavior ("1 = single-process,
no router/IPC"), contradicting the one-topology reality. Both spots now say
when to turn the knob: if the session server is the bottleneck, more workers =
more interpreters, so GIL-bound tokenization/deserialization scales with cores.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Goal and the requirements bullet now just say what runs (supervisor, N workers, one thin router) instead of arguing against a second chassis; the rationale stays in the PR/commit history. The workers=1-topology parity bullet goes too, restoring agreement with the design-targets line that references two known deltas. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Idle workers cost ~zero CPU (a sleeping event loop + IPC reader), and under concurrent multi-turn load a single worker's GIL serializes tokenization/deserialization, so a multi-core default speeds up typical agentic runs out of the box. Cost is memory (each worker is its own interpreter + tokenizer, ~0.3-0.5 GB) and 9 child processes instead of 2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rence The whole group (--use-session-server through --tito-allowed-append-roles) was never in the complete reference; --session-server-workers landing here makes the gap visible, so the section arrives with all six flags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntials Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The deleted chassis was imported at router_manager's module top too, so this restores the pre-switchover import weight — no reason to defer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rgon Also clears two leftovers the switchover missed: create_session's inline comment still described the deleted single-process path, and the doc's decomposition bullet still said 'reused unchanged by both chassis'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ose() dispose() released the metric checker and health monitors but left the supervisor's worker/router processes and their bound port to the atexit hook, which only fires when the actor process exits — an actor disposed and reused would still hold the port. shutdown() is idempotent and thread-safe, so the atexit backstop stays harmless. Addresses the Gemini review comment on #1519. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same comment-only sweep, applied to the docstrings this branch introduces or rewrites (core, worker, router_manager). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bed5328 to
d65f45b
Compare
Shi-Dong
left a comment
There was a problem hiding this comment.
LGTM! Great job.
But let's postpone merging the chain now, as it's intrusive and might break what is currently working on main.
Summary
The switchover:
start_session_servernow always builds and starts aSessionServerSupervisor— one topology for every worker count, so theif workers > 1launch branch never exists — and the single-process chassis is deleted in the same move.What's here
router_manager.start_session_server— always supervisor; returns it.rollout_managerholds it and callscheck()at the start of bothgenerate()andeval(), so a dead worker fails the rollout loudly instead of black-holing its hash shard.arguments.py—--session-server-workers(int, default 8: idle workers cost ~zero CPU, and under concurrent multi-turn load GIL-bound tokenization/deserialization scales with cores; memory scales with N — each worker loads its own tokenizer).sessions.py+server.pydeleted. Two chassis duplicate the route surface (the same six routes defined twice) and the proxy stack (SessionServer.do_proxyvs the worker'sProxyBackend, held identical only by equivalence tests), and every future behavior change pays that tax twice. The old workers=1 server was already a spawned child; the delta is one extra tokenizer-free router child and a per-request IPC hop measured in µs against LLM-turn latencies.docs/developer/multi-process-session-server.md,core.py,worker.py,processing_utils.py).Testing
pytest tests/fast/router— 96 passed at this commit (assertions carried unchanged through (6/7) test(session): InloopSessionServer replaces the in-process SessionServer test harness #1566);generate_hub/test_multi_turn+inference_rollout/integrationsuites pass on the reworked fixtures.Not here
Round-robin / no-HOL IPC stays cut (benchmark-gated, see the dev doc). The R3 client-strip landed below this stack in #1563. The worker-direct records path (decided, see the dev doc) and the benchmark port are follow-up PRs.
🤖 Generated with Claude Code