feat(engine): record a want for a part the owner cannot read

[tdakkota]

Step 2 of the design in #387, on top of #530 (claude/387-v5-foundation). It makes the v5 format mean something: an owner that cannot read a part records a Want for it, in the same CAS commit that drops it from Entries. Nothing consumes wants yet — no repair fetch, no reseed threshold, no revocable hole, no read policy.

What changed

loadPartsLocked in both recordengine/index.go and engine/index.go used to fail the whole engine on the first part it could not open. It now collects the parts whose objects are permanently absent, leaves them out of parts and out of indexed, and commits one index in which they are gone from Entries and present in Wanted.

Because the two halves are the same CompareAndSwap:

• no crash can land the drop without the want;
• a lost race leaves neither applied — the retry re-reads and re-derives both from the same evidence.

Leaving the prefix out of indexed is what keeps the existing removal diff from also calling it a removal: a loss restated as a deliberate deletion is exactly the ambiguity wants exist to remove.

Two smaller consequences of the entry no longer being there:

• foreignEntries refuses to adopt a wanted part back from a rival's index, which would put it in Entries again and discharge its own want while the bytes are still missing;
• sweepOrphansLocked spares a wanted part's surviving objects. They are the remains of a part repair is owed, not the residue of a failed flush.

Only an owner does this. The replica path (RefreshReplica, no sweep) keeps today's behaviour, because recording a want is committing an index and that is the owner's to write.

Which errors are permanent

partGone(err) == errors.Is(err, backend.ErrNotExist) — that and nothing else.

Every other failure leaves the part's existence unknown, and a want is a statement that it is gone. Recording one on an unknown drops a live part from the index and starts a repair for data that was never missing; on a shared backend a single transient fault touches every part at once, so the index would be stripped wholesale. This is ClickHouse's isRetryableException rethrow in checkDataPart.cpp, arrived at from the same direction.

A partially readable part — manifest present, a data object gone — classifies as absent, because block.OpenPart and the eager identity-column read propagate ErrNotExist from whichever object is missing. That is the right answer: the part cannot answer a read, so it is as lost as one with no objects at all.

Corrupt-but-present is deliberately not a want

Objects that exist but do not decode still fail the load. Argument for including them: from the reader's side the part is equally unusable, and repair would fix it. Argument against, which won: it is a different failure with a different remedy (a bit-rot / format problem, not a missing-bytes problem), the objects are still there so nothing is actually lost, and widening the trigger is precisely how a repair path becomes a data-destruction path — a decoder bug or a format skew would then convert every part on the node into a want and delete the index that names them. TestCorruptPartIsNotAWant pins this.

The invariant

A part leaves Entries only into Removed or into Wanted.

TestEntriesLeaveOnlyIntoRemovedOrWanted (both packages) is a property test over the real commit path: a backend wrapper intercepts every CompareAndSwap on the index key, decodes the previous and the new index, and asserts that every prefix that vanished from Entries landed in exactly one of the two lists. 24 seeds × 12 random steps of ingest+flush / merge / retention-drop / erase-a-part-and-reload, so every flush, merge, retention drop and lossy load in the test is checked.

Ablation for the transient guard

With partGone widened to err != nil, a single injected read timeout is enough:

--- FAIL: TestTransientOpenFailureRecordsNoWant/timeout
    Error: Should be empty, but was [{t/recs/01M1QAHBJVH7B4ZNYQA95BXDJB {0 0} {0 3}}
                                     {t/recs/01M1QAHBJVH7B4ZNYQA95BXDJC {0 0} {0 3}}]
    Messages: a transient failure is not evidence of loss
    Error: Not equal:
           expected: ["t/recs/…BXDJB", "t/recs/…BXDJC"]
           actual  : []
    Messages: a transient failure must not strip live parts from the index
    Error: Not equal: Generation{Counter:0x2} != Generation{Counter:0x3}
    Messages: nothing was committed

