[Japabu]

Problem

An interior node's first child received every key below the second child's, including keys below its own stored key, which the insert left as it was. When that child then split, its siblings were keyed below the stale key and placed by binary search before it. The children were then out of order, so a search for a key in the first child's kept half descended into a sibling and found nothing. Every entry stays in the tree and lists; it cannot be reached by name, so the file server answers NotFound for a file that exists.

It surfaced when installing the toolchain package (2025 entries) on a 1 GiB DATA volume. The file server logged the entry of '…/__type_traits/detected_or.h' failed: NotFound when closing a file it had just created (guest1.log). The hash seed comes from the block count, which is why a 1 GiB volume showed it and the smaller volumes the tests used did not.

Resulting behaviour

• Routing reads only the separators after the first child (n children, n−1 separators). The first child's key, which an insert of a new least key leaves stale, is never read. A split's siblings go directly after the child they came from. Child now documents its key as a separator: no key of its subtree is below it, and all are below the next child's.
• The on-disk version is 2. A version-1 volume can hold an out-of-order node, which mounts and then answers NotFound for names it lists. It is now refused by name (UnsupportedVersion, not disowned), so DataVolume::probe answers Unmountable with the error logged and the volume is absent that boot, not misread. ROOT is the same format and is built fresh with its kernel in every slot.

Who keeps a DATA volume across boots (checked before bumping)

• Metal: the flashed image carries no DATA partition. build/image.rs create_boot_image writes the ESP, the slot table, the log partition and slots A/B only, and no metal harness creates or keeps one. A machine has DATA only where a stamped partition was put by hand. Such a volume, if any exists, is now refused by name.
• QEMU tests: every boot gets a blank DATA (tests/common/qemu.rs).
• CI: no workflow keeps a volume.
• cargo run: keeps target/nvme.img across runs (build/build.rs, designated only when first created). An existing one from before this change is refused, and DATA is absent until it is deleted (rm target/nvme.img). Nothing on a bench or CI path depends on a volume surviving.

Checks and evidence (ToyOSOrg/evidence@15eca0f, r2 index)

• Host tests. keys_below_the_tree_s_least_split_its_first_child_and_every_key_is_still_found (400 keys, then 800 descending). A seeded model test, the_tree_agrees_with_a_map_through_inserts_replacements_and_deletes, with an inline xorshift and no dependency, runs 6000 operations against a BTreeMap: new keys, replacements whose values change size (2% at the largest entry size), deletes, and 600 keys below the least one. Every 500 operations it checks that every key is found with its value, that deleted keys are absent, and that for_each_live is strictly ascending and exactly the map's keys. The tree reaches level ≥ 2. A temporary, uncommitted count over that run saw 674 splits: 223 of a first child, 445 of a middle child, and 54 into more than one sibling (splitstat.txt).
• Negative controls at 4266bf883 (run-btree.log, patches and full logs beside it). The whole fix undone (routing over every child plus the old binary-search placement), and siblings spliced one slot early, each make both tests exit 101: "756 of 1200 keys are not found" (log), "op 824: the tree has no Key …, which the map holds, to delete" (log), and "994 of 1200" or "op 70" for the early splice. Unmutated, the crate's 74 tests pass.
• cargo run -- --ci host at 4266bf883: 86 of 87 steps green (r2-ci-host.log). The red is the /tmp leftover check naming /tmp/toyos-tmp-66751-0, a guest harness's TempDir::short("boot") root left by another worktree's killed guest run. This run booted no guest; the mechanism is issues/the-tmp-leftover-check-can-name-another-worktrees-killed-run.md.
• cargo run -- --build-only, x86_64 and aarch64: exit 0 at bf89b09d1, which differs from this head only inside a #[cfg(test)] function.
• Guest: hosted_clang_hello on pkg streams a package into /apps within its volume's room, and packages the toolchain #970 rebased on this head (2fd0798e6) boots a version-2 ROOT, formats a version-2 DATA, and installs 2025 entries with no file-server entry failure (r2-guest.log). Its exit 1 is pkg streams a package into /apps within its volume's room, and packages the toolchain #970's later clang -c on stdio.h, not this change.

