Skip to content

fix(storage): stop counting a peer that disconnects mid-tally as unreported - #228

Open
grumbach wants to merge 2 commits into
WithAutonomi:mainfrom
grumbach:fix/peer-count-disconnect
Open

grumbach wants to merge 2 commits into
WithAutonomi:mainfrom
grumbach:fix/peer-count-disconnect

Conversation

@grumbach

@grumbach grumbach commented Sep 15, 2026 •

Copy link
Copy Markdown
Member

Linear issue

Closes V2-1290

Split from V2-1260, whose saorsa-core half (saorsa-core #163) has merged.

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.

Changes the migration signal's output, in tally_peers only:

  • A peer that disconnected mid-tally is no longer counted, and no peer_state line is written for it.
  • A peer that disconnected and came back between the two reads is classified from its agent read again, not from the stale first read.
  • A peer that is connected but still has no agent stays in the unreported bucket, so the count never drops a peer this node can see.

No wire, format or penalty change.

Compatibility

  • Wire: none.
  • Storage: none.
  • API: none.

Semver impact

  • breaking
  • feature
  • fix

Test evidence

Rebased onto main at d9ff670. The diff is one file, src/storage/migration_signal.rs, in two commits: the original fix and the reconnect re-read. No Cargo.toml or Cargo.lock change.

  • What it fixes, from the 990-node testnet for refactor(storage)!: remove the LMDB chunk store, and never refuse a start over what it left behind #218: one tick in 21,402 had peers_unreported=1, a client/0.27.3 connection tallied with agent=none. The tally takes the peer list, then reads each agent, and saorsa-core drops the agent when the peer's last channel closes.
  • saorsa-core chore(release): promote rc-2026.6.4 #163 is in saorsa-core 0.28.0, which main already pins. It records and drops a peer's agent under the peer's connection entry, so a connected peer always has one: no agent means the peer was not connected at that read. The code does not depend on this. A connected peer with no agent still counts as unreported.
  • cargo test --lib --features test-utils migration_signal: 15 passed. Full --lib --features test-utils: 1153 passed. --lib --no-default-features: 1110 passed.
  • migration_reclaims_disk 2, migration_crash_safety 5, migration_shared_volume 5, e2e 97 passed (3 ignored, as on main), webrtc_direct_devnet 3, poc_commitment_audit_attacks 19, poc_audit_handler_live 16, poc_bootstrap_stall 3, poc_shutdown_lmdb_drain 1. All passed, none failed.
  • cargo clippy --all-targets --all-features -- -D warnings, RUSTFLAGS=-D warnings cargo check --lib --no-default-features --locked, cargo fmt --all -- --check, RUSTDOCFLAGS=-D warnings cargo doc --all-features --no-deps, scripts/adr-governance.py: clean.
  • No unit test for the race itself: it needs a peer to disconnect, or disconnect and reconnect, inside the loop, which the store harness cannot stage.

New dependency

none

ADR

https://github.com/WithAutonomi/ant-node/blob/main/docs/adr/ADR-0014-file-based-chunk-store-and-lmdb-retirement.md

Mitigation / rollback

Revert the two commits. They only change the migration signal's log output.

@dirvine dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

APPROVE — reviewed at exact head 674aa0d2eb9dcf794497a69ac6affc15accf47ff together with saorsa-core #163.

The tally fix is conservative in every relevant interleaving:

  • agent=None followed by disconnected means the peer left after the connected_peers snapshot, so skipping it removes the observed false Unreported tick.
  • If the peer reconnects between the two reads, it remains Unreported; that is a possible false outstanding count, not false clearance.
  • If an agent is read before a disconnect, the snapshot's last announced state is counted, matching the existing snapshot semantics.
  • A still-connected peer with no agent remains Unreported.

The change therefore cannot manufacture a Files result or falsely clear the migration gate. It is safe on its own and gains the stronger missing-agent meaning from saorsa-core #163.

Verification:

  • focused migration-signal suite: 15 passed
  • clippy all targets/features: passed
  • no-default-features library check: passed
  • formatting and rustdoc warnings check: passed
  • GitHub build/test/lint/filesystem/platform matrix is green

Non-blocking gap: no deterministic mid-tally disconnect/reconnect test was added. The branch logic is small and the paired transport invariant was checked directly, so I do not consider this a merge blocker.

The failing Security Audit is inherited, not introduced: Cargo.lock is byte-identical to the base and the newly published RUSTSEC-2026-0285 affects that existing rustls version. It should be handled by the release train in a separate dependency bump to rustls >=0.23.45.

Operationally, this is consistent with — and closes — the reported false-positive path in the #218 testnet. I have not independently reclassified the whole run from its Linear evidence here. The peer tally remains observational evidence rather than proof of whole-fleet completion, so the rest of the ADR-0015 gate still applies.

…ported

`tally_peers` lists the connected peers and then reads each one's user agent.
saorsa-core drops a peer's agent when its last channel closes, so a peer that
disconnects between the two reads has no agent, and the tally counted it as a
node running a build from before the migration signal. On a 990-node testnet
this put a departing client in `peers_unreported` on one tick in 21,402, which
is exactly the count the release gate for removing the LMDB store reads.

A peer with no agent that is no longer connected is now skipped: it is not a
peer this node can see. A peer that is still connected with no agent recorded
keeps its place in the unreported bucket, so the count stays conservative with
or without the saorsa-core change that records the agent under the peer's
connection entry.
saorsa-core records a peer's user agent and removes it under the same lock
as the peer's connection entry, so a connected peer always has an agent.
When `tally_peers` reads no agent and then finds the peer connected, the
peer disconnected and came back between the two reads. The tally counted it
as unreported from the first, stale read, even when the peer had announced
a finished store or was a client. That is the same false `unreported`
reading the previous commit removes for a departing peer.

The agent is now read a second time in that case and the peer is classified
from what it announced. If the second read still finds no agent, the peer
stays in the unreported bucket, so a connected peer is never dropped from
the count.
@grumbach
grumbach force-pushed the fix/peer-count-disconnect branch from 674aa0d to 019ec6a Compare September 24, 2026 08:17
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