Skip to content

feat: count failed sends and rejected or dropped inbound frames - #165

Open
jacderida wants to merge 1 commit into
WithAutonomi:mainfrom
jacderida:chrisoneil/v2-834-traffic-accounting-final-ground-truth-instrumentation
Open

feat: count failed sends and rejected or dropped inbound frames#165
jacderida wants to merge 1 commit into
WithAutonomi:mainfrom
jacderida:chrisoneil/v2-834-traffic-accounting-final-ground-truth-instrumentation

Conversation

@jacderida

@jacderida jacderida commented Sep 16, 2026

Copy link
Copy Markdown
Member

Summary

V2-834 Part C.5.

The V2-623 wire traffic summary counted only confirmed sends and
successfully decoded receives, so neither side summed to what the
transport actually moved. Three failure branches are now itemised on
TrafficCounters:

  • wire_failed_tx_*: send_on_channel send errors and identity-announce
    send errors (both sub-branches, including the PeerNotFound early
    return).
  • wire_rejected_rx_*: frames that reached a shard consumer but failed
    postcard decode or ML-DSA verification (both collapse to None in
    parse_protocol_message).
  • wire_dropped_rx_*: frames the dispatcher dropped before parsing
    because the shard channel was full or its consumer had exited —
    previously count-only via drop_counter, byte length now recovered
    from the TrySendError payload.

wire_rx_bytes + wire_rejected_rx_bytes + wire_dropped_rx_bytes is every
frame the transport handed to saorsa-core. All six fields are added to
the periodic INFO line (19 fields, under the 32-field cap).

Linear issue

Closes V2-834

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.

Counters and one extended INFO line; no behaviour change.

Compatibility

  • Wire: none
  • Storage: none
  • API: none (TrafficCounters is pub(crate); only the wire traffic summary INFO line gains six fields)

Semver impact

  • breaking
  • feature
  • fix

Test evidence

  • cargo clippy --lib --bins -- -D warnings: clean (the test-target network.rs:2997 empty-line-after-doc lint is pre-existing on main)
  • cargo fmt --check: clean
  • Emitted line is 19 fields (32-field cap)

New dependency

none

ADR

n/a

Mitigation / rollback

Revert the PR. Purely additive counters and INFO summary lines at the existing 300s cadence; no config, no wire, no storage change — nothing to migrate.

🤖 Generated with Claude Code

@jacderida @claude
feat: count failed sends and rejected or dropped inbound frames
62d5879 
V2-834 Part C.5.

The V2-623 `wire traffic summary` counted only confirmed sends and
successfully decoded receives, so neither side summed to what the
transport actually moved. Three failure branches are now itemised on
`TrafficCounters`:

- `wire_failed_tx_*`: `send_on_channel` send errors and identity-announce
  send errors (both sub-branches, including the PeerNotFound early
  return).
- `wire_rejected_rx_*`: frames that reached a shard consumer but failed
  postcard decode or ML-DSA verification (both collapse to `None` in
  `parse_protocol_message`).
- `wire_dropped_rx_*`: frames the dispatcher dropped before parsing
  because the shard channel was full or its consumer had exited —
  previously count-only via `drop_counter`, byte length now recovered
  from the `TrySendError` payload.

`wire_rx_bytes + wire_rejected_rx_bytes + wire_dropped_rx_bytes` is every
frame the transport handed to saorsa-core. All six fields are added to
the periodic INFO line (19 fields, under the 32-field cap).

Test evidence: `cargo clippy --lib --bins -- -D warnings` clean, `cargo fmt --check` clean. The
test-target clippy error at `network.rs:2997` is pre-existing on main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dirvine
dirvine approved these changes Sep 16, 2026
View reviewed changes

@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.

Approving. Small, clean, observability-only: 6 counters partition the failure branches so tx/rx sum by construction (failed_tx on send failures, rejected_rx on decode/ML-DSA verify failure, dropped_rx on dispatcher Full/Closed). The partition wire_rx + rejected + dropped = every frame handed to saorsa-core is sound. Emits on the existing periodic wire-traffic line, under the 32-field cap. No concerns. Note: Security Audit CI failure is pre-existing baseline (unmaintained/yanked transitive advisories), not introduced here.

