Repository navigation
refactor(relay)!: the cluster polls its own work instead of spawning - #7626
Conversation
Quest-Claim: 36abfa25ebce8563
kixelated
changed the title
`Cluster::new` returns `(Cluster, Driver)`: the origin driver and cache governor run as `kio::Tasks` children of a `Driver` the owner polls, and `Relay::run` polls it for its whole run, reporting a panic through the cluster's critical slot so teardown still runs. `Started::run` polls every peer dial, the `api.url` reconciler, and LAN discovery as `kio::Tasks` children; the sources queue dial-set changes the mesh applies, and `DialMap` owns its dial tasks by `TaskKey`. A dropped LAN dial's client close joins a polled set instead of a detached spawn. Each child's panic becomes an error naming it, so the relay still exits with "origin driver panicked: ..." and similar. Closes the quest quest/m1/poll-first/no-spawn/cluster.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 37 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3f262e6
No actionable bugs found. The explicit cluster driver and owner-polled dial set are a reasonable fit for the poll-first direction. Reusing kio::Tasks plus a command queue avoids detached-task ownership while preserving source precedence, cancellation, panic reporting, and shutdown draining. The constructor/API migration and shared-token handling are documented; LAN targets still exclude that token.
Verification limits: static review of the changes and relevant callers/callees only. I did not run builds, tests, LAN/shutdown scenarios, or reproduce the reported peer-dial measurements.
(Written by OpenAI)
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No behavioral issues found. Two stale docs still describe the cache governor as spawned at construction.
Reviewed changes
Cluster::newreturns aDriver: the origin driver and cache governor are nowkio::Tasks<Child<()>>children, andpoll/runreport the first panic.Relay::runpolls the driver: it runs besiderun_innerfor startup and teardown, feeding driver errors intoCritical::failsoStarted::runstill exits withorigin driver panicked: ....Started::rundrives aMesh: ownedDialMap<Child<()>>pluskio::Taskssources and akio::Queue<DialCommand>, replacing theJoinSetand detached per-peer tasks.Dialersclosing set:Held::dropqueues aChild<()>close polled by the mesh and drained by the shutdown, replacing the detached spawn and mpsc completion channel.- Call-site migration: moq-cli, benches, relay unit tests, integration tests, MIGRATE, CHANGELOG, and quest docs updated for the new driver contract.
I traced the task-retirement/wake behavior in kio::Tasks, the DialMap replace/release ordering against Mesh::apply and poll_closing, the lane where a cancelled Started::run defers link closes to Cluster::close, and every in-repo Cluster::new caller. No correctness issues found.
ℹ️ Docs still say the cache governor is spawned by construction
Cache's type docs and Relay::load's docs both describe the governor as starting in Cluster::new/load, but it now runs only while the returned Driver is polled. The governor docs are the user-facing contract for when it begins sampling, so they should match the new ownership.
Technical details
# Stale "spawned at construction" governor docs
## Affected sites
- `rs/moq-relay/src/cache.rs:74` — says `Cluster::new` "runs it as a critical task"; it is now a `Child` in the returned `Driver`.
- `rs/moq-relay/src/relay.rs:137` — says `Relay::load` performs "spawning the cache governor" as a startup side effect; `load` now only stores the `Driver`, and nothing runs until `Relay::run` polls it.
- `rs/moq-relay/src/cache.rs:99` (imprecise) — "The governor starts with the cluster the `Cache` is attached to"; it starts when the `Driver` is polled.
## Required outcome
- The docs say the governor (and the origin's lifecycle work) start only when the returned `Driver` is polled, and that `Relay::run` does that. No claim that `Cluster::new`/`Relay::load` spawns it.
## Suggested approach
- Rephrase both sites to name the `Driver` rather than a spawned critical task, matching `MIGRATE.md` and the updated `Cluster::new` docs.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…olled Cache and Relay::load still described the governor as spawned at construction; it now lives in the Driver that Cluster::new returns and runs only while Relay::run polls it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks pullfrog, agreed on the stale governor docs (review). Fixed in 89d2aef: the (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Governor docs corrected:
Cache's type docs,Config::init, andRelay::loadnow say the headroom governor moves into theDriverreturned byCluster::newand samples only while that driver is polled (whichRelay::rundoes).Relay::loadno longer claims to spawn anything.
I confirmed the new wording against the code: Cluster::new pushes the governor as a Child into Driver.work (rs/moq-relay/src/cluster.rs:1376) and returns Driver { work } (rs/moq-relay/src/cluster.rs:1408); Relay::load_inner only stores the driver (rs/moq-relay/src/relay.rs:424) and Relay::run polls it (rs/moq-relay/src/relay.rs:621). The prior review's finding is fully addressed.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Problem
Cluster polls its own work: moq-relay's
Clusterspawned its origin driver and cache governor, aJoinSetof static dials, one detached tokio task per API or LAN peer, theapi.urlreconciler, mDNS discovery, and a detached close for each dropped LAN client. The poll-first epic wants libraries to spawn nothing and hand that work to the owner askio::Taskschildren it polls.Approach
Follows the maintainer's recorded decisions (D2 and D5 on moq-dev/moq#7519).
Cluster::newreturns(Cluster, Driver).Driverholds the origin driver and the cache governor askio::Taskschildren. Itspoll(orrun) resolvesOkonce all of its work finishes, or with the first panic as"<task> panicked: <message>". Dropping it tears the origin down, so it is#[must_use].Relay::runpolls the driver for the whole run, startup and teardown included. A driver error goes into the cluster'sCriticalslot, soStarted::runreturns it through the mainselect!and the relay still closes its dials and listeners on the way out. The exit error staysorigin driver panicked: ....Started::rundrives aMesh: aDialMapthat owns its dial tasks bykio::TaskKey, plus akio::Tasksof peer sources (theapi.urlreconciler and mDNS discovery). Sources cannot reach the task set that is polling them, so they queueDialCommands on akio::Queue, and the mesh applies them on its own poll. Replacing or releasing a dial removes its task, which cancels it.Held::dropqueues the LAN client's close into a polledkio::TasksonDialers.Started::runpolls that set, andCluster::closedrains it, replacing the detached spawn and its mpsc completion channel.critical::Child<T>wraps a named boxed future as akio::Taskand turns its panic into an error. The driver, the mesh, and the close set all use it.Choices made without asking, for review:
--cluster-token. Before, the token a dial got depended on which source spawned it: an mDNS release that fell back to a static or API target redialed with no token. Tokens are still skipped for LAN targets (target.lan), so LAN behaviour is unchanged.moq_tokio::panic::isolate, so a panic that reaches the supervisor is in the redial loop itself.Started::runleaves its links' close toCluster::close, instead of the detached close starting on its own.Relay::runalways callsCluster::close; the docs say so.Spawns that stay:
Criticalstill runs the stats publisher (owned by Admissions and stats are polled), ACME renewal, and the web certificate reload (both owned by the spawn audit). Session I/O is still spawned inside moq-tokio's reconnect loop, which the spawn audit owns.The admissions quest's plan is updated for the publisher joining
Driver.Cluster::newhas already handed the driver out by the timewith_statsruns, so the publisher needs either a queue the driver polls or a field onOptions.Peer-dial cost (D5)
Every dial supervisor now runs on the task polling
Started::runinstead of its own tokio task. I measured this with a one-off in-crate test, not committed. It dialsnstatic peers at distinct paths of one in-processtest_relayover loopback QUIC, on a 4-worker runtime. It records the wall time and process CPU until the cluster's/nodesview shows allnlinks connected, then the process CPU per second over a 2 s idle window. Medians of 10 rounds, interleaved main/branch/main/branch underquest gate. Themainside is103e98e362.Putting the supervisors on one task costs nothing measurable; the branch was faster and used less CPU at every size. A supervisor only awaits status changes from moq-tokio's spawned reconnect loop, and
kio::Tasksre-polls only the dial that was woken. The speedup is probably from dropping one tokio spawn per peer, but I didn't break it down. The idle CPU is the post-handshake announce exchange settling in both builds, not the supervisors.Impact
moq_relay::cluster::Cluster::newreturnsanyhow::Result<(Cluster, Driver)>, and there is a newmoq_relay::cluster::Driverwithpollandrun. Embedders keep the driver and poll it, for exampletokio::spawn(driver.run());Relaydoes this itself. The migration is inrs/moq-relay/MIGRATE.md, with an entry inCHANGELOG.md. In-repo call sites are migrated: moq-cli spawns the driver into its task set, and the integration tests and benches spawn it as edges.Validation
quest gate just test -p moq-relay -p moq-cli --no-fail-fast: 668 passed, 26 skipped. This includes new tests: the driver panic stopsRelay::run; the mesh dials what a source queues and fails with it; a dial panic ends the mesh; the mesh finishes once nothing is left; acritical::Childpanic becomes its output.DialMapand the cache-governor tests were rewritten against the owned task set and the driver.quest gate just check -p moq-relay -p moq-cli(clippy on all targets, lint, hakari): passed.cargo check -p moq-relay -p moq-cli --all-targetswithtest-support,io-uring,stun, andiroh: passed.quest check: 582 documents OK.origin/main(d26e2aac82) and the doc fix:quest gate just check origin/main(rust check + tests, rustdoc with-D warnings, markdown, gh, quest, shell, toml) passed.Review
quest review), gpt-6.1-sol, medium effort, session01a19d7b-0ad9-7983-9e46-f08ab89f636c, round 2.89d2aefbf63c0d609b8b14b00b691a2266d453adagainstorigin/mainatd26e2aac82: 0 findings.7c464b0fa3) also reported 0 findings.3f262e6d77: OpenAI found nothing actionable; pullfrog found no behavioral issues and flagged stale docs saying the cache governor is spawned at construction (Cachetype docs,Config::init,Relay::load). Fixed in89d2aefbf6: those docs now say the governor runs inside the returnedDriver, only while it is polled (whichRelay::rundoes). CodeRabbit was rate-limited and posted no review.Follow-ups
None new. The remaining spawns already belong to Admissions and stats are polled and the spawn audit.
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)