Not here

search_by_hash has no callers and could miss same-hash entries a split separates. That belongs in its own change.

🤖 Generated with Claude Code

[Japabu]

Review of #969 at 976de35

Architectural assessment

The fix belongs in lib/bcachefs/src/btree.rs. The owner's ruling in issues/bcachefs-crate-is-not-bcachefs.md says this format is to be replaced by real bcachefs, but until then it is the format DATA uses. The change is small (+37/−12, of which about 20 lines are the test) and adds no new mechanism.

Root cause, which I checked independently. child_index sends any key below children[1].key to child 0. A key can only end up below its own child's stored key along the tree's leftmost spine, when an insert creates a new global minimum. For any idx > 0, routing already guarantees children[idx].key <= key < children[idx+1].key. A leaf split's siblings are keyed by the first entry of each later pack, so they sit above children[idx].key and below children[idx+1].key. The old binary-search placement was therefore wrong only for child 0 with a stale key, which matches the reported loss of 39 of 826 names.

The fix is the correct invariant at every position. Children are in key order. Each key is at most the least key of its subtree and above every key of the previous child. I traced each case:

• First child: lowering makes children[0].key at most every key routed to it, so the siblings sort after it and splice(idx+1..) puts them where their keys belong.
• Middle and last child: splice matches what the old binary search did. split_node returns siblings in key order, including multi-sibling pack splits.
• Interior split: split_key = right[0].key is a real separator, because index mid >= 1.
• Root split: insert keys the old root by min_key, which follows the leftmost spine down to the true minimum after lowering.
• Cascading split: each level lowers its own child 0 on the way down, so the levels stay consistent.
• Value replacement: Ok(i) => entries[i] = entry never lowers, and if the larger value causes a split, splice handles it.
• Error path: a failed recursive insert drops the lowered children without writing anything.

Deletes and merges. There is no merge (delete says so). A delete only shrinks a subtree, so a stored key stays a lower bound and the invariant holds. Empty leaves stay in place, and an insert into one is routed and split correctly. Delete does not have this defect.

Findings

BLOCKER 1 — lib/bcachefs/src/btree.rs:850-869: the test covers one shape; the change applies to every position.

• What the test misses: it inserts 400 ascending keys and then 800 descending ones, all with 200-byte values. That covers the first-child failure, splitting the last child, interior splits and a root split. It never splits a middle child. It never makes pack produce more than one sibling. It never mixes deletes or larger-value replacements with splits. It checks only search for each key, never the tree's structure.
• Why it matters: splice now replaces the placement rule everywhere, not just at child 0. The crate's existing integration tests (100 hashed names at most) never build a multi-level tree with a new minimum, which is how this data-loss bug shipped.
• Required: a deterministic host test driven by a seed, checked against a model. It needs no dependency: an inline xorshift generating u64 keys is enough.
  • Run a few thousand operations against a BTreeMap: insert a new key, replace an existing key with a value of a different size, and delete. Include some values large enough that pack splits three ways. Include a descending run, or keep the current test beside it, because random keys make a new global minimum only about ln(n) times.
  • Assert that the tree reaches level >= 2.
  • After the run (or every N operations), check that every model key is found by search with its value and every deleted key is absent.
  • Check that for_each_live yields exactly the model's keys in strictly ascending order. That is a cheap oracle for the whole tree's structure: on the old code, listing returns the sibling's subtree before child 0's lower keys, so it goes red even for keys no search probes.
  • Run it against the existing btree.patch mutation, and against one that places siblings at idx instead of idx + 1, as negative controls.

BLOCKER 2 — lib/bcachefs/src/superblock.rs:8: volumes written before the fix are an unrecorded silent failure.

