From 9f8b266e5dd913e6d7ae9cb65066f6ae32e2d58b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Thu, 1 Oct 2026 22:40:40 +0000 Subject: [PATCH] Describe the code in comments as it is, not as plan steps or missing commits The decrypt worker's module doc and its fallback-path comment framed the code as a step in an architectural plan this repository does not carry, and named a "DataShard end-state" that exists nowhere in the tree. State the property directly instead: each worker owns its session state, and a worker that also owned the rx_loop side could restore the EndpointData fast path. The reworded comments are rewrapped to the surrounding width. The metrics registry docs described off-thread reads as a later step, but the control read handle already serves show_metrics from the registry off the rx_loop task. Put both the module doc and the Padded doc in the present tense, with the single writer the code has. Three source comments cited commits by hash (an abandoned republish design, the change that made sessions shard-owned, and the fix that moved encrypt dispatch off round-robin). None of those hashes resolves in this repository, so a reader cannot follow them. Say what the commit did instead. The one hash cited in the routing tests is on every branch and stays. The bloom-storm scenario README pointed readers at a reproduction harness and results file outside this repository, told them to check out a commit and a branch that are not here, and named the regressing commit, which is not here either. Describe the harness and the regressed build without those references and anchor the confirmation procedure on the fix commit, which every branch contains. The off-loop snapshot dispatch test's name carried a stage label from an outside plan. Rename it to say what it checks: show_acl, show_stats_peers and show_stats_history_all_peers are served off-loop through snapshot_dispatch and render byte-identically to their on-loop forms. Nothing refers to the old name. --- src/control/queries.rs | 2 +- src/node/decrypt_worker.rs | 23 ++++++++----------- src/node/encrypt_worker.rs | 2 +- src/node/metrics.rs | 17 +++++++------- src/node/mod.rs | 3 ++- testing/chaos/scenarios/bloom-storm.README.md | 20 ++++++++-------- 6 files changed, 32 insertions(+), 35 deletions(-) diff --git a/src/control/queries.rs b/src/control/queries.rs index c1e03b73..53420106 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -3150,7 +3150,7 @@ mod tests { /// renders each equal their on-loop oracle byte-for-byte, and all three are /// served off-loop via `snapshot_dispatch`. #[test] - fn snapshot_dispatch_serves_r5_queries() { + fn snapshot_dispatch_serves_acl_and_stats_peer_queries_off_loop_byte_identical() { use super::super::protocol::Request; use super::super::read_handle::snapshot_dispatch; diff --git a/src/node/decrypt_worker.rs b/src/node/decrypt_worker.rs index a5d3e7aa..d4d5173c 100644 --- a/src/node/decrypt_worker.rs +++ b/src/node/decrypt_worker.rs @@ -1,11 +1,10 @@ //! Off-task FMP + FSP decrypt + delivery worker. //! -//! First incremental step of the data-plane shard restructure (per the -//! architectural plan): each worker now **owns its session state -//! directly** in a local `HashMap`, with no `Arc>` -//! cache on the Node side and no `Arc>` shared -//! with the rx_loop. The worker is the sole authority over the replay -//! window and the recv-side ciphers for every session it owns. +//! Each worker **owns its session state directly** in a local +//! `HashMap`, with no `Arc>` cache on the Node side and +//! no `Arc>` shared with the rx_loop. The worker is +//! the sole authority over the replay window and the recv-side ciphers +//! for every session it owns. //! //! Dispatch is **deterministic by session key**: rx_loop computes //! `worker_idx = hash(cache_key) % N` and routes both @@ -484,8 +483,8 @@ fn handle_job( // **decrypted-in-place** FMP plaintext back to rx_loop. // // Two problems with that path: - // 1. After the shard-owned-sessions refactor (01f6c62), the FSP - // replay window is owned by **this worker thread**. Once we + // 1. Since sessions became shard-owned, the FSP replay + // window is owned by **this worker thread**. Once we // `state.fsp_replay.accept(fsp_counter)`, the rx_loop's // `noise::Session::replay_window` is stale — it still has // old counters. When rx_loop tries to FSP-decrypt the @@ -511,11 +510,9 @@ fn handle_job( // still offloads the FMP AEAD (~half the per-packet decrypt // CPU). Correctness over micro-optimisation. // - // The DataShard end-state (per the architectural plan) re- - // introduces the EndpointData fast path correctly by having the - // shard worker also own the rx_loop side for its sessions — at - // that point there's no "rx_loop legacy path" for the worker to - // conflict with. + // A worker that also owned the rx_loop side of its sessions could + // restore the EndpointData fast path correctly: there would then + // be no "rx_loop legacy path" for the worker to conflict with. // Pass the buffer through by ownership + offset/length. No // per-packet allocation; rx_loop slices into `packet_data`. let _ = link_msg; // sanity-check borrow before sending buffer onward diff --git a/src/node/encrypt_worker.rs b/src/node/encrypt_worker.rs index 198d7fae..8f5e462d 100644 --- a/src/node/encrypt_worker.rs +++ b/src/node/encrypt_worker.rs @@ -423,7 +423,7 @@ type WorkerSender = Sender; /// /// **Ordering: hash-by-destination** so single-flow TCP keeps its /// FIFO ordering (round-robin caused 8000 retransmits in an earlier -/// experiment — see the git log for the 56e0ca8 fix). Multi-peer / +/// experiment, which is why dispatch hashes by destination). Multi-peer / /// multi-flow benches still get parallelism since different /// destinations hash to different workers. #[derive(Clone)] diff --git a/src/node/metrics.rs b/src/node/metrics.rs index 984c19c8..2ea8039e 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -1,10 +1,10 @@ //! Lock-free metric counters backed by atomics. //! //! Mirrors the `NodeStats` counter surface (`stats.rs`) but stores each -//! counter in an `AtomicU64`, so it can be bumped through `&self` and, in -//! a later step, sampled without dispatching through the rx_loop task. The -//! hottest counters are cache-line padded to avoid false sharing once -//! reads move off-thread. +//! counter in an `AtomicU64`, so it can be bumped through `&self` and read +//! off the rx_loop task (the control read handle serves `show_metrics` from +//! it directly). The hottest counters are cache-line padded to avoid false +//! sharing between the writer and off-thread readers. //! //! The forwarding, discovery, tree, bloom, congestion, and error families //! live here exclusively and are both written and served from the registry. @@ -43,11 +43,10 @@ impl Counter { /// Cache-line padding wrapper for the hottest counters. /// -/// Padding keeps a hot counter off shared cache lines so that concurrent -/// reads (introduced when metric sampling moves off the rx_loop task) do -/// not false-share with the writer. With a single writer today the padding -/// is forward-looking insurance. Derefs to the inner counter so the call -/// sites are identical to an unpadded one. +/// Padding keeps a hot counter off shared cache lines so that reads from +/// off the rx_loop task (the control read handle) do not false-share with +/// the single writer. Derefs to the inner counter so the call sites are +/// identical to an unpadded one. #[repr(align(64))] #[derive(Default)] pub struct Padded(pub T); diff --git a/src/node/mod.rs b/src/node/mod.rs index ee21d2a4..571c035f 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1766,7 +1766,8 @@ impl Node { // advance together. What is published is data, not a rendered response, // and it is published only here, rather than as a monolithic per-tick // rebuild of every query's result. It also is not gated behind any slow - // I/O on the tick the way the abandoned 2edc8a1 republish was. + // I/O on the tick, which was the shape of an earlier, abandoned + // republish design. // Per-stats-history-peer metadata. `show_stats_peers` / // `show_stats_history_all_peers` need each tracked peer's live // membership (`is_active`), resolved npub, and display name — all diff --git a/testing/chaos/scenarios/bloom-storm.README.md b/testing/chaos/scenarios/bloom-storm.README.md index 146ab79d..f02cb118 100644 --- a/testing/chaos/scenarios/bloom-storm.README.md +++ b/testing/chaos/scenarios/bloom-storm.README.md @@ -30,7 +30,7 @@ actual assignment. A spanning-tree update that changes only an internal path edge — no root change, no depth change — must not produce a sustained bloom announce storm at downstream nodes. The original regression -(rolled-back `0caef2a`, fixed in master `4cdf382`) had this property: +(since rolled back, and fixed in master `4cdf382`) had this property: in the field, a single mid-chain ancestor swap on an upstream node caused every downstream node in its subtree to issue a bloom announce on every parent re-evaluation tick of the upstream node, @@ -74,7 +74,7 @@ including a regressed one. ## Threshold derivation -The original `issues/2026-0019-repro/` reproduction harness measured +The original reproduction harness, kept outside this repository, measured (90s flap window, ~21 induced parent switches at the mid-chain node): @@ -93,8 +93,8 @@ n01=5 n02=5 n03=4 n04=12 n05=6 n06=0 n04 (the flapping node) is the highest because it is legitimately re-sending its filter on its own parent changes. n06 (the depth-4 -"tail") sees 0, matching the calm post-fix behavior recorded in -`issues/2026-0019-repro/RESULTS.md` for the `fix2` variant. +"tail") sees 0, matching the calm post-fix behavior that harness recorded +for its `fix2` variant. In the field, the regression's mesh-wide rate scaled ~480x above steady state. A `30 / 30s / node` ceiling sits ~2.5x above the @@ -132,7 +132,7 @@ applied to every chaos-spawned container. Rationale for ceiling = 40: lab max 30 + ~2σ headroom (≈ 39.4) rounds to 40, giving 33 % margin over the observed lab maximum while still firing -loud on a regression-class storm (the original `0caef2a` regression +loud on a regression-class storm (the original regression scaled mesh-wide bloom traffic ~480× above steady state, far above any plausible jitter band). @@ -140,11 +140,11 @@ plausible jitter band). - The bloom-storm regression has not been confirmed-failing here on a regressed binary in this harness directly; the threshold is - inferred from the values measured in the dedicated - `issues/2026-0019-repro/` post-mortem harness against - `0caef2a`. To gain that confirmation, check out `0caef2a` - (or the `backup-broadcast-gate-bloom-storm` branch if still - retained), build, copy binaries into `testing/docker/`, and rerun + inferred from the values measured in the dedicated post-mortem + harness against a regressed build whose commit is not in this + repository. To gain that confirmation, build a binary that + carries the regression and predates the `4cdf382` fix, copy + binaries into `testing/docker/`, and rerun this scenario; the bloom-rate assertion is expected to fail loud with n05/n06 deltas well above 40.