Skip to content

fix(node): keep protocol routing running when its event receiver lags - #285

Open
mickvandijke wants to merge 1 commit into
mainfrom
mickvandijke/protocol-loop-survive-lag
Open

mickvandijke wants to merge 1 commit into
mainfrom
mickvandijke/protocol-loop-survive-lag

Conversation

@mickvandijke

@mickvandijke mickvandijke commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Linear issue

Closes V2-1448

Protocol routing no longer stops for good when it falls behind the P2P event stream. This is Darwin disclosure issue #4.

The bug

  • The loop: RunningNode::start_protocol_routing read events with while let Ok(event) = events.recv().await (src/node.rs).
  • The channel: events is a tokio broadcast receiver from P2PNode::subscribe_events(). saorsa-core 0.30.0 fixes its capacity at DEFAULT_EVENT_CHANNEL_CAPACITY = 1000 (lib.rs:139, passed in at network.rs:1105). Every inbound app message on every topic goes through it, along with every peer connect and disconnect.
  • The failure: if routing falls more than 1000 events behind, recv() returns RecvError::Lagged(n) and the while let ends. The node stays in its peers' routing tables and keeps replicating, which uses a separate receiver. But it answers no chunk GET, PUT or quote until restart, and nothing is logged.
  • Same pattern elsewhere: the devnet (src/devnet.rs) and e2e harness (tests/e2e/testnet.rs) routing loops.

What changes

  • ProtocolEvents (ant_node::node::ProtocolEvents) wraps the receiver for all three loops. A lag is passed over and reading resumes at the oldest event the channel kept. Only RecvError::Closed ends routing. This is the rule ADR-0005 decision 4 already applies to replication.
  • Counted: skipped events go into routing_lagged_event_count, a new field on the group = 1 line of the periodic chunk RPC traffic summary. It covers all topics, since a skipped event can't be read.
  • Warned, throttled: after a lag, tokio resumes the receiver at the slot the next send overwrites, so under sustained overload it lags again almost at once (tokio-1.53.3/src/sync/broadcast.rs, recv_ref). A warning per lag would log at close to the event rate. So the warning fires at most once every 10 s and carries the total missed since the last one. It names the node: its peer ID on a real node, node N in the devnet and e2e harness.
  • Nothing to resync: routing keeps no state, so no recovery is needed after a lag. Clients whose requests were dropped time out and retry elsewhere, as they do today.

Not in this PR

  • The drops themselves: requests in the skipped window are still lost, because routing shares saorsa-core's single 1000-slot channel with all other traffic. The structural fix is a per-protocol channel or a configurable capacity in saorsa-core.
  • Replication: handle_replication_event_recv_error (src/replication/mod.rs) warns on every lag too, so it has the same log-flood behaviour. It could reuse the throttle in a follow-up.
  • Bootstrap gate: src/replication/bootstrap.rs ends its 60 s wait for BootstrapComplete early on a lag. It logs a warning and continues, and neighbour sync catches up later, so it's low severity.

Risk tier

  • T0 — docs / tooling / CI / pure UX-output. Repo CI only.
  • T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
  • T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
  • T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

Proposed T2: node behaviour changes, but only after a lag, where routing now keeps running instead of stopping. The healthy path, the wire format and storage are unchanged.

Compatibility

  • Wire: none
  • Storage: none
  • API: adds ant_node::node::ProtocolEvents (new, recv). Nothing existing changes. The chunk RPC traffic summary's group = 1 line gains a routing_lagged_event_count field.

Semver impact

  • breaking
  • feature
  • fix

feature because of the new public type. Behaviourally this is a fix.

Test evidence