• What happens: the PR says "nothing repairs one". A pre-fix volume with an out-of-order interior node mounts and then answers NotFound for names it lists. That breaks root's "fail clearly", and no issues/ record exists, which breaks root's no-silent-debt rule.
• Where such volumes exist: issues/a-renamed-services-state-stays-under-its-old-name-unread.md notes that every checkout's target/nvme.img, which cargo run keeps across runs, is one. Any hand-placed DATA partition is another.
• A per-node check at parse time would not catch it. The faulty parent's keys [s, K0, ...] are themselves sorted. The violation is that K0's subtree holds keys below s, so only a walk of the whole tree with [lo, hi) bounds can detect it.
• Cheapest fix that removes the debt (my recommendation): bump VERSION to 2. The existing check then refuses a pre-fix volume with UnsupportedVersion. disowns_volume() is false for that error, so DataVolume::probe answers Unmountable with the error logged. The volume is refused loudly instead of misread silently. Since there are no users and the fixtures are upstream volumes, nothing else depends on version 1.
• Heavier option: a walk at mount that checks those bounds and refuses with CorruptedNode. It would also catch other corruption, but it costs a read of every node at each mount.
• If neither lands here: file a record with an owner and an exit, as issues/README.md requires.

BLOCKER 3 — lib/bcachefs/src/btree.rs:146: the Child doc is false. It still says the key is "the minimum key of the subtree". The rewritten find_child contract (line 364) says, correctly, that it is a lower bound ("at most the least key"), which is also what deletes leave behind. The comment states the exact invariant this PR fixes and contradicts the new contract. Make it agree or delete it.

SUGGESTION — a representation that removes the lowered state. Routing could ignore children[0].key altogether, the usual "n children, n−1 separators" design: child_index would search children[1..]. Then splice alone fixes the placement. There would be no lowered flag and no extra write of each node on the leftmost spine when a key becomes the new minimum. Both designs are correct; this is optional.

SUGGESTION (unrelated discovery) — lib/bcachefs/src/btree.rs:411 search_by_hash has no callers, and its descent with the maximum key would miss entries with the same name_hash that a split separated into two leaves. Delete it in its own change.

Evidence (ToyOSOrg/evidence@371b2e4)

• Revision: every run is at b77180845, a child of this head. btree.rs there is blob d1edd8fa5, identical to this head. Its other lib/bcachefs changes only add code (spare made pub(crate), a new free_bytes), so they cannot affect insert, search or this test. Reusing the evidence is valid.
• Negative control: in mutations/run-final.log, btree.patch (the whole fix reverted, comments aside) and btree-nolower both exit 101; without mutation the test passes. That is the right control. The run log shows only the FAILED line, not the claimed "756 of 1200" message.
• Guest: guest1.log has 6 NotFound lines from the file server and guest-final.log has none. guest-final.log exits 1 on stdio.h in the compile step, after install exited 0. That failure belongs to pkg streams a package into /apps within its volume's room, and packages the toolchain #970's sysroot work and does not depend on this change.
• Host CI: ci-host2.log reports 87 steps, all green, exit 0.
• PR description (for the orchestrator): the claim "the full package installed through DataVolume on a 2^18-block RAM disk lost entries before the fix and none after" has no published log. Link one or drop the claim. The guest logs and the host test already carry the claim that the bug is fixed.

SEND BACK

[Japabu]

Review round 2 of #969 at 4266bf8

Earlier blockers

1. Model test: resolved. the_tree_agrees_with_a_map_through_inserts_replacements_and_deletes is seeded and deterministic, and needs no new dependency.

• Operations: inserts of new keys, replacements whose values change size (including the largest entry size, so pack splits into more than one sibling), deletes, and a run of 600 keys below the least key.
• Checks: every key the map holds is found with its value, every deleted key is absent, and for_each_live returns exactly the map's keys in strictly ascending order. The tree reaches level 2 or more.
• Split coverage: splitstat.txt counts 223 first-child, 445 middle-child and 54 multi-sibling splits.
• Negative controls: both are at this head (r2/mutations/run-btree.log, with patches beside it). btree-revert restores both the old routing and the old binary-search placement, so it is the whole fix undone. btree-early puts the siblings at idx. Each fails both tests with exit 101 and a meaningful message ("756 of 1200", "op 824 … to delete", "994 of 1200"). Without mutation, 74 tests pass.

2. Pre-fix volumes: resolved, and I confirm the persistence claim.

