custodian: backfill silently skips a committed record whose inode key will not parse — after #695 it lands on neither gauge and the pass still certifies

[eduralph]

Found by the Codex reviewer on #704 (issue #695's PR) and confirmed by hand. The skip itself is
pre-existing on origin/main @ 339da46; what #704 changes is the consequence, from a gauge that
silently never drains to a pass that reports nothing wrong at all. Filed rather than folded into
#695 so that slice stays one logical change — the same call #698 was carved out for.

The defect

custodian::backfill::reconcile walks scan(b"inode:") and, for a record it has already
established is Committed, drops it on the floor when the key will not parse:

// crates/custodian/src/backfill.rs:146-148 @ a9cfa8d (#704); :84-86 on main
let Some(inode_id) = parse_inode_key(&key) else {
    continue;
};

A bare continue. No report, no counter, no attribution. parse_inode_key (:64-70) is
from_utf8 → strip_prefix("inode:") → str::parse, so None means a non-UTF-8 key or an
InodeId spelling that will not parse — the scan's own prefix guarantees strip_prefix
succeeds.

Every other way this loop can fail over a committed object is fail-closed and attributed. This
one is not:

failure	line @ a9cfa8d	handling
record will not decode	:137-141	emit_unresolvable + incomplete += 1
key will not parse	:146-148	silent continue
resolver returns ChunkMapError	:167-171	emit_unresolvable + incomplete += 1
segmented map, fill declined	:219-222	emit_declined + incomplete += 1

The record cannot be filled by any later pass either — the CAS at :240 builds its write key
from metadata::inode_key(inode_id), so without a parsed id there is nothing to write to. It is
exactly what :104-106 defines incomplete to hold: a committed object this pass "could not
read at all, or read and may not fill".

What #704 changes

On main the record is invisible to the fill, but not to the gauge. emit_remaining
(339da46:171-190) is a second scan over for (_key, value) — it never looks at the key, so the
record's empty placements are still counted into remaining. An operator sees a population that
never drains to zero: unexplained, but visible.

#704 replaces that second scan with same-walk accounting, and the same walk skips the record
before remaining += to_fill.len() at :202. So after it lands the record contributes to
neither gauge, and the verdict at :287-295 is Blocked only when incomplete > 0. A store
whose sole unfilled object sits under an unparseable key therefore reports:

gauge.backfill_placement_remaining   = 0
gauge.backfill_placement_incomplete  = 0
reconcile(..)                        -> Reconciled::Satisfied

— the drain certified converged over an object the pass never read. That is the outcome the
module's own docs refuse for the three neighbouring cases, on docs/principles.md §5 C-1: an
operator reading Satisfied acts on it, decommissions the server, closes the ticket.

Reachability

Not reachable through any in-tree path today. metadata::inode_key
(crates/core/src/metadata.rs:33-36) is the sole writer of the inode: prefix and only ever
emits a canonical UTF-8 spelling, so nothing in the tree produces a key parse_inode_key
rejects. It needs a corrupt key, or a legacy/foreign writer.

Latent, then — the same standing as #698, and for the same reason: the guarantee is held up by
"no current caller does that" rather than by the loop refusing to certify what it did not read.
The decode failure one branch above is no more likely and is handled.

What a fix has to settle (not prescriptive)

• Attribute it. Mirroring the decode branch is a three-line change, and object_name
  (crates/custodian/src/gc.rs:470-480) escapes arbitrary bytes, so it is safe to call on the
  very keys that fail from_utf8:

  let Some(inode_id) = parse_inode_key(&key) else {
      emit_unresolvable(&object_name(&key), "unparseable inode key");
      incomplete += 1;
      continue;
  };

• Leave the ordering alone. The parse already sits after the state != Committed check
  (:143-145), so an uncommitted record under a bad key stays silently skipped, which is
  correct — nothing is owed for it.

• Decide whether remaining should carry it too. It could: resolve_chunk_map takes
  &key/&record and never needs the parsed id, so the empty placements are countable before
  the parse matters. Only the CAS needs the id. Counting the record on both gauges would be
  strictly more honest than incomplete alone; folding it into incomplete and stopping is
  simpler and matches the decode branch. Either is defensible; the choice should be explicit
  rather than incidental.

• Cover it. crates/custodian/tests/backfill.rs never constructs an inode: key — it only
  filters on starts_with(b"inode:") twice (:154, :166). A committed record under an
  unparseable key, asserting Blocked and incomplete = 1, is the missing case.

Related

• custodian: backfill reads a record at one inode key and CASes it at another #698 — the other parse_inode_key defect: the write key is re-derived from the parse
  rather than taken from the store, so a non-canonical spelling is read at one key and CASed at
  another. Same function, same call, opposite end — custodian: backfill reads a record at one inode key and CASes it at another #698 is about what happens when the parse
  succeeds on a spelling it should not have; this is about what happens when it fails. A fix
  for either should look at the other; a fix that made parse_inode_key strict would move
  records out of custodian: backfill reads a record at one inode key and CASes it at another #698's path and into this one, which is only safe once this one counts them.
• custodian: Reconciled::Blocked's contract says "could not be read", but three in-flight loops also return it for work they refuse #701 — Reconciled::Blocked's contract across the in-flight loops. This is one more
  committed-but-unread case that has to land inside whatever that settles.