[M3gA-Mind]

Summary

The broker answers the synchronous module-control members (LoadModule, LoadGithubModule, RescanModules, EnableModule, and the list/get reads) inside its async routing task. LoadModule maps a library, LoadGithubModule downloads and verifies a release (bounded by minutes of network budgets), and RescanModules walks directories. While one of them ran, the task's runtime thread was blocked: on a current-thread runtime every other connection's calls waited for it, and on a multi-thread runtime it pinned a worker.

Fix

The synchronous arm of module_method now runs under tokio::task::spawn_blocking. The admission check, the async StopModule/ReinitializeModule arms and the reply shapes are unchanged. A panicked or cancelled blocking task becomes a "module control call failed" error instead of tearing down the broker task.

Validation

Regression test a_slow_module_load_does_not_stall_other_bus_calls (in the module-control tests): on the default single-thread test runtime, one peer's LoadModule blocks for 1.5 s in the control callback while another peer calls ListModules.

cargo test -p tinybus --features modules --lib -- a_slow_module_load_does_not_stall_other_bus_calls:

	result
before (main's broker.rs)	FAILED: "ListModules waited 1.505s behind another peer's module load"
after	passed (with two existing module-control tests: 3 passed)

Ownership in the spawn_blocking closure

module_method takes body: Value by value (crates/tinybus/src/broker.rs:535), so the move closure handed to spawn_blocking takes ownership of it and is 'static with no copy. Only member is borrowed (&MemberName), so it is cloned once before the closure and re-borrowed inside it (let member = &member;).

Behaviour change

Other connections keep being routed while a module load, GitHub load or rescan runs. These calls now need a Tokio runtime with a blocking pool, which every runtime that serves the broker already has.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2c4f2447-c963-4071-a30a-1ab605ee3c57

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[tinysweeper]

Tiny Sweeper review

Reviewed head: 2725fdebc65facc3ddda812b0beea4273706ef57

Findings

No active actionable findings.

Resolved this pass

• Move the module body out of the Context capture
• Move the module body out of the Context capture

Verdict: Ready for maintainer review · Priority: none
Coverage: Reviewed 2 of 2 files.

Diagnostics and supporting details

Full summary

Pull request purpose (title): fix(broker): run module control calls on the blocking pool. Tiny Sweeper completed its review; deterministic results follow.

Review snapshot

Change surface	Files	Review signal	Count
Production	1	Active findings	0
Tests	1	Noted findings	0
Documentation	0	Resolved findings	2
Configuration	0	Pending checks/questions	0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

In `impl Broker {` (crates/tinybus/src/broker.rs), the synchronous module-control dispatch clones the member, rebinds it as `let member = &member;` inside a `move` closure handed to `tokio::task::spawn_blocking`, awaits the handle, and maps a `JoinError` through `unwrap_or_else` to `Error::failed("module control call failed: {error}")`. The critique lane reports the load, GitHub load, rescan and list/get work moving to the blocking pool, with the `Reinitialize`/`Stop`/`Enable` arms still borrowing correctly and the stop/reinitialize async paths returning early before the spawn; the security lane notes the join error carries only the JoinError display, not the request body, and that admission still runs synchronously before the closure. In crates/tinybus/src/broker_module_control_tests.rs, `RecordingControl`'s load callback sleeps 1.5 s for a `slow.so` path, and the new test `a_slow_module_load_does_not_stall_other_bus_calls` exercises the non-stalling behavior on the single-threaded default test runtime. The intended behavior is that other connections keep being routed during a long module operation, and these calls now require a runtime with a blocking pool.

Features

• Modified — Module control calls dispatched on the blocking pool: Load, GitHub load, rescan and list/get module-control work now runs inside `spawn_blocking` instead of on the routing task's runtime thread, so a slow load no longer stalls other connections' routing; admission still runs synchronously before the closure, and the async stop/reinitialize paths are unchanged. (crates/tinybus/src/broker.rs impl Broker {)
• Added — Blocking-task failure surfaced as an error: A panicked or cancelled blocking task is caught with `unwrap_or_else` and returned as a "module control call failed" error carrying only the JoinError display instead of unwinding the broker task; no test exercises this branch. (crates/tinybus/src/broker.rs impl Broker {)
• Modified — Slow-load simulation in the recording test control: `RecordingControl`'s load callback sleeps `SLOW_LOAD` (1.5 s) when the path is `slow.so`, letting the regression test simulate a blocking library map, GitHub download or rescan. (crates/tinybus/src/broker_module_control_tests.rs impl ModuleControl for RecordingControl {)
• Internal refactor — Closure capture ownership for the blocking dispatch: The `move` closure handed to `spawn_blocking` takes `body` by value and takes only a cloned-then-re-borrowed `member`, which the critique and description lanes describe as resolving the earlier `Send`/`'static` capture concern, so the dispatch body compiles and the state transitions still come back as a `Vec<Value>` to be broadcast as before. (crates/tinybus/src/broker.rs impl Broker {)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Before merge

None.

Agent review details

critique

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The critique lane returned Answered with no reportable findings, describing the earlier critical finding on the closure body as resolved and noting retrieval was degraded.
• Lane summary: The change moves the blocking module-control calls (load, GitHub load, rescan, list/get) onto the blocking pool so one slow load no longer stalls routing, and adds a test proving it. The earlier critical finding about the closure body—specifically that `member` was moved into the closure while it is still used by earlier arms—is resolved: `member` is now cloned and rebound as `let member = &member;`, and the `Reinitialize`/`Stop`/`Enable` arms it depends on still borrow it correctly, so the body compiles. The spawn_blocking join error is mapped to `Error::failed` and the `Some(...)` outcome is returned as before. One remaining concern: the closure captures `control: Arc<dyn ModuleControl>` and `body`, which are moved into the `'static` closure — the `stop`/`reinitialize` paths are `async` and return early, so they don't reach the spawn; the branches that do reach it are synchronous methods on `control`, and state transitions returned by the closure are sent back as a `Vec<Value>` to be broadcast as before. No concrete defect identified. _The code index is behind this pull request (indexed at `f392a3f1b3cd`), so retrieved context may be out of date._

security

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The security lane returned Answered with no reportable findings: `member` is cloned before the closure, the join error maps to a failure carrying only the JoinError display and not the request body, and the blocking boundary is handled with a `move` closure plus an `unwrap_or_else` fallback.
• Lane summary: The change looks sound. The prior critical finding about the Context capture was fixed by explicitly cloning `member` before the `spawn_blocking` closure. The new test demonstrates that a blocking load runs on the blocking pool and does not stall other bus calls, exactly as the comment in the diff describes. No new security or correctness issues are introduced: the `spawn_blocking` boundary is correctly handled with a `move` closure and a `catch_unwind`-like `unwrap_or_else` fallback; the member name is cloned before being moved into the closure and borrowed inside; the test uses deterministic timing with a known slow load duration and a generous assertion margin. (1 earlier finding(s) still open) _The code index is behind this pull request (indexed at `f392a3f1b3cd`), so retrieved context may be out of date._

tests

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The tests lane returned Answered with no reportable findings, describing the new test as exercising exactly the non-stalling property with a property-named test and a timing invariant that would fail if the fix regressed.
• Lane summary: The pull request looks sound. The behavioural change moves blocking module control operations (load, github load, rescan) from an inline closure into `spawn_blocking`, preventing them from stalling the async runtime's only thread. The new test exercises exactly this property with a 1.5-second blocking sleep in the fake `load`, asserts that `ListModules` completes well before half that time, and runs on a single-threaded runtime to reproduce the original stall. The test is named by property, uses no `sleep` as synchronisation (the only `sleep` is the injected blocking work inside `load`), and asserts a timing invariant that would fail if the fix regressed. No finding. (1 earlier finding(s) still open) _The code index is behind this pull request (indexed at `f392a3f1b3cd`), so retrieved context may be out of date._

commits

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The commits lane returned Answered, reporting no sensitive content in committed files.
• Lane summary: No sensitive content in committed files. (1 earlier finding(s) still open) _The code index is behind this pull request (indexed at `f392a3f1b3cd`), so retrieved context may be out of date._

description

• Conclusion: Success
• Scope reviewed: all assigned evidence
• Positive: The description lane returned Answered with no defect worth reporting: the diff matches the description, with `body` taken by value into the `'static` closure and the guard preserved.
• Lane summary: The change moves the synchronous module-control arms onto `tokio::task::spawn_blocking` with a `'static` closure over `body` and a cloned `member`, replacing a plain immediately-invoked closure; the earlier concern that `body` was captured by reference in a `'static` closure is fixed by taking `body` by value and the guard is preserved. The diff is small and internally consistent; I found no defect worth reporting in it. _The code index is behind this pull request (indexed at `f392a3f1b3cd`), so retrieved context may be out of date._

Evidence and run details

• Models: ladder/vectors-oai3, deepseek-v4.1-flash, deepseek-v4-flash
• Spend: $0.002079
• Completion usage: incomplete; some provider completion costs are unavailable.
• Tokens: 191154 input · 3900 output · 14336 cached · 170 embedding
• Updated: 2026-10-23T10:52:47Z

Head	State	Pass summary
c048d277b2c9	ready for maintainer review	0 active finding(s), 0 resolved finding(s) (at 2026-10-22T23:51:44Z)
250c0097852c	ready for maintainer review	0 active finding(s), 0 resolved finding(s) (at 2026-10-23T08:03:33Z)
b713bba0ce82	changes requested	1 active finding(s), 0 resolved finding(s) (at 2026-10-23T09:05:10Z)
2725fdebc65f	ready for maintainer review	0 active finding(s), 2 resolved finding(s) (at 2026-10-23T10:52:47Z)

[tinysweeper]

tinysweeper found nothing blocking. Approving.

Diagnostics

Total spend: $0.0011

[tinysweeper]

Requesting changes: 1 lane(s) blocking, worst finding is critical.

Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.

Diagnostics

Total spend: $0.0007

[M3gA-Mind]

Re tinysweeper's critical "Move the module body out of the Context capture": declining, because the premise is incorrect. body is an owned Value parameter of module_method (body: Value, crates/tinybus/src/broker.rs:535), so the move closure passed to spawn_blocking takes ownership of it and the closure is 'static. Only member is borrowed (&MemberName), and the PR already clones it before the closure. That compiles: module-ci / clippy, module-ci / test and module-ci / msrv all pass at b713bba. A body.clone() would copy the request for nothing. Leaving this open for a maintainer to dismiss.

[M3gA-Mind]

@tinysweeper review

[tinysweeper]

The previously-blocking findings are resolved. Clearing the changes request.

Diagnostics

Total spend: $0.0021