• Refusal path: VERSION = 2 produces UnsupportedVersion. disowns_volume() is false for that error, so DataVolume::probe answers Unmountable and logs the error.
• Metal: create_boot_image carries no DATA partition, consistent with issues/a-renamed-services-state-stays-under-its-old-name-unread.md.
• QEMU tests: guest tests call create_sparse afresh on every boot (tests/common/qemu.rs:1471).
• cargo run: only target/nvme.img persists, created only if missing (build/build.rs:1824-1826), and the PR says so.
• Fixtures: the committed fixtures are upstream bcachefs volumes, so nothing depends on version 1.
• Guest run: r2-guest.log has no NotFound failures from the file server.

3. Child doc: resolved. "Its separator … the first child's is never read" matches the code.

The simplification: routing ignores the first child's key

This is correct. child_index now counts the separators in children[1..] that are <= key. The invariant is: each separator children[i].key for i >= 1 is at most every key of subtree i and above every key of subtree i-1. I checked every place that writes or reads a child key:

• Leaf split: the siblings are keyed by the first entry of each later pack, which is above the kept pack and below children[idx+1].key. splice(idx+1..) places them correctly at every idx, including 0.
• Interior split: split_key = right[0].key = children[mid].key, and mid >= 1, so it is a real separator. The right node's own first key is never read.
• Delete: it only shrinks subtrees, so separators remain valid bounds.
• Search, delete and search_by_hash: they all route through child_index.
• Replacement and error paths: unchanged from round 1. Removing lowered also removes the extra write of each node down the left spine.

New finding

BLOCKER — lib/bcachefs/src/btree.rs:553-556 and :686-695: min_key is now dead machinery. Under the new representation, the new root's first child's key is never read, as the Child doc says. insert still calls min_key, which reads one block per level down the leftmost spine on every root split. The only purpose is to compute old_min_key, a value nothing uses, under a name that implies a contract which no longer exists. The simplification superseded it, so delete min_key and key the first child with Key::ZERO. This is a type-level and inspection matter: a compile and the crate's host tests are enough.

SUGGESTION — lib/bcachefs/src/superblock.rs:8: the comment on VERSION is history ("version 1's insert could …"), and it goes stale at the next version bump. The commit message already records why the version changed, so delete it or reduce it to the contract.

Evidence

• Build revision: the --build-only runs (x86_64 and aarch64, exit 0) are at bf89b09d1. That differs from this head only inside the #[cfg(test)] model test (I checked the diff), so the builds carry over. The min_key deletion needs only a compile and the host tests.
• Host CI: r2-ci-host.log reports 86 of 87 steps green. The red step is the /tmp leftover check naming /tmp/toyos-tmp-66751-0. I accept that it is independent of this change:
  • The only TempDir::short("boot") is in the guest harness (tests/common/qemu.rs:2483), and this host run booted no guest.
  • Its pid matches none of this run's processes in the log; this run's roots are under toyos-tmp-21665/21737.
  • The mechanism is recorded in issues/the-tmp-leftover-check-can-name-another-worktrees-killed-run.md.
  • This change touches only lib/bcachefs, which cannot create that directory.

SEND BACK

[Japabu]

Round 3 at 2e016b021a3d2f44c39324fa3fae96820fdd4be9:

• min_key is deleted, and the new root's first child is keyed Key::ZERO. Routing never reads a first child's key. Key::ZERO's type, Deleted (0), is one the node parser accepts, and it is the key an empty leaf's min_key used to answer anyway.
• The VERSION comment is dropped; the version-2 rationale is in the commit message.
• Checks: cargo clippy -p bcachefs --all-targets -- -D warnings exit 0, and cargo test -p bcachefs exit 0 (74 unit tests and 63 integration tests, 2 + 57 + 2 + 2 across its test targets).

[Japabu]

Orchestrator: round-2 blocker resolved at 2e016b0 (checked the diff 4266bf8..2e016b0: min_key deleted, the new root's first child takes Key::ZERO, the VERSION history comment dropped; clippy and cargo test -p bcachefs 74+63 pass). Round-2 review otherwise LAND. Data-loss fix; volumes written before it are refused by version.