Both live parts stripped from Entries, both recorded as wants, committed. With the guard in place the load fails and the stored index is byte-identical (same generation, same entries, no wants) across timeout / cancelled / ENOSPC / permission-denied / S3 throttle.

Tests

Mirrored in recordengine/lostpart_test.go and engine/lostpart_test.go:

• TestGonePartBecomesWant — the load succeeds, the remaining part still serves, the want is in the committed index and Removed is empty.
• TestPartiallyGonePartBecomesWant — half-erased part; also asserts the sweep spares what is left.
• TestTransientOpenFailureRecordsNoWant — 5 transient error shapes; no want, entries intact, generation unchanged, load still fails.
• TestCorruptPartIsNotAWant.
• TestWantIsRecordedInOneCommit — exactly one CAS on the index key.
• TestFailedWantCommitAppliesNeither — a refused CAS leaves the entry and no want; the retry re-derives and commits both.
• TestWantSurvivesLaterCommits — carried through a later flush, a merge, and a restart.
• TestEntriesLeaveOnlyIntoRemovedOrWanted — the property test above.

go build ./..., go test ./..., -race on ./recordengine/... ./engine/... ./backend/... ., golangci-lint run ./... → 0 issues.

Docs

engine/ARCH.md gains the section (invariant, permanent-vs-transient rule and its reasoning, corrupt-but-present, the indexed/foreignEntries/sweep consequences); recordengine/ARCH.md points at it as a shared behaviour; ARCHITECTURE.md gains the invariant as a cross-cutting bullet.

Refs #387.

🤖 Generated with Claude Code

[tdakkota]

Reviewed and independently verified. This is the step where the design could have become a data-destruction path, and it doesn't.

The permanent-vs-transient guard is correct and genuinely pinned. partGone is errors.Is(err, backend.ErrNotExist) and nothing else, with the right justification in its doc comment: every other failure leaves the part's existence unknown, and a want asserts it is gone.

Verified by ablation rather than reading — widening it to err != nil fails TestTransientOpenFailureRecordsNoWant across timeout, denied, and canceled, and TestCorruptPartIsNotAWant. The failure messages are the right ones to have written down: "a transient failure is not evidence of loss" and "a transient failure must not strip live parts from the index".

The invariant test is real, not decorative. Ablating the RecordWant call so a part leaves Entries into nothing fails TestEntriesLeaveOnlyIntoRemovedOrWanted on 4+ of 24 seeds with "a part vanished from Entries into neither Removed nor Wanted". Intercepting every CAS on the index key and diffing prev/next is the right shape — it tests the property over the real commit path rather than over a helper.

Agreed on corrupt-but-present staying out, and the reason given is the one that decides it: a decoder bug or format skew would convert every part on the node into a want and delete the index naming them. That is the repair-path-becomes-data-destruction failure in one sentence. Different failure, different remedy.

Three supporting decisions I want to note because they are load-bearing and easy to have missed:

• Owner-only. The replica path (RefreshReplica, sweep == false) keeps today's fail-the-load behaviour. Correct — only the owner writes its index, so a replica must not record wants, and this is what keeps Decision 1's "the owner pulls" from being quietly violated by the refresh path.
• foreignEntries refuses to re-adopt a wanted part. Without this a rival's index would put the part back into Entries and silently discharge a want for data nobody has.
• sweepOrphansLocked spares a wanted part's surviving objects. Sweeping them would destroy the partial evidence repair needs — and is the same class as the anti-pattern the design names.

Leaving the gone prefix out of e.indexed so the removal diff does not also call it a removal is the detail that keeps the two lists disjoint; worth the ARCH.md note it got.

Ran independently: go build ./..., full go test ./..., -race on ./recordengine/... ./engine/... ./backend/..., golangci-lint 0 issues.

The cluster/router flake you saw is pre-existing and unrelated — it is the embedded-etcd port bind race tracked in #509 and fixed by #513, which is still open.

[codecov]

Codecov Report

❌ Patch coverage is 91.17647% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
engine/index.go	89.65%	3 Missing ⚠️
recordengine/index.go	89.65%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!