Skip to content

The sentinels a Recovery and a Publisher must say are sayable from outside - #9

Merged
aledbf merged 1 commit into
mainfrom
fix/qcow-public-sentinels
Sep 8, 2026
Merged

The sentinels a Recovery and a Publisher must say are sayable from outside#9
aledbf merged 1 commit into
mainfrom
fix/qcow-public-sentinels

Conversation

@aledbf

@aledbf aledbf commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

The defect

qcow.Recovery is the guard that closed the blank-disk defect: RestoreFrom and Current
must wrap "this volume has never published" to mean an empty chain is correct, and every
other error means refuse. That sentence had exactly one spelling — internal/commit.ErrNoHead
— which no other module can import. An implementation written in spinbox could therefore say
every error meaning refuse and none meaning permit, so qcow.Open refused every volume it
was handed. The interface the promotion out of internal/ existed to expose could not be
implemented by the consumer it was exposed for.

The fix

qcow.ErrNoHistory is that condition under this package's name, and it is
commit.ErrNoHead rather than a second value meaning the same thing. Two sentinels would
have to be kept mutually wrapping by hand, and the day one was returned bare errors.Is
would answer differently on the two spellings of one condition. One value under two names
cannot drift, and everything in the tree that already returns ErrNoHead keeps working
untouched.

What the audit of the rest of the surface found

  • The same defect on Publisher, where it costs more. The Manager branches on two of
    the injected publisher's errors — commit.ErrHeadMoved, which stops the volume, and
    commit.ErrRootSuperseded, which abandons a collapse — and both were internal too. A
    publisher in another module could not say either; the consequence of not being able to say
    the first is a host that lost the compare-and-set on HEAD going on to serve a guest whose
    writes can never be published. Now qcow.ErrNotWriter and qcow.ErrRootSuperseded, the
    same values, with the requirement stated on the Publisher interface.
  • Paths.List's missing contract. A directory that is not there must be fs.ErrNotExist
    and never an empty list — Manager.volumeIDs branches on it, and sweeping a failed listing
    as if it were empty deletes every chain on the machine. One line of doc; an outside
    implementer had no way to know.
  • Deps is not implementable from outside, and this PR does not fix it. Clock, Disk
    and Dialer are internal/simio / internal/qmp interfaces whose own methods return
    internal types (clock.Instant, clock.Timer, disk.File, disk.Usage), and
    internal/simio/real — the implementations a real host actually wants — is unreachable for
    the same reason. Manager.Volumes returns internal/agent's status type, which a consumer
    can read but cannot name. So qcow.Manager is not yet the host-side facade ADR-0021 §4
    promises spin's runner; Open and the chain functions are.
    Closing it means deciding
    where the simio boundary sits relative to the public surface — not a doc comment's decision,
    and not a re-export either, because what a consumer needs is the real implementations and
    those are what INV-01 confines. Deps now says all of this at the line that makes it.
    Nothing was added to STATUS.md: it is at its 120-line cap, and this is a decision that lives
    in the code.
  • Everything else on the surface checks out. Runner, Witness, Publisher, Recovery
    and Paths take and return only public types; Chain, Config, CompactionPolicy,
    State and its members, Lineage, Ancestor, Restored, SealedLayer, LiveImage,
    OpenRequest have no internal-typed fields; the path helpers and Sweep/HeldCommits/
    state marshalling are strings and bytes. ErrBadState wraps framed.ErrCorrupt /
    framed.ErrFormatTooNew, which an outside caller cannot name — deliberate and already
    documented: ErrBadState is the one thing to branch on and the cause rides along for an
    operator.

Proof

qcow/api_external_test.go, in package qcow_test deliberately — an in-package test can
reach internal/commit and so proves nothing about what a consumer can reach. It implements
Recovery, Paths and Runner out of nothing but the exported surface and drives Open
through: the volume born empty, a reopened chain the store has not moved past (Current's
half of the sentence), and three refusals — the catalog naming a commit the store has no HEAD
for, a store that would not answer, and a published history that moved past this disk.

Proven able to fail: with ErrNoHistory planted as a separate errors.New and the package's
errors.Is calls left on commit.ErrNoHead, all three tests go red —

