Repository navigation
feat(l1,l2): adopt spawned v0.7 with one shutdown path, request timeouts, supervision, metrics and a testing roadmap - #7518
unbalancedparentheses wants to merge 14 commits into
Conversation
Move spawned-concurrency and spawned-rt from 0.5.0 to v0.7.0, and add the spawned-test crate as a workspace dependency for the actor tests that follow. All three point at one git revision (39be3f3b: the v0.7.0 release, 02b9400b, plus the unreleased spawned-test crate, lambdaclass/spawned#198), so they share one set of types. Move to the crates.io release once spawned-test is published. Mechanical only: 0.5 protocol and handler signatures are unchanged in 0.7, and no source change is needed. Behavior to keep in mind (from 0.6): once an actor is stopped its queued messages can be discarded and pending requests return `ActorStopped`. Ethrex already treats `ActorStopped` from the peer table as "p2p is shutting down". ethlambda depends on ethrex-p2p, so this lets it drop its duplicate spawned 0.5 once it moves to a revision with this change.
Shutdown was wired by hand and raced itself. The peer table, the RLPx initiator and discovery each stopped on their own ctrl-c listener, while the main path cancelled the node's token, asked the peer table for its connected peers (saving an empty list if it had already stopped), flushed the database and slept 1 s. A sync cycle that saw the stopped peer table before the token was cancelled took `ActorStopped` as fatal and called `exit(2)`, skipping the flush; the same race exited the ethrex_tests binary in 6 of 25 failed CI runs (#7491). Now one spawned `ActorSystem`, owned by `L1Node`, holds the node's long-lived actors on a `p2p` async group sized like the default Tokio runtime: the peer table, the tx broadcaster, the RLPx initiator, the WebSocket subscription manager and discovery, admitted in that order so the peer table, which the others call, stops last. No actor listens for ctrl-c any more. SIGINT, SIGTERM and a fatal subsystem all lead to `shutdown_l1`, which, under an 8 s deadline (Docker gives 10 s): 1. cancels the node's token, so sync, RPC and the block producer stop first; 2. snapshots the connected peers while the peer table still runs, keeping the previously saved peers if it cannot answer within 2 s instead of saving an empty list; 3. flushes the database; 4. saves node_config.json; 5. stops the owned actors with what is left of the deadline (at least 1 s). A shutdown that misses the deadline logs the unfinished groups and actors by name and exits 1 rather than wait on them with no bound; the database is already flushed by then. The 1 s sleep is gone: it waited on nothing. The sync cycle now decides through `SyncError::failure_action`: once the node is shutting down, a failed cycle stops (`StopForShutdown`) instead of exiting, and the sync loop does not start another. Outside shutdown, `ActorStopped` stays fatal. Per-connection actors stay outside the system: they are created per peer and spawned does not have a dynamic supervisor yet. The plain `spawn` functions remain for L2, the partial node, tests and ethrex-p2p's other users; they no longer stop on ctrl-c either, so their owner decides when they stop. L2 dev mode keeps the dev L1 node's `L1Node` alive for as long as it runs.
Every peer-table request carried spawned's hardwired five-second timeout. A timed-out `get_best_peer` under mailbox pressure once exited the node (#6823); that fix made the timeout recoverable, but the limit stayed five seconds for every method, and a caller that gives up only sleeps 10 ms and asks again, adding one more request to the same backed-up mailbox. spawned v0.7 sets timeouts per protocol method. The peer table now uses: - `PEER_SELECTION_TIMEOUT` (15 s) on `get_best_peer`, `get_best_peer_excluding` and `get_best_n_peers`: the sync download tasks' peer picks, which wait behind discovery and connection traffic. A slot reserved for a caller that already gave up is released when the reply is dropped (`RequestPermit`), so waiting longer leaks nothing. - `PEER_LISTING_TIMEOUT` (10 s) on `get_peers_data` and `get_peer_diagnostics`: RPC admin and diagnostics listings that copy every peer. - spawned's default five seconds for everything else. The shutdown path's peer snapshot already sets its own two seconds on the call. `ActorError` from the peer table is now classified in one place, `peer_table_error_is_recoverable`, with an exhaustive match, and used by every path a peer-table error takes into the sync classifier: `SyncError` directly, through `PeerHandlerError`, and through `SnapError::PeerTable`, which the `SyncError::Snap(_) => false` catch-all used to make fatal whatever its cause. Today only sends reach that variant (so only `ActorStopped`), but a timeout there would have exited the node. Tests use spawned-test mocks behind the protocol reference with paused time: a pick or listing that replies after 6 s succeeds, another request times out at 5 s, and each classification path retries on a timeout and exits on a stopped table only while the node runs.
Put discovery under a OneForOne supervisor in the node's actor system. A panic in a discovery handler used to stop discovery for the rest of the run: the node kept the peers it had but found no new ones. Discovery is the one long-lived p2p actor that restarts safely. Nothing holds a reference to it (it is fed by its own UDP listener and timers), and its state is soft: sessions, pending pings and lookups, IP votes. A fresh instance rebuilds them from the peer table and the bootnodes, as at startup. Every generation shares the UDP socket bound at startup; a stopped generation's listener stops with it, so two never split the packets. A replacement starts from the identity its predecessor published (its ENR with a new IP or fork id) rather than the startup record. Budget: Transient (a deliberate stop stays stopped), at most 3 restarts a minute, backoff 1 s to 10 s. Discovery that keeps crashing exhausts it, the supervisor gives up with an error, and the node runs on without discovery, as it did before. Restarts and the give-up are logged by a watcher, since spawned logs neither. The shutdown report names the current discovery generation. The other actors stay unsupervised, with the reasons in `init_l1`: the peer table holds the node's unpersisted view of the network (scores, sessions, connection registrations, in-flight counts) and everything holds an `ActorRef` to it; the tx broadcaster and RLPx initiator are referenced through `P2PContext` and `PeerHandler`, which a replacement would not rebind; the subscription manager owns the WebSocket subscribers. Supervising them needs reference rebinding first. For canary nodes only, `ETHREX_FAULT_INJECTION=1` makes SIGUSR1 crash the current discovery actor with a real handler panic, to exercise recovery.
…shot #7491 was a shutdown-order bug: the peer table stopped (on its own ctrl-c listener) before the node's token was cancelled, so a sync cycle took the `ActorStopped` it got as fatal and exited with code 2. The new tests watch the peer table stop and classify that `ActorStopped` with the node's shutdown state at that instant, as `Syncer::start_sync` does: - `shutdown_l1` must leave it `StopForShutdown`, and must save the peers it snapshotted while the table still ran. - A negative control runs the old order (actors stopped, then the token cancelled) and gets `Exit`. Swapping the two steps inside `shutdown_l1` fails the first test with `left: Exit`. The peer snapshot now takes the peer table as a protocol reference (`PeerTableServerRef`) with the timeout applied by the caller (`peer_snapshot_source`), so tests stand a spawned-test mock in for a table with live connections, which a real table only gets from completed RLPx handshakes. They cover the connected peers being saved, and a slow or stopped table keeping the previously saved peers instead of an empty list. Most other ethrex tests that spawn a real peer table with a dummy P2P context (`dummy_peer_handler`, the tx broadcaster and connection tests) keep doing so: `PeerHandler` and `P2PContext` hold the concrete `ActorRef<PeerTableServer>`, which a mock cannot stand in for without changing those types.
Enable spawned-concurrency's `metrics` feature with ethrex's `metrics` feature, and append its Prometheus text to the `/metrics` response (new `actors` feature of ethrex-metrics): actors spawned, active and exited by reason; messages enqueued, handled and discarded; mailbox depth; request outcomes (replied, timed out, actor stopped, cancelled); and queue-wait, handler-duration and request-latency histograms. All series are prefixed `spawned_` and summed over every actor in the process, with no per-actor labels. They are what to watch on a canary of the v0.7 runtime: a supervised discovery restart shows as a panic exit and a new spawn, peer-table pressure as queue wait and mailbox depth, and peer-selection timeouts in `spawned_requests_timed_out_total`. With `--metrics`, timings are sampled 1 in 16 per message kind (`ACTOR_TIMING_SAMPLE_EVERY`): the peer table handles every discovery packet and connection update, and timing all of them reads the clock twice per message on the busiest actor. Without `--metrics` only the counters are kept. Counters count every message either way. The sampling is set before the first actor starts, as spawned requires. Checked on a hoodi node: after 25 s, `/metrics` reported 215 actors spawned (66 active, mostly RLPx connections), 1497 messages handled and 94 timing samples.
`eth::call_bundle::tests::malformed_raw_transactions` overflowed the 2 MiB test-thread stack in debug builds after the spawned upgrade (macOS aarch64). It was already at the edge on origin/main: it needs between 1.95 MB and 2 MiB there, and 2.1-2.3 MB after the upgrade, whose request futures are somewhat larger (`map_http_requests`' future grows from 33,728 to 35,168 bytes). The test helper now awaits the dispatch future boxed, so its ~35 KB state lives on the heap rather than in every debug poll frame. The whole module then passes with 1.8 MB of stack. Test-only; RPC dispatch itself is unchanged.
Add a Testing & Verification section to ROADMAP.md with proposed priorities, and docs/developers/l1/testing/roadmap.md covering current suites, gaps from recent incidents, techniques used by database teams and a phased plan with acceptance criteria.
…and fairness testing
…tion blockers and more incidents Lead with per-incident failpoint tests that need no spawned work; make the spawned v0.7 upgrade an explicit prerequisite; record what blocks simulation (clock, RNG, sockets, process::exit) with file:line; split subsystem from whole-node simulation; add five more incidents; refine reference-model, metamorphic, Kani and TLA+ items; note what is shared with ethlambda.
|
Lines of code reportTotal lines added: Detailed view |
Hoodi canary results (branch 044057c vs main 1dfaa09)Release builds on a 256 GB Linux server. Lighthouse v8.3.3 with checkpoint sync. Snap sync from an empty datadir took about 25 min, with peak RSS 17.5 GB. Main then ran on the same synced datadir for comparison. No regressions found. All shutdowns were clean, discovery supervision behaved as designed, and the branch used about the same resources as main. Shutdown
Fault injection (
|
| branch | main | |
|---|---|---|
| CPU (median) | 5–8% | 3–8% |
| RSS | 1.4–1.5 GB | 1.3–1.7 GB |
| threads | 145 | 112 (the branch's separate p2p execution group runtime adds about 33) |
| open fds | about 460 | about 450 |
| lag behind CL head | 0 blocks | 0 blocks |
Peers: lower peer counts looked like a regression at first. Two A/B runs started together, with the ports swapped between them, showed the gap follows the port and start order, not the binary.
Actor metrics (branch)
- During sync: 108M messages at about 73k/s, mean queue wait 2 µs, max mailbox depth 2.
- Steady state: about 360 msg/s, mean queue wait 56 µs, max 247 ms.
- Throughout: 0 timed-out requests and 0
ActorStopped. The only panics were the 7 injected ones. - Correction to the description:
/metricsexposes 29spawned_*families (23 scalars, 3 histograms, 3_maxgauges), not 23 series. I've updated the description.
ethrex issues found along the way (not caused by this PR)
- Node cannot restart after snap sync is interrupted: "Unknown state found in DB" #7521: a node stopped cleanly during snap sync cannot restart (
Unknown state found in DB). Reproduced on main. - removedb silently no-ops and reports success when run without the node's --network #6925 (existing, reproduction added):
removedbwithout--networkdeletes the wrong directory and still reports success.
Next
- Repin to spawned v0.7.1 once it's released, then rerun this canary.
- Update the comment in
discovery/supervised.rs: spawned now logs the give-up itself.
A soak of the branch is still running on Hoodi.
|
Bumped spawned to 0.7.1 from crates.io (1146541), dropping the git pin. Also updated the stale comment in Gates on Linux x86_64 (fede), toolchain 1.93.0:
The Minimum
On macOS the same group needed ~2035 KB and passed, which is why earlier gates were green. The v0.7 upgrade costs ~136 KiB more stack in debug than main. We're investigating on the spawned side (future sizes on the request/protocol path) so the fix lands in spawned rather than in individual tests. |
… rpc stack overflow)
|
Bumped spawned to 0.7.2 (crates.io). 0.7.2 makes |
|
Bumped spawned to 0.7.3 (crates.io). One supervision behaviour change, aligned with OTP: a failed restart (a replacement that stops before readiness) is retried within the restart budget instead of failing the supervisor, and OneForAll rebuilds the whole cohort; see migration notes. If a supervised child here relied on a failing replacement to stop its tree, it should call |
Why
ActorStoppedduring that window calledexit(2), which failed 6 of 25 CI runs (test(l1): stop sync_race_tests from exiting the ethrex_tests binary at teardown #7491).get_best_peertiming out under mailbox pressure exited the node (fix(l1): classify actor request timeouts as recoverable in sync error handling #6823); every peer-table request used the hardwired 5 s.What changes
chore(l1,l2): upgrade spawned to v0.7refactor(l1): own L1 actors in an ActorSystem with one shutdown pathActorSystem(p2pgroup: peer table, tx broadcaster, RLPx initiator, subscription manager, discovery; peer table stops last). SIGINT, SIGTERM and fatal subsystem errors all go throughshutdown_l1with an 8 s deadline: cancel, snapshot peers (2 s), flush DB, savenode_config.json, stop actors. A failed snapshot keeps the previously saved peers. The 1 s sleep is gone. A sync hitting a stopped actor during shutdown stops the cycle instead ofexit(2). Missing the deadline names unfinished actors and exits 1.fix(l1): give slow peer-table requests their own timeoutspeer_table_error_is_recoverable).feat(l1): supervise the discovery actorinit_l1). Optional fault injection:ETHREX_FAULT_INJECTION=1+ SIGUSR1 crashes discovery.test(l1): pin the shutdown order against #7491 and mock the peer snapshotspawned-testmocks with paused time; reintroducing the old shutdown order fails the test every run.feat(l1): export spawned actor metrics on /metricsspawned_*metric families: 23 scalars, 3 histograms and 3_maxgauges.test(l1): box the RPC dispatch future in the eth_callBundle testschore(l1,l2): pin spawned to main b1a17e6c plus spawned-testspawned-test(lambdaclass/spawned#198).docs(l1): …testing and verification roadmap(3 commits)docs/developers/l1/testing/roadmap.mdand aTesting & Verificationsection inROADMAP.md: testing ethrex the way FoundationDB, TigerBeetle, Dropbox, etcd and AWS test their systems, tied to real incidents. Priorities are proposals.Measured so far (local, debug builds)
--dev, SIGINT ×5Fault injection on hoodi: two injected discovery crashes gave generations 2 and 3, then a 23 ms shutdown. A canary on a synced node with a consensus client is next; results will be posted here.
Follow-ups
dummy_peer_handler-style test setups can't use mocks yet becausePeerTableis a concreteActorRef<PeerTableServer>; spawned tracks concrete-ref mocking.Background: spawned roadmap, database-grade testing program.