Local, macOS, rustc 1.96.1, at 8a61dcd:

  • Regression test node::tests::protocol_routing_survives_a_lagged_event_receiver runs against a real node:
    • It starts routing on a single-threaded runtime, then publishes 2,000 events with no peers. publish doesn't suspend, so routing can't run during the flood.
    • A receiver the test subscribed after routing must report Lagged, which proves the channel overflowed.
    • Routing must still be running afterwards.
    • A pre-paid chunk PUT sent after the lag must be stored in that node's own store. This proves it is still handled, without reading counters that other tests share.
  • Unit test node::tests::protocol_events_pass_over_lags_and_end_on_close:
    • Reading resumes at the oldest kept event after each of two lags.
    • The second lag, inside the interval, is counted but not warned about.
    • Every event after the lag arrives, and the stream ends only on close.
  • Mutation checks:
    • With recv returning on Lagged, the old behaviour, the regression test fails with "protocol routing stopped after its event receiver lagged".
    • With that liveness assertion switched off as well, it fails with "a chunk PUT sent after the lag was never handled".
    • With the throttle removed, the unit test fails with "the second lag was not counted".
  • Repeat runs: both new tests passed 20 of 20 runs.
  • Test suites: cargo test --lib -- node::tests storage::traffic passes 28 tests, and 27 with --no-default-features.
  • Builds:
    • cargo check --all-targets --all-features, which includes the e2e target, is clean.
    • cargo check --lib --no-default-features and RUSTDOCFLAGS="-D warnings" cargo doc --all-features --no-deps are clean.
    • cargo fmt --all -- --check is clean.
  • Clippy: cargo clippy --all-targets --all-features -- -D warnings finds nothing in the changed files. Local 1.96 flags 6 findings in untouched files (replication/pointer.rs, storage/chunk_store.rs, devnet/run_dir.rs), the same as on unchanged origin/main.
  • CI (8a61dcd, run 42617525621): all green. Quick checks pass on CI's newer stable: fmt, clippy, docs and the no-default-features check. Tests pass on ubuntu, macOS and Windows. The MSRV, storage-filesystem, release-build, security-audit and template checks pass too.
  • Dev testnet: not run. A normal testnet run wouldn't make routing lag, so it would only show the healthy path is unchanged.

New dependency

none

ADR

ADR-0005, decision 4: "Treat closed event streams as terminal; keep lag recoverable". This PR applies that existing rule to protocol routing; no new decision.

Mitigation / rollback

Revert. The only behaviour change is that routing keeps running after a lag where it used to stop.

🤖 Generated with Claude Code

Protocol routing read the P2P event stream with
`while let Ok(event) = events.recv().await`. saorsa-core's event channel
holds 1000 events, shared by every inbound message and peer event. If
routing fell further behind than that, `recv()` returned
`RecvError::Lagged` and the loop ended. The node stayed in its peers'
routing tables but answered no chunk request until restart, and nothing
was logged.

`ProtocolEvents` now wraps the receiver for routing. On a lag it counts
the skipped events (`routing_lagged_event_count` in the chunk RPC traffic
summary) and carries on from the oldest event the channel kept. It warns
at most once every 10 s, naming the node, because under sustained
overload the receiver lags again almost at once. Only a closed stream
ends routing, as ADR-0005 decision 4 already does for replication.

The devnet and e2e harness routing loops had the same pattern and use
the same type.

Closes V2-1448

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@MaidSafe-Hermes MaidSafe-Hermes left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — reviewed commit 8a61dcd5cbaf155bd4dab556c8dce5c441a77e81.

Both seats agree (two model families): 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3).

ProtocolEvents::recv recovers from RecvError::Lagged, counts every skipped event, throttles per-reader warnings, and exits only when the stream closes. The live node, devnet and e2e routing loops all use it. The new tests cover overflow recovery, continued post-lag chunk PUT handling, warning suppression, retained events and closure. I found no blocking defect.

All current PR checks pass, including Linux, macOS and Windows tests, Rust 1.95 MSRV, storage, security audit and CI Gate. The test suite does not directly assert the exported lag metric; this is a non-blocking coverage gap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants