From affbd65caecee5d95e4344b98bfa0825495bc341 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Tue, 8 Sep 2026 00:54:48 -0300 Subject: [PATCH] The sentinels a Recovery and a Publisher must say are sayable from outside MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a --- qcow/api_external_test.go | 301 ++++++++++++++++++++++++++++++++++++++ qcow/chain.go | 34 ++++- qcow/compact.go | 5 +- qcow/manager.go | 46 +++++- 4 files changed, 374 insertions(+), 12 deletions(-) create mode 100644 qcow/api_external_test.go diff --git a/qcow/api_external_test.go b/qcow/api_external_test.go new file mode 100644 index 0000000..0c19b2f --- /dev/null +++ b/qcow/api_external_test.go @@ -0,0 +1,301 @@ +// This file is deliberately in package qcow_test and not in package qcow. +// +// What it asserts is not a behaviour of Open, it is that Open can be *reached* from +// another module: the Recovery it insists on is implemented here out of nothing but the +// package's exported surface — its interfaces, its types and its sentinels — the way +// spinbox has to implement it (ADR-0021 §4). An in-package test proves nothing about +// that, because it can reach internal/commit and every other thing a consumer cannot. +// +// It went red for the reason it exists: the only spelling of "this volume has never +// published" was internal/commit.ErrNoHead, so nothing outside this module could say the +// one condition Recovery is built around, and every volume handed to Open was refused. +// Delete qcow.ErrNoHistory and this file stops compiling. +package qcow_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io/fs" + "path" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/spin-stack/storage/qcow" +) + +// memPaths is a Paths a consumer could write: a map, and not one syscall. It is what the +// interface being implementable from outside means in practice, so it uses no helper of +// this repository's either. +type memPaths struct { + files map[string][]byte + dirs map[string]bool +} + +func newMemPaths() *memPaths { + return &memPaths{files: map[string][]byte{}, dirs: map[string]bool{}} +} + +func (m *memPaths) MkdirAll(dir string) error { + for d := filepath.Clean(dir); d != "/" && d != "."; d = filepath.Dir(d) { + m.dirs[d] = true + } + return nil +} + +func (m *memPaths) Exists(p string) (bool, error) { + _, ok := m.files[p] + return ok, nil +} + +func (m *memPaths) Size(p string) (int64, error) { + body, ok := m.files[p] + if !ok { + return 0, fmt.Errorf("size %s: %w", p, fs.ErrNotExist) + } + return int64(len(body)), nil +} + +func (m *memPaths) ReadFile(p string) ([]byte, error) { + body, ok := m.files[p] + if !ok { + return nil, fmt.Errorf("read %s: %w", p, fs.ErrNotExist) + } + return body, nil +} + +func (m *memPaths) WriteAtomic(p string, data []byte) error { + if err := m.MkdirAll(filepath.Dir(p)); err != nil { + return err + } + m.files[p] = data + return nil +} + +// List answers fs.ErrNotExist for a directory that is not there, which is the contract +// the interface states and the one a sweep depends on. +func (m *memPaths) List(dir string) ([]string, error) { + dir = filepath.Clean(dir) + if !m.dirs[dir] { + return nil, fmt.Errorf("list %s: %w", dir, fs.ErrNotExist) + } + seen := map[string]bool{} + for p := range m.files { + if filepath.Dir(p) == dir { + seen[path.Base(p)] = true + } + } + for d := range m.dirs { + if filepath.Dir(d) == dir { + seen[path.Base(d)] = true + } + } + names := make([]string, 0, len(seen)) + for n := range seen { + names = append(names, n) + } + sort.Strings(names) + return names, nil +} + +func (m *memPaths) Rename(oldPath, newPath string) error { + body, ok := m.files[oldPath] + if !ok { + return fmt.Errorf("rename %s: %w", oldPath, fs.ErrNotExist) + } + delete(m.files, oldPath) + m.files[newPath] = body + return nil +} + +func (m *memPaths) Remove(p string) error { + delete(m.files, p) + return nil +} + +// memRunner stands in for qemu-img: `create` puts a file where the package says the layer +// goes, `info` answers about the files that are there. A consumer outside this module +// gets exactly this interface — a process to run — and nothing about how it is run. +type memRunner struct { + paths *memPaths + size int64 + created []string +} + +func (r *memRunner) Run(_ context.Context, _ string, args ...string) ([]byte, error) { + switch args[0] { + case "create": + image := args[len(args)-2] + r.created = append(r.created, image) + return nil, r.paths.WriteAtomic(image, []byte("qcow2")) + case "info": + image := args[len(args)-1] + if ok, _ := r.paths.Exists(image); !ok { + return nil, fmt.Errorf("qemu-img: %s: %w", image, fs.ErrNotExist) + } + info := []map[string]any{{ + "format": "qcow2", + "virtual-size": r.size, + "filename": image, + }} + if !strings.Contains(strings.Join(args, " "), "--backing-chain") { + return json.Marshal(info[0]) + } + return json.Marshal(info) + } + return nil, fmt.Errorf("qemu-img: unexpected %v", args) +} + +// extRecovery is the whole point: a Recovery written with nothing but the exported +// surface, saying "this volume has never published" the only way a consumer can. +type extRecovery struct { + // head is the commit the object store holds, empty when it holds none — which is + // what ErrNoHistory says, and what an outside implementation could not say at all. + head string + restored qcow.Restored + // storeDown is an error that is not about history: a store that would not answer, + // which must never be read as "born empty". + storeDown error +} + +func (e extRecovery) RestoreFrom(context.Context, qcow.Lineage, int64) (qcow.Restored, error) { + switch { + case e.storeDown != nil: + return qcow.Restored{}, fmt.Errorf("rebuilding this volume: %w", e.storeDown) + case e.head == "": + return qcow.Restored{}, fmt.Errorf("volume has no HEAD object: %w", qcow.ErrNoHistory) + } + return e.restored, nil +} + +func (e extRecovery) Current(context.Context, string) (string, error) { + switch { + case e.storeDown != nil: + return "", fmt.Errorf("reading HEAD: %w", e.storeDown) + case e.head == "": + return "", fmt.Errorf("volume has no HEAD object: %w", qcow.ErrNoHistory) + } + return e.head, nil +} + +const ( + extRoot = "/data" + extVolume = "0199bd2f-0000-7000-8000-00000000c0de" + extLayer = "0199bd2f-0001-7000-8000-00000000face" + extSize = int64(64 << 20) +) + +func extOpen(t *testing.T, p *memPaths, r *memRunner, req qcow.OpenRequest) (*qcow.Chain, error) { + t.Helper() + req.Root, req.SizeBytes, req.NewLayerID = extRoot, extSize, extLayer + req.VolumeID = extVolume + return qcow.Open(t.Context(), r, p, "qemu-img", req) +} + +// TestExternalRecoveryBornEmpty is the path a consumer's very first volume takes: nothing +// has ever published it, its Recovery says so with qcow.ErrNoHistory, and Open creates +// the first layer instead of refusing. +func TestExternalRecoveryBornEmpty(t *testing.T) { + p := newMemPaths() + r := &memRunner{paths: p, size: extSize} + + chain, err := extOpen(t, p, r, qcow.OpenRequest{Recovery: extRecovery{}}) + if err != nil { + t.Fatalf("a volume nothing has published must be born empty, and this consumer's Recovery said so: %v", err) + } + image := qcow.LayerImage(extRoot, extLayer) + if chain.Active != image { + t.Fatalf("active layer = %q, want %q", chain.Active, image) + } + if len(r.created) != 1 || r.created[0] != image { + t.Fatalf("qemu-img create calls = %v, want exactly [%s]", r.created, image) + } + // What the launcher reads, and the only half of the contract that leaves this + // process: the pointer must name the layer QEMU is to be started against. + pointer, err := p.ReadFile(qcow.ActivePointer(extRoot, extVolume)) + if err != nil { + t.Fatalf("reading the active pointer: %v", err) + } + if string(pointer) != image { + t.Fatalf("active pointer = %q, want %q", pointer, image) + } +} + +// TestExternalRecoveryKeepsLocalChain is Current's half of the same sentence: a chain +// already on this disk is served when the store holds no history, so a consumer that +// cannot say ErrNoHistory loses its volume on the second open as well as the first. +func TestExternalRecoveryKeepsLocalChain(t *testing.T) { + p := newMemPaths() + r := &memRunner{paths: p, size: extSize} + if _, err := extOpen(t, p, r, qcow.OpenRequest{Recovery: extRecovery{}}); err != nil { + t.Fatalf("first open: %v", err) + } + + chain, err := extOpen(t, p, r, qcow.OpenRequest{Recovery: extRecovery{}}) + if err != nil { + t.Fatalf("reopening a chain the store has not moved past: %v", err) + } + if want := qcow.LayerImage(extRoot, extLayer); chain.Active != want { + t.Fatalf("active layer = %q, want the layer already on disk %q", chain.Active, want) + } + if len(r.created) != 1 { + t.Fatalf("qemu-img create calls = %v, want the first open's one and no more", r.created) + } +} + +// TestExternalRecoveryRefusals is the other side, and the reason ErrNoHistory has to be a +// sentinel and not "any error": every one of these is a volume Open must refuse rather +// than create empty, and each is expressible from outside this module too. +func TestExternalRecoveryRefusals(t *testing.T) { + storeDown := errors.New("the object store did not answer") + + tests := []struct { + name string + req qcow.OpenRequest + // local seeds a chain on this host before the open under test. + local bool + want error + }{{ + // §14's recovery path: a host that has never seen the volume, a bucket whose + // HEAD is gone. "No history" is true of the bucket and false of the volume, and + // creating it empty here is the blank-disk defect. + name: "the catalog says this volume published and the store has no HEAD", + req: qcow.OpenRequest{Recovery: extRecovery{}, HeadCommitID: "0199bd2f-0002-7000-8000-0000000000c1"}, + want: qcow.ErrChainMissing, + }, { + name: "the store could not be asked at all", + req: qcow.OpenRequest{Recovery: extRecovery{storeDown: storeDown}}, + want: qcow.ErrChainMissing, + }, { + // Two hosts have held this volume. The local chain is a fork of a history that + // moved on, and which of the two survives is not this process's to decide. + name: "the published history moved past the chain on this disk", + local: true, + req: qcow.OpenRequest{Recovery: extRecovery{head: "0199bd2f-0003-7000-8000-0000000000c2"}}, + want: qcow.ErrStaleChain, + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + p := newMemPaths() + r := &memRunner{paths: p, size: extSize} + if tc.local { + if _, err := extOpen(t, p, r, qcow.OpenRequest{Recovery: extRecovery{}}); err != nil { + t.Fatalf("seeding a local chain: %v", err) + } + } + before := len(r.created) + + chain, err := extOpen(t, p, r, tc.req) + if !errors.Is(err, tc.want) { + t.Fatalf("Open = (%v, %v), want an error wrapping %v", chain, err, tc.want) + } + if len(r.created) != before { + t.Fatalf("a refused volume had layers created for it: %v", r.created[before:]) + } + }) + } +} diff --git a/qcow/chain.go b/qcow/chain.go index b8b8f77..85f5c3c 100644 --- a/qcow/chain.go +++ b/qcow/chain.go @@ -67,6 +67,21 @@ var ErrChainMissing = errors.New("qcow: this volume has published commits and th // volume and somebody has to decide which of the two histories is the one to keep. var ErrStaleChain = errors.New("qcow: this host's chain is behind the published history") +// ErrNoHistory means the object store holds no HEAD for this volume: nothing has ever +// published it, and a chain born empty is the correct answer. It is the one condition a +// Recovery must be able to say, so it has to be sayable with nothing but this package — +// which is what it was not while the only spelling lived in internal/commit, where a +// consumer in another module cannot reach it. An implementation there could return every +// error meaning "refuse" and none meaning "permit", so Open refused every volume. +// +// It *is* internal/commit's ErrNoHead and not a second sentinel that means the same +// thing: two values would have to be kept mutually wrapping by hand, and the day one of +// them was returned bare, errors.Is would answer differently on the two spellings of one +// condition. One value under two names cannot drift. The name is this package's because +// this is where the surface is, and the sentence — "this volume has no history" — is the +// one a caller of Open is asking. +var ErrNoHistory = commit.ErrNoHead + // Recovery rebuilds a volume's published chain on this host. // // An interface and not a bool the caller computes, because the question — may this volume @@ -78,13 +93,12 @@ var ErrStaleChain = errors.New("qcow: this host's chain is behind the published // "born empty". type Recovery interface { // RestoreFrom rebuilds this volume's published chain locally, including its parent's - // up to the named commit when it is a clone. A wrapped commit.ErrNoHead means the + // up to the named commit when it is a clone. A wrapped ErrNoHistory means the // volume has never published and an empty chain is correct; every other error means - // refuse. commit.ErrNoHead is reused rather than a sentinel of our own because the - // condition *is* "this volume has no HEAD". + // refuse. RestoreFrom(ctx context.Context, l Lineage, sizeBytes int64) (Restored, error) // Current is the commit the object store says is this volume's newest, wrapping - // commit.ErrNoHead when it has never published. It is Restore's question without + // ErrNoHistory when it has never published. It is Restore's question without // Restore's work, and it is asked on every open of a chain that is already here: // a local chain is only current if the published history has not moved past it. Current(ctx context.Context, volumeID string) (string, error) @@ -253,6 +267,10 @@ type Paths interface { // List names the entries of a directory, without their paths. What a sweep is looking // for is precisely the files no record names, so it cannot be found by asking about // paths this host already knows. + // + // A directory that is not there is fs.ErrNotExist and never an empty list: a host on + // its first cycle and a listing that failed are the same answer otherwise, and + // sweeping against the second deletes every chain on the machine. List(dir string) ([]string, error) // Rename moves a file within the layers directory. It is how a file that is built in // several steps — a compaction's flattened root — only ever appears under its real @@ -448,7 +466,7 @@ func Open(ctx context.Context, r Runner, p Paths, qemuImg string, req OpenReques func checkNotStale(ctx context.Context, p Paths, req OpenRequest, image string) error { head, err := req.Recovery.Current(ctx, req.VolumeID) switch { - case errors.Is(err, commit.ErrNoHead): + case errors.Is(err, ErrNoHistory): return nil case err != nil: return fmt.Errorf("%w: volume %s has a local chain and the object store could not say whether it is current: %w", @@ -486,7 +504,7 @@ func checkNotStale(ctx context.Context, p Paths, req OpenRequest, image string) func born(ctx context.Context, r Runner, p Paths, qemuImg string, req OpenRequest, pointer string, local State) (*Chain, error) { restored, err := req.Recovery.RestoreFrom(ctx, req.Lineage, req.SizeBytes) switch { - case errors.Is(err, commit.ErrNoHead): + case errors.Is(err, ErrNoHistory): // This host's own record outranks the bucket's answer, in exactly one direction and // only here. An Agent that published commits for this volume and is pointed at no // object store — or at the wrong one — is told "never published", and would create a @@ -543,7 +561,7 @@ func born(ctx context.Context, r Runner, p Paths, qemuImg string, req OpenReques // // Restore and not Current, and the difference is the whole decision: Current asks whether // the local chain is behind and can only refuse, while a host with a guest waiting needs -// the store to put the history on this disk. Its ErrNoHead is the case that keeps the +// the store to put the history on this disk. Its ErrNoHistory is the case that keeps the // local chain — nothing was ever published by anyone, so replacing the local layers with // an empty image is the blank-disk defect with a fence in front of it. // @@ -551,7 +569,7 @@ func born(ctx context.Context, r Runner, p Paths, qemuImg string, req OpenReques func regrant(ctx context.Context, r Runner, p Paths, qemuImg string, req OpenRequest, pointer, image string, local State) (chain *Chain, keepLocal bool, err error) { restored, err := req.Recovery.RestoreFrom(ctx, req.Lineage, req.SizeBytes) switch { - case errors.Is(err, commit.ErrNoHead): + case errors.Is(err, ErrNoHistory): slog.Info("this volume was granted back to this host and the object store holds no history for it, so the local chain is the only one there is", "volume_id", req.VolumeID, "tip", image, "fenced_at_epoch", local.Fenced.Epoch) if err := clearFence(p, req.Root, req.VolumeID); err != nil { diff --git a/qcow/compact.go b/qcow/compact.go index a9af5f3..43b0f10 100644 --- a/qcow/compact.go +++ b/qcow/compact.go @@ -9,7 +9,6 @@ import ( "slices" storagev1 "github.com/spin-stack/storage/api/gen/spin/storage/v1" - "github.com/spin-stack/storage/internal/commit" "github.com/spin-stack/storage/internal/ids" ) @@ -389,12 +388,12 @@ func (m *Manager) collapse(ctx context.Context, v *volume, st *State) error { } if err := m.pub.Publish(ctx, layer); err != nil { switch { - case errors.Is(err, commit.ErrRootSuperseded): + case errors.Is(err, ErrRootSuperseded): // The object store's own statement of the check above, for the window this // host cannot see from its record: HEAD is not the commit these bytes // reconstruct. return m.abandonCompaction(v, st, err.Error()) - case errors.Is(err, commit.ErrHeadMoved): + case errors.Is(err, ErrNotWriter): // Another host is this volume's writer. A collapse is a publish, so it meets // the same fence as any other one, and for the same reason: the alternative is // this host serving a guest whose writes can never land. diff --git a/qcow/manager.go b/qcow/manager.go index 07555ac..2197c2d 100644 --- a/qcow/manager.go +++ b/qcow/manager.go @@ -172,11 +172,52 @@ type SealedLayer struct { // // A nil Publisher is a host that keeps its layers locally and publishes nothing, which // is every lane before v6 §23.3 and is not an error. +// +// Two of its failures are not failures of the transfer and the Manager branches on them, +// so an implementation must be able to say them: ErrNotWriter, which stops the volume, +// and ErrRootSuperseded, which abandons a collapse. Any other error is a publish that did +// not land and is retried. They are exported for the same reason ErrNoHistory is — a +// Publisher living in another module has no other way to say either sentence, and an +// implementation that cannot say ErrNotWriter leaves a fenced host serving a guest whose +// writes can never be published. type Publisher interface { Publish(ctx context.Context, layer SealedLayer) error } +// ErrNotWriter means the compare-and-set on HEAD lost: something else published for this +// volume, so this host is not its writer any more. It is the one publishing failure that +// stops the volume rather than being retried — a host that goes on taking writes it can +// never publish is the fencing defect §15 is about. +// +// It *is* internal/commit's ErrHeadMoved, under this package's name; ErrNoHistory carries +// why one value under two names is the shape and two values is not. +var ErrNotWriter = commit.ErrHeadMoved + +// ErrRootSuperseded means a compacted root was published for a HEAD that has moved on: +// the flattened bytes reconstruct a commit that is no longer the newest, so taking them +// would move HEAD backwards over a commit that returned SUCCESS. Nobody else took the +// volume — that is ErrNotWriter — so this host goes on serving it and plans the next +// collapse over the history as it is now. +// +// It *is* internal/commit's ErrRootSuperseded, under the same name. +var ErrRootSuperseded = commit.ErrRootSuperseded + // Deps are the Manager's injected collaborators (INV-01). +// +// **Three of them cannot be supplied from outside this module today, so Manager is not +// yet the facade ADR-0021 §4 promises spin's runner — Open and the chain functions are.** +// Clock and Disk are internal/simio interfaces whose own methods return internal types +// (clock.Instant, clock.Timer, disk.File, disk.Usage), so no implementation of them can +// be written elsewhere, and internal/simio/real — the implementations a real host wants +// — is unreachable for the same reason. Dialer is nameable in shape but lives internal +// too. Closing this means deciding where the simio boundary sits relative to the public +// surface, which is a decision of its own and not a doc comment's to make; it is not +// closed by re-exporting, because what a consumer needs is the *real* implementations and +// those are the ones INV-01 confines. +// +// Publisher, Witness, Recovery, Runner and Paths are the ones that were designed to be +// implemented by a caller, and they are: every type in their signatures is public, and +// every sentinel their errors must carry is exported by this package. type Deps struct { Clock clock.Clock Disk disk.Disk @@ -824,7 +865,7 @@ func (m *Manager) publish(ctx context.Context, v *volume) error { return m.refuse(v, storagev1.VolumeRefusal_VOLUME_REFUSAL_DURABILITY_LOST, err) } if err := m.pub.Publish(ctx, layer); err != nil { - if errors.Is(err, commit.ErrHeadMoved) { + if errors.Is(err, ErrNotWriter) { // Another host published for this volume, so this one is not its writer: it // would keep accepting writes that can never be published. That is the one // publishing failure that stops the volume. @@ -1517,6 +1558,9 @@ type volumeGap struct { } // Volumes reports what this host is holding, ordered by volume id (deterministic). +// +// It returns internal/agent's type, which a caller outside this module can read but +// cannot name — part of the same gap Deps carries, and it moves when that one does. func (m *Manager) Volumes(context.Context) ([]agent.VolumeStatus, error) { m.mu.Lock() defer m.mu.Unlock()