feat(replication): stop penalising a peer for not holding a close-group chunk - #215
feat(replication): stop penalising a peer for not holding a close-group chunk#215grumbach wants to merge 3 commits into
Conversation
…up chunk The chunk store can only grow. LMDB returns a deleted page to its own free list and never to the filesystem, so a node that deletes chunks frees no disk. Moving the fleet onto a store that does return space means a node short of disk will have to give up some chunks while it moves the rest across. It cannot avoid being seen doing that, and it cannot stop the consequence, because the penalty is the auditor's decision, not the audited node's. So the auditors stop one release ahead of the migration, and this is that release. What is withheld is deliberately narrow: only the accusation "you did not have a chunk you were supposed to be holding". That covers the responsible-chunk audit, the fresh-replication possession check, the prune audit, a sole-source replica hint whose sender then denies possession, and the fetch paths where a peer that answered Present could not serve the bytes. A node giving up chunks produces every one of those, so withholding some and not others would stop only some of its accusers. The commitment-bound subtree audit is untouched and still penalises. That is not a compromise, it is what makes the rest work: a migrating node reduces its signed commitment precisely so its peers hold it to the smaller claim, and suspending that enforcement would make the reduction meaningless. A sole-source hint the close group rejects outright is also still punished, because that is a claim about a key that does not exist rather than about the sender's own storage. Audits of both kinds keep running and keep recording. Only the trust event is withheld, and the record they leave is how we will know when it is safe to switch the penalty back on, which is a later release rather than a compiled-in expiry so the date can move on evidence. The switch is a build constant, not a configuration field: a node writes its effective configuration back to disk, so shipping it as a setting would bake this release's value into every operator's file and the next release would change nothing. It is initialised from that constant rather than defaulting to "penalise", so a construction path that never applies the policy behaves like this release instead of the previous one. Known cost, accepted: between this release and the one that restores the penalty, a peer that publishes no commitment at all can answer Present, fail to serve, and pay nothing for it. It is bounded by the restore and visible in the audit record. See ADR-0012.
…ging feature `AuditType::as_str` was gated on the `logging` feature because every caller was inside a log macro, which compiles to nothing when that feature is off. The penalty helper takes the label as an ordinary argument, and arguments are evaluated whether or not the macro that consumes them survives, so a `--no-default-features` build stopped compiling. Ungated rather than worked around at the call sites: it is a `const fn` over a three-variant enum returning a string literal, so it costs nothing in a build that never logs, and passing hand-written literals instead would let the structured-log labels drift from the enum they are meant to name.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Reviewed at exact head 0edc96f152dcce8f006181be04dff9386079c8df.
The release switch is applied before audit activity, defaults to the intended R1 state, has an immediate env rollback, and routes the seven non-commitment close-group “cannot serve” penalty paths consistently. Signed commitment/subtree failures, malformed responses, corrupt fetch payloads, bootstrap abuse, bad hints and payment commitment mismatches remain enforced. Mixed-version behaviour and the temporary availability risk are stated plainly in ADR-0012.
Verified locally: focused switch/unit tests, the possession-check e2e (1 passed), default and no-default-feature builds, and clippy. Current CI is fully green across Linux, macOS and Windows.
Non-blocking follow-ups: restore the e2e test switch to the release default, add switch regression coverage for the remaining audit/fetch lanes, document the canary env var for operators, and separate responder storage-read/integrity errors from generic FetchResponse::Error before R3 restores penalties.
APPROVE — no material blockers found.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Follow-up after the final adversarial review: REQUEST_CHANGES at 0edc96f152dcce8f006181be04dff9386079c8df.
FetchResponse::Error at src/replication/mod.rs:8850-8867 should not be routed through penalise_unheld_close_group_chunk. Its sole producer (handle_fetch_request, around mod.rs:6983-6992) maps LmdbStorage::get() -> Err to this variant. LmdbStorage::get returns Err for LMDB transaction/read failures, blocking-task failures, and hash-verification corruption (src/storage/lmdb.rs:507-547). A chunk the peer no longer holds maps separately to Ok(None) -> FetchResponse::NotFound.
This also explains ADR-0012’s stated “one helper, six call sites”: the implementation currently has seven, with fetch_error being the extra site. The inline claim that a disk-short node “answers exactly this way” is not supported by the producer semantics.
Please restore a direct ApplicationFailure for FetchResponse::Error; keep FetchResponse::NotFound behind the temporary switch. A focused regression test for both classifications would be useful.
The rest of the review remains positive: CI and focused tests are green, call-site coverage is otherwise consistent, and the rollout/configuration path matches the plan.
…iled Review found that `FetchResponse::Error` was routed through the suspended lane, and it should not be. Its only producer is the responder's storage read returning an error: an I/O fault, an exhausted descriptor table, or a chunk whose bytes no longer hash to their address. A peer that simply does not hold the chunk answers `NotFound`, which is a separate variant and stays suspended. Nothing about a node giving chunks up produces an error answer, so withholding the penalty for one hid real faults for no benefit. The response mapping and the charging decision are now two small functions used by the real paths, so the meaning a responder puts on the wire and the charge a fetcher applies cannot drift apart. Tests pin both: a key the node does not hold reads as a plain miss and is answered `NotFound`, a failed read is answered `Error`, and the two answers are classified as different faults. This brings the count back to the six call sites the ADR describes, and the ADR now says explicitly that a failed responder read is not one of them.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head ac68e5915c03150ff6931dd9f08b59424740fac1.
The requested blocker is resolved: responder reads now map Ok(None) to NotFound and Err to Error; NotFound remains behind the temporary unheld-chunk suspension, while Error reports ApplicationFailure directly and cannot be suppressed by that switch. ADR-0012 now states the distinction explicitly and the implementation has the documented six switched call sites.
Verified locally: both new producer/classification boundary tests pass, cargo check --lib --no-default-features passes, and format/diff checks are clean. Two independent three-reviewer groups unanimously found no material blocker; broader replication tests and clippy also passed in independent seats. All 15 GitHub checks are green.
APPROVE. Non-blocking follow-ups: directly test the charge_fetch_fault trust-event side effect under suspension, and add the singleton-hint denied-possession lane to the helper documentation enumeration.
2 participants
Linear issue
V2-1033
Risk tier
Compatibility
replication::configgainspenalise_unheld_close_group_chunk,set_close_group_storage_penalty_suspended,close_group_storage_penalty_suspended,apply_close_group_storage_penalty_policy, and the release constant. Nothing is removedor changed in shape.
Behavioural change, which is the point of the PR: peers running this build stop applying
one trust penalty. Mixed-version fleets are fine in both directions — an older peer keeps
applying it, which is exactly why this ships a release ahead of the migration rather than
alongside it.
Semver impact
Test evidence
possession_check_penalises_absent_peer_only_and_obeys_the_release_switchasserts bothdirections on one harness: with the penalty explicitly on, an absent peer is penalised and
a present one is not; with the switch in the position this build ships, the same probe
leaves the absent peer's trust untouched.
Reviewed by codex at xhigh, which enumerated and classified every
ApplicationFailureemission in
src/replication. It found one lane I had missed, a sole-source replica hintwhose sender then denies possession, and a default that pointed the wrong way. Both fixed
here. Its remaining finding is recorded in the ADR rather than fixed; see below.
New dependency
none
ADR
https://github.com/grumbach/ant-node/blob/audit/suspend-unheld-chunk-penalty/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md
Mitigation / rollback
Revert the commit, or set
ANT_SUSPEND_UNHELD_CHUNK_PENALTY=0on any node to restore thepenalty without a redeploy. Both are immediate and neither touches stored data. A node
running the previous build is unaffected and keeps penalising, so a partial rollout has no
correctness consequence in either direction.
What this is, and why it ships alone
The chunk store can only grow. LMDB returns a deleted page to its own free list and never
to the filesystem, so deleting chunks frees no disk: last week the fleet deleted 2.29
million chunks and got back zero bytes. Moving to a store that does return space means a
node short of disk has to give some chunks up while it moves the rest across.
It cannot avoid being seen doing that, and it cannot stop the consequence, because the
penalty is the auditor's decision. So the auditors stop first, one release ahead, and
the migration follows in the next one.
What is withheld, and what is not
Withheld — every way a node short of disk gets accused of not having a chunk:
AbsentPresentreturnsNotFoundorErrorNot withheld:
specific keys and could not answer for them
that does not exist rather than about the sender's own storage
Keeping the commitment lane is load-bearing, not a nicety. The migration works by having a
node reduce its signed commitment to what it can actually hold, so its peers hold it to
the smaller claim. Suspending that enforcement would make the reduction meaningless.
Audits of both kinds keep running and keep recording. Only the trust event is withheld, and
that record is how we decide when to switch the penalty back on.
Why a build constant and not a config field
A node writes its effective configuration back to disk. Shipping this as an ordinary
setting would bake this release's value into every operator's file, and the release that
flips it back would change nothing for anyone who had ever started a node.
ANT_SUSPEND_UNHELD_CHUNK_PENALTYexists for a canary and warns loudly when it disagreeswith the build.
The cost, stated plainly
Between this release and the one that restores the penalty, a peer that publishes no
commitment at all is credited as a legacy node, can answer
Present, and can then fail toserve without a trust cost. It pays for an identity and some traffic. One identity removes
one of seven replicas; controlling all seven positions removes the chunk's availability.
The commitment-bound audit is untouched, so this only works for a peer publishing no
commitment, which is itself visible in the record. The mitigation is not a code change: it
is not letting the third release slip. That release is deliberately a release rather than a
compiled-in expiry, so the date can move on evidence, and this is the risk that decides how
far it may move.