mickvandijke added a commit to mickvandijke/saorsa-core that referenced this pull request Sep 21, 2026
@mickvandijke @claude
docs(adr): record replication repair hardening decisions (PR WithAuto…
3a7bdba 
…nomi#165)

Add ADR-0005 capturing the design decisions behind the replication
repair hardening work: responsibility decided at download against live
routing state, fresh offers off the serial loop, detached async/LMDB
lifecycle tracking, terminal closed streams, eager neighbor-sync drain,
TTL'd bootstrap accounting, source-aware bounded verification, retry
reservation, O(1) hint merge, and shared audit coordination.

Status: Proposed. Passes scripts/adr-governance.py.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mickvandijke added a commit to mickvandijke/saorsa-core that referenced this pull request Sep 21, 2026
@mickvandijke @claude
docs(adr): correct ADR-0005 compatibility claims
9734808 
ADR-0005 asserted "SemVer: patch. No wire-format or public-API change".
The wire-format half is true and verified: REPLICATION_PROTOCOL_ID stays
at v2 and no ReplicationMessageBody variant or field changes (the only
protocol.rs edit in the branch widens a private const to pub(crate)).

The public-API half is false. src/lib.rs exposes `pub mod replication`,
and the branch breaks it: audit_tick and run_prune_pass removed,
AuditTickResult::Failed and PrunePassContext and VerificationTargets
gained required fields, VerificationEntry and FetchCandidate
restructured (FetchCandidate also losing its Ord/PartialOrd/Eq impls to
the new FetchOrder), MAX_PENDING_VERIFY_PER_PEER and
MAX_PRUNE_AUDIT_CHALLENGES_PER_PASS removed, plus
pending_count_for_sender and evict_stale changes.

Replace the claim with a Compatibility bullet that states both halves
accurately and enumerates every break against the decision that forced
it, and record the absence of compatibility shims as a decision: the
changed items are the data structures this ADR restructures, so a
parallel deprecated surface would pin the old representation in place.

Carries no versioning or release semantics — the required version
position is the release manager's call, not the ADR's. This also drops
the same stale "patch-level" claim from the Constraints section, which
stated it a second time.

Raised as blocking issue 3 in review of PR WithAutonomi#165.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mickvandijke added a commit to mickvandijke/saorsa-core that referenced this pull request Sep 21, 2026
@mickvandijke @claude
docs(adr): record why the possession-check backlog stays unbounded
fd4a97e 
Review of PR WithAutonomi#165 asked for the possession-check scheduler backlog to be
bounded or coalesced. Bounding it is the wrong move and the ADR now says
why, so the decision survives the next reader who notices the unbounded
channel.

The queue is fed from exactly one place: a FreshWriteEvent this node
accepted and fanned out. Every entry is therefore a chunk this node took
a client PUT for and was paid for — nobody inflates it without paying per
chunk and having this node accept it. That is categorically unlike the
inbound serial queue of decision 12, where a stranger's bytes size the
queue, which is why a bound is right there and wrong here. A cap would
also discard what the queue is for: a dropped possession check is a peer
that failed to store going unpunished.

Memory does not motivate one either — ~224 bytes plus task overhead per
event, steady state being arrival rate times the ~10 minute settle
window, so ~6k parked tasks at a sustained 40 MB/s of accepted ingest.

What is genuinely unbounded is lateness. A single-key probe against a
per-target limit of 2 yields ~40 probes/s from a healthy peer but ~0.45/s
from one burning the full 4.4s deadline, so a backlog forms only when
peers are already timing out — and a late verdict is a wrong verdict,
since neighbor sync may have delivered the chunk by then. A count cap
does not address that; it discards a different arbitrary subset.

Records the cancellability fix that did matter, the two rejected
alternatives (capping the queue; closing the coordinator semaphore, which
would report MalformedResponse against innocent peers on every shutdown),
and coalescing per target as the deferred real fix — AuditChallenge.keys
is already a Vec answered by per-key digests, and
audit_response_timeout(key_count) already gives the batch a principled
ceiling. Re-open trigger is probe lateness, not queue depth.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mickvandijke added a commit to mickvandijke/saorsa-core that referenced this pull request Sep 21, 2026
@mickvandijke @claude
fix(replication): make the fresh-offer proof cap a lifetime budget
4963130 
MAX_FRESH_OFFER_ATTEMPTS_PER_KEY gated on `pending.len()`, the queue's
instantaneous depth. The handler pops a proof before verifying it, so every
pop returned a slot that a fresh source could refill. Since the per-source
set only bars repeats, a stream of distinct peers kept one entry alive
indefinitely: unbounded sequential payment verifications — EVM and DHT
work — while holding an admission permit and one of only four fresh-offer
worker slots. Four such keys idle the whole pool. The staleness shed is no
backstop; it runs once before the loop, not inside it.

Count admissions instead, and never decrement. A popped proof has spent
its slot rather than returned it, so a key costs at most
CLOSE_GROUP_MAJORITY verifications no matter how many peers offer it. This
also bounds the entry's source set, which previously grew one PeerId per
sybil for the life of the entry.

Regression test drives pop-then-admit with a fresh source each time and
asserts the lifetime count holds; it accepted 16 proofs against a budget
of 4 before the fix. Reported by AI review of PR WithAutonomi#165, independently
reproduced here before fixing.

Verified with the commands CI runs, including the no-default-features
build and test that the previous commit tripped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mickvandijke added a commit to mickvandijke/saorsa-core that referenced this pull request Sep 21, 2026
@mickvandijke
Merge pull request WithAutonomi#165 from WithAutonomi/fix/neighbor-sy…
796734c 
…nc-drain-window

fix(replication): drain priority sync queue and recover from routing-event lag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dirvine dirvine dirvine approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jacderida @dirvine