--- FAIL: TestExternalRecoveryBornEmpty (0.00s)
    api_external_test.go:207: a volume nothing has published must be born empty, and this
    consumer's Recovery said so: qcow: this volume has published commits and this host has
    no copy of them: ... volume has no HEAD object: qcow: planted second sentinel
--- FAIL: TestExternalRecoveryKeepsLocalChain (0.00s)
--- FAIL: TestExternalRecoveryRefusals/the_published_history_moved_past_the_chain_on_this_disk

Delete qcow.ErrNoHistory and the file stops compiling, which is the other half of the guard.

Verification

task ci — green, exit 0 (fmt:check, build, lint incl. the simulable analyzer and buf breaking, deadcode, dev:entries, comment:rot, refs, status:cap, go test -race ./..., dst,
workflows:verify). No Postgres needed; no VM lane runs inside it. task ci:full and the
demo:stage* targets were deliberately not run — this machine's QEMU/Docker/Postgres are in
use elsewhere.

🤖 Generated with Claude Code

https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a

…tside

Promoting qcow out of internal/ moved the types and left the vocabulary behind.
`qcow.Recovery` is the guard the blank-disk defect closed: its RestoreFrom and
Current must wrap "this volume has never published" to mean an empty chain is
correct, and every other error means refuse. That sentence had exactly one
spelling, `internal/commit.ErrNoHead`, which another module cannot import — so
an implementation written in spinbox could say every error meaning "refuse" and
none meaning "permit", and `qcow.Open` refused every volume it was handed. The
interface the promotion existed to expose could not be implemented by the
consumer it was exposed for.

`ErrNoHistory` is that condition under this package's name, and it *is*
`commit.ErrNoHead` rather than a second value that means the same thing. Two
sentinels would have to be kept mutually wrapping by hand, and the day one was
returned bare, errors.Is would answer differently on the two spellings of one
condition; one value under two names cannot drift, and everything in this tree
that already returns ErrNoHead keeps working untouched.

The audit that followed found the same defect once more, on the path where it
costs more. The Manager branches on two of Publisher's errors — `ErrHeadMoved`,
which stops the volume, and `ErrRootSuperseded`, which abandons a collapse — and
both lived in internal/commit as well. A publisher in another module could not
say either, and the one it could not say that matters is the first: a host whose
compare-and-set on HEAD lost, unable to report it, goes on serving a guest whose
writes can never be published, which is the fencing failure §15 is about. They
are now `qcow.ErrNotWriter` and `qcow.ErrRootSuperseded`, the same values, and
the Publisher interface documents that an implementation has to be able to
produce them. Paths.List gained the one line of contract a caller outside this
repository cannot guess and a sweep depends on: a missing directory is
fs.ErrNotExist and never an empty list.

The test is in `package qcow_test`, deliberately: an in-package test can reach
internal/commit and so can prove nothing about what a consumer can reach. It
implements Recovery, Paths and Runner out of nothing but the exported surface and
drives Open through the volume that is born empty, the reopened chain the store
has not moved past, and three refusals. Planted against the sentinel being a
second value, it goes red on all three.

What the audit did *not* fix, and could not: `Deps` still asks for
`clock.Clock`, `disk.Disk` and `qmp.Dialer`, whose methods return internal types,
so no implementation of them exists outside this module and `internal/simio/real`
— the implementations a real host wants — is unreachable too. `Manager.Volumes`
returns `internal/agent`'s status type, which a consumer can read and cannot
name. So `qcow.Manager` is not yet the host-side facade ADR-0021 §4 promises
spin's runner; `Open` and the chain functions are, and Deps now says so at the
line that makes it. Closing it means deciding where the simio boundary sits
relative to the public surface, which is not a doc comment's decision and not a
re-export either: what a consumer needs is the real implementations, and those
are the ones INV-01 confines.

Verified: task ci.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a
@aledbf
aledbf merged commit 1e87b03 into main Sep 8, 2026
4 of 6 checks passed
@aledbf
aledbf deleted the fix/qcow-public-sentinels branch September 8, 2026 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant