refactor(broker): split main entrypoint#906
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplits the broker binary into a CLI module ( ChangesBroker CLI + Runtime Split
Sequence Diagram(s)sequenceDiagram
participant Cli as Cli::run()
participant Init as run_init()
participant Relay as Relaycast (Auth/HTTP)
participant Runtime as BrokerRuntime
participant Worker as WorkerRegistry
Cli->>Init: parse args & invoke run_init / subcommand
Init->>Relay: connect_relay (AuthClient / startup session)
Relay-->>Init: workspace/session info
Init->>Runtime: construct BrokerRuntime & start run()
Runtime->>Worker: queue_and_try_delivery / retry_pending_delivery
Worker-->>Runtime: delivery result / events
Runtime->>Relay: relaycast pre-register / mark online/offline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
willwashburn
force-pushed
the
codex/issue-875-split-broker-main
branch
from
74b8327 to
cb59df6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/broker/src/runtime/tests.rs (1)
1318-1337: 🏗️ Heavy liftAvoid mirroring spawn-flag logic inside tests.
compute_bypass_flagandcompute_model_flagduplicate production decision logic, so tests can drift from real behavior. Prefer asserting against extracted production helpers used byWorkerRegistry::spawn().Also applies to: 1664-1676
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/runtime/tests.rs` around lines 1318 - 1337, The test-local functions compute_bypass_flag and compute_model_flag duplicate production logic and should be removed; update the tests to call the shared decision helpers used by WorkerRegistry::spawn() instead of mirroring logic. Replace calls to compute_bypass_flag/compute_model_flag in tests.rs with the real helper functions (the same helper(s) that WorkerRegistry::spawn() uses to decide flags/model), import them into the test module, and update assertions to reflect the helper outputs; remove the duplicated functions at both locations (around the shown block and lines 1664–1676).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/broker/src/runtime/messages.rs`:
- Around line 278-289: The parse_sort_key_from_raw_timestamp function mixes
units: numeric strings are returned as raw i64 (seconds-like) while RFC3339
parsing yields milliseconds, so normalize and/or document the behavior; modify
parse_sort_key_from_raw_timestamp to, after parsing trimmed.parse::<i64>(),
apply a seconds-vs-milliseconds heuristic (e.g., if value < 4_102_444_800 then
treat as seconds and multiply by 1000) so the function always returns
milliseconds, keep the RFC3339 branch unchanged, and add/update a doc comment on
parse_sort_key_from_raw_timestamp explaining that returned values are
milliseconds and update relevant tests to expect milliseconds.
In `@crates/broker/src/runtime/paths.rs`:
- Around line 37-48: ensure_ephemeral_paths currently builds the temp root only
from std::process::id(), causing multiple ephemeral brokers in the same process
to collide; use the unused _broker_name parameter and/or a per-instance random
id to make the directory unique. Update the root construction in
ensure_ephemeral_paths to include a sanitized broker identifier and a randomly
generated component (e.g., UUID or secure random hex) — e.g., join
format!("agent-relay-ephemeral-{}-{}", std::process::id(), uuid) or include
broker_name in the format — then create_dir_all on that new root and keep
building RuntimePaths (state, pending, _lock) from it so each broker instance
gets its own state.json/pending.json files.
In `@crates/broker/src/runtime/session.rs`:
- Around line 168-193: Remove token-derived data from the debug artifact by
deleting the token_prefix line and any use of self_token when building
identity_debug in session.rs (the formatted string assigned to identity_debug).
Ensure the formatted output no longer references &self_token or slices thereof
and only includes non-token fields (agent_name, requested_name, agent_id,
default_workspace, workspace_count, timestamp); keep the existing
AGENT_RELAY_NO_DEBUG_FILES gating unchanged so files are still prevented when
the env var is set.
In `@crates/broker/src/runtime/tests.rs`:
- Around line 453-475: This test mutates process-global environment variables
causing flakiness in parallel runs; protect env changes by acquiring a global
test mutex before modifying env and releasing after (use a static
Lazy<Mutex<()>> via once_cell or lazy_static), so in
contract_startup_429_fixture_requires_degraded_health_status lock the mutex
before std::env::set_var and hold it until after std::env::remove_var (or use a
RAII guard to ensure removal in a finally/drop), and apply the same pattern to
the other env-mutating tests (the ones referenced around the other fixture
block) so all tests that set AGENT_RELAY_STARTUP_ERROR_CODE (and similar env
keys) serialize their execution.
---
Nitpick comments:
In `@crates/broker/src/runtime/tests.rs`:
- Around line 1318-1337: The test-local functions compute_bypass_flag and
compute_model_flag duplicate production logic and should be removed; update the
tests to call the shared decision helpers used by WorkerRegistry::spawn()
instead of mirroring logic. Replace calls to
compute_bypass_flag/compute_model_flag in tests.rs with the real helper
functions (the same helper(s) that WorkerRegistry::spawn() uses to decide
flags/model), import them into the test module, and update assertions to reflect
the helper outputs; remove the duplicated functions at both locations (around
the shown block and lines 1664–1676).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4231966b-9535-4bfa-b89b-73523704da1f
📒 Files selected for processing (17)
.trajectories/completed/2026-05/traj_47akjihewlow.json.trajectories/completed/2026-05/traj_47akjihewlow.md.trajectories/completed/2026-05/traj_47akjihewlow.trace.json.trajectories/index.jsoncrates/broker/src/runtime/connection.rscrates/broker/src/runtime/delivery.rscrates/broker/src/runtime/headless.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/io.rscrates/broker/src/runtime/messages.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/paths.rscrates/broker/src/runtime/session.rscrates/broker/src/runtime/spawn_spec.rscrates/broker/src/runtime/system.rscrates/broker/src/runtime/tests.rscrates/broker/src/runtime/util.rs
✅ Files skipped from review due to trivial changes (3)
- .trajectories/completed/2026-05/traj_47akjihewlow.json
- .trajectories/completed/2026-05/traj_47akjihewlow.md
- .trajectories/completed/2026-05/traj_47akjihewlow.trace.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .trajectories/index.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/broker/src/runtime/api.rs`:
- Around line 1042-1087: The SubscribeChannels/UnsubscribeChannels branches only
mutate handle.spec.channels locally; after updating that vector you must
propagate changes upstream and into persisted agent state: for
SubscribeChannels, after pushing new channels (added), call the
workspace/relaycast subscription API to subscribe the worker's workspace to the
newly added channels (e.g., a helper like subscribe_worker_channels(name,
&added) or workers.workspace.subscribe_channels(&name, &added)), and also update
state.agents (the persisted agent entry for name) to include the new channels
when running in persist mode; for UnsubscribeChannels, after retain() completes
call the workspace/relaycast unsubscription API to remove those channels and
mirror the remaining channel list into state.agents (or remove entries) so
persisted state matches handle.spec.channels. Ensure both branches handle empty
added/removed lists gracefully and send the reply only after upstream and
state.agents are updated.
In `@crates/broker/src/runtime/event_loop.rs`:
- Around line 60-71: The select is repeatedly polling closed channels (api_rx,
ws_inbound_rx, worker_event_rx) which returns None immediately and spins; wrap
each of these fields as Option<...> (e.g. self.api_rx: Option<Receiver<...>>)
and only poll them when Some, using Option::as_mut()/take() in the select arms;
when recv() yields None, set that Option to None (drop the receiver) and emit
the appropriate single-time closed event (e.g. RuntimeEvent::ApiClosed) so the
arm stops being polled thereafter; apply the same pattern for ws_inbound_rx and
worker_event_rx (also at the other select block referenced) so closed receivers
are disabled instead of repeatedly selected.
In `@crates/broker/src/runtime/maintenance.rs`:
- Around line 304-343: After a successful workers.spawn (inside the Ok(_)
branch) refresh the persisted agent metadata for this agent name so the saved
PID/start time/restart_count are updated and not left pointing at the crashed
process; specifically, update the state.agents entry (or the persistence layer
holding agent metadata) for name with the new runtime fields (new PID, new start
timestamp, updated restart_count) right after workers.supervisor.on_restarted
and workers.metrics.on_restart and before sending the agent_restarted
event/publishing the agent state transition (so run_init won’t later reap the
live agent due to a stale persisted PID).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9aee47e7-eabd-45fb-b2e7-6571f22eab21
📒 Files selected for processing (11)
.trajectories/completed/2026-05/traj_x37bhga2j5ph.json.trajectories/completed/2026-05/traj_x37bhga2j5ph.md.trajectories/index.jsoncrates/broker/src/runtime/api.rscrates/broker/src/runtime/event_loop.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/maintenance.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/tests.rscrates/broker/src/runtime/worker_events.rs
✅ Files skipped from review due to trivial changes (2)
- .trajectories/completed/2026-05/traj_x37bhga2j5ph.json
- .trajectories/index.json
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/broker/src/runtime/mod.rs
- crates/broker/src/runtime/tests.rs
barryollama
approved these changes
May 19, 2026
barryollama
left a comment
barryollama
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: ✅ APPROVE
This is a well-executed refactoring that successfully decomposes the monolithic ~7,700-line main.rs into a clean, modular structure.
Highlights
Clean module organization:
- event_loop.rs — central event dispatch with BrokerRuntime struct
- init.rs — startup/initialization logic
- api.rs — HTTP request handlers
- headless.rs, delivery.rs, worker_events.rs, relaycast_events.rs — focused domain modules
- session.rs, paths.rs, connection.rs — supporting utilities
Simplified main.rs: Now just 29 lines — module declarations and a delegating main().
Tests preserved: All 369 tests pass (357 + 12 continuity tests), confirming no regressions.
Clean build: cargo check and cargo clippy -D warnings both pass.
Architecture
The BrokerRuntime struct provides explicit long-lived state, and the RuntimeEvent enum keeps the tokio::select! loop clean and manageable. The delegation pattern from cli/mod.rs to runtime/ modules is well-structured.
Good work on this organizational improvement — the broker code is now much more maintainable.
This was referenced May 19, 2026
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
2 participants
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.
Summary
Closes #875
Verification