diff --git a/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md b/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md new file mode 100644 index 00000000..c3103c09 --- /dev/null +++ b/docs/adr/ADR-0012-file-based-chunk-store-and-lmdb-retirement.md @@ -0,0 +1,401 @@ +# ADR-0012: One File Per Chunk, and Retiring LMDB Without Losing Data + +- **Status:** Proposed +- **Date:** 2026-08-25 +- **Decision owners:** Anselme Gaeremynck +- **Reviewers:** David Irvine, Chris O'Neil, Mick van der Most van Spijk +- **Supersedes:** none +- **Superseded by:** none +- **Related:** ADR-0002 (gossip-triggered subtree audit), ADR-0003 (possession checks), + ADR-0004 (commitment-bound quote pricing), ADR-0007 (Windows LMDB map headroom cap, + retired by this decision) + +## Context + +The node stores chunks in LMDB. LMDB returns a deleted page to its own free list and never +to the filesystem, so **deleting chunks does not free disk**. In one week the fleet deleted +2.29 million chunks and got back zero bytes. Operators read that as a bug and are tempted +to wipe node directories to reclaim space, which costs the network real replicas. + +There is no partial way out. Compaction needs free space equal to the live data, which is +exactly the condition a full node does not meet, and it does not get us off LMDB anyway. +Punching holes in `data.mdb` is Linux-only, needs LMDB internals to identify free pages, +and reads back as zeros. Disk comes back exactly once: when `chunks.mdb` is removed whole. + + peak disk during migration = allocated chunks.mdb (unchanged) + files written so far + +So a local migration is possible if and only if `free >= live payload`. On production +volumes today (55 volumes at 492 GiB, free median 25.8 GiB, p10 9.8 GiB, about 12 nodes +per volume, about 38 GiB of LMDB per node of which about 24 GiB is live) migrating one +node costs 24 GiB and returns 38 GiB. One at a time the host gains about 14 GiB per node +and the queue accelerates. All twelve at once need 288 GiB and all twelve stall. + +The chunk workload is the easiest possible case for a filesystem: content-addressed, +immutable, write once, read many, delete whole, and **4 MiB**, confirmed by the team +rather than assumed. That size is what makes one file per chunk the right shape; see the +Storj and borgbackup note under Validation for what would change the answer. + +## Decision Drivers + +- Deleting a chunk must return its blocks to the filesystem, on a full disk, with no free + space required and no compaction to schedule. +- No chunk may lose its last replica, including during a fleet rollback, a skipped + upgrade, or a crash halfway through the migration. +- Mass audit failures are as damaging as data loss. Nothing here may cause them. +- It has to work for every operator, not for our fleet. Most node operators are not us and + cannot be told to attach a second volume. +- No opt-in. Whatever we ship is what every node does by default. + +## Considered Options + +1. **Stay on LMDB and compact.** Needs free space equal to the live data, which is the + condition we are trying to escape, and leaves us on LMDB. +2. **Append-only packs** (borg segments, Storj hashstore). Reintroduces compaction, a free + list, and a cross-file index. That is LMDB's disease with a different allocator. +3. **Fixed-size slots** (Sia `hostd`, Swarm sharky). Cheaper than log packing, and Sia's + sector size is exactly our 4 MiB. But a freed slot returns space to the *store*, never + to the *filesystem*: the volume file never shrinks. It is the right design once a node + has a declared capacity, and the wrong one while our whole complaint is that disks stay + full as chunk counts drop. +4. **One file per chunk, sharded on the address prefix.** Broken for us, see below. +5. **One file per chunk, sharded on the address suffix.** Chosen. + +## Decision + +### The store + +One immutable file per chunk: + +```text +{root}/chunks/layout.json versioned layout marker +{root}/chunks//<64-hex> xy = the LAST two hex characters of the address +``` + +**Suffix, never prefix.** A node holds keys it is among the `CLOSE_GROUP_SIZE` closest to, +so its holdings share roughly `log2(N / 7)` leading bits with its own node ID, and that +shared prefix grows as the network grows. Distinct directories a single node would actually +use, sharding on the first hex characters: + +| nodes | shared bits | 2 hex | 3 hex | 4 hex | +|---:|---:|---:|---:|---:| +| 1,000 | 7.2 | 1.8 | 29 | 459 | +| 10,000 | 10.5 | 1 | 2.9 | 46 | +| 100,000 | 13.8 | 1 | 1 | 4.6 | +| 1,000,000 | 17.1 | 1 | 1 | 1 | + +At today's ~800 nodes a two-hex prefix is already down to about two directories. Prefix +sharding does not degrade, it fails, and it fails later for the nodes that grow into it. +Close-group membership constrains the leading bits and places no constraint at all on the +trailing ones, and the address is a BLAKE3 output, so the last byte is uniform by +construction at every network size. IPFS shipped the same fix for a different reason: its +prefixes were constant because of the CID encoding, not because of clustering, and the +flatfs `_README` still says *"Previously, we used prefixes, we now use the next-to-last two +characters."* The generalisation is the part worth keeping: **shard on bits you can prove +are uniform, not on bits that happen to be uniform today.** + +**256 shards, one level.** 23 files per directory at today's ~6,000 chunks per node, 977 at +a 1 TiB node, 39,000 at 10 TiB, for 1 MiB of directory inodes. 4,096 shards only starts to +pay past several million chunks and costs sixteen times the directory overhead for every +node that is not that large. + +**Lowercase hex filenames, full 64 characters.** NTFS and default APFS fold case, so under +base64url or base58 two distinct keys can share one case-folded filename, which is a silent +overwrite. No hex string can spell `CON`, `NUL`, `AUX`, `COM1` or `LPT1`, because none of +those letters is in `0-9a-f`. Keeping the whole key in the name means a `find` over the tree +recovers the store even if the directory layer is lost. + +**The scheme is recorded in `layout.json` at creation.** Nobody in this survey shipped an +in-place re-sharder and all of them paid for it: IPFS says export and re-import, Storj ran a +multi-year satellite-controlled backend migration, borg rewrites only on the next +compaction. One small file is the difference between changing the default later and never +being able to. + +### The index + +**The filesystem is the sole authority.** The key set is a `BTreeSet` rebuilt at +every open by reading directory entries, names only: no `stat`, no content read. A `stat` +per entry costs about ten times the enumeration on Linux and macOS and fifty to sixty times +on Windows, and buys nothing, because the filename is the key. + +No sidecar database, because a persistent index **cannot remove reconciliation**. Commit +the index first and a crash leaves a phantom key; rename the file first and a crash leaves +an unindexed file. Repairing either means looking at the filesystem anyway, so the +filesystem may as well be the authority, and then nothing can drift. Ceph FileStore's +tracker #17177 is the cautionary tale: a crash between `unlink` and the LevelDB flush +orphaned omap keys that were silently reattached to a different object later. + +`BTreeSet` rather than a hash set for three reasons: `all_keys()` must be sorted (the +commitment builder truncates the responsible subset with `take(cap)` *before* the Merkle +tree sorts it, so an unstable order would make the published commitment depend on iteration +luck), it never spikes memory while growing, and bulk-building it from a sorted vector packs +every node to capacity where repeated insertion converges on 68% fill for the same keys. + +**One process per data directory, enforced.** LMDB was genuinely multi-process safe. This +store is not: two of them keep independent in-memory indices, so both would report the same +write as newly stored and each would keep serving keys the other had deleted. A node whose +store is already held by another process refuses to start and says so. + +**Every in-memory mutation mirrors a filesystem operation that has already completed**, and +never anticipates one. Bitcask's issue #114 is what the opposite order looks like: an index +rebuilt at startup and then mutated in place drifted to 2,400 keys pointing at fewer than +100 files. + +### Durability + +Write: reserve capacity, create a temp in the **destination** directory, write, flush the +file, rename, flush the shard directory, then admit the key. The publish is an +intra-directory rename, so it is atomic on every filesystem we support and only that one +directory needs flushing. The final name can never appear on partial content, because the +name is the hash. Delete: unlink, flush the shard directory, then drop the key. + +Per platform, honestly: + +| | rename atomic | fsync(temp) + rename durable | directory fsync | +|---|---|---|---| +| ext4 | yes | **no**, `auto_da_alloc` only orders data before the rename's commit | yes, required | +| XFS | yes | not by that sequence | yes | +| btrfs | yes | **uncertain**, ALICE found reordering | yes | +| APFS | yes | `sync_all` already uses `F_FULLFSYNC` on Apple targets | returns 0, effect undocumented | +| NTFS | **not documented as atomic** | unknown | **no documented way** | + +On Windows a node cannot make the rename durable through the standard library at all. The +content is content-addressed and re-replicable, so the position we take is: accept it, +detect a missing or corrupt file on read, repair from the network, and **refuse to delete +the legacy environment on Windows** unless an operator explicitly overrides after +power-loss testing. + +### Retiring LMDB + +Three releases, because slashing is the *auditor's* decision. A node that has to give up +chunks cannot stop its auditors from penalising it, so the auditors have to stop first. + +| Release | Penalise a peer for not holding a close-group chunk? | Delete `chunks.mdb`? | +|---|---|---| +| **First**: stop one penalty | no | no | +| **Second**: migrate | no | yes | +| **Third**: restore it | yes | yes | + +What the first release withholds is deliberately narrow: only the penalty for **not holding a close-group +chunk you were supposed to be holding**. The commitment-bound subtree audit still +penalises, in every release. So does a responder whose own storage fails: a fetch answered +with an error means the read faulted or the bytes no longer hash to their address, which is +never what a node giving chunks up looks like, and a node that does not hold the chunk says +so with `NotFound` instead. That is not a compromise, it is what makes the rest work: a +node reduces its commitment precisely so its peers hold it to the smaller claim, and +suspending that enforcement would make the reduction meaningless. Audits of both kinds run +and record throughout. + +Both are **build constants with environment overrides, never serialised config**. A node +writes its effective configuration back to disk, so shipping them as ordinary fields would +bake the first release's values into every operator's file and the next would change nothing. + +Per node, in order: + +1. **Open both stores.** Reads are the union, writes go to files. New chunks are also + written to LMDB **first** while it exists: a chunk uploaded during the bridge to holders + that all revert to a pre-migration build would otherwise be gone from every one of them, + and that is client data, not a replica. +2. **Copy closest first**, throttled, stopping at a slack floor above the disk reserve. +3. **Settle.** The node commits only to its file-backed keys from here, while still serving + everything it ever committed to. Serving reads the union; the commitment reads the + file-backed set. A node is at worst over-honest. Nothing is deleted at this step: it + only narrows the claim, so the close group can learn the new one before anything goes. +4. **Verify.** Every chunk both stores hold is re-hashed and recopied from LMDB on + mismatch. A filename is not proof the bytes behind it are good, and the startup scan + reads names only. +5. **Retire.** Once the retirement delay has elapsed, at least two commitment rebuilds have + been published, and no key the node is giving up is still answerable under a retained + commitment slot: rename `chunks.mdb` aside, flush the parent, record the node as + file-only, and only then delete it. The rename is what makes the state change atomic, + because `remove_dir_all` is not: a failure partway through leaves a directory that can + no longer be opened as an environment, and recording completion on top of that would + have the node claim it had finished over a half-deleted store. **This is where the disk + comes back.** Every gate is rechecked inside the destructive step itself, in the same + critical section that proves no other task holds the store, because the verification + pass alone can run for hours and a write whose file half failed adds a key in the + meantime. +6. **Refetch** the shortfall through ordinary replication, with the freed space to do it in. + +The delete gate is the pruner's existing retention contract +(`ResponderCommitmentState::is_held`, `GOSSIP_ANSWERABILITY_TTL` three hours). No new +protocol. + +**Nothing is given up without proof it exists elsewhere.** Only nodes that cannot fit +their payload give up anything at all, and such a node must clear three gates, in this +order, before a byte is deleted: + +1. **It is not near the front of the group for the chunk.** Only the last two positions of + the *admission group* (`storage_admission_width`, the close group plus its margin) are + eligible, which is the width the pruner treats as strictly in-range and refuses to + delete inside. A one-off migration must not be more willing to drop a chunk than the + thing that runs every day. +2. **Its close group has received the reduced commitment.** The node narrows what it claims + first, and only once peers have demonstrably received that narrower claim, proven by + them answering a neighbour sync that carried it, may anything be deleted. Until then + they audit it against the set it used to hold, and a wave of audit failures is as + damaging as losing the chunks. +3. **Other nodes have proven they hold the chunk, and are currently publishing a claim.** + All but one of its current close group must answer a cryptographic possession challenge + over a nonce they have never seen. This is the pruner's own evidence, reused + deliberately, and it is deliberately not the cheap `VerificationRequest`: that carries a + self-reported `present: bool`, and a node that has silently lost a chunk still answers + yes. A peer only counts if this node has also heard a commitment from it recently, which + excludes a peer sitting between a retired commitment and its next rotation. That gap is + exactly what a node in the middle of its own migration looks like, and counting it would + let two migrating nodes each conclude the other was covering the chunk. + +Rank alone would not do. Being far from a chunk says something about who *should* hold it, +not about who *does*, and in a fleet-wide migration the nodes that should hold it are +exactly the ones that may also be short of space. Without gate 3 the safety property is +merely statistical: every holder could be short at once and each drop the same chunk, and a +per-volume lock cannot see that, because it serialises one volume and this is a +network-wide question. + +A node that cannot clear these gates keeps both stores, does not free its disk, and tells +the operator to add storage. That is the correct answer, not a smaller replica count. + +Gates 2 and 3 are re-checked immediately before the environment is removed, not once when +the node settled hours earlier. The group moves, and two paths can put a key back into the +legacy-only set in between: a file that failed verification and is now being served from +the legacy copy, and a write whose file half failed. + +**Two of a close group at a time, not seven.** The gates above are per chunk, and they are +safe, but on their own they deadlock: if every holder migrates at once, none can prove to +the others that a copy survives and the whole group sits waiting. So each node derives a +migration wave from a hash of its own ID, and a group of seven is split into four waves. +Wave `w` opens `w * wave_hours` after the build first starts. It needs no coordination and +no protocol change, which matters because a node cannot usefully ask its close group "are +you migrating?" and would not trust the answer by the time it arrived. + +It is a stagger, not a guarantee: seven IDs hashed into four waves will not always land two, +two, two, one. What makes it safe rather than merely tidy is that it composes with the +possession gate. A node whose turn has come still cannot give a chunk up until its +neighbours prove they hold it, so an unlucky wave waits instead of over-shedding. Only nodes +that have to give something up wait for a wave; a node with room copies and retires +immediately, because it is never unable to serve. + +Separately, a host-wide advisory lock serialises migrations sharing a volume, held from the +first copy through retirement, so a node cannot release it and let eleven others start +before it has returned a byte. The two limits answer different questions: the lock is about +one machine's disk, the wave is about one chunk's replicas. + +## Consequences + +### Positive + +- `unlink` returns blocks immediately. No free list, no compaction, no free space required + to reclaim space. This is the entire point. +- `exists()` and `current_chunks()` become in-memory lookups with no syscall, cheaper than + the LMDB reads they replace. +- `all_keys()` gains a stable ascending order, which the commitment builder needs and the + pruning cursor wants. +- A fresh node never opens a memory map at all. `storage.db_size_gb` and ADR-0007's Windows + map headroom cap die with LMDB. +- The store is self-describing: the filename is the hash, so an operator can verify a chunk + with `b3sum`, and a scrambled directory layer is recoverable with `find`. + +### Negative / Trade-offs + +- **There is no rollback once a node has deleted its LMDB.** The staged rollout is the only + control: a small leading batch, ours, and a wide window. +- **The window between the first and third releases is publicly known, and in it nobody is + penalised for failing to hold a close-group chunk.** The cheapest way to exploit it is + precise and worth writing down: a modified peer that never gossips a commitment at all is + credited as a legacy node, can answer `Present`, and can then return `NotFound` or fail a + possession check with no trust cost. It pays only for an identity and the traffic. One + such identity removes one of seven replicas; control of all seven positions removes the + chunk's availability. The commitment-bound audit is untouched, so this only works for a + peer that publishes no commitment at all, which is itself visible. The mitigation is not + a code change, it is not letting the third release slip. + It is bounded, because the third release evicts afterwards, and audits keep recording so we + can see it happening, but it is a real invitation for the duration. +- `exists()` is now an index lookup rather than a read of the backing store, so something + outside the node deleting files is not noticed until the next read of that key. The read + path self-heals, and a `stat` per call on the node's hottest path is not worth it. +- One inode and one directory entry per chunk. At 4 MiB per object that is 0.05% overhead + and block rounding for a full chunk is exactly zero, but it is real. +- Windows retirement is off by default, so Windows nodes keep both stores until we can test + power loss on NTFS. +- The paid list is still LMDB. It is a fixed 256 MiB map that contributes nothing to the + disk problem, but it is why `heed` cannot be dropped yet. +- **Narrowing the commitment cuts the quoted price.** Price is quadratic in the committed + key count, so a node that has just proved it is short of disk advertises a cheaper quote + than its close-group peers and then refuses the store on capacity. A wasted round trip + rather than a mispayment. The fix belongs to the quote path and is a separate decision. +- **A cancelled awaiter drops the per-key lock while its blocking write runs on.** The two + consequences are bounded: a pruned chunk can be re-created, which the pruner deletes + again, and a cancelled write can leave an orphan in the legacy store, which retirement + removes and whose client was never acknowledged. + +### Neutral / Operational + +- Startup cost is the directory scan: 122 ms warm and 1.55 s cold at 250,000 files across + 256 shards on APFS, of which the index build is 2 to 11 ms. No fast-start snapshot in v1. + If one is ever added, validate it with the Merkle root of the sorted key set (which + ADR-0004 already computes) rather than a checksum, because a checksum passes for an + operator who restores yesterday's data directory and leaves yesterday's snapshot. +- APFS enumeration degrades with churn, not just size: a million files went from about 72 + to about 306 microseconds per entry over twenty cycles of 5% replacement. A long-lived + macOS node will get slower to start in a way a fresh benchmark never shows. +- NTFS 8.3 short-name generation is worse for us than for most, because a node's filenames + genuinely share a long prefix. Microsoft advises disabling it above 300,000 files per + directory. + +## Validation + +**Already proved, locally:** publish is exactly-once under sixteen concurrent writers of one +address; the index rebuilds from the filesystem across restarts with a stable order; a file +in the wrong shard, an uppercase name, and a non-hex name are all refused; an interrupted +write is swept; a corrupt file is removed and repaired from the legacy copy; a missing file +drops out of the index so replication repairs it; the copier is resumable and cannot +resurrect a pruned chunk; retirement is refused while any gate is unmet and removes the +environment when they are all met; the release switches never round-trip through a config +file. + +**Fleet gates, which cannot be closed from a workstation:** + +- Forced power loss on ext4, XFS, btrfs, APFS and NTFS showing old-or-new, with antivirus + and 8.3 generation enabled on the NTFS run. Windows retirement stays off until this passes. +- Startup scan, RSS and inode use at 100k, 1M and 10M keys on each filesystem. +- The first release gates on no audit-timeout regression on the quiet responsible lane and on + disk growth + matching prediction. +- The second gates on a soak of the first, plus a verified retirement returning the + predicted space. +- The third gates on migration-complete lines across the fleet, refetch backlogs drained, and the + recorded audit failure rate back to its pre-migration baseline. The first release's + observability is + what makes that decidable. +- **How often a short-of-disk node can actually clear the possession gate.** A node whose + close group is also short of space will not clear it, will not free its disk, and will + tell its operator to add storage. That is the intended answer, but the fleet needs to + show how large that population is before the second release, because it decides whether the + migration + completes on its own or needs operator action at scale. +- **Chunk size is 4 MiB, confirmed.** This was the open question that gated the whole + design and it is now answered. Storj and borgbackup both ran one file per object at scale + and reversed to packing, and both did so for *small* objects: Storj's pieces are *"often + smaller than a hard drive sector"* and over 60% of borg's chunks are under 8 KiB. Nobody + has reversed this decision for large objects. The tripwire remains: if the network ever + starts storing a large share of small records, this ADR should be revisited, and the + inode exposure below comes with it. + +**Review trigger:** if the network ever adopts a declared node capacity, fixed-slot packing +becomes the better store design and this decision should be reopened. + +## Implementation slices + +This ADR is landed by two pull requests, in this order: + +1. **Stop penalising a node for not holding a close-group chunk.** One switch, one helper, + six call sites. It must ship a release ahead of the migration, because the penalty is + the auditor's decision and a node cannot stop its peers applying it. The commitment-bound + subtree audit keeps penalising throughout. +2. **The file store and the migration.** Everything else in this document. + +A third release flips the switch from (1) back, gated on fleet evidence rather than a date, +which is why it is a release and not an expiry constant compiled into the first one. + +## Notes for AI-assisted work + +Drafted with AI assistance. Not to be marked Accepted without human review. diff --git a/src/node.rs b/src/node.rs index 65b66b4f..f98f4dee 100644 --- a/src/node.rs +++ b/src/node.rs @@ -97,6 +97,12 @@ impl NodeBuilder { // Ensure root directory exists std::fs::create_dir_all(&self.config.root_dir)?; + // One release-level decision, applied before anything can audit: while the fleet + // moves off the legacy chunk store, a peer is not penalised for failing to hold a + // chunk it was supposed to be holding. It is still penalised for failing a + // commitment-bound audit. Audits of both kinds run and record throughout. + crate::replication::config::apply_close_group_storage_penalty_policy(); + // Create shutdown token let shutdown = CancellationToken::new(); diff --git a/src/replication/audit_metrics.rs b/src/replication/audit_metrics.rs index 5646a889..e1f2cf85 100644 --- a/src/replication/audit_metrics.rs +++ b/src/replication/audit_metrics.rs @@ -407,9 +407,12 @@ static DIGEST_DISPATCH_LATENCY_COUNT: AtomicU64 = AtomicU64::new(0); static DIGEST_DISPATCH_LATENCY_TOTAL_MS: AtomicU64 = AtomicU64::new(0); static DIGEST_DISPATCH_LATENCY_MAX_MS: AtomicU64 = AtomicU64::new(0); -#[cfg(feature = "logging")] impl AuditType { /// Stable structured-log label. + /// + /// Not gated on the `logging` feature: it is passed as an ordinary argument to the + /// penalty helper, which evaluates its arguments whether or not the log macro that + /// consumes them compiles to anything. #[must_use] pub const fn as_str(self) -> &'static str { match self { diff --git a/src/replication/config.rs b/src/replication/config.rs index 66c8e0bd..55bc7b60 100644 --- a/src/replication/config.rs +++ b/src/replication/config.rs @@ -15,6 +15,11 @@ use std::time::Duration; use rand::Rng; use crate::ant_protocol::CLOSE_GROUP_SIZE; +use crate::logging::{debug, info, warn}; +use saorsa_core::identity::PeerId; +use saorsa_core::{P2PNode, TrustEvent}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Arc; // --------------------------------------------------------------------------- // Static constants (compile-time reference profile) @@ -668,6 +673,145 @@ pub(crate) const CAPACITY_BLOCKED_RETRY: Duration = /// Trust event weight for confirmed audit failures. pub const AUDIT_FAILURE_TRUST_WEIGHT: f64 = 5.0; +/// Whether this build penalises a peer for not holding a chunk it was supposed to hold. +/// +/// **`true` while the fleet moves off the legacy LMDB chunk store; back to `false` once it +/// has.** Flipping it is a one-line change in one release. +/// +/// Deliberately narrow. It covers exactly one accusation: "you did not have a chunk you +/// were supposed to be holding". It does **not** cover the commitment-bound subtree audit, +/// where a peer published a signed claim to hold specific keys and could not answer for +/// them. That contract stays enforced in every release. +/// +/// The reason it has to exist at all is that the penalty is the *auditor's* decision. A +/// node that has to give up chunks, because it cannot fit them while it moves them out of +/// a store that never returns disk, cannot stop its peers penalising it for that. So the +/// peers stop first, one release ahead, and the node moves in the next one. +/// +/// A build constant rather than a config field on purpose: a node writes its effective +/// configuration back to disk, so shipping this as an ordinary setting would bake this +/// release's value into every operator's file and the next release would change nothing. +pub const RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY: bool = true; + +/// Environment override for [`RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY`], for a canary. +pub const SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV: &str = "ANT_SUSPEND_UNHELD_CHUNK_PENALTY"; + +/// The live switch. +/// +/// Initialised **from the release constant**, not to `false`. That matters: a code path +/// that never applies the policy then behaves like this release rather than the previous +/// one. Defaulting the other way meant any constructor that skipped the startup call would +/// keep penalising nodes for the very thing this release exists to stop penalising, and +/// `ReplicationEngine::new` is public and is constructed directly by test harnesses. +/// +/// Process-wide rather than threaded through a parameter because it is exactly that: one +/// release-level decision that every affected site has to obey identically, and those +/// sites are spread across call graphs that share no configuration object. +static CLOSE_GROUP_STORAGE_PENALTY_SUSPENDED: AtomicBool = + AtomicBool::new(RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY); + +/// Apply this release's decision. Called once, before anything can audit. +pub fn apply_close_group_storage_penalty_policy() { + let Ok(raw) = std::env::var(SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV) else { + apply_and_announce(RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY); + return; + }; + let suspended = match raw.trim().to_ascii_lowercase().as_str() { + "1" | "true" | "yes" | "on" => true, + "0" | "false" | "no" | "off" => false, + other => { + warn!( + "{SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV}={other} is not a boolean; \ + using the build default {RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY}" + ); + RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY + } + }; + apply_and_announce(suspended); +} + +/// Set the switch and say so, once, where an operator will see it. +/// +/// Both states are logged. An operator reading "penalties are suspended" and an operator +/// reading nothing at all cannot tell the second from a missing log line, and the state +/// that most needs to be visible is the one that disagrees with what the release intended. +fn apply_and_announce(suspended: bool) { + set_close_group_storage_penalty_suspended(suspended); + if suspended != RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY { + warn!( + close_group_storage_penalty_suspended = suspended, + "{SUSPEND_CLOSE_GROUP_STORAGE_PENALTY_ENV} overrides this build: the penalty \ + for not holding a close-group chunk is {}, where the release intends {}. \ + Clear that variable unless this node is a deliberate canary.", + if suspended { "SUSPENDED" } else { "APPLIED" }, + if RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY { + "SUSPENDED" + } else { + "APPLIED" + } + ); + } + if suspended { + info!( + close_group_storage_penalty_suspended = true, + "This release does NOT penalise a peer for failing to hold a close-group \ + chunk. Commitment-bound audits still penalise. Audits run and record \ + throughout." + ); + } else { + info!( + close_group_storage_penalty_suspended = false, + "This release penalises a peer for failing to hold a close-group chunk." + ); + } +} + +/// Set whether failing to hold a close-group chunk penalises. +/// +/// Startup applies the release policy through this. Tests that mean to exercise the +/// penalty itself set it explicitly, so what they assert is not an accident of whichever +/// release they happen to be compiled against. +pub fn set_close_group_storage_penalty_suspended(suspended: bool) { + CLOSE_GROUP_STORAGE_PENALTY_SUSPENDED.store(suspended, Ordering::Relaxed); +} + +/// Whether failing to hold a close-group chunk currently penalises. +#[must_use] +pub fn close_group_storage_penalty_suspended() -> bool { + CLOSE_GROUP_STORAGE_PENALTY_SUSPENDED.load(Ordering::Relaxed) +} + +/// Penalise `peer` at `weight` for not holding a chunk it was supposed to be holding, +/// unless this release withholds that particular penalty. +/// +/// Covers the responsible-chunk audit, the fresh-replication possession check, the prune +/// audit, and the fetch paths where a peer that answered `Present` could not then serve +/// the bytes. A node short of the disk to hold its chunks produces every one of those, so +/// leaving any of them out would stop some of its accusers and not others. +/// +/// Only the penalty is withheld. The caller has already logged the failure with its type, +/// class and key, and that record is what tells us when it is safe to switch the penalty +/// back on. +pub async fn penalise_unheld_close_group_chunk( + p2p_node: &Arc, + peer: &PeerId, + audit_type: &str, + weight: f64, +) { + if close_group_storage_penalty_suspended() { + debug!( + audit_type, + peer = %peer, + "Recorded but not penalised: this release withholds the penalty for not \ + holding a close-group chunk. Commitment-bound audits still penalise." + ); + return; + } + p2p_node + .report_trust_event(peer, TrustEvent::ApplicationFailure(weight)) + .await; +} + /// Probability of launching a subtree audit when a peer's *changed* commitment /// is ingested via gossip (ADR-0002). Keeps audits occasional surprise exams. pub const AUDIT_ON_GOSSIP_PROBABILITY: f64 = 0.2; @@ -1258,6 +1402,7 @@ fn random_duration_in_range(min: Duration, max: Duration) -> Duration { #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { use super::*; + use serial_test::serial; #[test] fn defaults_pass_validation() { @@ -1290,6 +1435,34 @@ mod tests { assert!((AUDIT_FAILURE_TRUST_WEIGHT - 5.0).abs() <= f64::EPSILON); } + /// One test rather than several, because the switch is process-wide: separate tests + /// would race each other under the default parallel runner. + #[test] + #[serial] + fn the_unheld_chunk_penalty_switch_follows_the_release_it_is_compiled_into() { + // A build that never applies the policy still behaves like THIS release, not the + // previous one. `ReplicationEngine::new` is public and is constructed directly by + // test harnesses, so defaulting the other way would leave those engines penalising + // exactly what the release exists to stop penalising. + assert_eq!( + close_group_storage_penalty_suspended(), + RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY + ); + + set_close_group_storage_penalty_suspended(true); + assert!(close_group_storage_penalty_suspended()); + set_close_group_storage_penalty_suspended(false); + assert!(!close_group_storage_penalty_suspended()); + + // And applying the release policy lands on whatever this build ships, without + // asserting the constant itself, which the follow-up release flips on purpose. + apply_and_announce(RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY); + assert_eq!( + close_group_storage_penalty_suspended(), + RELEASE_SUSPEND_CLOSE_GROUP_STORAGE_PENALTY + ); + } + #[test] fn core_replication_id_stays_v2_and_subtree_rides_its_own_id() { // Core replication, including all digest audit lanes, stays on v2. diff --git a/src/replication/mod.rs b/src/replication/mod.rs index 0b25e38c..30792ba0 100644 --- a/src/replication/mod.rs +++ b/src/replication/mod.rs @@ -5815,7 +5815,10 @@ async fn dispatch_fresh_offer( responder_class = "fresh_offer", source = %source, key = %hex::encode(key), - "Fresh offer refused at admission — this node will be penalised for the resulting absence: {failure}" + penalty_suspended = config::close_group_storage_penalty_suspended(), + "Fresh offer refused at admission; the resulting absence is recorded \ + against this node, and penalised unless the release withholds it: \ + {failure}" ); // Release the key explicitly rather than on drop, so the next offer // opens a fresh entry rather than queueing behind a handler that was @@ -6969,6 +6972,86 @@ fn request_is_stale(received_at: Instant, timeout: Duration) -> bool { received_at.elapsed() >= timeout } +/// How a fetch responder's answer is charged against its reputation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FetchFault { + /// The peer does not hold a chunk it was expected to hold. + /// + /// This is the lane the release withholds, because a node part-way through moving + /// off the legacy store answers exactly this way about chunks it has legitimately + /// given up. + UnheldChunk, + /// The peer's own storage failed, or served bytes that no longer hash to their + /// address. + /// + /// Never withheld. `FetchResponse::Error` has one producer, and it is the responder's + /// storage read returning an error: an I/O fault, an exhausted descriptor table, or a + /// failed integrity check. A peer that merely does not hold the chunk answers + /// `NotFound` instead, so nothing about the migration produces this. + ResponderFault, +} + +/// Classify a fetch response that did not carry the chunk. +/// +/// `Success` yields `None`. Every other answer is a fault of one kind or the other, and +/// which kind decides whether this release charges for it. +fn fetch_fault_for(response: &protocol::FetchResponse) -> Option { + match response { + protocol::FetchResponse::Success { .. } => None, + protocol::FetchResponse::NotFound { .. } => Some(FetchFault::UnheldChunk), + protocol::FetchResponse::Error { .. } => Some(FetchFault::ResponderFault), + } +} + +/// Charge a fetch fault to the responder. +/// +/// The only place the two kinds are treated differently. An unheld chunk goes through the +/// release switch, which is currently withholding it; a responder fault is charged +/// directly and is not affected by the switch at all. +async fn charge_fetch_fault( + p2p_node: &Arc, + source: &PeerId, + fault: FetchFault, + lane: &'static str, +) { + match fault { + FetchFault::UnheldChunk => { + config::penalise_unheld_close_group_chunk( + p2p_node, + source, + lane, + REPLICATION_TRUST_WEIGHT, + ) + .await; + } + FetchFault::ResponderFault => { + p2p_node + .report_trust_event( + source, + TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), + ) + .await; + } + } +} + +/// Turn the responder's storage read into the answer it sends back. +/// +/// The whole distinction the fetch lanes rest on is made here. A key this node does not +/// hold reads as `Ok(None)` and is answered `NotFound`. A read that fails, from an I/O +/// fault, an exhausted descriptor table, or a failed integrity check, is answered `Error`. +/// Nothing about a node giving chunks up produces the second. +fn fetch_response_for(key: XorName, read: Result>>) -> protocol::FetchResponse { + match read { + Ok(Some(data)) => protocol::FetchResponse::Success { key, data }, + Ok(None) => protocol::FetchResponse::NotFound { key }, + Err(e) => protocol::FetchResponse::Error { + key, + reason: format!("{e}"), + }, + } +} + async fn handle_fetch_request( source: &PeerId, request: &protocol::FetchRequest, @@ -6977,17 +7060,7 @@ async fn handle_fetch_request( request_id: u64, rr_message_id: Option<&str>, ) -> Result<()> { - let response = match storage.get(&request.key).await { - Ok(Some(data)) => protocol::FetchResponse::Success { - key: request.key, - data, - }, - Ok(None) => protocol::FetchResponse::NotFound { key: request.key }, - Err(e) => protocol::FetchResponse::Error { - key: request.key, - reason: format!("{e}"), - }, - }; + let response = fetch_response_for(request.key, storage.get(&request.key).await); send_replication_response( source, @@ -8171,7 +8244,7 @@ async fn run_verification_cycle(ctx: VerificationCycleContext<'_>) { } // Step 5: Update queues with the evaluated outcomes. - let mut bad_singleton_hints: HashMap = HashMap::new(); + let mut bad_singleton_hints: HashMap<(PeerId, SingletonHintFault), usize> = HashMap::new(); let mut q = queues.write().await; for (key, outcome) in evaluated { let replica_hint_sources = q @@ -8232,20 +8305,38 @@ async fn run_verification_cycle(ctx: VerificationCycleContext<'_>) { } drop(q); - for (peer, bad_hint_count) in bad_singleton_hints { + for ((peer, fault), bad_hint_count) in bad_singleton_hints { let reports = bad_hint_count.min(MAX_BAD_HINT_TRUST_REPORTS_PER_PEER_PER_CYCLE); warn!( "Peer {peer} submitted {bad_hint_count} rejected or self-contradicting \ - sole-source replica hints; \ + sole-source replica hints ({fault:?}); \ reporting {reports} bounded trust failure(s)" ); for _ in 0..reports { - p2p_node - .report_trust_event( - &peer, - TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), - ) - .await; + match fault { + // A claim about a key that does not exist. Punishable whatever the + // sender's disk is doing. + SingletonHintFault::RejectedByCloseGroup => { + p2p_node + .report_trust_event( + &peer, + TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), + ) + .await; + } + // "I advertised it and no longer have it." That is the one statement a + // node short of disk cannot avoid making while it moves its chunks, so + // it goes through the release switch. + SingletonHintFault::DeniedPossession => { + config::penalise_unheld_close_group_chunk( + p2p_node, + &peer, + "replica_hint_denied_possession", + REPLICATION_TRUST_WEIGHT, + ) + .await; + } + } } } } @@ -8297,25 +8388,43 @@ fn add_replica_hint_sources(sources: &mut Vec, replica_hint_sources: &Ha } } +/// Why a sole-source replica hint is punishable. +/// +/// The two cases look alike and are not. A hint the close group rejects outright is a +/// claim about a key that does not exist, which is a bad hint however the sender's disk is +/// doing. A sender that advertised a key and then answers `Absent` for it is making a +/// statement about its own storage, and that is the one thing a node short of disk cannot +/// avoid saying while it moves its chunks out of a store that will not give the space back. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +enum SingletonHintFault { + /// The close group says the key does not exist. + RejectedByCloseGroup, + /// The sender advertised the key and then denied holding it. + DeniedPossession, +} + /// Return the sole replica advertiser when either the close group definitively -/// rejects the key or the advertiser explicitly denies possessing it. +/// rejects the key or the advertiser explicitly denies possessing it, and say which. /// Paid-only advertisements, corroborated replica hints, and inconclusive /// rounds without that direct contradiction are deliberately non-penalizing. fn punishable_singleton_replica_hint_source( replica_hint_sources: &HashSet, outcome: &KeyVerificationOutcome, evidence: &crate::replication::types::KeyVerificationEvidence, -) -> Option { +) -> Option<(PeerId, SingletonHintFault)> { // A paid-only advertiser leaves this set empty, so the sole-source lane is // reserved for peers that actually claimed possession. if replica_hint_sources.len() != 1 { return None; } let source = *replica_hint_sources.iter().next()?; - let rejected_by_close_group = matches!(outcome, KeyVerificationOutcome::QuorumFailed); - let denied_possession = evidence.presence.get(&source) == Some(&PresenceEvidence::Absent); - - (rejected_by_close_group || denied_possession).then_some(source) + if matches!(outcome, KeyVerificationOutcome::QuorumFailed) { + return Some((source, SingletonHintFault::RejectedByCloseGroup)); + } + if evidence.presence.get(&source) == Some(&PresenceEvidence::Absent) { + return Some((source, SingletonHintFault::DeniedPossession)); + } + None } /// Post-verification bootstrap bookkeeping: remove terminal keys from the @@ -8781,43 +8890,33 @@ async fn execute_single_fetch( result: FetchResult::Stored, } } - ReplicationMessageBody::FetchResponse(protocol::FetchResponse::NotFound { - .. - }) => { - // This peer was selected as a fetch source because it - // recently answered `Present` during verification. A - // subsequent NotFound is evidence of a stale/false claim - // or chunk wiping, so penalize lightly and try another - // verified source. - warn!( - "Fetch: verified source {source} returned NotFound for {}", - hex::encode(key) - ); - p2p_node - .report_trust_event( - &source, - TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), - ) - .await; - FetchOutcome { - key, - result: FetchResult::SourceFailed, + ReplicationMessageBody::FetchResponse( + ref response @ (protocol::FetchResponse::NotFound { .. } + | protocol::FetchResponse::Error { .. }), + ) => { + // This peer was selected as a fetch source because it recently + // answered `Present` during verification, so either answer is + // evidence of something. Which one decides what it is charged: a peer + // that does not hold the chunk is the lane this release withholds, a + // peer whose own read failed is not. + if let protocol::FetchResponse::Error { reason, .. } = response { + warn!( + "Fetch: peer {source} returned error for {}: {reason}", + hex::encode(key) + ); + } else { + warn!( + "Fetch: verified source {source} returned NotFound for {}", + hex::encode(key) + ); + } + if let Some(fault) = fetch_fault_for(response) { + let lane = match fault { + FetchFault::UnheldChunk => "fetch_not_found", + FetchFault::ResponderFault => "fetch_error", + }; + charge_fetch_fault(&p2p_node, &source, fault, lane).await; } - } - ReplicationMessageBody::FetchResponse(protocol::FetchResponse::Error { - reason, - .. - }) => { - warn!( - "Fetch: peer {source} returned error for {}: {reason}", - hex::encode(key) - ); - p2p_node - .report_trust_event( - &source, - TrustEvent::ApplicationFailure(REPLICATION_TRUST_WEIGHT), - ) - .await; FetchOutcome { key, result: FetchResult::SourceFailed, @@ -8905,6 +9004,10 @@ async fn handle_subtree_failed_audit( let mut provers_guard = recent_provers.write().await; apply_audit_failure_credit_revocation(&mut provers_guard, challenged_peer, reason); } + // Deliberately NOT routed through the release switch. This is the commitment-bound + // subtree audit: the peer published a signed claim to hold these keys and could not + // answer for them. That contract is enforced in every release, including the one that + // withholds the penalty for merely not holding a close-group chunk. p2p_node .report_trust_event( challenged_peer, @@ -9097,12 +9200,13 @@ async fn handle_audit_result( } else { debug!("Audit timeout for {challenged_peer}; retaining active bootstrap claim"); } - p2p_node - .report_trust_event( - challenged_peer, - TrustEvent::ApplicationFailure(config::AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + config::penalise_unheld_close_group_chunk( + p2p_node, + challenged_peer, + crate::replication::audit_metrics::AuditType::ResponsibleChunk.as_str(), + config::AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; } } AuditTickResult::BootstrapClaim { peer } => { @@ -9903,6 +10007,78 @@ async fn rebuild_and_rotate_commitment( #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod tests { + + /// The two fetch failures mean different things and must be charged differently. + /// + /// `NotFound` is a peer saying it does not hold the chunk, which is what a node + /// part-way through the migration says about chunks it has legitimately given up, so + /// it is the lane this release withholds. `Error` has a single producer, the + /// responder's own storage read failing, and that is never about the migration. + #[test] + fn a_missing_chunk_and_a_failed_read_are_different_faults() { + let key = [7u8; 32]; + assert_eq!( + fetch_fault_for(&protocol::FetchResponse::NotFound { key }), + Some(FetchFault::UnheldChunk) + ); + assert_eq!( + fetch_fault_for(&protocol::FetchResponse::Error { + key, + reason: "read failed".to_string(), + }), + Some(FetchFault::ResponderFault) + ); + assert_eq!( + fetch_fault_for(&protocol::FetchResponse::Success { + key, + data: vec![1, 2, 3], + }), + None + ); + } + + /// The responder's answer says which fault it is, so the mapping from a storage read + /// to a response is what the classification above rests on. + /// + /// A key the peer does not hold reads as `Ok(None)`. A read that fails, whether from + /// an I/O fault or a failed integrity check, reads as `Err`. Nothing in the migration + /// turns the first into the second. + #[tokio::test] + async fn a_missing_key_reads_as_a_plain_miss_and_a_failed_read_as_a_fault() { + let dir = tempfile::tempdir().expect("temp dir"); + let storage = LmdbStorage::new(crate::storage::LmdbStorageConfig { + root_dir: dir.path().to_path_buf(), + verify_on_read: true, + max_map_size: 0, + disk_reserve: 0, + }) + .await + .expect("open store"); + + let absent = [9u8; 32]; + assert!( + matches!(storage.get(&absent).await, Ok(None)), + "a chunk this node does not hold must read as a plain miss, not a fault" + ); + + // And the answer each read produces. A miss is `NotFound`, which is the withheld + // lane; a failed read is `Error`, which is not. + assert!(matches!( + fetch_response_for(absent, Ok(None)), + protocol::FetchResponse::NotFound { .. } + )); + assert!(matches!( + fetch_response_for(absent, Ok(Some(vec![1, 2, 3]))), + protocol::FetchResponse::Success { .. } + )); + assert!(matches!( + fetch_response_for( + absent, + Err(crate::error::Error::Storage("read failed".into())) + ), + protocol::FetchResponse::Error { .. } + )); + } use super::*; use super::{ apply_audit_failure_credit_revocation, audit_failure_clears_bootstrap_claim, @@ -10364,7 +10540,9 @@ mod tests { assert_eq!( punishable_singleton_replica_hint_source(&HashSet::from([source]), &failed, &evidence), - Some(source) + Some((source, SingletonHintFault::RejectedByCloseGroup)), + "a close-group rejection outranks the denial: the key does not exist, which is \ + a bad hint however the sender's own disk is doing" ); assert_eq!( punishable_singleton_replica_hint_source( @@ -10387,7 +10565,7 @@ mod tests { .insert(source, PresenceEvidence::Unresolved); assert_eq!( punishable_singleton_replica_hint_source(&HashSet::from([source]), &failed, &evidence), - Some(source), + Some((source, SingletonHintFault::RejectedByCloseGroup)), "definitive close-group rejection is punishable without direct contradiction" ); assert_eq!( @@ -10409,8 +10587,10 @@ mod tests { }, &evidence, ), - Some(source), - "an explicit denial is punishable regardless of the overall outcome" + Some((source, SingletonHintFault::DeniedPossession)), + "an explicit denial is punishable regardless of the overall outcome, and is \ + classified separately because it is a statement about the sender's own \ + storage rather than about the key" ); } diff --git a/src/replication/possession.rs b/src/replication/possession.rs index 72c4e969..b01f9678 100644 --- a/src/replication/possession.rs +++ b/src/replication/possession.rs @@ -225,15 +225,16 @@ async fn report_possession_confirmed_failure( peer = %peer, key = %key_hex, trust_weight = AUDIT_FAILURE_TRUST_WEIGHT, - "Possession check: {peer} failed to prove possession for {key_hex} ({}); penalising at audit severity", + "Possession check: {peer} failed to prove possession for {key_hex} ({}); recorded at audit severity", failure_reason.as_str() ); - p2p_node - .report_trust_event( - peer, - TrustEvent::ApplicationFailure(AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + crate::replication::config::penalise_unheld_close_group_chunk( + p2p_node, + peer, + AuditType::Possession.as_str(), + AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; } async fn report_possession_audit_failure( @@ -248,15 +249,16 @@ async fn report_possession_audit_failure( peer = %peer, key = %key_hex, trust_weight = AUDIT_FAILURE_TRUST_WEIGHT, - "Possession check: {peer} {} for {key_hex}; penalising at audit severity", + "Possession check: {peer} {} for {key_hex}; recorded at audit severity", failure_class.as_str() ); - p2p_node - .report_trust_event( - peer, - TrustEvent::ApplicationFailure(AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + crate::replication::config::penalise_unheld_close_group_chunk( + p2p_node, + peer, + AuditType::Possession.as_str(), + AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; } async fn handle_possession_bootstrap_claim( diff --git a/src/replication/pruning.rs b/src/replication/pruning.rs index 10acee66..f1ad98d2 100644 --- a/src/replication/pruning.rs +++ b/src/replication/pruning.rs @@ -1980,12 +1980,13 @@ async fn report_prune_audit_failure_once( "Prune audit failure: peer={peer}, audit_failure_class={audit_failure_class}, key={}", hex::encode(key) ); - p2p_node - .report_trust_event( - peer, - saorsa_core::TrustEvent::ApplicationFailure(AUDIT_FAILURE_TRUST_WEIGHT), - ) - .await; + crate::replication::config::penalise_unheld_close_group_chunk( + p2p_node, + peer, + AuditType::Prune.as_str(), + AUDIT_FAILURE_TRUST_WEIGHT, + ) + .await; true } diff --git a/tests/e2e/replication.rs b/tests/e2e/replication.rs index 841da90a..3d359c54 100644 --- a/tests/e2e/replication.rs +++ b/tests/e2e/replication.rs @@ -266,7 +266,7 @@ async fn test_fresh_replication_propagates_to_close_group() { /// eviction acts on), via `P2PNode::peer_trust`. #[tokio::test] #[serial] -async fn possession_check_penalises_absent_peer_only() { +async fn possession_check_penalises_absent_peer_only_and_obeys_the_release_switch() { let harness = TestHarness::setup_small().await.expect("setup"); harness.warmup_dht().await.expect("warmup"); @@ -324,6 +324,10 @@ async fn possession_check_penalises_absent_peer_only() { "precondition: C must hold the chunk" ); + // Switched on explicitly, so this half keeps testing the possession mechanism rather + // than whichever release it happens to be compiled against. + ant_node::replication::config::set_close_group_storage_penalty_suspended(false); + let trust_b_before = p2p_a.peer_trust(&peer_b); let trust_c_before = p2p_a.peer_trust(&peer_c); @@ -345,6 +349,25 @@ async fn possession_check_penalises_absent_peer_only() { "present peer C must not be penalised: {trust_c_before} -> {trust_c_after}" ); + // And the other half of the contract, on the same harness. The release that moves + // nodes off the legacy chunk store withholds exactly this penalty: a node short of + // disk cannot avoid answering "absent" while it moves its chunks out of a store that + // never returns space, and it cannot stop its peers penalising it for that, because + // the penalty is the auditor's decision. So the auditors stop one release ahead. + ant_node::replication::config::set_close_group_storage_penalty_suspended(true); + let trust_b_suspended_before = p2p_a.peer_trust(&peer_b); + engine_a + .run_possession_check_now(address, vec![peer_b, peer_c]) + .await; + let trust_b_suspended_after = p2p_a.peer_trust(&peer_b); + ant_node::replication::config::set_close_group_storage_penalty_suspended(false); + + assert!( + trust_b_suspended_after >= trust_b_suspended_before - f64::EPSILON, + "an absent peer must not be penalised while the release withholds that penalty: \ + {trust_b_suspended_before} -> {trust_b_suspended_after}" + ); + harness.teardown().await.expect("teardown"); }