From 84460abc221e29c47d90d6eb2e7b716c768e4574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 11:53:46 +0100 Subject: [PATCH 1/7] feat(cli): the observable state a reset will be judged against (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first slice of #853, and deliberately not a command. The issue refuses the shortcut in one line — "reset is not successful because JSON was restored" — so the verdict has to be a COMPARISON, and this is the thing compared. Writing `feint checkpoint` and `feint reset` first would have produced two commands whose success was their own opinion of themselves, which is the verdict this repository exists to refuse. ## Three halves, and why any two of them can lie - the control plane, from `/_feint/resources`: what the API says exists; - the bytes, read THROUGH the doors: what a client can fetch; - the host, from `Runtime.Survey`: what the emulator actually created. Restoring the store alone leaves containers running that the API no longer mentions. Restoring the store and the host leaves bytes from a later test in a bucket the checkpoint had empty. The issue's own list of guarantees names all three, so the comparison does too. `Survey` is reused rather than reimplemented: it already names only what the emulator labelled, it already issues no mutating command, and its own test holds that. A second traversal here would be a second place for the ownership rule to drift, on the one layer where that costs an operator their own containers. ## Why the bytes go through the door The authoritative view of an object is the one a client gets. A comparison reading the store directly would pass while the door served something else, which is the half that matters to a test using it. It does make the comparison depend on the door answering, and that is not a circularity worth avoiding: bytes a door cannot serve are bytes no client can observe, and a reset whose doors are broken has not converged. The listing paginates. A comparison that read the first page would report two different stores as equal, which is a verdict measuring its own window rather than its subject. ## What is deliberately left out Created and Updated, because a restored resource carries the instant it was restored and the comparison could then never be equal. Attrs field by field, because a pack's view can hold a derived value and a reset is about the state, not about how a pack renders it. What IS carried, and was nearly not, is the backing host object behind each record: a resource whose container changed under it is a resource a reset did not reconcile, even when both lists are the same length. It is read as the whole Runtime map sorted, not from a known key, because three packs spell that key differently and a hardcoded one would silently observe nothing for the fourth. ## The two tests, and what each would miss alone TestTwoReadsOfOneLabAreEqual drives the reader against a fake lab whose resources come back out of order and whose objects come from a Go map, which has no order at all. It then asserts that something WAS observed: two empty states compare equal too, and would pass a stability test while measuring nothing. TestADifferenceIsNamedWhereverItIs puts one difference on every axis at once, in both directions, because #853 asks a reset to report "exactly what could not be reconciled". A verdict that found the extra container but not the changed digest would send an operator round the loop once per problem. It also asserts that an axis which CONVERGED says nothing: a verdict listing what worked buries what did not. And that a state does not differ from itself, which is what will make a second reset a no-op. Two of those assertions were earned rather than foreseen: the digest is compared instead of the size because two objects of one size are not one object, and the resource state is compared because a reset that left a stopped server stopped has not converged. Not here yet: `feint checkpoint`, `feint reset`, `feint run --reset-after`, and the reconciliation itself. The host half is exercised through the injected surveyor only; a real runtime comes with the commands that act on it. mise run prepush green. Assisted-by: Claude Code (claude-opus-5) --- internal/cli/observable.go | 373 ++++++++++++++++++++++++++++++++ internal/cli/observable_test.go | 192 ++++++++++++++++ 2 files changed, 565 insertions(+) create mode 100644 internal/cli/observable.go create mode 100644 internal/cli/observable_test.go diff --git a/internal/cli/observable.go b/internal/cli/observable.go new file mode 100644 index 00000000..7c607f62 --- /dev/null +++ b/internal/cli/observable.go @@ -0,0 +1,373 @@ +package cli + +import ( + "context" + "encoding/json" + "encoding/xml" + "fmt" + "io" + "net/http" + "slices" + "strings" + "time" + + "github.com/stephrobert/feint/internal/core/machine" +) + +// The observable state of a lab, normalised so that two reads of the same +// state are equal. +// +// This exists before `feint checkpoint` and `feint reset` (#853) because +// without it those two commands cannot be honest. The issue says so in one +// line — "reset is not successful because JSON was restored" — and a reset +// that reported success from the absence of an error would be exactly the +// verdict this repository refuses: an operation's own opinion of itself. +// +// So the verdict is a COMPARISON, and this file is the thing compared. +// +// ## What is in it, and why those three +// +// - the control plane, from /_feint/resources: what the API says exists; +// - the bytes, read THROUGH the doors: what a client can fetch; +// - the host, from Runtime.Survey: what the emulator actually created. +// +// Any two of the three without the third is a reset that can lie. Restoring +// the store alone leaves containers running that the API no longer mentions; +// restoring the store and the host leaves bytes from a later test in a bucket +// the checkpoint had empty. +// +// ## Why the bytes are read through the door rather than from the store +// +// The authoritative view of an object is the one a client gets, and this +// project's whole claim is that a client cannot tell the difference. A +// comparison that read the store directly would pass while the door served +// something else, which is the half that matters to a test using it. +// +// It does mean the comparison depends on the door answering. That is not a +// circularity worth avoiding: bytes a door cannot serve are bytes no client +// can observe, and a reset whose doors are broken has not converged. +// +// ## What is deliberately NOT in it +// +// Created and Updated timestamps, because a restored resource carries the +// instant it was restored and a comparison including them could never be +// equal. What survives of time is ordering, and the store already keeps that +// in the identifiers. Attrs are not compared field by field either: a pack's +// own view can hold a derived value, and a reset is about the state, not about +// how a pack renders it. The identity, the kind and the state are what a test +// builds on. +// +// TestTwoReadsOfOneLabAreEqual and TestADifferenceIsNamedWhereverItIs fail +// without this. + +// observed is one lab's normalised state. +type observed struct { + Resources []observedResource `json:"resources"` + Objects []observedObject `json:"objects"` + Machines []string `json:"machines"` + Networks []string `json:"networks"` + Firewalls []string `json:"firewalls"` +} + +// observedResource is a control-plane record, without what cannot be equal. +type observedResource struct { + Kind string `json:"kind"` + ID string `json:"id"` + State string `json:"state,omitempty"` + // Backing is the host object this record names, and it is what ties the + // two halves together: a resource whose container changed under it is a + // resource a reset did not reconcile, even when both lists are the same + // length. + Backing string `json:"backing,omitempty"` +} + +// observedObject is one object, identified by its content rather than by a +// length: two objects of the same size are not the same object, and a reset +// that swapped them would otherwise compare equal. +type observedObject struct { + Door string `json:"door"` + Bucket string `json:"bucket"` + Key string `json:"key"` + Digest string `json:"digest"` + Size int64 `json:"size"` +} + +// observeLab reads the three halves and normalises them. +// +// A runtime that answers nothing is not an error: `--vm off` is the ordinary +// case and its host half is legitimately empty. An unreachable emulator IS an +// error, because then nothing was observed and a comparison would be between +// two silences. +func observeLab(ctx context.Context, endpoint string, rt machine.Runtime) (observed, error) { + var state observed + + inventory, err := readInventoryView(endpoint) + if err != nil { + return observed{}, fmt.Errorf("reading the control plane: %w", err) + } + for _, view := range inventory { + state.Resources = append(state.Resources, observedResource{ + Kind: view.Kind, ID: view.ID, State: view.State, + Backing: backingOf(view.Runtime), + }) + } + + objects, err := observeObjects(ctx, endpoint) + if err != nil { + return observed{}, fmt.Errorf("reading the bytes: %w", err) + } + state.Objects = objects + + // Survey names only what the emulator labelled, and its own test asserts + // it issues no mutating command. A driver that cannot survey answers + // asked=false, and the host half stays empty rather than guessed. + if left, asked, err := rt.Survey(ctx); err != nil { + return observed{}, fmt.Errorf("surveying the host: %w", err) + } else if asked { + state.Machines = append(state.Machines, left.Machines...) + state.Networks = append(state.Networks, left.Networks...) + state.Firewalls = append(state.Firewalls, left.Firewalls...) + } + + state.normalise() + return state, nil +} + +// normalise sorts every list so that two reads of one state are equal. +func (o *observed) normalise() { + slices.SortFunc(o.Resources, func(a, b observedResource) int { + if c := strings.Compare(a.Kind, b.Kind); c != 0 { + return c + } + return strings.Compare(a.ID, b.ID) + }) + slices.SortFunc(o.Objects, func(a, b observedObject) int { + for _, pair := range [][2]string{{a.Door, b.Door}, {a.Bucket, b.Bucket}, {a.Key, b.Key}} { + if c := strings.Compare(pair[0], pair[1]); c != 0 { + return c + } + } + return 0 + }) + slices.Sort(o.Machines) + slices.Sort(o.Networks) + slices.Sort(o.Firewalls) +} + +// backingOf answers the host object a record names, whatever key the pack +// used for it. +// +// Sorted and joined rather than read from one known key: the Runtime map is a +// pack's own vocabulary, three packs spell it differently, and a hardcoded key +// here would silently observe nothing for the fourth pack. What matters for a +// comparison is that the value changes when the host object changes. +func backingOf(runtime map[string]string) string { + if len(runtime) == 0 { + return "" + } + pairs := make([]string, 0, len(runtime)) + for key, value := range runtime { + pairs = append(pairs, key+"="+value) + } + slices.Sort(pairs) + return strings.Join(pairs, " ") +} + +// readInventoryView fetches /_feint/resources with the fields a comparison +// needs. up.go's readInventory drops the identity and the state, which are the +// two a reset is judged on, so this reads the fuller view rather than widening +// a struct three ready conditions already depend on. +func readInventoryView(endpoint string) ([]emulatorResourceView, error) { + client := &http.Client{Timeout: 10 * time.Second} + res, err := client.Get(strings.TrimSuffix(endpoint, "/") + "/_feint/resources") + if err != nil { + return nil, err + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return nil, fmt.Errorf("the inventory answered %d", res.StatusCode) + } + var body struct { + Resources []emulatorResourceView `json:"resources"` + } + if err := json.NewDecoder(io.LimitReader(res.Body, maxStateBody)).Decode(&body); err != nil { + return nil, err + } + return body.Resources, nil +} + +// emulatorResourceView is the part of emulator.ResourceView a comparison uses. +// Declared here rather than imported so that a field added upstream cannot +// silently join the comparison: a reset's verdict must change only when +// somebody decides it does. +type emulatorResourceView struct { + ID string `json:"id"` + Kind string `json:"kind"` + State string `json:"state"` + Runtime map[string]string `json:"runtime"` +} + +// observeObjects lists every door's buckets and their objects, through the +// doors. +// +// An emulator serving no doors answers an empty list and no error: object +// storage is opt-in, and a lab without it has no bytes to converge. +func observeObjects(ctx context.Context, endpoint string) ([]observedObject, error) { + report, err := fetchS3(endpoint) + if err != nil { + return nil, err + } + if !report.Enabled || len(report.Doors) == 0 { + return nil, nil + } + client := &http.Client{Timeout: 30 * time.Second} + var out []observedObject + for _, door := range report.Doors { + base := strings.TrimSuffix(endpoint, "/") + door.Path + buckets, err := listBuckets(ctx, client, base) + if err != nil { + return nil, fmt.Errorf("%s: %w", door.Path, err) + } + for _, bucket := range buckets { + objects, err := listObjects(ctx, client, base, bucket) + if err != nil { + return nil, fmt.Errorf("%s%s: %w", door.Path, bucket, err) + } + for _, object := range objects { + out = append(out, observedObject{ + Door: strings.Trim(door.Path, "/"), Bucket: bucket, + Key: object.Key, Digest: object.ETag, Size: object.Size, + }) + } + } + } + return out, nil +} + +type bucketListing struct { + Buckets struct { + Bucket []struct { + Name string `xml:"Name"` + } `xml:"Bucket"` + } `xml:"Buckets"` +} + +type objectListing struct { + Contents []struct { + Key string `xml:"Key"` + ETag string `xml:"ETag"` + Size int64 `xml:"Size"` + } `xml:"Contents"` + IsTruncated bool `xml:"IsTruncated"` + NextToken string `xml:"NextContinuationToken"` +} + +func listBuckets(ctx context.Context, client *http.Client, base string) ([]string, error) { + var listing bucketListing + if err := getXML(ctx, client, base, &listing); err != nil { + return nil, err + } + names := make([]string, 0, len(listing.Buckets.Bucket)) + for _, bucket := range listing.Buckets.Bucket { + names = append(names, bucket.Name) + } + slices.Sort(names) + return names, nil +} + +func listObjects(ctx context.Context, client *http.Client, base, bucket string) ([]struct { + Key string `xml:"Key"` + ETag string `xml:"ETag"` + Size int64 `xml:"Size"` +}, error, +) { + var all []struct { + Key string `xml:"Key"` + ETag string `xml:"ETag"` + Size int64 `xml:"Size"` + } + // Paginated, because a comparison that read the first page only would + // report two different stores as equal — the exact shape of a verdict + // measuring its own window rather than its subject. + token := "" + for page := 0; page < 1000; page++ { + url := base + bucket + "?list-type=2" + if token != "" { + url += "&continuation-token=" + token + } + var listing objectListing + if err := getXML(ctx, client, url, &listing); err != nil { + return nil, err + } + all = append(all, listing.Contents...) + if !listing.IsTruncated || listing.NextToken == "" { + return all, nil + } + token = listing.NextToken + } + return nil, fmt.Errorf("%s did not stop paginating", bucket) +} + +func getXML(ctx context.Context, client *http.Client, url string, into any) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return err + } + res, err := client.Do(req) + if err != nil { + return err + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return fmt.Errorf("answered %d", res.StatusCode) + } + return xml.NewDecoder(io.LimitReader(res.Body, maxStateBody)).Decode(into) +} + +// diff names every difference, in both directions. +// +// Every one of them, and not the first: a reset reports exactly what it could +// not reconcile (#853), and a verdict that stopped at the first difference +// would send an operator round the loop once per problem. The wording says +// which side a thing is on, because "extra" and "missing" need opposite fixes. +func (o observed) diff(other observed) []string { + var out []string + + resourceKey := func(r observedResource) string { + return r.Kind + " " + r.ID + " state=" + r.State + " backing=[" + r.Backing + "]" + } + objectKey := func(ob observedObject) string { + return ob.Door + "/" + ob.Bucket + "/" + ob.Key + " digest=" + ob.Digest + } + + out = append(out, compareSets("resource", mapAll(o.Resources, resourceKey), mapAll(other.Resources, resourceKey))...) + out = append(out, compareSets("object", mapAll(o.Objects, objectKey), mapAll(other.Objects, objectKey))...) + out = append(out, compareSets("machine", o.Machines, other.Machines)...) + out = append(out, compareSets("network", o.Networks, other.Networks)...) + out = append(out, compareSets("firewall", o.Firewalls, other.Firewalls)...) + return out +} + +func mapAll[T any](items []T, key func(T) string) []string { + out := make([]string, 0, len(items)) + for _, item := range items { + out = append(out, key(item)) + } + return out +} + +// compareSets answers what is extra on the right and what is missing from it. +func compareSets(kind string, want, got []string) []string { + var out []string + for _, item := range got { + if !slices.Contains(want, item) { + out = append(out, kind+" present and not in the checkpoint: "+item) + } + } + for _, item := range want { + if !slices.Contains(got, item) { + out = append(out, kind+" in the checkpoint and not present: "+item) + } + } + return out +} diff --git a/internal/cli/observable_test.go b/internal/cli/observable_test.go new file mode 100644 index 00000000..4c8d4408 --- /dev/null +++ b/internal/cli/observable_test.go @@ -0,0 +1,192 @@ +package cli + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/s3" +) + +// s3ReportVersionForTest keeps the fake honest: the reader refuses any other +// version, so the fake follows the constant rather than a copy of its value. +var s3ReportVersionForTest = fmt.Sprint(s3.ReportVersion) + +// The observable state a reset is judged against (#853). +// +// These tests exist before the commands do, and that order is the point: the +// issue refuses a reset that reports success because no error occurred — +// "reset is not successful because JSON was restored" — so the verdict has to +// be a comparison, and a comparison is only worth the normalisation under it. + +// fakeLab serves the three endpoints observeLab reads, with the bytes of one +// door, so that the reader can be driven without starting an emulator (`start` +// re-execs os.Executable(), which under `go test` is the test binary). +func fakeLab(t *testing.T, objects map[string]map[string]string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"count":2,"resources":[ + {"id":"b","kind":"instance/server","state":"running", + "runtime":{"container":"feint-scw-b"},"created":"2026-11-11T09:00:00Z"}, + {"id":"a","kind":"instance/ip","state":"available","created":"2026-11-11T09:00:01Z"}]}`)) + }) + mux.HandleFunc("/_feint/s3", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // The whole report, because fetchS3 validates it: a version, a + // non-nil request list and limits that agree with the activation. + // A fake that answered less would fail on the reader's own guard + // rather than on what this test is about. + _, _ = w.Write([]byte(`{"schema_version":` + s3ReportVersionForTest + `, + "enabled":true,"max_blob_bytes":1073741824,"max_object_bytes":268435456, + "http_requests":{"GET":1,"PUT":1}, + "doors":[{"provider":"scaleway","region":"fr-par", + "path":"/_feint/s3/scaleway/fr-par/"}]}`)) + }) + mux.HandleFunc("/_feint/s3/scaleway/fr-par/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/xml") + rest := strings.TrimPrefix(r.URL.Path, "/_feint/s3/scaleway/fr-par/") + if rest == "" { + var body strings.Builder + body.WriteString("") + for bucket := range objects { + body.WriteString("" + bucket + "") + } + body.WriteString("") + _, _ = w.Write([]byte(body.String())) + return + } + var body strings.Builder + body.WriteString("false") + for key, digest := range objects[strings.TrimSuffix(rest, "/")] { + body.WriteString("" + key + "" + digest + + "7") + } + body.WriteString("") + _, _ = w.Write([]byte(body.String())) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// TestTwoReadsOfOneLabAreEqual is cited by the observable state's doc. +// +// Without this there is no verdict at all: a comparison whose two sides +// disagree about ordering reports a difference on every reset and an operator +// learns to ignore it. The fake answers its resources out of order and its +// objects from a map, which has no order at all, precisely so the sort is the +// thing under test. +func TestTwoReadsOfOneLabAreEqual(t *testing.T) { + lab := fakeLab(t, map[string]map[string]string{ + "images": {"disk.qcow2": "d41d8", "boot.img": "a1b2c"}, + "backup": {"db.sql": "99aa0"}, + }) + rt := machine.Use(surveyingDriver{left: machine.Leftovers{ + Machines: []string{"feint-scw-b", "feint-scw-a"}, + Networks: []string{"scw-net-2", "scw-net-1"}, + }}) + + first, err := observeLab(t.Context(), lab.URL, rt) + if err != nil { + t.Fatalf("first read: %v", err) + } + second, err := observeLab(t.Context(), lab.URL, rt) + if err != nil { + t.Fatalf("second read: %v", err) + } + if diff := first.diff(second); len(diff) != 0 { + t.Fatalf("two reads of one lab differ:\n%s", strings.Join(diff, "\n")) + } + + // And it actually observed something: a comparison between two empty + // states is equal too, and would pass this test while measuring nothing. + if len(first.Resources) != 2 { + t.Errorf("resources %+v", first.Resources) + } + if len(first.Objects) != 3 { + t.Errorf("objects %+v", first.Objects) + } + if len(first.Machines) != 2 || len(first.Networks) != 2 { + t.Errorf("host half %v %v", first.Machines, first.Networks) + } + // Sorted, not merely stable: the identity of a resource is its kind and + // id, and a reader of the JSON must not have to sort it again. + if first.Resources[0].ID != "a" || first.Objects[0].Bucket != "backup" { + t.Errorf("not sorted: %+v %+v", first.Resources, first.Objects) + } + // The backing is carried, because a resource whose container changed + // under it is a resource a reset did not reconcile even when the lists + // are the same length. + if first.Resources[1].Backing != "container=feint-scw-b" { + t.Errorf("backing %q", first.Resources[1].Backing) + } +} + +// TestADifferenceIsNamedWhereverItIs is cited by the observable state's doc. +// +// #853 requires a reset to report "exactly what could not be reconciled", so +// one difference per axis is asserted here, in both directions. A verdict that +// found the extra container but not the changed digest would send an operator +// round the loop once per problem, and one that stopped at the first would do +// it every time. +func TestADifferenceIsNamedWhereverItIs(t *testing.T) { + checkpoint := observed{ + Resources: []observedResource{{Kind: "instance/server", ID: "a", State: "running"}}, + Objects: []observedObject{{Door: "d", Bucket: "b", Key: "k", Digest: "before"}}, + Machines: []string{"feint-scw-a"}, + Networks: []string{"scw-net-1"}, + Firewalls: []string{"scw-acl-1"}, + } + now := observed{ + // The state moved, which is a difference even though the resource is + // the same one: a reset that left a stopped server stopped has not + // converged. + Resources: []observedResource{ + {Kind: "instance/server", ID: "a", State: "stopped"}, + {Kind: "instance/ip", ID: "extra", State: "available"}, + }, + // Same door, same bucket, same key, different content. A comparison + // on sizes would call these equal. + Objects: []observedObject{{Door: "d", Bucket: "b", Key: "k", Digest: "after"}}, + Machines: []string{"feint-scw-a", "feint-scw-extra"}, + Networks: nil, + Firewalls: []string{"scw-acl-1"}, + } + + diff := checkpoint.diff(now) + joined := strings.Join(diff, "\n") + for _, want := range []string{ + "state=stopped", // the moved state + "instance/ip extra", // the resource nobody checkpointed + "digest=after", // the content that changed + "digest=before", // and the content that is gone + "feint-scw-extra", // the machine in excess + "scw-net-1", // the network a reset failed to bring back + } { + if !strings.Contains(joined, want) { + t.Errorf("the verdict never mentions %q:\n%s", want, joined) + } + } + // Both directions are worded apart, because "extra" and "missing" need + // opposite fixes and a single word for both sends the reader the wrong way. + if !strings.Contains(joined, "present and not in the checkpoint") || + !strings.Contains(joined, "in the checkpoint and not present") { + t.Errorf("the verdict does not say which side a thing is on:\n%s", joined) + } + // And an unchanged axis says nothing at all: a verdict that listed the + // firewall that did converge would bury the five that did not. + if strings.Contains(joined, "scw-acl-1") { + t.Errorf("an axis that converged was reported anyway:\n%s", joined) + } + + // Equal states produce no verdict, which is the half that makes a second + // reset a no-op (#853). + if diff := checkpoint.diff(checkpoint); len(diff) != 0 { + t.Errorf("a state differs from itself:\n%s", strings.Join(diff, "\n")) + } +} From 2f7c9571cd895004ce3366912e552138f1323e9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 12:00:07 +0100 Subject: [PATCH 2/7] feat(machine): a store that changed under the host can be reconciled (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gesture #853 needs and nothing above it yet. `PUT /_feint/state` and `feint snapshot load` restore the control plane verbatim and touch nothing else, so a resource that disappears leaves its container running while the API no longer mentions it. Nobody destroys it, and the next `incus list` is the first time anybody finds out. ## Why the destroying half is here and the recreating half is not Destroying is the same gesture for every pack: take the machine the record names, check it is ours, remove it. Written three times it would be fixed once, which is the defect CLAUDE.md names and this repository has already paid for on the lifecycle itself. Recreating is NOT the same gesture. Starting a machine needs a Boot — the image, the login, the cloud-init a pack renders for its own product — and that is pack knowledge by construction. #853 hedges in the same place with "missing checkpoint resources recreated where the runtime contract supports it", so that half belongs to each pack and is bounded by what that pack can boot. It is sequenced after this, not dropped. ## The guard I nearly wrote twice The first draft refused a machine name outside the binding's prefix. That is a second guard beside a working one: `Remove` already asks the ownership question, and `ours` already decided what the answer DOES — fall back to the name the emulator could have created rather than refuse, because a record may well have a machine and that is the only name it could ever have had. Keeping my version would have changed that decision in one path only, and two answers to one question are how they start disagreeing. The test asserts the existing behaviour instead: a crafted `production-database` reaches no command as itself, and the derived name is removed, so a tampered record does not leave its real container running either. ## Four populations, and the default is one of them - a record with a machine of ours: destroyed, and the names reaching the driver are asserted rather than the return value — a reconciliation is judged by the commands it emits; - a record with no machine: not an event at all, which is the ordinary case for an address, a volume or a security group. Reporting it would bury the machines under records that never had one; - a foreign name: no command carries it; - **no runtime: nothing destroyed, and said so through Skipped.** Metadata only is the default, so this is the common case. An empty Destroyed read as success is how a caller comes to believe containers were removed on a station that never started any. Not here: the emulator does not call this yet, and nothing computes the "gone" list. That is the next slice, with `feint checkpoint` and `feint reset` on top of the comparison the previous commit added. mise run prepush green. Assisted-by: Claude Code (claude-opus-5) --- internal/core/machine/reconcile.go | 100 +++++++++++++++++++++ internal/core/machine/reconcile_test.go | 114 ++++++++++++++++++++++++ 2 files changed, 214 insertions(+) create mode 100644 internal/core/machine/reconcile.go create mode 100644 internal/core/machine/reconcile_test.go diff --git a/internal/core/machine/reconcile.go b/internal/core/machine/reconcile.go new file mode 100644 index 00000000..1dafabc7 --- /dev/null +++ b/internal/core/machine/reconcile.go @@ -0,0 +1,100 @@ +package machine + +import ( + "context" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// Reconciling the host with a store that just changed under it (#853). +// +// `PUT /_feint/state` and `feint snapshot load` restore the control plane +// verbatim and touch nothing else, which is the gap #853 names: a resource +// that disappears from the store leaves its container running, and the API no +// longer mentions it. Nobody destroys it, and the next `incus list` is the +// first time anybody finds out. +// +// ## Why the destroying half lives here and the recreating half does not +// +// Destroying is the same gesture for every pack: take the machine the record +// names, check it is ours, remove it. Written three times it would be fixed +// once, which is the defect CLAUDE.md names and this repository has already +// paid for on the lifecycle. +// +// Recreating is NOT the same gesture. Starting a machine needs a Boot — the +// image, the login, the cloud-init a pack renders for its own product — and +// that is pack knowledge by construction. #853 hedges in the same place, with +// "missing checkpoint resources recreated where the runtime contract supports +// it", so the recreating half belongs to each pack and is bounded by what that +// pack can actually boot. +// +// ## Why it goes through Remove rather than the driver +// +// Remove answers the ownership question before the shape one: `Binding.ours` +// refuses a machine name outside this binding's prefix, and the driver's +// mustOwn reads the label the emulator itself wrote. A restored store is +// untrusted input — snapshot.go documents the format as meant to outlive its +// instance — so a reconciliation driven by names that came out of a file is +// exactly the path that must not shortcut either check. + +// Reconciled reports what a reconciliation did, so a caller can say it rather +// than assume it. +type Reconciled struct { + // Destroyed names the machines taken down because their record is gone. + Destroyed []string + // Skipped names records whose machine this binding does not own, or which + // name no machine at all. Reported rather than silently passed over: a + // record nobody could act on is what an operator needs to hear about. + Skipped []string +} + +// ReconcileGone destroys the machines of records that no longer exist. +// +// The input is what the store held BEFORE the restore and no longer holds. The +// caller computes it, because only the caller saw both sides, and it is a +// plain list of resources so that nothing about a provider reaches this +// package. +// +// A binding with no runtime destroys nothing and says so through Skipped: the +// metadata-only mode is the default, and a reconciliation that claimed to have +// removed containers nobody started would be the lie this project exists to +// avoid. +// +// There is no ownership branch here, and that absence is deliberate: Remove +// already asks the question, and `ours` already decided what the answer does +// — fall back to the derived name rather than refuse. A second guard beside a +// working one is how two answers to one question start disagreeing. +// +// TestReconcileDestroysOnlyTheMachinesItOwns fails without this. +func (b Binding) ReconcileGone(ctx context.Context, gone []*resource.Resource) Reconciled { + var done Reconciled + for _, res := range gone { + if res == nil { + continue + } + name := res.Runtime[b.RuntimeKey] + if name == "" { + // Nothing was ever started for it. Not an error and not a skip + // worth reporting: a record with no machine is the ordinary case + // for an address, a volume or a security group. + continue + } + if b.driver == nil { + // Metadata only, which is the default: nothing was started, so + // nothing is destroyed and the caller is told rather than left to + // read an empty Destroyed as a success. + done.Skipped = append(done.Skipped, name) + continue + } + // Remove answers the ownership question itself, and answers it the way + // this package already decided: `ours` does not refuse a foreign name, + // it falls back to the one the emulator could have created, because a + // record may well have a machine and that is the only name it could + // ever have had. An ownership branch here would have been a second + // guard beside a working one, which is how two answers to one question + // start disagreeing. + b.Remove(ctx, res.ID, name) + done.Destroyed = append(done.Destroyed, name) + } + return done +} diff --git a/internal/core/machine/reconcile_test.go b/internal/core/machine/reconcile_test.go new file mode 100644 index 00000000..dba0152c --- /dev/null +++ b/internal/core/machine/reconcile_test.go @@ -0,0 +1,114 @@ +package machine + +import ( + "context" + "slices" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// removingDriver records the names a reconciliation asked to remove, which is +// the only thing worth asserting here: a reconciliation is judged by the +// commands it emits, not by what it returns about itself. +type removingDriver struct { + recordingDriver + removed []string +} + +func (d *removingDriver) Remove(_ context.Context, name string) error { + d.removed = append(d.removed, name) + return nil +} + +func record(id, machine string) *resource.Resource { + res := resource.New(id, "instance/server", + resource.Tenant{Provider: "acme", Project: "p", Zone: "z"}, "running", + time.Unix(1700000000, 0).UTC()) + if machine != "" { + res.Runtime = map[string]string{"machine": machine} + } + return res +} + +// TestReconcileDestroysOnlyTheMachinesItOwns is cited by ReconcileGone. +// +// #853's gap in one sentence: a restore brings the control plane back and +// touches nothing else, so a resource that disappeared leaves its container +// running and the API no longer mentions it. This is the gesture that closes +// it, and the four populations below are the ones that can go wrong. +func TestReconcileDestroysOnlyTheMachinesItOwns(t *testing.T) { + t.Run("a record with a machine of ours is destroyed", func(t *testing.T) { + d := &removingDriver{} + done := bootBinding(d).ReconcileGone(t.Context(), []*resource.Resource{ + record("a", "feint-acme-a"), + record("b", "feint-acme-b"), + }) + if !slices.Equal(d.removed, []string{"feint-acme-a", "feint-acme-b"}) { + t.Errorf("removed %v", d.removed) + } + if len(done.Destroyed) != 2 || len(done.Skipped) != 0 { + t.Errorf("reported %+v", done) + } + }) + + t.Run("a record with no machine is not an event", func(t *testing.T) { + // The ordinary case for an address, a volume or a security group. + // Reporting it would bury the machines under records that never had + // one. + d := &removingDriver{} + done := bootBinding(d).ReconcileGone(t.Context(), []*resource.Resource{record("a", "")}) + if len(d.removed) != 0 { + t.Errorf("removed %v for a record that named no machine", d.removed) + } + if len(done.Destroyed) != 0 || len(done.Skipped) != 0 { + t.Errorf("reported %+v", done) + } + }) + + t.Run("a foreign name reaches no command as itself", func(t *testing.T) { + // A restored store is untrusted input: snapshot.go documents the + // format as meant to outlive its instance, and an audit turned a + // crafted one into a forced removal of a name the emulator never + // created. The reconciliation must not be the path that reopens it. + d := &removingDriver{} + bootBinding(d).ReconcileGone(t.Context(), []*resource.Resource{ + record("a", "production-database"), + }) + if slices.Contains(d.removed, "production-database") { + t.Fatalf("a name outside this binding's prefix reached a removal: %v", d.removed) + } + // And it did not become a silent no-op either: the derived name is the + // only machine that record could ever have had, so that is what is + // removed. A reconciliation that skipped it would leave the real + // container of a tampered record running. + if !slices.Equal(d.removed, []string{"feint-acme-a"}) { + t.Errorf("removed %v, want the derived name", d.removed) + } + }) + + t.Run("no runtime destroys nothing and says so", func(t *testing.T) { + // Metadata only is the DEFAULT, so this is the common case rather + // than an edge one. An empty Destroyed read as success is how a + // caller comes to believe containers were removed on a station that + // never started any. + done := bootBinding(nil).ReconcileGone(t.Context(), []*resource.Resource{ + record("a", "feint-acme-a"), + }) + if len(done.Destroyed) != 0 { + t.Errorf("a binding with no runtime claims to have destroyed %v", done.Destroyed) + } + if !slices.Equal(done.Skipped, []string{"feint-acme-a"}) { + t.Errorf("skipped %v", done.Skipped) + } + }) + + t.Run("nothing to reconcile emits nothing", func(t *testing.T) { + d := &removingDriver{} + done := bootBinding(d).ReconcileGone(t.Context(), nil) + if len(d.removed) != 0 || len(done.Destroyed) != 0 || len(done.Skipped) != 0 { + t.Errorf("an empty reconciliation did something: %v %+v", d.removed, done) + } + }) +} From 90f2a60cdcaed70a298e537ea8149d09d76ebbf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 12:08:50 +0100 Subject: [PATCH 3/7] feat(emulator): a lab takes a named checkpoint and resets to it (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lifecycle guarantee #853 asks for, on top of two storage primitives that had no caller: `feint snapshot` persists the control plane and `objectstore.SaveCheckpoint` persists bytes, and until now nothing put them together. Putting them together is the whole point, and the issue says what separates the two in one line — "reset is not successful because JSON was restored". POST /_feint/checkpoint/{name} DELETE /_feint/checkpoint/{name} POST /_feint/reset/{name} ## Why the reset is server-side Restoring the store is the easy half. The half that matters is the host: a resource that disappears in a restore leaves its container running while the API no longer mentions it, and nobody takes it down. Only this process holds both the store and the packs' bindings, so only this process can act on the difference. A CLI-side reset would have to name each pack's delete route, which is provider knowledge in the one place this repository keeps free of it. ## Why the VERDICT is not server-side This answers what it did. Whether the lab converged is a comparison of what a client can observe, and that belongs to whoever asked — the observable state two commits ago. An operation that graded itself is the verdict this repository refuses everywhere else. ## The order inside a reset, and why it is not interchangeable 1. read what the store holds NOW, because after step 2 it is gone and the machines it named would be unreachable; 2. restore the control plane, so the target is in place before anything is destroyed — a failure here leaves the host untouched rather than half swept; 3. restore the bytes, which objectstore does atomically; 4. take down the machines of records that no longer exist, through each pack's own binding and therefore through the ownership checks. The difference is computed by IDENTITY, not by count: a restore that replaced one server with another of the same kind leaves the count equal and the host wrong. ## The seam, and what the packs had to write `Reconciler` is an optional pack half, in the shape the others use (EnvHazards, StackHazards, FirewallEnforcer): a pack that drives no machine does not implement it, and the core holds no list of which do. Each pack sees only its own gone resources, so the core stays a loop over whatever is mounted. Each pack wrote two lines. That is the measure of whether the shared gesture was put in the right place, and `machine.PackSurface` now names `Binding.ReconcileGone` and `Reconciled` with their reasons — the discipline gate refused the three packs until it did, which is it working. ## What a reset does NOT do, said on the wire It does not recreate a machine the checkpoint had and the host no longer does. Starting one needs a Boot — an image, a login, the cloud-init a pack renders — and #853 bounds that in the same words, "where the runtime contract supports it". So the answer carries `recreated: []` and `recreates: false` rather than leaving a caller to infer it from a quiet success. That half is sequenced next, per pack. ## Tests, and what each would miss alone TestACheckpointTakesBothHalvesOrNeither: both halves or neither, because a checkpoint whose bytes were not taken would fail later, somewhere else, with no mention of this call. It also asserts the answer says whether bytes were part of it, and that a name which is a path component before it is a name — "..", "a/b" — is refused at the door, since the byte store writes a directory per checkpoint. TestAResetDestroysWhatTheCheckpointDoesNotHold asserts on what the pack WAS ASKED to take down, never on the status code. It mounts a second pack that cannot reconcile, because that is the ordinary case and the loop must skip it rather than fail; it checks the kept resource was NOT handed over, which is the direction that would destroy a machine the checkpoint expects; it runs reset twice and requires the second to ask for nothing, which is the issue's own no-op criterion; and it requires the refusal of an unknown checkpoint to name the command that makes one, because a 404 alone sends the reader to the source. Not here yet: `feint checkpoint` and `feint reset` as commands, the verdict built on the observable state, `feint run --reset-after`, and the recreating half. mise run prepush green. Assisted-by: Claude Code (claude-opus-5) --- internal/core/emulator/checkpoint.go | 284 ++++++++++++++++++++++ internal/core/emulator/checkpoint_test.go | 187 ++++++++++++++ internal/core/emulator/emulator.go | 11 + internal/core/emulator/schema.go | 9 + internal/core/machine/surface.go | 2 + internal/providers/exoscale/machines.go | 17 ++ internal/providers/outscale/machines.go | 17 ++ internal/providers/scaleway/machines.go | 17 ++ 8 files changed, 544 insertions(+) create mode 100644 internal/core/emulator/checkpoint.go create mode 100644 internal/core/emulator/checkpoint_test.go diff --git a/internal/core/emulator/checkpoint.go b/internal/core/emulator/checkpoint.go new file mode 100644 index 00000000..1cddfb9f --- /dev/null +++ b/internal/core/emulator/checkpoint.go @@ -0,0 +1,284 @@ +package emulator + +import ( + "bytes" + "context" + "net/http" + "slices" + "sort" + "sync" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" +) + +// Named checkpoints of a whole lab, and the reset that returns to one (#853). +// +// `feint snapshot` already persists the control plane, and +// objectstore.SaveCheckpoint already persists bytes. Neither had a caller that +// put them together, and putting them together is the point: #853 asks for a +// LIFECYCLE guarantee rather than two storage primitives, and says what +// separates them in one line — "reset is not successful because JSON was +// restored". +// +// ## Why the reset happens here rather than in the CLI +// +// Restoring the store is the easy half. The half that matters is the host: a +// resource that disappears in a restore leaves its container running while the +// API no longer mentions it, and nobody destroys it. Only this process holds +// both the store and the packs' bindings, so only this process can act on the +// difference. +// +// A CLI-side reset would have to name each pack's delete route to take a +// machine down, which is provider knowledge in the one place this repository +// keeps free of it. +// +// ## Why the VERDICT is not here +// +// This answers what it did. Whether the lab converged is a comparison of what +// a client can observe, and that belongs to whoever asked for the reset — +// internal/cli's observable state. An operation that graded itself would be +// the verdict this repository refuses everywhere else. + +// Reconciler is the optional half of a pack that can bring the host back in +// line with its own stored resources. +// +// Optional in the shape the other halves use (EnvHazards, StackHazards, +// FirewallEnforcer): a pack that drives no machine does not implement it, and +// nothing here holds a list of which do. +// +// The input is the pack's OWN resources that no longer exist, so no pack ever +// sees another's, and the core stays a loop over whatever is mounted. +type Reconciler interface { + ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled +} + +// checkpoints holds the control-plane half of each named checkpoint. +// +// In memory, and that is the scope #853 describes: "returns to the session's +// initial state". Bytes are durable because objectstore's checkpoints are, and +// a reader who wants the control plane to outlive the process has +// `feint snapshot save`, which writes a file and is a different promise. +type checkpoints struct { + mu sync.Mutex + saved map[string][]byte +} + +func (c *checkpoints) put(name string, state []byte) { + c.mu.Lock() + defer c.mu.Unlock() + if c.saved == nil { + c.saved = map[string][]byte{} + } + c.saved[name] = state +} + +func (c *checkpoints) get(name string) ([]byte, bool) { + c.mu.Lock() + defer c.mu.Unlock() + state, ok := c.saved[name] + return state, ok +} + +func (c *checkpoints) drop(name string) bool { + c.mu.Lock() + defer c.mu.Unlock() + _, ok := c.saved[name] + delete(c.saved, name) + return ok +} + +// handleCheckpointSave records the control plane and the bytes under one name. +// +// Both halves or neither: a checkpoint whose store was taken and whose bytes +// were not is a checkpoint a reset cannot honour, and it would fail later, +// somewhere else, with no mention of this call. So the byte half goes first — +// it is the one that can fail on a disk — and the store half follows only +// when it succeeded. +// +// TestACheckpointTakesBothHalvesOrNeither fails without this. +func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + if !validCheckpointName(name) { + writeJSON(w, http.StatusBadRequest, map[string]any{ + "error": "a checkpoint name is 1 to 64 characters of letters, digits, dash, dot or underscore", + }) + return + } + if s.env.Objects != nil { + if err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the bytes could not be checkpointed: " + err.Error(), + }) + return + } + } + var state bytes.Buffer + if err := s.env.Store.Snapshot(&state); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the control plane could not be checkpointed: " + err.Error(), + }) + return + } + s.checkpoints.put(name, state.Bytes()) + writeJSON(w, http.StatusOK, map[string]any{ + "schema_version": CheckpointSchemaVersion, + "name": name, + "resources": s.env.Store.Len(), + "bytes": s.env.Objects != nil, + }) +} + +// handleCheckpointDelete forgets a checkpoint, both halves. +func (s *Server) handleCheckpointDelete(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + held := s.checkpoints.drop(name) + if s.env.Objects != nil { + if err := s.env.Objects.DeleteCheckpoint(r.Context(), name); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the bytes could not be forgotten: " + err.Error(), + }) + return + } + } + if !held { + writeJSON(w, http.StatusNotFound, map[string]any{ + "error": "no checkpoint named " + name, + }) + return + } + writeJSON(w, http.StatusOK, map[string]any{"status": "ok", "name": name}) +} + +// handleReset returns the lab to a checkpoint and reconciles the host with it. +// +// The order is not interchangeable, and each step's reason is its own: +// +// 1. read what the store holds NOW, because after step 2 it is gone and the +// machines it named would be unreachable; +// 2. restore the control plane, so the target state is in place before +// anything is destroyed — a failure here leaves the host untouched rather +// than half swept; +// 3. restore the bytes, which objectstore does atomically; +// 4. destroy the machines of records that no longer exist, through each +// pack's own binding and therefore through the ownership checks. +// +// What it does NOT do is recreate a machine a checkpoint had and the host no +// longer does. Starting one needs a Boot — an image, a login, the cloud-init a +// pack renders — which is pack knowledge, and #853 bounds it in the same words: +// "where the runtime contract supports it". The answer says so rather than +// leaving a caller to infer it from a quiet success. +// +// TestAResetDestroysWhatTheCheckpointDoesNotHold fails without this. +func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + state, ok := s.checkpoints.get(name) + if !ok { + writeJSON(w, http.StatusNotFound, map[string]any{ + "error": "no checkpoint named " + name + "; take one with POST /_feint/checkpoint/" + name, + }) + return + } + + before := s.env.Store.All() + if err := s.env.Store.Restore(bytes.NewReader(state)); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the control plane could not be restored: " + err.Error(), + }) + return + } + if s.env.Objects != nil { + if err := s.env.Objects.ResetCheckpoint(r.Context(), name); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the bytes could not be restored: " + err.Error(), + }) + return + } + } + + gone := goneAfterRestore(before, s.env.Store.All()) + reconciled := map[string]any{} + var destroyed, skipped []string + for _, pack := range s.packs { + reconciler, ok := pack.(Reconciler) + if !ok { + continue + } + mine := make([]*resource.Resource, 0, len(gone)) + for _, res := range gone { + if res.Tenant.Provider == pack.Name() { + mine = append(mine, res) + } + } + if len(mine) == 0 { + continue + } + done := reconciler.ReconcileGone(r.Context(), mine) + destroyed = append(destroyed, done.Destroyed...) + skipped = append(skipped, done.Skipped...) + reconciled[pack.Name()] = map[string]any{ + "destroyed": done.Destroyed, "skipped": done.Skipped, + } + } + sort.Strings(destroyed) + sort.Strings(skipped) + + writeJSON(w, http.StatusOK, map[string]any{ + "schema_version": CheckpointSchemaVersion, + "name": name, + "resources": s.env.Store.Len(), + "destroyed": destroyed, + "skipped": skipped, + "per_provider": reconciled, + // Said on the wire, because a caller cannot see it any other way and a + // quiet success would read as "the host matches the checkpoint". + "recreated": []string{}, + "recreates": false, + }) +} + +// goneAfterRestore answers the resources the restore removed. +// +// By identity rather than by count: a restore that replaced one server with +// another of the same kind leaves the count equal and the host wrong, which is +// the comparison a length check would miss. +func goneAfterRestore(before, after []*resource.Resource) []*resource.Resource { + keys := make([]string, 0, len(after)) + for _, res := range after { + keys = append(keys, resourceKey(res)) + } + slices.Sort(keys) + var gone []*resource.Resource + for _, res := range before { + if _, found := slices.BinarySearch(keys, resourceKey(res)); !found { + gone = append(gone, res) + } + } + return gone +} + +func resourceKey(res *resource.Resource) string { + return res.Tenant.Provider + "\x00" + res.Kind + "\x00" + res.ID +} + +// validCheckpointName keeps a name out of a path and out of a shell. +// +// The byte store writes a directory per checkpoint, so a name is a path +// component before it is anything else. Refused at the door rather than +// escaped later: this repository's order of preference is a serialiser first, +// a refusal at intake second, escaping last. +func validCheckpointName(name string) bool { + if name == "" || len(name) > 64 { + return false + } + for _, r := range name { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '-', r == '.', r == '_': + default: + return false + } + } + // A name of dots is a path, not a name. + return name != "." && name != ".." +} diff --git a/internal/core/emulator/checkpoint_test.go b/internal/core/emulator/checkpoint_test.go new file mode 100644 index 00000000..6ad933b7 --- /dev/null +++ b/internal/core/emulator/checkpoint_test.go @@ -0,0 +1,187 @@ +package emulator_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/emulator" + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" + "github.com/stephrobert/feint/internal/core/store" +) + +// Named checkpoints and the reset that returns to one (#853). +// +// What these tests hold is the half the issue says cannot be skipped: a reset +// is not successful because JSON was restored. So the assertion is never "the +// call answered 200", it is which machines the pack was asked to take down. + +// reconcilingPack is a stubPack that can reconcile, and records what it was +// handed. A pack that drives no machine does not implement the interface at +// all, which is the other population below. +type reconcilingPack struct { + stubPack + got [][]string + done machine.Reconciled +} + +func (p *reconcilingPack) ReconcileGone(_ context.Context, gone []*resource.Resource) machine.Reconciled { + var names []string + for _, res := range gone { + names = append(names, res.ID) + } + p.got = append(p.got, names) + return p.done +} + +func labServer(t *testing.T, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + held := store.New() + env := &emulator.Env{ + Store: held, + Now: func() time.Time { return time.Unix(1700000000, 0).UTC() }, + NewID: func() string { return "00000000-0000-4000-8000-000000000001" }, + } + srv, err := emulator.NewServer(env, packs...) + if err != nil { + t.Fatalf("build the emulator: %v", err) + } + server := httptest.NewServer(srv.Handler()) + t.Cleanup(server.Close) + return server, held +} + +func checkpointPost(t *testing.T, url string) (int, map[string]any) { + t.Helper() + res, err := http.Post(url, "application/json", nil) //nolint:noctx // a loopback test server + if err != nil { + t.Fatalf("POST %s: %v", url, err) + } + defer func() { _ = res.Body.Close() }() + var body map[string]any + _ = json.NewDecoder(res.Body).Decode(&body) + return res.StatusCode, body +} + +func put(t *testing.T, held *store.Store, provider, kind, id string) { + t.Helper() + res := resource.New(id, kind, resource.Tenant{Provider: provider, Project: "p", Zone: "z"}, + "running", time.Unix(1700000000, 0).UTC()) + res.Runtime = map[string]string{"machine": "feint-" + provider + "-" + id} + held.Put(res) +} + +// TestACheckpointTakesBothHalvesOrNeither is cited by handleCheckpointSave. +// +// A checkpoint whose store was taken and whose bytes were not is a checkpoint +// a reset cannot honour, and it would fail later, somewhere else, with no +// mention of this call. With no byte store there is only one half to take, and +// the answer says which — a caller that reset expecting bytes back would +// otherwise find none and blame the reset. +func TestACheckpointTakesBothHalvesOrNeither(t *testing.T) { + server, held := labServer(t, stubPack{name: "acme"}) + put(t, held, "acme", "instance/server", "a") + + code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline") + if code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + if body["name"] != "baseline" { + t.Errorf("name %v", body["name"]) + } + if count, _ := body["resources"].(float64); int(count) != 1 { + t.Errorf("resources %v", body["resources"]) + } + // Said rather than guessed: this lab has no byte store, so a reset will + // restore no bytes, and the caller is told here rather than discovering it + // after a reset that looked clean. + if carried, _ := body["bytes"].(bool); carried { + t.Error("a lab with no byte store reported carrying bytes") + } + if version, _ := body["schema_version"].(float64); int(version) != 1 { + t.Errorf("schema_version %v", body["schema_version"]) + } + + // A name that is a path component before it is a name. Refused at the + // door, because the byte store writes a directory per checkpoint and this + // repository's order is a refusal at intake rather than escaping later. + for _, bad := range []string{"..", ".", "a/b", "a b", ""} { + if code, _ := checkpointPost(t, server.URL+"/_feint/checkpoint/"+bad); code == http.StatusOK { + t.Errorf("the name %q was accepted", bad) + } + } +} + +// TestAResetDestroysWhatTheCheckpointDoesNotHold is cited by handleReset. +// +// The assertion is on what the PACK WAS ASKED to take down, not on the status +// code: a reset that restored the store and told nobody about the container +// still running is the exact gap #853 names. +func TestAResetDestroysWhatTheCheckpointDoesNotHold(t *testing.T) { + acme := &reconcilingPack{ + stubPack: stubPack{name: "acme"}, + done: machine.Reconciled{Destroyed: []string{"feint-acme-later"}}, + } + // A second pack that cannot reconcile, mounted on purpose: the loop must + // skip it rather than fail, because a pack that drives no machine is the + // ordinary case. + server, held := labServer(t, acme, stubPack{name: "quiet"}) + + put(t, held, "acme", "instance/server", "kept") + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + + // The lab moves on: one more resource of each pack. + put(t, held, "acme", "instance/server", "later") + put(t, held, "quiet", "instance/server", "alsolater") + + code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") + if code != http.StatusOK { + t.Fatalf("reset: %d %v", code, body) + } + + // The store is back to one resource. + if count, _ := body["resources"].(float64); int(count) != 1 { + t.Errorf("resources after the reset: %v", body["resources"]) + } + // And the pack was handed exactly the resource the checkpoint does not + // hold — not the one it does, which is the direction that would destroy a + // machine the checkpoint expects to keep. + if len(acme.got) != 1 || len(acme.got[0]) != 1 || acme.got[0][0] != "later" { + t.Fatalf("the pack was asked to reconcile %v", acme.got) + } + if destroyed, _ := body["destroyed"].([]any); len(destroyed) != 1 || destroyed[0] != "feint-acme-later" { + t.Errorf("destroyed %v", body["destroyed"]) + } + // Recreation is not attempted, and the answer says so: a quiet success + // would read as "the host matches the checkpoint". + if recreates, _ := body["recreates"].(bool); recreates { + t.Error("the reset claims it recreates missing machines") + } + + // Twice is a no-op, which is #853's own criterion: nothing is left to + // reconcile, so the pack is not asked again. + if code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline"); code != http.StatusOK { + t.Fatalf("second reset: %d %v", code, body) + } + if len(acme.got) != 1 { + t.Errorf("a second reset asked for more reconciliation: %v", acme.got) + } + + // An unknown checkpoint is refused by name, and the refusal says how to + // make one: a 404 alone sends the reader to the source. + code, body = checkpointPost(t, server.URL+"/_feint/reset/nothing-here") + if code != http.StatusNotFound { + t.Fatalf("reset of an unknown checkpoint: %d %v", code, body) + } + if message, _ := body["error"].(string); message == "" || + !strings.Contains(message, "nothing-here") || !strings.Contains(message, "/_feint/checkpoint/") { + t.Errorf("the refusal does not say what to do: %v", body["error"]) + } +} diff --git a/internal/core/emulator/emulator.go b/internal/core/emulator/emulator.go index dd457954..8b24bb22 100644 --- a/internal/core/emulator/emulator.go +++ b/internal/core/emulator/emulator.go @@ -360,6 +360,11 @@ type Server struct { // self records the patterns of the endpoints the emulator serves about // itself, so they can be enumerated rather than remembered. self []string + // checkpoints holds the control-plane half of each named checkpoint + // (#853). In memory, which is the scope the issue describes: a reset + // returns to the session's initial state, and a checkpoint meant to + // outlive the process is what `feint snapshot save` writes to a file. + checkpoints checkpoints // shapeCovered is the set of operations an observed real-cloud shape // covers, handed in by SetShapeCovered. nil means no catalogue was given, // which the evidence record reports as "unknown" rather than "unobserved". @@ -456,6 +461,12 @@ func NewServer(env *Env, packs ...Pack) (*Server, error) { // conformance script or a CI job reads after the fact. Tying it to the page // would make it unavailable to exactly the callers it was asked for. s.mountSelf("GET /_feint/trace", s.handleTrace) + // Named checkpoints of a whole lab, and the reset that returns to one + // (#853). Mounted beside /_feint/state because they are the same + // subject seen as a lifecycle rather than as storage. + s.mountSelf("POST /_feint/checkpoint/{name}", s.handleCheckpointSave) + s.mountSelf("DELETE /_feint/checkpoint/{name}", s.handleCheckpointDelete) + s.mountSelf("POST /_feint/reset/{name}", s.handleReset) s.mountSelf("GET /_feint/state", s.handleStateRead) s.mountSelf("PUT /_feint/state", s.handleStateWrite) // The assertion spans behind the behaviour and negative evidence axes: a diff --git a/internal/core/emulator/schema.go b/internal/core/emulator/schema.go index 4dc1e93c..9a9879ce 100644 --- a/internal/core/emulator/schema.go +++ b/internal/core/emulator/schema.go @@ -126,4 +126,13 @@ const ( // because a suite that arms a fault from a committed file is a consumer on // day one — the same reasoning that put the other four here. FaultsSchemaVersion = 1 + // CheckpointSchemaVersion is the shape of POST /_feint/checkpoint/{name}, + // DELETE /_feint/checkpoint/{name} and POST /_feint/reset/{name}. + // + // 1 is the first: the name, the resource count and whether bytes were part + // of it; and for a reset, what was destroyed, what was skipped, the same + // split per provider, and the two fields that say recreation is not + // attempted. Frozen from the start like the four above, because a suite + // that resets between tests is a consumer on day one. + CheckpointSchemaVersion = 1 ) diff --git a/internal/core/machine/surface.go b/internal/core/machine/surface.go index 2d6ee666..6e296971 100644 --- a/internal/core/machine/surface.go +++ b/internal/core/machine/surface.go @@ -125,6 +125,8 @@ func PackSurface() map[string]string { "Binding.Observe": "read a resource back after an effect, with the same write-back discipline", "Binding.PowerOff": "stop the machine behind a resource", "Binding.Destroy": "destroy the machine behind a resource", + "Binding.ReconcileGone": "take down the machines of records a reset removed (#853), in one shared gesture", + "Reconciled": "what a reconciliation did: what it destroyed, and what it could not act on", "Binding.RefreshIfRunning": "re-read what the host holds for a machine the store calls running", "Binding.RuntimeKey": "the pack's own Runtime key, to read the machine name it is about to hand to Unroute", "Boot": "what to boot: image, login, keys, user data", diff --git a/internal/providers/exoscale/machines.go b/internal/providers/exoscale/machines.go index 23fe0a3a..5ac45266 100644 --- a/internal/providers/exoscale/machines.go +++ b/internal/providers/exoscale/machines.go @@ -279,3 +279,20 @@ func (p *Pack) refresh(ctx context.Context, res *resource.Resource) bool { func (p *Pack) addressOf(res *resource.Resource) string { return p.reconciler().PublicAddressOf(res) } + +// ReconcileGone takes down the machines of records a reset removed (#853). +// +// Two lines, and that is the point: the gesture is identical for every pack, +// so it lives on the shared binding where a defect is fixed once. What this +// pack adds is which binding — its prefix, its runtime key, its ownership +// mark — and nothing else. +// +// The recreating half is deliberately absent here too. Starting a machine +// needs a Boot this pack renders from its own catalogue, and #853 bounds that +// in the same words: "where the runtime contract supports it". +// emulator.TestAResetDestroysWhatTheCheckpointDoesNotHold drives this through +// the server; machine.TestReconcileDestroysOnlyTheMachinesItOwns holds what it +// delegates to. +func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { + return p.binding().ReconcileGone(ctx, gone) +} diff --git a/internal/providers/outscale/machines.go b/internal/providers/outscale/machines.go index 90ceef88..c0df98f0 100644 --- a/internal/providers/outscale/machines.go +++ b/internal/providers/outscale/machines.go @@ -272,3 +272,20 @@ func (p *Pack) attachmentOf(res *resource.Resource) []machine.Attachment { PrefixLen: prefix.Bits(), }} } + +// ReconcileGone takes down the machines of records a reset removed (#853). +// +// Two lines, and that is the point: the gesture is identical for every pack, +// so it lives on the shared binding where a defect is fixed once. What this +// pack adds is which binding — its prefix, its runtime key, its ownership +// mark — and nothing else. +// +// The recreating half is deliberately absent here too. Starting a machine +// needs a Boot this pack renders from its own catalogue, and #853 bounds that +// in the same words: "where the runtime contract supports it". +// emulator.TestAResetDestroysWhatTheCheckpointDoesNotHold drives this through +// the server; machine.TestReconcileDestroysOnlyTheMachinesItOwns holds what it +// delegates to. +func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { + return p.binding().ReconcileGone(ctx, gone) +} diff --git a/internal/providers/scaleway/machines.go b/internal/providers/scaleway/machines.go index 91a54d9b..85b8125c 100644 --- a/internal/providers/scaleway/machines.go +++ b/internal/providers/scaleway/machines.go @@ -333,3 +333,20 @@ func (p *Pack) gatewayPushesDefaultRoute(privateNetworkID string) bool { } return false } + +// ReconcileGone takes down the machines of records a reset removed (#853). +// +// Two lines, and that is the point: the gesture is identical for every pack, +// so it lives on the shared binding where a defect is fixed once. What this +// pack adds is which binding — its prefix, its runtime key, its ownership +// mark — and nothing else. +// +// The recreating half is deliberately absent here too. Starting a machine +// needs a Boot this pack renders from its own catalogue, and #853 bounds that +// in the same words: "where the runtime contract supports it". +// emulator.TestAResetDestroysWhatTheCheckpointDoesNotHold drives this through +// the server; machine.TestReconcileDestroysOnlyTheMachinesItOwns holds what it +// delegates to. +func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { + return p.binding().ReconcileGone(ctx, gone) +} From 48f90b8daf711b49fb8360de70e8081a31b18e6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 12:36:43 +0100 Subject: [PATCH 4/7] feat(cli): feint checkpoint and feint reset, judged by what a client observes (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit feint checkpoint baseline # run tests, create resources, fail halfway feint reset baseline Measured end to end against a running emulator with a byte store armed, which is #853's "Done means" list in the order it names: checkpoint "baseline": 4 resource(s), 1 object(s), 0 machine(s) # one extra server, one extra object, and a.txt overwritten ressources : 7 objets : [a.txt b.txt] reset "baseline": the lab converged exit 0 ressources : 4 objets : [a.txt] a.txt: "the bytes the checkpoint holds" # the content, not just the key # a second reset: exit 0, nothing left to do ## The split that is the whole design The emulator owns the state transitions. These two commands own the VERDICT, and #853 says why in one line: "reset is not successful because JSON was restored". So a reset is judged by comparing what a client can observe before and after, never by the status code of the call that performed it. Exit 0 when it converged, 1 with every remaining difference named. Not 2 — this repository reserves that for upstream drift — and never 0 with differences printed, which would be a gate nobody can build on. ## Why a checkpoint records the observation too Deriving the expected state from the restored store would compare a restore to itself: the containers and the bytes would be out of the picture, which is exactly the half that can diverge. So the observation is recorded beside the instance, it belongs to that emulator, and it goes when it does. A reset against a name the record does not hold is refused rather than guessed: comparing one checkpoint's restore to another's observation would report differences that are not the reset's fault, and an operator would chase them. ## Two defects the measurements found, not the tests **`feint checkpoint baseline --addr …` was refused by its own usage line.** Go's flag package stops at the first non-flag argument, so every flag behind the name became a positional. A unit test of the parsing would not have caught it, because I would have written the test with the flags first. The first real run did. **`feint checkpoint -h` printed no flags.** The name was taken off before the flag set existed, so the help had nothing to print. Found by the frozen-surface observation, which refuses a verb that builds no flag set — it freezes nothing for it. The order is now flag set first, name second, and that is not cosmetic. ## Two test gaps the falsification found tools/falsify/specs/a-reset-is-not-a-restore.json is the spec #853's last criterion asks for: "a falsification that restores only the store but not runtime makes the test fail". Six mutations, each turning the reset back into a restore by a different route. Two of them stayed GREEN on the first run, and both were my tests' fault: - the mutation that compares resources by identifier alone, dropping the kind and the provider, passed because every id in the test was distinct. The test now holds two records SHARING an identifier across kinds, and a reset that dropped the kind would take the extra volume for the server the checkpoint keeps and leave its machine running; - the mutation that skips the byte half of a checkpoint passed because the test's emulator served no objects. There is now a second lab with a byte store, and the assertion is not the status code: the byte checkpoint is reset afterwards, because that is the only way to see it exists. Both gaps were invisible to a reader of the tests and obvious to the harness. ## Not here yet `feint run --reset-after` and its three exits, the recreating half, and a conformance suite replaying this cycle in CI. The cycle above was driven by hand, and this commit does not claim otherwise. mise run prepush green. `cliSurfaceVersion` 30 → 31, fixture regenerated, both READMEs carry the two verbs. Assisted-by: Claude Code (claude-opus-5) --- README.fr.md | 2 + README.md | 2 + internal/cli/checkpoint.go | 353 ++++++++++++++++++ internal/cli/checkpoint_test.go | 133 +++++++ internal/cli/cli.go | 24 +- internal/cli/testdata/frozen/cli.json | 247 ++++++++++++ internal/core/emulator/checkpoint_test.go | 81 +++- .../specs/a-reset-is-not-a-restore.json | 48 +++ 8 files changed, 879 insertions(+), 11 deletions(-) create mode 100644 internal/cli/checkpoint.go create mode 100644 internal/cli/checkpoint_test.go create mode 100644 tools/falsify/specs/a-reset-is-not-a-restore.json diff --git a/README.fr.md b/README.fr.md index f9afee5f..e1c40178 100644 --- a/README.fr.md +++ b/README.fr.md @@ -666,6 +666,8 @@ n'est une étape dans le travail de qui que ce soit. | `feint doctor` | diagnostique l'hôte : le port, le runtime, les clients | | `feint env ` | l'environnement dont un vrai client de ce provider a besoin | | `feint snapshot` | nomme l'état d'un émulateur qui tourne et y revient : `save`, `load`, `list`, `rm` | +| `feint checkpoint` | nomme l'état d'un émulateur qui tourne : plan de contrôle, octets, et ce qu'un client peut en observer | +| `feint reset` | revient à ce point, retire ce qu'il ne contient pas, et dit si le laboratoire a convergé | | `feint images` | construit les images de machines, qui portent un démon ssh pour répondre sans dépôt de paquets | | `feint clean` | retire les machines, réseaux et jeux de règles que l'émulateur a créés | | `feint version` | affiche la version | diff --git a/README.md b/README.md index b94e2303..17b7a64f 100644 --- a/README.md +++ b/README.md @@ -1064,6 +1064,8 @@ workflow. | `feint env ` | the environment a real client needs, for `eval` | | `feint doctor` | diagnose the host: the port, the machine runtime, the clients, the ssh trap | | `feint snapshot` | `save`, `load`, `list`, `rm` — name a running emulator's state and come back to it | +| `feint checkpoint` | name the state of a running emulator — control plane, object bytes, and what a client can observe of both | +| `feint reset` | return to that checkpoint, take down what it does not hold, and say whether the lab converged | | `feint serve` | serve the three emulated clouds on one port, in the foreground | | `feint images` | build the machine images, which carry an ssh daemon so a machine answers without a package repository | | `feint clean` | remove every machine, network and rule set the emulator created | diff --git a/internal/cli/checkpoint.go b/internal/cli/checkpoint.go new file mode 100644 index 00000000..1d16e209 --- /dev/null +++ b/internal/cli/checkpoint.go @@ -0,0 +1,353 @@ +package cli + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "time" + + "github.com/stephrobert/feint/internal/core/machine" +) + +// `feint checkpoint` and `feint reset`: transaction-like test isolation (#853). +// +// feint checkpoint baseline +// # run tests, create resources, fail halfway +// feint reset baseline +// +// The emulator owns the state transitions, and these two own the VERDICT. +// That split is the whole design, and #853 states why in one line: "reset is +// not successful because JSON was restored". So a reset here is judged by +// comparing what a client can observe before and after, never by the status +// code of the call that performed it. +// +// ## What a checkpoint records locally, and why +// +// The emulator holds the state. These commands additionally record the +// OBSERVED state beside the instance, because that is what a later reset is +// measured against. Deriving it from the restored store instead would compare +// a restore to itself: the containers and the bytes would be out of the +// picture, which is exactly the half that can diverge. +// +// ## The exit code +// +// 0 when the observable state converged, 1 when it did not, with every +// difference named. Not 2: this repository reserves that for upstream drift, +// and a reset that failed to deliver its guarantee is an error in the ordinary +// sense. A reset that printed differences and exited 0 would be a gate nobody +// can build on. + +// checkpointStateVersion is the shape of the file a checkpoint records beside +// its instance. Frozen from the start, like the emulator's own schemas: a +// suite that reads it is a consumer on day one. +const checkpointStateVersion = 1 + +// recordedCheckpoint is what `feint checkpoint` writes and `feint reset` reads. +type recordedCheckpoint struct { + SchemaVersion int `json:"schema_version"` + Name string `json:"name"` + Taken string `json:"taken"` + Runtime string `json:"runtime"` + State observed `json:"state"` +} + +// checkpointArgName takes the leading name off the arguments, so that the +// flags behind it are still flags. +// +// Shared with the refusals a name must survive: the byte store writes a +// directory per checkpoint, so a name is a path component before it is a +// name, and the emulator refuses the same shapes at its own door. Refusing +// here too means the operator hears it before a request is sent. +// +// TestACheckpointNameIsTakenBeforeTheFlags fails without this. +func checkpointArgName(args []string, stderr io.Writer, verb string) (string, []string, bool) { + if len(args) == 0 || strings.HasPrefix(args[0], "-") { + fmt.Fprintf(stderr, "feint %s: name one checkpoint: feint %s [flags]\n", verb, verb) + return "", nil, false + } + name := args[0] + if name == "." || name == ".." || strings.ContainsAny(name, `/\`) { + fmt.Fprintf(stderr, "feint %s: %q is not a checkpoint name: no path separators, no . or ..\n", verb, name) + return "", nil, false + } + return name, args[1:], true +} + +// checkpointCommand takes a named checkpoint of a running lab. +func checkpointCommand(args []string, stdout, stderr io.Writer) int { + // The name comes first and the flags after it, the way `feint snapshot + // save --addr …` already reads. Go's flag package stops at the + // first non-flag argument, so a name parsed by it swallows every flag + // behind it into the positionals — which is exactly what the first + // measurement of this command did, refusing `feint checkpoint baseline + // --addr 127.0.0.1:4621` on its own usage line. + // The flag set is built BEFORE the name is taken off, and that order is + // not cosmetic: `feint checkpoint -h` must print the flags, and the first + // version refused on its own usage line instead. The frozen-surface + // observation found it, because a verb that builds no flag set freezes + // nothing. + fs := newFlagSet("checkpoint") + addr := fs.String("addr", DefaultAddr, "listen address of the emulator to checkpoint") + if len(args) != 0 && strings.HasPrefix(args[0], "-") { + // -h, --help, or a mistyped flag: let the flag set answer, which is + // the only thing that can print the usage. + if err := fs.Parse(args); err != nil { + return exitError + } + } + name, rest, ok := checkpointArgName(args, stderr, "checkpoint") + if !ok { + return exitError + } + if err := fs.Parse(rest); err != nil { + return exitError + } + if fs.NArg() != 0 { + fmt.Fprintf(stderr, "feint checkpoint: unexpected argument %q; one name, then flags\n", fs.Arg(0)) + return exitError + } + endpoint := "http://" + *addr + + // The emulator's half first: a checkpoint whose state was observed and + // whose store was never taken is a checkpoint a reset cannot honour, and + // the observation would read as a success. + answer, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/checkpoint/"+name) + if err != nil { + fmt.Fprintf(stderr, "feint checkpoint: %v\n", err) + return exitError + } + + state, runtimeName, err := observeFor(endpoint) + if err != nil { + fmt.Fprintf(stderr, "feint checkpoint: the checkpoint was taken and could not be read back: %v\n", err) + return exitError + } + if err := writeRecordedCheckpoint(*addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: name, + Taken: time.Now().UTC().Format(time.RFC3339), Runtime: runtimeName, State: state, + }); err != nil { + fmt.Fprintf(stderr, "feint checkpoint: %v\n", err) + return exitError + } + + fmt.Fprintf(stdout, "checkpoint %q: %d resource(s), %d object(s), %d machine(s)\n", + name, len(state.Resources), len(state.Objects), len(state.Machines)) + if carried, _ := answer["bytes"].(bool); !carried { + // Said rather than left out: a reader who resets expecting bytes back + // would otherwise find none and blame the reset. + fmt.Fprintln(stdout, " bytes: this emulator serves no object store, so none are part of it") + } + return exitOK +} + +// resetCommand returns a lab to a checkpoint and judges whether it converged. +func resetCommand(args []string, stdout, stderr io.Writer) int { + // The name is optional here, so the split is by shape rather than by + // position: a leading argument that does not start with a dash is the + // name, everything else is flags. Same reason as above. + fs := newFlagSet("reset") + addr := fs.String("addr", DefaultAddr, "listen address of the emulator to reset") + asked, rest := "", args + if len(args) != 0 && !strings.HasPrefix(args[0], "-") { + asked, rest = args[0], args[1:] + } + if err := fs.Parse(rest); err != nil { + return exitError + } + if fs.NArg() != 0 { + fmt.Fprintf(stderr, "feint reset: unexpected argument %q; one name at most, then flags\n", fs.Arg(0)) + return exitError + } + endpoint := "http://" + *addr + + recorded, err := readRecordedCheckpoint(*addr) + if err != nil { + fmt.Fprintf(stderr, "feint reset: %v\n", err) + fmt.Fprintf(stderr, " Take one first: feint checkpoint --addr %s\n", *addr) + return exitError + } + name := recorded.Name + if asked != "" { + name = asked + } + if name != recorded.Name { + // Refused rather than guessed. The recorded state is what a verdict + // compares against, and comparing one checkpoint's emulator state to + // another's observation would report differences that are not the + // reset's fault. + fmt.Fprintf(stderr, "feint reset: this instance recorded %q, not %q, so a verdict on %q would compare the wrong states\n", + recorded.Name, name, name) + return exitError + } + + answer, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/reset/"+name) + if err != nil { + fmt.Fprintf(stderr, "feint reset: %v\n", err) + return exitError + } + reportReconciliation(stdout, answer) + + state, _, err := observeFor(endpoint) + if err != nil { + fmt.Fprintf(stderr, "feint reset: the reset ran and could not be read back: %v\n", err) + return exitError + } + + // The verdict, and the only one worth having: what a client can observe, + // compared with what it could observe at the checkpoint. + differences := recorded.State.diff(state) + if len(differences) == 0 { + fmt.Fprintf(stdout, "reset %q: the lab converged\n", name) + return exitOK + } + fmt.Fprintf(stderr, "feint reset: %q did not converge; %d difference(s) remain:\n", name, len(differences)) + for _, difference := range differences { + fmt.Fprintf(stderr, " %s\n", difference) + } + if recreates, _ := answer["recreates"].(bool); !recreates { + fmt.Fprintln(stderr, " This reset removes what the checkpoint does not hold and recreates nothing:") + fmt.Fprintln(stderr, " a machine the checkpoint had and the host no longer does needs a boot only") + fmt.Fprintln(stderr, " its pack can render, and docs/limits.md says so.") + } + return exitError +} + +// reportReconciliation prints what the emulator says it did, before the +// verdict says whether it was enough. +func reportReconciliation(stdout io.Writer, answer map[string]any) { + destroyed := stringsOf(answer["destroyed"]) + skipped := stringsOf(answer["skipped"]) + if len(destroyed) != 0 { + fmt.Fprintf(stdout, " destroyed: %s\n", strings.Join(destroyed, ", ")) + } + if len(skipped) != 0 { + // A record nobody could act on is what an operator needs to hear + // about: it is the difference between "nothing to do" and "this run + // has no runtime". + fmt.Fprintf(stdout, " not acted on: %s\n", strings.Join(skipped, ", ")) + } +} + +func stringsOf(value any) []string { + items, ok := value.([]any) + if !ok { + return nil + } + out := make([]string, 0, len(items)) + for _, item := range items { + if text, ok := item.(string); ok { + out = append(out, text) + } + } + return out +} + +// observeFor reads the observable state, resolving the runtime the emulator +// actually declares. +// +// From /_feint/health rather than from a flag, for the reason the run receipt +// does the same: a capability is declared by the driver, and asking the host +// for a mode nobody is running would refuse on a station that has none. +func observeFor(endpoint string) (observed, string, error) { + name, _, _ := labHealth(endpoint) + rt := machine.Runtime{} + if name != "" && name != "none" && name != "off" { + resolved, err := resolveRuntime(name, io.Discard) + if err != nil { + // Not fatal: the host half stays empty and the verdict says so by + // comparing fewer axes. Refusing here would make a reset + // impossible on a station whose runtime went away between the + // checkpoint and now, which is the moment a reset is most needed. + return mustObserve(endpoint, rt), name, nil + } + rt = resolved + } + return mustObserve(endpoint, rt), name, nil +} + +func mustObserve(endpoint string, rt machine.Runtime) observed { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + state, err := observeLab(ctx, endpoint, rt) + if err != nil { + // Returned empty rather than propagated, because the caller already + // distinguishes "could not read it back" from "it did not converge" + // and an empty state would make every axis differ. + return observed{} + } + return state +} + +// callCheckpoint drives one of the emulator's checkpoint endpoints. +func callCheckpoint(method, url string) (map[string]any, error) { + req, err := http.NewRequestWithContext(context.Background(), method, url, nil) + if err != nil { + return nil, err + } + client := &http.Client{Timeout: 2 * time.Minute} + res, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("the emulator did not answer: %w", err) + } + defer func() { _ = res.Body.Close() }() + var body map[string]any + _ = json.NewDecoder(io.LimitReader(res.Body, maxStateBody)).Decode(&body) + if res.StatusCode != http.StatusOK { + if message, ok := body["error"].(string); ok { + return nil, fmt.Errorf("%s", message) + } + return nil, fmt.Errorf("the emulator answered %d", res.StatusCode) + } + return body, nil +} + +// checkpointPath is where the observed state lives: beside the instance +// record, because it belongs to that emulator and goes when it does. +func checkpointPath(addr string) (string, error) { + dir, err := instanceDir(addr) + if err != nil { + return "", err + } + return filepath.Join(dir, "checkpoint.json"), nil +} + +func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error { + path, err := checkpointPath(addr) + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return err + } + body, err := json.MarshalIndent(recorded, "", " ") + if err != nil { + return err + } + return os.WriteFile(path, append(body, '\n'), 0o600) +} + +func readRecordedCheckpoint(addr string) (recordedCheckpoint, error) { + path, err := checkpointPath(addr) + if err != nil { + return recordedCheckpoint{}, err + } + body, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr) + } + var recorded recordedCheckpoint + if err := json.Unmarshal(body, &recorded); err != nil { + return recordedCheckpoint{}, fmt.Errorf("the recorded checkpoint is unreadable: %w", err) + } + if recorded.SchemaVersion != checkpointStateVersion { + return recordedCheckpoint{}, fmt.Errorf( + "the recorded checkpoint is version %d and this binary reads %d", + recorded.SchemaVersion, checkpointStateVersion) + } + return recorded, nil +} diff --git a/internal/cli/checkpoint_test.go b/internal/cli/checkpoint_test.go new file mode 100644 index 00000000..48d21217 --- /dev/null +++ b/internal/cli/checkpoint_test.go @@ -0,0 +1,133 @@ +package cli + +import ( + "bytes" + "strings" + "testing" +) + +// TestACheckpointNameIsTakenBeforeTheFlags is cited by checkpointArgName. +// +// It exists because the first version of these commands let Go's flag package +// parse the name, and flag stops at the first non-flag argument: every flag +// behind the name became a positional. The first real measurement of +// `feint checkpoint baseline --addr 127.0.0.1:4621` was refused by the +// command's own usage line, which is a defect no unit test of the parsing +// would have found, because I would have written the test with the flags +// first. +func TestACheckpointNameIsTakenBeforeTheFlags(t *testing.T) { + var stderr bytes.Buffer + + name, rest, ok := checkpointArgName([]string{"baseline", "--addr", "127.0.0.1:4621"}, &stderr, "checkpoint") + if !ok { + t.Fatalf("a name followed by flags was refused: %s", stderr.String()) + } + if name != "baseline" { + t.Errorf("name %q", name) + } + if len(rest) != 2 || rest[0] != "--addr" { + t.Errorf("the flags did not survive the split: %v", rest) + } + + // No name at all, and a flag where the name should be: both are the same + // mistake from the operator's side and both must say what to type. + for _, probe := range [][]string{nil, {"--addr", "127.0.0.1:4599"}} { + stderr.Reset() + if _, _, ok := checkpointArgName(probe, &stderr, "checkpoint"); ok { + t.Errorf("%v was accepted as a name", probe) + } + if !strings.Contains(stderr.String(), "") { + t.Errorf("the refusal for %v does not say what to type: %s", probe, stderr.String()) + } + } + + // A name that is a path component before it is a name. The emulator + // refuses the same shapes at its own door; refusing here too means the + // operator hears it before a request leaves. + for _, bad := range []string{".", "..", "a/b", `a\b`} { + stderr.Reset() + if _, _, ok := checkpointArgName([]string{bad}, &stderr, "checkpoint"); ok { + t.Errorf("the name %q was accepted", bad) + } + if !strings.Contains(stderr.String(), "path separators") { + t.Errorf("the refusal of %q does not say why: %s", bad, stderr.String()) + } + } +} + +// TestAResetRefusesToJudgeAgainstAnotherCheckpointsState is the other half of +// the verdict's honesty. +// +// The recorded observable state belongs to ONE checkpoint. Comparing the +// emulator's restore of one against the observation of another would report +// differences that are not the reset's fault, and an operator would chase +// them. So a name that disagrees with the record is refused rather than +// guessed. +func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "baseline", + }); err != nil { + t.Fatalf("record a checkpoint: %v", err) + } + + var stdout, stderr bytes.Buffer + if code := resetCommand([]string{"something-else", "--addr", addr}, &stdout, &stderr); code != exitError { + t.Fatalf("a reset against another checkpoint's state was allowed (%d)", code) + } + for _, want := range []string{"baseline", "something-else", "compare the wrong states"} { + if !strings.Contains(stderr.String(), want) { + t.Errorf("the refusal does not mention %q: %s", want, stderr.String()) + } + } + + // And with no record at all it says how to make one, rather than failing + // on a missing file an operator did not know existed. + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + stderr.Reset() + if code := resetCommand([]string{"--addr", addr}, &stdout, &stderr); code != exitError { + t.Fatalf("a reset with no checkpoint was allowed (%d)", code) + } + if !strings.Contains(stderr.String(), "feint checkpoint") { + t.Errorf("the refusal does not say how to take one: %s", stderr.String()) + } +} + +// TestARecordedCheckpointRefusesAVersionItCannotRead is the forward +// compatibility half. +// +// The file is a published shape — a suite that resets between tests reads it — +// so a binary meeting a version it does not know must say so rather than +// decode what it can and compare against a half-read state. A verdict built +// on a partially understood record is worse than no verdict. +func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion + 1, Name: "from-the-future", + }); err != nil { + t.Fatalf("record a checkpoint: %v", err) + } + if _, err := readRecordedCheckpoint(addr); err == nil { + t.Fatal("a record from a later version was read anyway") + } else if !strings.Contains(err.Error(), "version") { + t.Errorf("the refusal does not say it is a version: %v", err) + } + + // The round trip of the current version, so this cannot be satisfied by + // refusing everything. + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "baseline", + State: observed{Machines: []string{"feint-scw-a"}}, + }); err != nil { + t.Fatalf("record a checkpoint: %v", err) + } + back, err := readRecordedCheckpoint(addr) + if err != nil { + t.Fatalf("read it back: %v", err) + } + if back.Name != "baseline" || len(back.State.Machines) != 1 { + t.Errorf("the record did not survive the round trip: %+v", back) + } +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 5a0b849e..01b076e5 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -249,7 +249,7 @@ const ( // TestTheFrozenSurfacesStillMatchTheirFixture, and a fixture regenerated // without bumping this constant fails TestASurfaceChangeDemandsItsVersionBump. // The procedure for a deliberate change is in RELEASING.md ("Frozen surfaces"). -const cliSurfaceVersion = 30 +const cliSurfaceVersion = 31 // Run executes one command and returns the process exit code. func Run(args []string, stdout, stderr io.Writer) int { @@ -316,6 +316,10 @@ func Run(args []string, stdout, stderr io.Writer) int { return images(args[2:], stdout, stderr) case "snapshot": return snapshot(args[2:], stdout, stderr) + case "checkpoint": + return checkpointCommand(args[2:], stdout, stderr) + case "reset": + return resetCommand(args[2:], stdout, stderr) case "version", "--version", "-v": // The flag forms are aliases, not a second way of doing it: a user who // types `feint --version` and gets "unknown command" concludes the @@ -566,6 +570,24 @@ Usage: station, so it is asked for, and it needs a lab this command owns. + feint checkpoint [--addr 127.0.0.1:4599] + Name the state of a running emulator: the control plane + and, when one is armed, the object bytes. It also records + what a client can OBSERVE at that moment, beside the + instance, because that is what a later reset is measured + against. + + feint reset [] [--addr 127.0.0.1:4599] + Return to that checkpoint and say whether the lab + converged. The store is restored, the bytes are restored, + and the machines of records the checkpoint does not hold + are taken down through the ownership checks. Then the + observable state is read again and compared: exit 0 when + it matches, 1 with every remaining difference named. A + reset is not successful because JSON was restored. + It recreates nothing: a machine the checkpoint had and the + host no longer does needs a boot only its pack can render. + feint transcript [--shape OP [--against emu.jsonl]] [--sanitise corpus.jsonl --contract contracts/.json] [--format text|json] diff --git a/internal/cli/testdata/frozen/cli.json b/internal/cli/testdata/frozen/cli.json index 43ea4b68..d17a1a43 100644 --- a/internal/cli/testdata/frozen/cli.json +++ b/internal/cli/testdata/frozen/cli.json @@ -5637,6 +5637,253 @@ ] } } + }, + { + "schema_version": 31, + "content": { + "exit_codes": { + "drift": 2, + "error": 1, + "ok": 0 + }, + "verbs": { + "catalog": [ + "--format" + ], + "checkpoint": [ + "--addr" + ], + "clean": [ + "--check", + "--closing", + "--doorstep", + "--force", + "--format", + "--vm" + ], + "corpus": [ + "--accepted", + "--against-cloud", + "--bind", + "--check", + "--credential", + "--dir", + "--dry-run", + "--endpoint", + "--file", + "--format", + "--mark-stale", + "--timeout" + ], + "coverage": [ + "--artefact", + "--axis", + "--baseline", + "--contract", + "--contract-only", + "--document", + "--evidence", + "--fail-on-unknown", + "--format", + "--gaps", + "--observed", + "--products", + "--provider", + "--sdk", + "--write-baseline" + ], + "docs": [ + "--check", + "--client-pins", + "--confidence", + "--contracts", + "--coverage", + "--file", + "--install", + "--limits", + "--proved", + "--routes", + "--screenshots", + "--ui-manifest", + "--workflow" + ], + "doctor": [ + "--addr", + "--s3", + "--vm" + ], + "down": [ + "--file", + "--keep-emulator" + ], + "env": [ + "--client", + "--endpoint", + "--s3", + "--s3-region", + "--shell", + "--unset" + ], + "evidence": [ + "--allow-narrowing", + "--contracts", + "--endpoint", + "--join", + "--out", + "--reshape", + "--shapes", + "--suites" + ], + "evidence baseline": [ + "--axes", + "--evidence", + "--out" + ], + "evidence verify": [ + "--accepted", + "--baseline", + "--evidence" + ], + "images": [ + "--check", + "--only", + "--vm" + ], + "logs": [ + "--addr", + "-f", + "-n" + ], + "probe": [ + "--contracts", + "--endpoint", + "--provider" + ], + "proxy": [ + "--addr", + "--expose-to-network", + "--forward", + "--intercept", + "--max-body", + "--provider", + "--queue", + "--record", + "--upstream" + ], + "replay": [ + "--endpoint", + "--format", + "--refusals-only", + "--timeout" + ], + "reset": [ + "--addr" + ], + "restart": [ + "--addr", + "--timeout" + ], + "run": [ + "--addr", + "--endpoint", + "--json", + "--keep-on-failure", + "--provider", + "--runtime", + "--unsafe-real-credentials" + ], + "serve": [ + "--addr", + "--cleanup", + "--consistency", + "--contracts", + "--coverage", + "--expose-to-network", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--shapes", + "--state", + "--strict-catalog", + "--vm" + ], + "shapes": [ + "--check", + "--dir", + "--dry-run", + "--profile", + "--provider", + "--record" + ], + "snapshot list": [ + "--format" + ], + "snapshot load": [ + "--addr" + ], + "snapshot save": [ + "--addr", + "--force" + ], + "start": [ + "--addr", + "--cleanup", + "--contracts", + "--detach", + "--foreground", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--state", + "--strict-catalog", + "--timeout", + "--vm" + ], + "status": [ + "--addr", + "--coverage", + "--format" + ], + "stop": [ + "--addr", + "--timeout" + ], + "transcript": [ + "--against", + "--contract", + "--format", + "--sanitise", + "--shape" + ], + "ui": [ + "--addr", + "--print" + ], + "up": [ + "--file", + "--no-iac", + "--runtime", + "--timeout" + ], + "version": [ + "--check" + ], + "wait": [ + "--addr", + "--timeout" + ] + } + } } ] } diff --git a/internal/core/emulator/checkpoint_test.go b/internal/core/emulator/checkpoint_test.go index 6ad933b7..84d8e66a 100644 --- a/internal/core/emulator/checkpoint_test.go +++ b/internal/core/emulator/checkpoint_test.go @@ -5,12 +5,16 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" + "path/filepath" + "sort" "strings" "testing" "time" "github.com/stephrobert/feint/internal/core/emulator" "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/objectstore" "github.com/stephrobert/feint/internal/core/resource" "github.com/stephrobert/feint/internal/core/store" ) @@ -40,12 +44,22 @@ func (p *reconcilingPack) ReconcileGone(_ context.Context, gone []*resource.Reso } func labServer(t *testing.T, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + return labServerWithBytes(t, nil, packs...) +} + +// labServerWithBytes is the same lab with an optional byte store, because a +// checkpoint's two halves cannot both be asserted against a lab that has one +// of them. The falsification found that: skipping the byte half changed +// nothing in a test whose emulator served no objects. +func labServerWithBytes(t *testing.T, objects *objectstore.Store, packs ...emulator.Pack) (*httptest.Server, *store.Store) { t.Helper() held := store.New() env := &emulator.Env{ - Store: held, - Now: func() time.Time { return time.Unix(1700000000, 0).UTC() }, - NewID: func() string { return "00000000-0000-4000-8000-000000000001" }, + Store: held, + Objects: objects, + Now: func() time.Time { return time.Unix(1700000000, 0).UTC() }, + NewID: func() string { return "00000000-0000-4000-8000-000000000001" }, } srv, err := emulator.NewServer(env, packs...) if err != nil { @@ -107,6 +121,37 @@ func TestACheckpointTakesBothHalvesOrNeither(t *testing.T) { t.Errorf("schema_version %v", body["schema_version"]) } + // With a byte store armed, both halves are taken and the answer says so. + // Asserted on a SECOND lab rather than by widening the first, because the + // absence of a byte store is the ordinary case and must keep its own + // assertion above. + root := t.TempDir() + if err := os.Chmod(root, 0o700); err != nil { + t.Fatal(err) + } + bytesStore, err := objectstore.Open(t.Context(), filepath.Join(root, "objects")) + if err != nil { + t.Fatalf("open the byte store: %v", err) + } + t.Cleanup(func() { + if err := bytesStore.Close(); err != nil { + t.Error(err) + } + }) + withBytes, _ := labServerWithBytes(t, bytesStore, stubPack{name: "acme"}) + code, body = checkpointPost(t, withBytes.URL+"/_feint/checkpoint/baseline") + if code != http.StatusOK { + t.Fatalf("checkpoint with bytes: %d %v", code, body) + } + if carried, _ := body["bytes"].(bool); !carried { + t.Error("a lab with a byte store reported carrying none") + } + // And the byte checkpoint really exists, which is the half a status code + // cannot show: resetting it must find something to restore. + if err := bytesStore.ResetCheckpoint(t.Context(), "baseline"); err != nil { + t.Errorf("the checkpoint named the store and did not take its bytes: %v", err) + } + // A name that is a path component before it is a name. Refused at the // door, because the byte store writes a directory per checkpoint and this // repository's order is a refusal at intake rather than escaping later. @@ -133,12 +178,20 @@ func TestAResetDestroysWhatTheCheckpointDoesNotHold(t *testing.T) { server, held := labServer(t, acme, stubPack{name: "quiet"}) put(t, held, "acme", "instance/server", "kept") + // A second record SHARING that identifier across kinds, which is the + // population a narrower comparison cannot see. The falsification found + // this gap: a difference computed on identifiers alone passed every + // assertion here until these two existed, because every id in the test + // was distinct and a narrowed key still told them apart. + put(t, held, "acme", "instance/ip", "kept") if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { t.Fatalf("checkpoint: %d %v", code, body) } - // The lab moves on: one more resource of each pack. + // The lab moves on: one more resource of each pack, and one that shares + // its identifier with a record the checkpoint DOES hold, on another kind. put(t, held, "acme", "instance/server", "later") + put(t, held, "acme", "instance/volume", "kept") put(t, held, "quiet", "instance/server", "alsolater") code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") @@ -146,16 +199,24 @@ func TestAResetDestroysWhatTheCheckpointDoesNotHold(t *testing.T) { t.Fatalf("reset: %d %v", code, body) } - // The store is back to one resource. - if count, _ := body["resources"].(float64); int(count) != 1 { + // The store is back to the two the checkpoint held. + if count, _ := body["resources"].(float64); int(count) != 2 { t.Errorf("resources after the reset: %v", body["resources"]) } - // And the pack was handed exactly the resource the checkpoint does not - // hold — not the one it does, which is the direction that would destroy a - // machine the checkpoint expects to keep. - if len(acme.got) != 1 || len(acme.got[0]) != 1 || acme.got[0][0] != "later" { + // And the pack was handed exactly the records the checkpoint does not + // hold — not the ones it does, which is the direction that would destroy a + // machine the checkpoint expects to keep. TWO of them, and the second + // shares its identifier with a kept record: a comparison that dropped the + // kind would have taken it for the one that survived and left its machine + // running. + if len(acme.got) != 1 || len(acme.got[0]) != 2 { t.Fatalf("the pack was asked to reconcile %v", acme.got) } + handed := append([]string(nil), acme.got[0]...) + sort.Strings(handed) + if handed[0] != "kept" || handed[1] != "later" { + t.Fatalf("the pack was handed %v, want the volume sharing an id and the extra server", handed) + } if destroyed, _ := body["destroyed"].([]any); len(destroyed) != 1 || destroyed[0] != "feint-acme-later" { t.Errorf("destroyed %v", body["destroyed"]) } diff --git a/tools/falsify/specs/a-reset-is-not-a-restore.json b/tools/falsify/specs/a-reset-is-not-a-restore.json new file mode 100644 index 00000000..2f9751f9 --- /dev/null +++ b/tools/falsify/specs/a-reset-is-not-a-restore.json @@ -0,0 +1,48 @@ +{ + "package": "./internal/core/emulator/", + "note": "The subject is #853's own line: \"reset is not successful because JSON was restored\". `feint snapshot load` and `PUT /_feint/state` restore the control plane verbatim and touch nothing else, so a resource that disappears leaves its container running while the API no longer mentions it. The issue's last closing criterion asks for exactly this spec: \"a falsification that restores only the store but not runtime makes the test fail\".\n\nThe mutations are therefore all of the same family: each one turns the reset back into a restore, by a different route. The route matters, because each is a shape that would look reasonable in review — skipping the reconciliation loop, computing the difference by count instead of identity, handing a pack the resources that SURVIVED, or taking the \"before\" picture after the restore, when it is already gone.\n\nThe last one is the one worth reading twice. Reading the store after the restore instead of before is a one-line reordering that compiles, runs, reports a clean reset, and reconciles nothing at all — because by then there is no difference left to see.", + "mutations": [ + { + "label": "the reconciliation loop is skipped, so a reset restores the control plane and leaves every machine the checkpoint does not hold running. This is the gap #853 names, reproduced exactly: the API no longer mentions the container and nobody takes it down", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\treconciler, ok := pack.(Reconciler)\n\t\tif !ok {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\treconciler, ok := pack.(Reconciler)\n\t\tif !ok || true {\n\t\t\t_ = reconciler\n\t\t\tcontinue\n\t\t}", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the before picture is taken AFTER the restore instead of before, so there is no difference left to compute and the reset reconciles nothing while reporting a clean pass. A one-line reordering that compiles, runs and lies", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tbefore := s.env.Store.All()\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", + "replace": "\tbefore := []*resource.Resource(nil)\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "each pack is handed the resources that SURVIVED the restore rather than the ones that did not, so a reset destroys exactly the machines the checkpoint expects to keep. The opposite error of the first mutation, and the expensive one: it takes down what the operator asked to come back", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\t\tif res.Tenant.Provider == pack.Name() {\n\t\t\t\tmine = append(mine, res)\n\t\t\t}", + "replace": "\t\t\tif res.Tenant.Provider != pack.Name() {\n\t\t\t\tmine = append(mine, res)\n\t\t\t}", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the difference is computed by comparing identifiers only, dropping the kind and the provider, so two resources that share an id across products are taken for one another. Written as a narrowed key rather than a count because the count version does not compile, and the narrowed one is what a reviewer would wave through", + "file": "internal/core/emulator/checkpoint.go", + "find": "\treturn res.Tenant.Provider + \"\\x00\" + res.Kind + \"\\x00\" + res.ID", + "replace": "\treturn res.ID", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the byte half of a checkpoint is skipped, so the store is named and the bytes are not. A later reset restores the control plane, finds no byte checkpoint, and an operator reads a clean reset over a store whose objects are whatever the last test left", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", + "replace": "\tif s.env.Objects != nil && false {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", + "test": "TestACheckpointTakesBothHalvesOrNeither" + }, + { + "label": "a checkpoint name stops being validated, so a name that is a path component before it is a name reaches the byte store, which writes a directory per checkpoint. This repository's order is a refusal at intake rather than escaping later, and `..` is the whole reason", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tif !validCheckpointName(name) {", + "replace": "\tif !validCheckpointName(name) && false {", + "test": "TestACheckpointTakesBothHalvesOrNeither" + } + ] +} From 2cef2afe107d4da0f27d796943f9f59328a903e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 12:59:04 +0100 Subject: [PATCH 5/7] test(conformance): a reset reconciles the host and touches nothing else (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #853's closing criterion that no unit test can reach — "no foreign runtime object is touched" — closed with a measurement. Run on this station, Incus 7.2, `--vm incus`: ok: the checkpoint's machine exists: feint-scw-d539d3eb-… ok: the second machine exists: feint-scw-60c339c4-… destroyed: feint-scw-60c339c4-… reset "reset-host": the lab converged ok: the machine the checkpoint does not hold is gone ok: the machine the checkpoint holds is still there ok: every container feint did not create is still there ## The assertion in the middle is the product A reset that swept both machines would return an operator to a checkpoint whose servers answer nothing, which is worse than no reset at all. The reconciliation is exercised elsewhere through a recording driver, and that proves which names the code emits; only a real runtime proves what happens to them. The third assertion reads `incus list` before and after and requires every stranger's container BY NAME, rather than counting: a count cannot tell a neighbour's container from one of ours, and this repository has already measured a forced removal reaching a name it did not own. ## The leak this suite had on its first run It ends with a machine alive on purpose, which is the point of its third assertion. The first version therefore left that container running on the maintainer's station, and it was found by reading `incus list` after the pass rather than by any gate here — the same shape as the lab the run tests leaked two days ago, and the second time in one week that my own harness was the thing left behind. It now sweeps through `feint clean`, which removes what the emulator created by the labels it wrote and nothing else. The trap is armed BEFORE the first server is created, so a failure in the middle does not leak either: a cleanup that only runs when the suite passes is not a cleanup, and the run that leaks is by definition the one that failed. ## Both populations driven before committing With no runtime it skips and says why. Under `--vm incus` it asserts all five. Measured after the pass: only the operator's own container remains, and the station's available memory is where it started. ## Where it runs The runtime leg of `tools/conformance/leg.sh`, the full pass in mise.toml, and `runtime-proof.yml`. The testplan gate refused the rule until the file was tracked — "matches no tracked path: it is either a directory that moved, or coverage of nothing" — which is it working on a file I had not added yet. What it does NOT cover is in the rule's own Unproven: a reset interrupted part-way, a checkpoint whose machine died on its own, and more than two machines. shellcheck clean, YAML parsed, mise run prepush green. Assisted-by: Claude Code (claude-opus-5) --- .github/workflows/runtime-proof.yml | 7 ++ mise.toml | 1 + tools/conformance/leg.sh | 5 ++ tools/conformance/reset-host.sh | 109 ++++++++++++++++++++++++++++ tools/testplan/rules.go | 13 ++++ 5 files changed, 135 insertions(+) create mode 100755 tools/conformance/reset-host.sh diff --git a/.github/workflows/runtime-proof.yml b/.github/workflows/runtime-proof.yml index 37b5771c..27682020 100644 --- a/.github/workflows/runtime-proof.yml +++ b/.github/workflows/runtime-proof.yml @@ -415,6 +415,13 @@ jobs: - name: feint run sweeps the machines it created, and only those run: sudo tools/conformance/run-sweep.sh http://127.0.0.1:4599 + # The host half of a reset (#853). Its third assertion is the + # product: a reset that swept both machines would return an + # operator to a checkpoint whose servers answer nothing, which is + # worse than no reset at all. + - name: a reset reconciles the host and touches nothing else + run: sudo tools/conformance/reset-host.sh http://127.0.0.1:4599 + # The end of the chain the emulator exists for: a key registered through # the API, a server booted by the official client, a real shell on the # provider's own default login — three providers, three logins. diff --git a/mise.toml b/mise.toml index 92b46559..2ca2a924 100644 --- a/mise.toml +++ b/mise.toml @@ -331,6 +331,7 @@ run = [ # itself under --vm incus and --vm incus-vm rather than failing there. "tools/conformance/outscale/balancer.sh http://$FEINT_ADDR", "tools/conformance/run-sweep.sh http://$FEINT_ADDR", + "tools/conformance/reset-host.sh http://$FEINT_ADDR", ] [tasks."conformance:exoscale-terraform"] diff --git a/tools/conformance/leg.sh b/tools/conformance/leg.sh index 8300aa35..b09dae31 100755 --- a/tools/conformance/leg.sh +++ b/tools/conformance/leg.sh @@ -177,6 +177,11 @@ case "$leg" in # command that starts and stops one, so the endpoint is read only to ask # whether a runtime is configured at all (#848). tools/conformance/run-sweep.sh "$endpoint" + # The host half of a reset (#853), which no unit test can reach: a + # recording driver proves which names the code emits, and this proves what + # a real runtime does with them — above all that the machine the + # checkpoint HOLDS is still there afterwards. + tools/conformance/reset-host.sh "$endpoint" ;; esac diff --git a/tools/conformance/reset-host.sh b/tools/conformance/reset-host.sh new file mode 100755 index 00000000..27e20d6f --- /dev/null +++ b/tools/conformance/reset-host.sh @@ -0,0 +1,109 @@ +#!/usr/bin/env bash +# Conformance check: a reset reconciles the HOST, and touches nothing else. +# +# This is #853's closing criterion that no unit test can reach: "no foreign +# runtime object is touched". The reconciliation is exercised elsewhere through +# a recording driver, which proves which names the code emits; this proves what +# a real runtime does with them. +# +# Three assertions, and the middle one is the product: +# +# - the machine of a resource the checkpoint does NOT hold is gone; +# - the machine of a resource the checkpoint DOES hold is still running. A +# reset that swept both would return an operator to a checkpoint whose +# servers answer nothing, which is worse than no reset; +# - every container the emulator did not create is still there, by name. +# +# The third is why `incus list` is read before and after rather than counted: +# a count cannot tell a neighbour's container from one of ours, and this +# repository has already measured a forced removal reaching a name it did not +# own. +# +# Usage: tools/conformance/reset-host.sh [endpoint] +set -euo pipefail + +ENDPOINT="${1:-http://127.0.0.1:4599}" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +FEINT="${FEINT_BIN:-./feint}" + +# shellcheck source=/dev/null +. "$SCRIPT_DIR/guard.sh" +guard_local "$ENDPOINT" + +fail() { echo "FAIL: $*" >&2; exit 1; } +ok() { echo " ok: $*"; } +skip() { echo " SKIP: $*" >&2; } + +echo "conformance: a reset reconciles the host and touches nothing else" + +mode="$(curl -fsS --max-time 20 "$ENDPOINT/_feint/health" 2>/dev/null \ + | python3 -I -c 'import sys,json; print(json.load(sys.stdin).get("machines",""))' 2>/dev/null || true)" +if [ -z "$mode" ] || [ "$mode" = "none" ] || [ "$mode" = "off" ]; then + skip "no machine runtime (start with FEINT_VM=incus); the host half cannot be measured" + exit 0 +fi +command -v incus >/dev/null 2>&1 || { skip "incus is not on PATH, so what survives cannot be read"; exit 0; } +command -v scw >/dev/null 2>&1 || { skip "the scw CLI is absent, and this suite drives a real client"; exit 0; } + +# This suite ENDS with a machine alive on purpose: the point of its third +# assertion is that the reset did not take it down. So it sweeps after itself, +# through `feint clean`, which removes what the emulator created by the labels +# it wrote and nothing else. Armed before the first server is created, so a +# failure in the middle does not leak either — the first version of this suite +# left a container running on the maintainer's station, found by reading +# `incus list` after the pass rather than by any gate. +sweep() { "$FEINT" clean --vm "$mode" >/dev/null 2>&1 || true; } +trap sweep EXIT + +ADDR="${ENDPOINT#http://}" +set -a +# shellcheck source=/dev/null +. "$SCRIPT_DIR/scaleway/fake-credentials.env" +set +a +export SCW_API_URL="$ENDPOINT" + +names() { incus list -f csv -c n 2>/dev/null | tr -d '\r'; } +ours() { names | grep '^feint-' || true; } +strangers(){ names | grep -v '^feint-' || true; } + +before_strangers="$(strangers)" +[ -z "$(ours)" ] || fail "feint-owned containers exist before this suite; nothing below could be attributed" + +echo "-- a machine the checkpoint will hold" +scw instance server create type=DEV1-S image=ubuntu_jammy name=reset-kept zone=fr-par-1 -o json >/dev/null +kept="$(ours | head -1)" +[ -n "$kept" ] || fail "no container was created for the first server, so this suite would measure nothing" +ok "the checkpoint's machine exists: $kept" + +"$FEINT" checkpoint reset-host --addr "$ADDR" >/dev/null || fail "the checkpoint was refused" + +echo "-- a machine it will not" +scw instance server create type=DEV1-S image=ubuntu_jammy name=reset-later zone=fr-par-1 -o json >/dev/null +after_second="$(ours | wc -l)" +[ "$after_second" -eq 2 ] || fail "expected two feint-owned containers, found $after_second" +extra="$(ours | grep -v "^$kept$" | head -1)" +ok "the second machine exists: $extra" + +echo "-- feint reset reset-host" +"$FEINT" reset reset-host --addr "$ADDR" || fail "the reset reported that the lab did not converge" +ok "the reset reports convergence" + +left="$(ours)" +printf '%s\n' "$left" | grep -qx "$extra" && fail "the machine the checkpoint does not hold outlived the reset: $extra" +ok "the machine the checkpoint does not hold is gone" + +# The half a sweep would get wrong in the expensive direction. +printf '%s\n' "$left" | grep -qx "$kept" || fail "the reset destroyed the machine the checkpoint HOLDS: $kept" +ok "the machine the checkpoint holds is still there" + +missing="" +while IFS= read -r stranger; do + [ -n "$stranger" ] || continue + printf '%s\n' "$(strangers)" | grep -qx "$stranger" || missing="$missing $stranger" +done < Date: Wed, 11 Nov 2026 13:10:25 +0100 Subject: [PATCH 6/7] feat(cli): feint run --reset-after, on all three ways out (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #853's crash behaviour asks for a reset attempted on command success, command failure and SIGINT/SIGTERM, and concedes the fourth in the same breath: a hard kill cannot promise anything. The three it asks for are measured against a SHARED emulator, which is where the flag earns its place — an owned lab disappears at the end whatever happens. exit 1: the command succeeds code 0 reset=converged resources 0 exit 2: the command fails code 7 reset=converged resources 0 exit 3: a relayed signal code 255 reset=converged resources 0 control, without the flag resources 4 The control line is the one that gives the other three meaning: without `--reset-after`, the run leaves four resources in the shared emulator. ## Two flags that cannot both be right `--reset-after` erases the state of a failed run; `--keep-on-failure` keeps it for inspection. Refused together rather than silently ordered, because either order is wrong for somebody and a reader cannot tell which they got. ## A reset's failure is reported, never raised into the child's verdict The child has already run when the reset is attempted, and its exit code is what a caller builds on. So the verdict is a word the receipt carries, and the four are not interchangeable: "not attempted", "failed", "unverified" and "converged" send a reader to four different places. The emulator's own reason is relayed rather than summarised — a reset refused because the store could not be restored and one refused because the bytes could not be are different problems. **But a reset that did not converge fails the command**, even when the child passed: the caller asked for the lab to be left as found and it was not, and a quiet 0 is how the next test inherits the mess and blames itself. The session is named after the run's own directory, so two runs against one shared emulator cannot reset each other's state. A mismatch is "not attempted" rather than a reset of somebody else's checkpoint. ## A number nobody can act on, made readable A signal-killed child gives Go an exit code of -1, and the receipt printed `failed (-1)`. It now says "failed (the command was killed by a signal)" in prose while the JSON keeps the number, documented on the field. The measurement is what showed it: the third exit above printed -1 and I could not tell from the receipt what had happened. ## The mutation that caught my tests again Fourteen mutations on the spec now, and one of the two added here stayed GREEN on the first run: "a reset that did not converge stops failing the command". No test drove a reset that fails to converge, so the guard had nothing holding it. TestARunWhoseResetDidNotConvergeFails drives a lab that answers one more resource after the reset than at the checkpoint — the reset ran, the emulator said ok, the state did not come back — with a child that PASSES on purpose. A reset's failure must not hide behind a failing test, and must not hide behind a passing one either. It also asserts the receipt blames the right half: `reset: did not converge` with `exit: 0`. ## Not here The recreating half, which is #853's remaining criterion and needs a Boot only each pack can render. `--reset-after` on an emulator with no S3 door is refused by `feint run`'s own interception requirement, which is #849's design and not a new limit. `cliSurfaceVersion` 31 → 32, fixture regenerated, help names the flag. mise run prepush green. Assisted-by: Claude Code (claude-opus-5) --- internal/cli/checkpoint.go | 54 +++++ internal/cli/cli.go | 10 +- internal/cli/reset_after_test.go | 250 ++++++++++++++++++++ internal/cli/run.go | 40 ++++ internal/cli/run_lifecycle.go | 30 ++- internal/cli/testdata/frozen/cli.json | 248 +++++++++++++++++++ tools/falsify/specs/a-run-owns-its-lab.json | 14 ++ 7 files changed, 641 insertions(+), 5 deletions(-) create mode 100644 internal/cli/reset_after_test.go diff --git a/internal/cli/checkpoint.go b/internal/cli/checkpoint.go index 1d16e209..d3d7fdf7 100644 --- a/internal/cli/checkpoint.go +++ b/internal/cli/checkpoint.go @@ -351,3 +351,57 @@ func readRecordedCheckpoint(addr string) (recordedCheckpoint, error) { } return recorded, nil } + +// takeSessionCheckpoint names the state a `feint run --reset-after` returns to. +// +// It is the lab's own session identity, not a name the operator typed: two +// runs against one shared emulator must not be able to reset each other's +// state, and a fixed name would let them. +// +// TestASessionCheckpointIsNamedAfterItsRun fails without this. +func takeSessionCheckpoint(endpoint, session, addr string) error { + if _, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/checkpoint/"+session); err != nil { + return fmt.Errorf("the session checkpoint could not be taken: %w", err) + } + state, runtimeName, err := observeFor(endpoint) + if err != nil { + return fmt.Errorf("the session checkpoint could not be read back: %w", err) + } + return writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: session, + Taken: time.Now().UTC().Format(time.RFC3339), Runtime: runtimeName, State: state, + }) +} + +// resetToSession returns the lab to its session checkpoint and answers a +// verdict plus whatever is left over. +// +// It never returns an error to the caller, and that is deliberate: the child +// has already run, and its exit code is what a caller builds on. A reset that +// could not even be attempted must be REPORTED rather than swallowed, and +// rather than raised into the child's verdict where it would read as a failing +// test. So the verdict is a word the receipt carries, and the four words are +// not interchangeable — "not attempted", "failed", "unverified" and +// "converged" send a reader to four different places. +// +// TestAFailedResetAfterIsReportedAndNotRaised fails without this. +func resetToSession(endpoint, session, addr string, stderr io.Writer) (string, []string) { + recorded, err := readRecordedCheckpoint(addr) + if err != nil || recorded.Name != session { + fmt.Fprintf(stderr, "feint run: --reset-after has no session checkpoint to return to: %v\n", err) + return "not attempted", nil + } + if _, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/reset/"+session); err != nil { + fmt.Fprintf(stderr, "feint run: the reset failed: %v\n", err) + return "failed", nil + } + state, _, err := observeFor(endpoint) + if err != nil { + fmt.Fprintf(stderr, "feint run: the reset ran and could not be read back: %v\n", err) + return "unverified", nil + } + if differences := recorded.State.diff(state); len(differences) != 0 { + return "did not converge", differences + } + return "converged", nil +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 01b076e5..b25dfcd0 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -249,7 +249,7 @@ const ( // TestTheFrozenSurfacesStillMatchTheirFixture, and a fixture regenerated // without bumping this constant fails TestASurfaceChangeDemandsItsVersionBump. // The procedure for a deliberate change is in RELEASING.md ("Frozen surfaces"). -const cliSurfaceVersion = 31 +const cliSurfaceVersion = 32 // Run executes one command and returns the process exit code. func Run(args []string, stdout, stderr io.Writer) int { @@ -535,7 +535,8 @@ Usage: behind it on the network. feint run [--provider scaleway] [--endpoint http://127.0.0.1:4599] - [--addr 127.0.0.1:0] [--keep-on-failure] [--json] + [--addr 127.0.0.1:0] [--keep-on-failure] [--reset-after] + [--json ] [--unsafe-real-credentials] -- [args...] Run one command against a local cloud this command owns. With no --endpoint it starts an emulator, waits for it, @@ -564,6 +565,11 @@ Usage: --json writes the receipt as JSON to a path, or to stdout with a single dash; stdout otherwise belongs to your command alone. The exit code is the child's. + --reset-after takes a checkpoint once the lab is ready + and returns to it when the command ends, however it ends: + success, failure, or a signal this command relayed. It is + refused with --keep-on-failure, which keeps the state it + would erase. --runtime asks the host for a machine runtime before anything starts, with the refusal up makes. off is the default: starting containers is a side effect on this diff --git a/internal/cli/reset_after_test.go b/internal/cli/reset_after_test.go new file mode 100644 index 00000000..309c0ba4 --- /dev/null +++ b/internal/cli/reset_after_test.go @@ -0,0 +1,250 @@ +package cli + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// `feint run --reset-after` (#853's crash behaviour), in the parts a unit test +// can reach. +// +// What it asks for is a reset attempted on command success, command failure +// and a relayed signal, and it concedes the fourth case in the same breath: a +// hard kill cannot promise anything. So these tests hold the decisions, and +// the three ways out are driven against the built binary where the signal is +// real. + +// TestResetAfterAndKeepOnFailureAreRefusedTogether is cited by runCommand. +// +// Two flags that mean opposite things about a failed run: one erases the +// state, the other keeps it for inspection. Refused rather than silently +// ordered, because either order is wrong for somebody and a reader cannot tell +// which they got. +func TestResetAfterAndKeepOnFailureAreRefusedTogether(t *testing.T) { + server := reportServer(t, door("scaleway", "fr-par")) + var stdout, stderr bytes.Buffer + code := runCommand([]string{ + "--endpoint", server.URL, "--reset-after", "--keep-on-failure", "--", "true", + }, &stdout, &stderr) + if code != exitError { + t.Fatalf("both flags together were accepted (%d)", code) + } + for _, want := range []string{"--reset-after", "--keep-on-failure", "pick one"} { + if !strings.Contains(stderr.String(), want) { + t.Errorf("the refusal does not mention %q: %s", want, stderr.String()) + } + } + + // And each alone is not refused, so this cannot be satisfied by refusing + // the pair's members. + for _, flag := range []string{"--reset-after", "--keep-on-failure"} { + stdout.Reset() + stderr.Reset() + if code := runCommand([]string{ + "--endpoint", server.URL, flag, "--", "true", + }, &stdout, &stderr); code == exitError && strings.Contains(stderr.String(), "pick one") { + t.Errorf("%s alone was refused as a pair: %s", flag, stderr.String()) + } + } +} + +// TestAFailedResetAfterIsReportedAndNotRaised is cited by resetToSession. +// +// The child has already run when a reset is attempted, and its exit code is +// what a caller builds on. A reset that could not be attempted must be +// reported rather than swallowed, and rather than raised into the child's +// verdict where it would read as a failing test. The four words are not +// interchangeable: they send a reader to four different places. +func TestAFailedResetAfterIsReportedAndNotRaised(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + + t.Run("no session checkpoint recorded", func(t *testing.T) { + var stderr bytes.Buffer + verdict, left := resetToSession("http://"+addr, "feint-lab-1", addr, &stderr) + if verdict != "not attempted" { + t.Errorf("verdict %q", verdict) + } + if len(left) != 0 { + t.Errorf("leftovers %v for a reset that never ran", left) + } + if stderr.Len() == 0 { + t.Error("nothing was reported") + } + }) + + t.Run("the emulator refuses the reset", func(t *testing.T) { + refusing := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"the control plane could not be restored"}`)) + })) + defer refusing.Close() + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "feint-lab-1", + }); err != nil { + t.Fatal(err) + } + var stderr bytes.Buffer + verdict, _ := resetToSession(refusing.URL, "feint-lab-1", addr, &stderr) + if verdict != "failed" { + t.Errorf("verdict %q", verdict) + } + // The emulator's own words, not a summary of them: a reset refused + // because the store could not be restored and one refused because the + // bytes could not be are different problems. + if !strings.Contains(stderr.String(), "control plane could not be restored") { + t.Errorf("the emulator's reason was dropped: %s", stderr.String()) + } + }) + + t.Run("a session that is not this run's is not reset", func(t *testing.T) { + // Two runs against one shared emulator must not reset each other's + // state. The recorded name decides, and a mismatch is "not attempted" + // rather than a reset of somebody else's checkpoint. + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "feint-lab-OTHER", + }); err != nil { + t.Fatal(err) + } + var stderr bytes.Buffer + verdict, _ := resetToSession("http://"+addr, "feint-lab-1", addr, &stderr) + if verdict != "not attempted" { + t.Errorf("verdict %q for another run's session", verdict) + } + }) +} + +// TestASessionCheckpointIsNamedAfterItsRun is cited by takeSessionCheckpoint. +// +// The name is the lab's own session identity and not one the operator typed, +// so that two runs against one shared emulator cannot reset each other's +// state. A fixed name would let them, and the failure would look like a +// flaky test rather than a collision. +func TestASessionCheckpointIsNamedAfterItsRun(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + + var asked []string + lab := checkpointingLab(t, &asked) + if err := takeSessionCheckpoint(lab.URL, "feint-lab-4021", addr); err != nil { + t.Fatalf("take the session checkpoint: %v", err) + } + if len(asked) != 1 || asked[0] != "/_feint/checkpoint/feint-lab-4021" { + t.Fatalf("the emulator was asked for %v", asked) + } + recorded, err := readRecordedCheckpoint(addr) + if err != nil { + t.Fatalf("read the record back: %v", err) + } + if recorded.Name != "feint-lab-4021" { + t.Errorf("the record names %q", recorded.Name) + } + if recorded.Taken == "" { + t.Error("the record carries no time, so two runs cannot be told apart in a log") + } +} + +// checkpointingLab answers the endpoints takeSessionCheckpoint drives, and +// records which paths were asked for. +func checkpointingLab(t *testing.T, asked *[]string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/_feint/checkpoint/", func(w http.ResponseWriter, r *http.Request) { + *asked = append(*asked, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"schema_version":1,"name":"x","resources":0,"bytes":false}`)) + }) + mux.HandleFunc("/_feint/health", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"machines":"none","capabilities":null,"enforced":{}}`)) + }) + mux.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"count":0,"resources":[]}`)) + }) + mux.HandleFunc("/_feint/s3", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":2,"enabled":false,"doors":[],"http_requests":{}}`)) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// TestARunWhoseResetDidNotConvergeFails is cited by runCommand's exit code. +// +// The expensive case, and the one the falsification caught my tests not +// covering: a shared emulator left dirty by a run that reported success. The +// caller asked for the lab to be left as found, and a quiet 0 is how the next +// test inherits the mess and blames itself. +// +// The child passes on purpose. A reset's failure must not be hidden behind a +// failing test, and it must not be hidden behind a passing one either. +func TestARunWhoseResetDidNotConvergeFails(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + + // A lab that answers one more resource after the reset than it did at the + // checkpoint: the reset ran, the emulator said ok, and the state did not + // come back. That is exactly the shape #853 refuses to call a success. + var resourceReads int + mux := http.NewServeMux() + mux.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + resourceReads++ + if resourceReads == 1 { + _, _ = w.Write([]byte(`{"count":0,"resources":[]}`)) + return + } + _, _ = w.Write([]byte(`{"count":1,"resources":[` + + `{"id":"leftover","kind":"instance/server","state":"running"}]}`)) + }) + mux.HandleFunc("/_feint/checkpoint/", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":1,"name":"x","resources":0,"bytes":false}`)) + }) + mux.HandleFunc("/_feint/reset/", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":1,"name":"x","resources":0,` + + `"destroyed":[],"skipped":[],"recreated":[],"recreates":false}`)) + }) + mux.HandleFunc("/_feint/health", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"machines":"none","capabilities":null,"enforced":{}}`)) + }) + mux.HandleFunc("/_feint/trace", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":1,"count":0,"kept":256,"exchanges":[]}`)) + }) + mux.HandleFunc("/_feint/s3", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":2,"enabled":true,` + + `"max_blob_bytes":1073741824,"max_object_bytes":268435456,` + + `"http_requests":{"GET":1},"doors":[{"provider":"scaleway","region":"fr-par",` + + `"path":"/_feint/s3/scaleway/fr-par/"}]}`)) + }) + mux.HandleFunc("/_feint/s3/scaleway/fr-par/", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("")) + }) + lab := httptest.NewServer(mux) + defer lab.Close() + + var stdout, stderr bytes.Buffer + code := runCommand([]string{ + "--endpoint", lab.URL, "--reset-after", "--json", "-", "--", "true", + }, &stdout, &stderr) + if code == exitOK { + t.Fatalf("a run whose reset left a resource behind reported success\nstdout:%s\nstderr:%s", + stdout.String(), stderr.String()) + } + + // And the receipt says which it was, because "the test failed" and "the + // lab was left dirty" send a reader to two different places. + var receipt runReceipt + if err := json.Unmarshal(stdout.Bytes(), &receipt); err != nil { + t.Fatalf("stdout is not the receipt alone: %v\n%s", err, stdout.String()) + } + if receipt.Reset != "did not converge" { + t.Errorf("the receipt reports reset=%q", receipt.Reset) + } + if receipt.Exit != exitOK { + t.Errorf("the receipt blames the child, which passed: exit=%d", receipt.Exit) + } + if len(receipt.Unreconciled) == 0 { + t.Error("the receipt names nothing it could not reconcile, which is what #853 asks for") + } +} diff --git a/internal/cli/run.go b/internal/cli/run.go index 008beb30..8e41a8ed 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -81,6 +81,7 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { provider := fs.String("provider", "", "resolve only this provider's client environment into the child; empty resolves every pack's") addr := fs.String("addr", "127.0.0.1:0", "the proxy's listen address; the default takes a free loopback port and prints it") runtimeMode := fs.String("runtime", "off", "the machine runtime the lab starts with: off, incus, incus-vm, incus-ovn, auto. Anything but off starts containers on this host, so it is asked for") + resetAfter := fs.Bool("reset-after", false, "take a checkpoint once the lab is ready and return to it when the command ends, however it ends (#853). Refused with --keep-on-failure, which preserves the state this would erase") keep := fs.Bool("keep-on-failure", false, "when the child fails, leave the lab running and print how to reach and remove it") receiptPath := fs.String("json", "", "write the receipt as JSON to this path; `-` means stdout, which is only safe when the command itself is quiet") unsafe := fs.Bool("unsafe-real-credentials", false, "allow the child to carry credentials that are not known fakes. The emulator never checks a signature, so this buys nothing and risks sending real keys to a third party") @@ -102,6 +103,16 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { return exitError } + // Two flags that mean opposite things about a failed run: one erases the + // state, the other preserves it for inspection. Refused together rather + // than silently ordered, because either order is wrong for somebody and a + // reader cannot tell which they got. + // TestResetAfterAndKeepOnFailureAreRefusedTogether fails without this. + if *resetAfter && *keep { + fmt.Fprintln(stderr, "feint run: --reset-after erases the state --keep-on-failure keeps for you; pick one") + return exitError + } + // The runtime, asked of the host before anything starts, which is `up`'s // rule and for its reason: a refusal must fire with nothing running. The // same seam answers, so the refusal says what `up`'s says. @@ -181,6 +192,18 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { stripped := hazardVariables() childEnv := childEnvironment{strip: stripped, set: exports} + // The session's initial state, named after the run's own directory so two + // runs against one shared emulator cannot reset each other's. + session := labSession(owned) + if session == "" { + session = "feint-run-session" + } + if *resetAfter { + if err := takeSessionCheckpoint(target, session, *addr); err != nil { + return fail(err) + } + } + // Counted before and after, so an ATTACHED lab's earlier traffic is not // reported as this run's. The owned case starts at zero and the delta is // the same number; the attached case is where a total would lie. @@ -196,6 +219,15 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { // restart. Report what was last seen and say it is a window. calls, truncated = after, true } + // The reset, attempted on every way out of the child: success, failure, + // and a SIGTERM this command relayed and waited for. #853 asks for the + // three and concedes the fourth — a hard kill of this process cannot + // promise anything, and `feint doctor` is what finds that afterwards. + resetVerdict, unreconciled := "", []string(nil) + if *resetAfter { + resetVerdict, unreconciled = resetToSession(target, session, *addr, stderr) + } + runtimeName, capabilities, proof := labHealth(target) cleanup := "not owned" @@ -230,6 +262,7 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { Providers: providers, Intercepted: intercepted, Command: command, Calls: calls, CallsTruncated: truncated, Stripped: stripped, Exit: code, Cleanup: cleanup, + Reset: resetVerdict, Unreconciled: unreconciled, RealCloud: "no known escape observed", } // stdout belongs to the CHILD, and to nothing else. Measured rather than @@ -245,6 +278,13 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { return exitError } } + // A reset that did not converge is this command's failure even when the + // child passed: the caller asked for the lab to be left as found, and it + // was not. Reported separately from the child's code so a reader can tell + // a failing test from a leaking lab. + if resetVerdict == "did not converge" && code == exitOK { + return exitError + } // The child's code, not this command's: a wrapper that swallowed a // failing test would be worse than no wrapper. return code diff --git a/internal/cli/run_lifecycle.go b/internal/cli/run_lifecycle.go index 660a1557..7f98e21c 100644 --- a/internal/cli/run_lifecycle.go +++ b/internal/cli/run_lifecycle.go @@ -300,8 +300,20 @@ type runReceipt struct { // window. The emulator publishes the window for exactly this reason. CallsTruncated bool `json:"calls_truncated"` Stripped []string `json:"stripped"` - Exit int `json:"exit"` - Cleanup string `json:"cleanup"` + // Exit is the child's code, and NEGATIVE means a signal killed it, which + // is what Go answers and what the prose above says in words. A reader of + // the JSON gets the number; a reader of the receipt gets the sentence. + // Reset is empty when --reset-after was not asked for, and otherwise + // says what happened: a run that reset and a run that did not are + // different facts, and an empty Unreconciled must not be read as + // "it converged". + Reset string `json:"reset,omitempty"` + // Unreconciled names what the reset could not bring back, which is + // #853's own wording: a reset reports exactly what it could not + // reconcile. + Unreconciled []string `json:"unreconciled,omitempty"` + Exit int `json:"exit"` + Cleanup string `json:"cleanup"` // RealCloud is a field and not a sentence render() prints, because the // prose and the JSON must not be able to disagree about the one claim a // reader of this receipt cares about most. @@ -342,7 +354,13 @@ func callsSoFar(endpoint string) (int, bool) { // render writes the compact receipt #848 describes. func (r runReceipt) render(w io.Writer) { verdict := "passed" - if r.Exit != 0 { + switch { + case r.Exit < 0: + // Go answers -1 for a process a signal killed, and -1 is a number + // nobody can act on. The word is what a reader needs; the field keeps + // the number for a machine, and the JSON documents it. + verdict = "failed (the command was killed by a signal)" + case r.Exit != 0: verdict = fmt.Sprintf("failed (%d)", r.Exit) } fmt.Fprintf(w, "feint run: %s\n", verdict) @@ -369,6 +387,12 @@ func (r runReceipt) render(w io.Writer) { if len(r.RuntimeProof) != 0 { row("runtime proof", strings.Join(r.RuntimeProof, ", ")) } + if r.Reset != "" { + row("reset", r.Reset) + for _, difference := range r.Unreconciled { + fmt.Fprintf(w, " %s\n", difference) + } + } row("cleanup", r.Cleanup) // Deliberately narrow wording, and a field rather than a literal here: // "no known escape observed" is what #848's own example prints and what diff --git a/internal/cli/testdata/frozen/cli.json b/internal/cli/testdata/frozen/cli.json index d17a1a43..904f4214 100644 --- a/internal/cli/testdata/frozen/cli.json +++ b/internal/cli/testdata/frozen/cli.json @@ -5884,6 +5884,254 @@ ] } } + }, + { + "schema_version": 32, + "content": { + "exit_codes": { + "drift": 2, + "error": 1, + "ok": 0 + }, + "verbs": { + "catalog": [ + "--format" + ], + "checkpoint": [ + "--addr" + ], + "clean": [ + "--check", + "--closing", + "--doorstep", + "--force", + "--format", + "--vm" + ], + "corpus": [ + "--accepted", + "--against-cloud", + "--bind", + "--check", + "--credential", + "--dir", + "--dry-run", + "--endpoint", + "--file", + "--format", + "--mark-stale", + "--timeout" + ], + "coverage": [ + "--artefact", + "--axis", + "--baseline", + "--contract", + "--contract-only", + "--document", + "--evidence", + "--fail-on-unknown", + "--format", + "--gaps", + "--observed", + "--products", + "--provider", + "--sdk", + "--write-baseline" + ], + "docs": [ + "--check", + "--client-pins", + "--confidence", + "--contracts", + "--coverage", + "--file", + "--install", + "--limits", + "--proved", + "--routes", + "--screenshots", + "--ui-manifest", + "--workflow" + ], + "doctor": [ + "--addr", + "--s3", + "--vm" + ], + "down": [ + "--file", + "--keep-emulator" + ], + "env": [ + "--client", + "--endpoint", + "--s3", + "--s3-region", + "--shell", + "--unset" + ], + "evidence": [ + "--allow-narrowing", + "--contracts", + "--endpoint", + "--join", + "--out", + "--reshape", + "--shapes", + "--suites" + ], + "evidence baseline": [ + "--axes", + "--evidence", + "--out" + ], + "evidence verify": [ + "--accepted", + "--baseline", + "--evidence" + ], + "images": [ + "--check", + "--only", + "--vm" + ], + "logs": [ + "--addr", + "-f", + "-n" + ], + "probe": [ + "--contracts", + "--endpoint", + "--provider" + ], + "proxy": [ + "--addr", + "--expose-to-network", + "--forward", + "--intercept", + "--max-body", + "--provider", + "--queue", + "--record", + "--upstream" + ], + "replay": [ + "--endpoint", + "--format", + "--refusals-only", + "--timeout" + ], + "reset": [ + "--addr" + ], + "restart": [ + "--addr", + "--timeout" + ], + "run": [ + "--addr", + "--endpoint", + "--json", + "--keep-on-failure", + "--provider", + "--reset-after", + "--runtime", + "--unsafe-real-credentials" + ], + "serve": [ + "--addr", + "--cleanup", + "--consistency", + "--contracts", + "--coverage", + "--expose-to-network", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--shapes", + "--state", + "--strict-catalog", + "--vm" + ], + "shapes": [ + "--check", + "--dir", + "--dry-run", + "--profile", + "--provider", + "--record" + ], + "snapshot list": [ + "--format" + ], + "snapshot load": [ + "--addr" + ], + "snapshot save": [ + "--addr", + "--force" + ], + "start": [ + "--addr", + "--cleanup", + "--contracts", + "--detach", + "--foreground", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--state", + "--strict-catalog", + "--timeout", + "--vm" + ], + "status": [ + "--addr", + "--coverage", + "--format" + ], + "stop": [ + "--addr", + "--timeout" + ], + "transcript": [ + "--against", + "--contract", + "--format", + "--sanitise", + "--shape" + ], + "ui": [ + "--addr", + "--print" + ], + "up": [ + "--file", + "--no-iac", + "--runtime", + "--timeout" + ], + "version": [ + "--check" + ], + "wait": [ + "--addr", + "--timeout" + ] + } + } } ] } diff --git a/tools/falsify/specs/a-run-owns-its-lab.json b/tools/falsify/specs/a-run-owns-its-lab.json index f8081dfe..eff81685 100644 --- a/tools/falsify/specs/a-run-owns-its-lab.json +++ b/tools/falsify/specs/a-run-owns-its-lab.json @@ -85,6 +85,20 @@ "find": "\tif mode != \"off\" {\n\t\targs = append(args, \"--cleanup\")\n\t}", "replace": "\tif mode != \"off\" && false {\n\t\targs = append(args, \"--cleanup\")\n\t}", "test": "TestAnOwnedLabSweepsItsRuntimeObjects" + }, + { + "label": "--reset-after and --keep-on-failure are accepted together, so one of them silently loses: a failed run is either erased or kept, and whichever order the code happens to apply, half the users get the opposite of what they asked for and nothing says so", + "file": "internal/cli/run.go", + "find": "\tif *resetAfter && *keep {", + "replace": "\tif *resetAfter && *keep && false {", + "test": "TestResetAfterAndKeepOnFailureAreRefusedTogether" + }, + { + "label": "a reset that did not converge stops failing the command, so a shared emulator is left dirty by a run that reported success. The caller asked for the lab to be left as found; a quiet 0 is how the next test inherits the mess and blames itself", + "file": "internal/cli/run.go", + "find": "\tif resetVerdict == \"did not converge\" && code == exitOK {", + "replace": "\tif resetVerdict == \"did not converge\" && code == exitOK && false {", + "test": "TestARunWhoseResetDidNotConvergeFails" } ] } From 450aa504c83e5890884cde88ab089b2b8bb2fa03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 13:25:03 +0100 Subject: [PATCH 7/7] feat(machine): a reset starts again the machines the host has lost (#853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #853's last open criterion, "runtime reconciled to the checkpoint", and the half I had sequenced last out of a worry the measurement then overturned. Measured under Incus 7.2, with the container removed OUT OF BAND so that the state exists at all — the record is there, the machine is not: checkpoint "rc": 4 resource(s), 0 object(s), 1 machine(s) # incus delete --force, behind the emulator's back containers: [] the store still says: state=running machine=feint-scw-4dbb6d76-… reset "rc": the lab converged containers: [feint-scw-4dbb6d76-…] # back, under the same name ## The worry that sequenced this last, and why it was wrong I had written that a Boot rebuilt from a restored record might not be the original's: an image gone from the catalogue, a key deleted, a user data that no longer exists. Reading where those values come from says otherwise, and that reading is the reason this half turned out cheap: - the record's own attributes, which the restore brought back verbatim; - the store, for the project's SSH keys — which are part of the checkpoint; - the image catalogue, compiled into the binary and depending on no state. A label the catalogue does not know produces an empty image reference, which is exactly what the ORIGINAL poweron produced for that same record. So the rebuilt boot is the boot it had. ## Two interfaces, because one was a silent trap The first version added a second method to `Reconciler`. Every pack instantly stopped satisfying it, the type assertion in the reset handler failed, the reconciliation silently stopped happening, and the call still answered 200 with a clean body. Nothing logged it. The only reason it surfaced is that a test asserted which records a pack had been HANDED rather than that the call succeeded. `MissingReconciler` is now its own optional half, so a pack gains one when it is ready and keeps the other working, and internal/cli's TestAPackThatDrivesMachinesReconcilesBothWays closes the other side: a pack that drives machines must satisfy both, or a reset quietly does half its job for that provider. ## The core stopped guessing, and the falsify baseline is what caught it The first version picked the missing records in the CORE, by looking for a "feint-" prefix in any Runtime value. Two defects in one line: a provider's vocabulary in the neutral layer, and a match broad enough to pick up a test's own bookkeeping stamp — which it then tried to boot. The falsification baseline went red with nine identical warnings naming that stamp. The core now asks the runtime what it HOLDS and hands that to each pack, which decides with its own RuntimeKey. A host nobody can survey answers asked=false, and then nothing is offered for recreation at all: taking it for an empty host would start a second container beside every one already running. ## What it refuses to call a success PowerOn answers false and sets FailedState when a runtime was configured and did not deliver. Those are reported as Failed, never folded into Recreated: a reset claiming it brought three machines back when one is in FailedState would be caught by the verdict above it anyway, loudly, in the wrong place. ## Tests - machine: six populations, including a machine the host still HOLDS being left alone, and a record whose key names no machine not being a candidate — the two the core's guess got wrong; - emulator: the records a pack is handed, and a host nobody can survey being asked for nothing; - conformance: `reset-host.sh` now measures all three on a real runtime — one kept, one removed, one lost out of band and started again under the same name. The name is the assertion and not the count: a reset that started SOME container would satisfy a count while every record named one that does not exist, and the next reset would report it missing again, forever. /falsify: eight mutations on a-reset-is-not-a-restore.json, all biting, including one that reproduces the core's guess and one that takes a survey-less host for an empty one. `machine.PackSurface` names `Binding.ReconcileMissing` with its reason. mise run prepush green. Only the operator's own container remains on the station, and its memory is where it started. Assisted-by: Claude Code (claude-opus-5) --- internal/cli/reconcile_discipline_test.go | 60 +++++++ internal/core/emulator/checkpoint.go | 117 ++++++++++++-- internal/core/emulator/checkpoint_test.go | 31 +++- internal/core/emulator/recreate_test.go | 128 +++++++++++++++ internal/core/machine/reconcile.go | 85 ++++++++++ internal/core/machine/recreate_test.go | 149 ++++++++++++++++++ internal/core/machine/surface.go | 1 + internal/providers/exoscale/machines.go | 17 ++ internal/providers/outscale/machines.go | 17 ++ internal/providers/scaleway/machines.go | 17 ++ tools/conformance/reset-host.sh | 24 +++ .../specs/a-reset-is-not-a-restore.json | 27 +++- tools/testplan/rules.go | 5 +- 13 files changed, 649 insertions(+), 29 deletions(-) create mode 100644 internal/cli/reconcile_discipline_test.go create mode 100644 internal/core/emulator/recreate_test.go create mode 100644 internal/core/machine/recreate_test.go diff --git a/internal/cli/reconcile_discipline_test.go b/internal/cli/reconcile_discipline_test.go new file mode 100644 index 00000000..b60e67cf --- /dev/null +++ b/internal/cli/reconcile_discipline_test.go @@ -0,0 +1,60 @@ +package cli + +import ( + "testing" + + "github.com/stephrobert/feint/internal/core/emulator" +) + +// TestAPackThatDrivesMachinesReconcilesBothWays is cited by +// emulator.MissingReconciler, and it exists because of a defect that happened +// while that interface was being written. +// +// The first version carried both halves on ONE interface. Adding the second +// method turned every pack into a non-implementer: the type assertion in the +// reset handler failed, the reconciliation silently stopped happening, and the +// call still answered 200 with a clean-looking body. Nothing logged it. The +// only reason it surfaced is that a test asserted which records a pack had +// been HANDED rather than that the call succeeded. +// +// Splitting the interface in two removes that class of defect for the future — +// a pack gains one half when it is ready and keeps the other working — and +// this test closes the other side of it: a pack that drives machines must +// satisfy both, or a reset would quietly do half its job for that provider. +// +// The subject is "drives machines", and it is read from the pack itself rather +// than from a list of provider names: a fourth pack must inherit this without +// anyone editing a list here, which is rule 5 applied to a test. +func TestAPackThatDrivesMachinesReconcilesBothWays(t *testing.T) { + srv, _, err := newServer(nil) + if err != nil { + t.Fatalf("build the emulator: %v", err) + } + + examined := 0 + for _, pack := range srv.Packs() { + gone, reconciles := pack.(emulator.Reconciler) + missing, starts := pack.(emulator.MissingReconciler) + if !reconciles && !starts { + // A pack that drives no machine implements neither, which is the + // ordinary case and not a failure. + continue + } + examined++ + if !reconciles { + t.Errorf("%s can start machines again and cannot take them down: a reset would "+ + "leave every machine the checkpoint does not hold running", pack.Name()) + } + if !starts { + t.Errorf("%s can take machines down and cannot start them again: a reset would "+ + "return an operator to a checkpoint whose servers answer nothing", pack.Name()) + } + _, _ = gone, missing + } + + // A denominator, because a loop over nothing passes silently and would + // have passed before either half existed. + if examined < 3 { + t.Fatalf("only %d pack(s) reconcile at all; this build drives machines for three", examined) + } +} diff --git a/internal/core/emulator/checkpoint.go b/internal/core/emulator/checkpoint.go index 1cddfb9f..b149039a 100644 --- a/internal/core/emulator/checkpoint.go +++ b/internal/core/emulator/checkpoint.go @@ -53,6 +53,36 @@ type Reconciler interface { ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled } +// MissingReconciler is the other half, and a SEPARATE interface rather than a +// second method on Reconciler. +// +// That is not style. A single interface carrying both means a pack +// implementing one of them satisfies neither, the type assertion fails, and +// the reconciliation silently stops happening — no error, no log, a reset that +// reports a clean pass and reconciles nothing. It happened while this half was +// being written: adding the second method to Reconciler turned every pack into +// a non-implementer, and the only reason it was caught is that a test asserted +// which records a pack was HANDED rather than that the call answered 200. +// +// Two optional halves cannot do that: a pack gains one when it is ready, and +// the one it already had keeps working. +// +// internal/cli's TestAPackThatDrivesMachinesReconcilesBothWays fails without +// both halves being satisfied by every pack that drives machines. +type MissingReconciler interface { + // ReconcileMissing starts again the machines of this pack's records that + // the host no longer holds. + // + // It takes the records and the machines the host DOES hold, and the pack + // decides which of its records are missing. The core deliberately does not + // decide: a record's machine name lives under a key that is the pack's own + // vocabulary, and a first version of this guessed with a "feint-" prefix + // in the core. It was wrong twice over — a provider-shaped assumption in + // the neutral layer, and broad enough to match a test's own bookkeeping + // stamp, which it then tried to boot. + ReconcileMissing(ctx context.Context, records []*resource.Resource, held []string) machine.Reconciled +} + // checkpoints holds the control-plane half of each named checkpoint. // // In memory, and that is the scope #853 describes: "returns to the session's @@ -196,32 +226,54 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { } } - gone := goneAfterRestore(before, s.env.Store.All()) + restored := s.env.Store.All() + gone := goneAfterRestore(before, restored) + // What the host actually holds, asked of the runtime rather than deduced + // from the store: the store is exactly what just changed, it says a server + // is running, and whether anything backs it is the question. A driver that + // cannot be surveyed answers asked=false, and then nothing is offered for + // recreation at all — claiming a machine is absent because nobody could be + // asked would start containers beside ones already running. + held, surveyed := heldOnHost(r.Context(), s.env) reconciled := map[string]any{} - var destroyed, skipped []string + var destroyed, skipped, recreated, failed []string for _, pack := range s.packs { - reconciler, ok := pack.(Reconciler) - if !ok { + mine := ownedBy(gone, pack.Name()) + records := ownedBy(restored, pack.Name()) + if len(mine) == 0 && (!surveyed || len(records) == 0) { continue } - mine := make([]*resource.Resource, 0, len(gone)) - for _, res := range gone { - if res.Tenant.Provider == pack.Name() { - mine = append(mine, res) - } + done := machine.Reconciled{} + if reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 { + done = reconciler.ReconcileGone(r.Context(), mine) } - if len(mine) == 0 { - continue + if starter, ok := pack.(MissingReconciler); ok && surveyed && len(records) != 0 { + back := starter.ReconcileMissing(r.Context(), records, held) + done.Recreated = append(done.Recreated, back.Recreated...) + done.Failed = append(done.Failed, back.Failed...) + done.Skipped = append(done.Skipped, back.Skipped...) + // A recreation rewrites the record's Runtime and State, so the + // store is written back here rather than left holding the machine + // name of a container that is gone. Put rather than Commit: the + // resource came out of this store a moment ago and nothing else + // writes during a reset, which holds the whole store's lock. + for _, res := range records { + s.env.Store.Put(res) + } } - done := reconciler.ReconcileGone(r.Context(), mine) destroyed = append(destroyed, done.Destroyed...) skipped = append(skipped, done.Skipped...) + recreated = append(recreated, done.Recreated...) + failed = append(failed, done.Failed...) reconciled[pack.Name()] = map[string]any{ "destroyed": done.Destroyed, "skipped": done.Skipped, + "recreated": done.Recreated, "failed": done.Failed, } } sort.Strings(destroyed) sort.Strings(skipped) + sort.Strings(recreated) + sort.Strings(failed) writeJSON(w, http.StatusOK, map[string]any{ "schema_version": CheckpointSchemaVersion, @@ -229,11 +281,13 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { "resources": s.env.Store.Len(), "destroyed": destroyed, "skipped": skipped, - "per_provider": reconciled, - // Said on the wire, because a caller cannot see it any other way and a - // quiet success would read as "the host matches the checkpoint". - "recreated": []string{}, - "recreates": false, + "recreated": recreated, + // Named apart from skipped: a start that was attempted and refused and + // a record nobody could act on need different answers from whoever + // reads this. + "failed": failed, + "per_provider": reconciled, + "recreates": true, }) } @@ -282,3 +336,32 @@ func validCheckpointName(name string) bool { // A name of dots is a path, not a name. return name != "." && name != ".." } + +// ownedBy narrows a list of resources to one pack's own, so no pack ever sees +// another's. +func ownedBy(resources []*resource.Resource, provider string) []*resource.Resource { + out := make([]*resource.Resource, 0, len(resources)) + for _, res := range resources { + if res.Tenant.Provider == provider { + out = append(out, res) + } + } + return out +} + +// heldOnHost answers the machines the runtime actually holds, and whether it +// could be asked at all. +// +// Through Survey, which names only what the emulator labelled and issues no +// mutating command. The second return is not a convenience: a driver nobody +// can survey is not an empty host, and treating it as one would have a reset +// start a container beside every one already running. +// +// TestAResetAsksNothingOfAHostNobodyCanSurvey fails without this. +func heldOnHost(ctx context.Context, env *Env) ([]string, bool) { + left, asked, err := env.machines.Survey(ctx) + if !asked || err != nil { + return nil, false + } + return left.Machines, true +} diff --git a/internal/core/emulator/checkpoint_test.go b/internal/core/emulator/checkpoint_test.go index 84d8e66a..8200ccba 100644 --- a/internal/core/emulator/checkpoint_test.go +++ b/internal/core/emulator/checkpoint_test.go @@ -53,6 +53,20 @@ func labServer(t *testing.T, packs ...emulator.Pack) (*httptest.Server, *store.S // of them. The falsification found that: skipping the byte half changed // nothing in a test whose emulator served no objects. func labServerWithBytes(t *testing.T, objects *objectstore.Store, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + return labServerWith(t, objects, nil, packs...) +} + +// labServerWithRuntime is the same lab with a runtime that can be surveyed, +// which is the only way to measure what a reset considers MISSING: the store +// is what just changed, so whether anything backs a record is a question only +// the host can answer. +func labServerWithRuntime(t *testing.T, rt machine.Runtime, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + return labServerWith(t, nil, &rt, packs...) +} + +func labServerWith(t *testing.T, objects *objectstore.Store, rt *machine.Runtime, packs ...emulator.Pack) (*httptest.Server, *store.Store) { t.Helper() held := store.New() env := &emulator.Env{ @@ -61,6 +75,9 @@ func labServerWithBytes(t *testing.T, objects *objectstore.Store, packs ...emula Now: func() time.Time { return time.Unix(1700000000, 0).UTC() }, NewID: func() string { return "00000000-0000-4000-8000-000000000001" }, } + if rt != nil { + env.UseMachines(*rt) + } srv, err := emulator.NewServer(env, packs...) if err != nil { t.Fatalf("build the emulator: %v", err) @@ -220,10 +237,16 @@ func TestAResetDestroysWhatTheCheckpointDoesNotHold(t *testing.T) { if destroyed, _ := body["destroyed"].([]any); len(destroyed) != 1 || destroyed[0] != "feint-acme-later" { t.Errorf("destroyed %v", body["destroyed"]) } - // Recreation is not attempted, and the answer says so: a quiet success - // would read as "the host matches the checkpoint". - if recreates, _ := body["recreates"].(bool); recreates { - t.Error("the reset claims it recreates missing machines") + // Recreation IS attempted now, and the answer says so. Nothing was + // recreated here because this lab has no runtime: the records the + // checkpoint holds name machines the survey cannot be asked about, and a + // reset that started containers because nobody could be asked would be + // the opposite defect. + if recreates, _ := body["recreates"].(bool); !recreates { + t.Error("the reset says it does not recreate, which stopped being true") + } + if back, _ := body["recreated"].([]any); len(back) != 0 { + t.Errorf("a lab with no runtime recreated %v", back) } // Twice is a no-op, which is #853's own criterion: nothing is left to diff --git a/internal/core/emulator/recreate_test.go b/internal/core/emulator/recreate_test.go new file mode 100644 index 00000000..0070cd1f --- /dev/null +++ b/internal/core/emulator/recreate_test.go @@ -0,0 +1,128 @@ +package emulator_test + +import ( + "context" + "net/http" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" +) + +// restartingPack implements BOTH halves, and records what each was handed. +type restartingPack struct { + stubPack + gone [][]string + missing [][]string + back machine.Reconciled +} + +func (p *restartingPack) ReconcileGone(_ context.Context, gone []*resource.Resource) machine.Reconciled { + p.gone = append(p.gone, idsOf(gone)) + return machine.Reconciled{} +} + +func (p *restartingPack) ReconcileMissing(_ context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + // The pack decides which of its records is missing, with its own key. + // Here the key is "machine", which is what put() writes. + var missing []*resource.Resource + for _, res := range records { + name := res.Runtime["machine"] + if name == "" { + continue + } + found := false + for _, candidate := range held { + if candidate == name { + found = true + } + } + if !found { + missing = append(missing, res) + } + } + p.missing = append(p.missing, idsOf(missing)) + return p.back +} + +func idsOf(resources []*resource.Resource) []string { + var out []string + for _, res := range resources { + out = append(out, res.ID) + } + return out +} + +// surveyingDriver answers what the host holds, and nothing else. +type surveyingDriver struct { + machine.Noop + held []string +} + +func (d surveyingDriver) Survey(context.Context) (machine.Leftovers, error) { + return machine.Leftovers{Machines: d.held}, nil +} + +// TestAResetStartsAgainWhatTheHostNoLongerHolds is cited by missingOnHost and +// by the three packs' ReconcileMissing. +// +// The question a reset has to ask is not what the store says — the store is +// exactly what just changed, and it says the server is running. It is whether +// anything backs it, and only the runtime can answer. +func TestAResetStartsAgainWhatTheHostNoLongerHolds(t *testing.T) { + acme := &restartingPack{ + stubPack: stubPack{name: "acme"}, + back: machine.Reconciled{Recreated: []string{"feint-acme-alive"}}, + } + server, held := labServerWithRuntime(t, machine.Use(surveyingDriver{held: []string{"feint-acme-alive"}}), acme) + + // Two records: one whose machine the host holds, one whose machine it does + // not. Only the second is missing. + put(t, held, "acme", "instance/server", "alive") + put(t, held, "acme", "instance/server", "vanished") + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + + code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") + if code != http.StatusOK { + t.Fatalf("reset: %d %v", code, body) + } + if len(acme.missing) != 1 || len(acme.missing[0]) != 1 || acme.missing[0][0] != "vanished" { + t.Fatalf("the pack was asked to start %v; only the record the host does not back is missing", + acme.missing) + } + if back, _ := body["recreated"].([]any); len(back) != 1 { + t.Errorf("recreated %v", body["recreated"]) + } + // And the record it DOES back was not touched, which is the direction + // that would start a second container beside one already running. + if len(acme.gone) != 0 { + t.Errorf("a reset that changed nothing asked for destructions: %v", acme.gone) + } +} + +// TestAResetAsksNothingOfAHostNobodyCanSurvey is the refusal half. +// +// A driver that cannot be surveyed answers asked=false, and then nothing is +// reported missing. Claiming a machine is absent because nobody could be asked +// would have a reset start containers on a host that already holds them — +// which is the expensive direction, and the one an undeclared capability is +// supposed to prevent everywhere else in this repository. +func TestAResetAsksNothingOfAHostNobodyCanSurvey(t *testing.T) { + acme := &restartingPack{stubPack: stubPack{name: "acme"}} + // The zero runtime: Noop, which is not a Surveyor. + server, held := labServer(t, acme) + put(t, held, "acme", "instance/server", "alive") + if code, _ := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatal("checkpoint refused") + } + if code, _ := checkpointPost(t, server.URL+"/_feint/reset/baseline"); code != http.StatusOK { + t.Fatal("reset refused") + } + if len(acme.missing) != 0 { + t.Errorf("a host nobody can survey was taken for an empty one: %v", acme.missing) + } +} diff --git a/internal/core/machine/reconcile.go b/internal/core/machine/reconcile.go index 1dafabc7..38049bec 100644 --- a/internal/core/machine/reconcile.go +++ b/internal/core/machine/reconcile.go @@ -2,6 +2,7 @@ package machine import ( "context" + "slices" "github.com/stephrobert/feint/internal/core/resource" ) @@ -42,6 +43,14 @@ import ( type Reconciled struct { // Destroyed names the machines taken down because their record is gone. Destroyed []string + // Recreated names the machines started again because a record the + // checkpoint holds had none. + Recreated []string + // Failed names the records whose machine could not be started again. + // Separate from Skipped, because a start that was attempted and refused + // and a record nobody could act on need different answers from the + // operator. + Failed []string // Skipped names records whose machine this binding does not own, or which // name no machine at all. Reported rather than silently passed over: a // record nobody could act on is what an operator needs to hear about. @@ -98,3 +107,79 @@ func (b Binding) ReconcileGone(ctx context.Context, gone []*resource.Resource) R } return done } + +// ReconcileMissing starts again the machines of records that have none. +// +// It is the other half of #853's "runtime reconciled to the checkpoint", and +// the issue bounds it in the same breath: "where the runtime contract supports +// it". What that turned out to be here is better than I expected, and it was +// measured rather than assumed. +// +// ## Why a rebuilt boot is the boot the original had +// +// A pack builds its Boot from three places, and a reset has already put all +// three back by the time this runs: +// +// - the record's own attributes, which the restore brought back verbatim; +// - the store, for things like the project's SSH keys, which the restore +// brought back too — the keys are part of the checkpoint; +// - the image catalogue, which is compiled into the binary and depends on no +// state at all. +// +// So the fear that sequenced this half last — an image gone from the +// catalogue, a key deleted, a user data that no longer exists — does not +// survive reading where those values come from. What a label the catalogue +// does not know produces is an empty image reference, which is exactly what +// the ORIGINAL poweron produced for that same record. +// +// ## Why the Boot comes in as a function +// +// Building one is pack knowledge: the image table, the login, the cloud-init a +// pack renders for its own product. Taking a builder rather than a Boot keeps +// the loop, the ownership path and the honest reporting in one place while the +// one thing that genuinely varies stays with the pack. Each pack writes one +// line. +// +// ## What it refuses to call a success +// +// PowerOn already answers false when a runtime was configured and did not +// deliver, and sets FailedState rather than Running. This reports those as +// Failed instead of folding them into Recreated: a reset that said it brought +// three machines back when one of them is in FailedState would be the half- +// success this project exists to avoid, and the verdict above it compares the +// observable state anyway — so a lie here would be caught, loudly, in the +// wrong place. +// +// TestReconcileStartsOnlyTheMachinesThatAreMissing fails without this. +func (b Binding) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, boot func(*resource.Resource) Boot, +) Reconciled { + var done Reconciled + for _, res := range records { + if res == nil || boot == nil { + continue + } + // Which of these records is missing a machine is decided HERE, with + // this binding's own key, and not by the caller. The core tried to + // guess it with a "feint-" prefix and was wrong twice: a provider's + // vocabulary in the neutral layer, and a match broad enough to pick up + // a test's bookkeeping stamp and try to boot it. + name := res.Runtime[b.RuntimeKey] + if name == "" || slices.Contains(held, name) { + continue + } + if b.driver == nil { + // Metadata only: PowerOn would answer running without starting + // anything, which is the documented degraded mode and not + // something to report as a recreation. + done.Skipped = append(done.Skipped, b.Name(res.ID)) + continue + } + if !b.PowerOn(ctx, res, boot(res)) { + done.Failed = append(done.Failed, b.Name(res.ID)) + continue + } + done.Recreated = append(done.Recreated, res.Runtime[b.RuntimeKey]) + } + return done +} diff --git a/internal/core/machine/recreate_test.go b/internal/core/machine/recreate_test.go new file mode 100644 index 00000000..55159041 --- /dev/null +++ b/internal/core/machine/recreate_test.go @@ -0,0 +1,149 @@ +package machine + +import ( + "context" + "errors" + "slices" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// startingDriver records what a reconciliation asked to start, and can refuse. +type startingDriver struct { + recordingDriver + started []string + refuse bool +} + +func (d *startingDriver) Start(_ context.Context, spec Spec) (Machine, error) { + d.started = append(d.started, spec.Name) + if d.refuse { + return Machine{}, errors.New("the runtime refused") + } + return Machine{Name: spec.Name, Addresses: []string{"10.42.0.9"}, Running: true}, nil +} + +func absent(id string) *resource.Resource { + res := resource.New(id, "instance/server", + resource.Tenant{Provider: "acme", Project: "p", Zone: "z"}, "running", + time.Unix(1700000000, 0).UTC()) + res.Runtime = map[string]string{"machine": "feint-acme-" + id} + return res +} + +// TestReconcileStartsOnlyTheMachinesThatAreMissing is cited by +// ReconcileMissing and by the three packs that delegate to it. +// +// The measurement that sequenced this half last is worth repeating here, +// because it turned out the other way: a Boot rebuilt from a restored record +// is the Boot the original had, since every input is the record itself, the +// store the reset has already restored, or the image catalogue compiled into +// the binary. So what is left to get wrong is the reporting, and that is what +// these four populations are about. +func TestReconcileStartsOnlyTheMachinesThatAreMissing(t *testing.T) { + boot := func(res *resource.Resource) Boot { + return Boot{Image: "img", User: "root", Hostname: res.ID} + } + + t.Run("a record with no machine on the host is started again", func(t *testing.T) { + d := &startingDriver{} + first, second := absent("a"), absent("b") + done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{first, second}, nil, boot) + if len(d.started) != 2 { + t.Fatalf("started %v", d.started) + } + if len(done.Recreated) != 2 || len(done.Failed) != 0 { + t.Errorf("reported %+v", done) + } + // The record is rewritten, which is the half that makes the next + // reset a no-op: a resource still naming the container that is gone + // would be reported missing again, forever. + if first.Runtime["machine"] == "" || first.State != "running" { + t.Errorf("the record was not brought up to date: %+v", first.Runtime) + } + }) + + t.Run("a runtime that refuses is Failed, not Recreated", func(t *testing.T) { + // PowerOn answers false and sets FailedState rather than Running. A + // reset that folded this into Recreated would say it brought a machine + // back that is in FailedState, and the verdict above it compares the + // observable state anyway — so the lie would surface loudly, in the + // wrong place. + d := &startingDriver{refuse: true} + res := absent("a") + done := bootBinding(d).ReconcileMissing(t.Context(), []*resource.Resource{res}, nil, boot) + if len(done.Recreated) != 0 { + t.Errorf("a refused start was reported as recreated: %+v", done) + } + if !slices.Equal(done.Failed, []string{"feint-acme-a"}) { + t.Errorf("failed %v", done.Failed) + } + if res.State != "failed" { + t.Errorf("the record says %q after a refused start", res.State) + } + }) + + t.Run("no runtime starts nothing and says so", func(t *testing.T) { + // Metadata only is the DEFAULT. PowerOn would answer running without + // starting anything, which is the documented degraded mode and not a + // recreation to report. + done := bootBinding(nil).ReconcileMissing(t.Context(), + []*resource.Resource{absent("a")}, nil, boot) + if len(done.Recreated) != 0 || len(done.Failed) != 0 { + t.Errorf("a binding with no runtime reported %+v", done) + } + if !slices.Equal(done.Skipped, []string{"feint-acme-a"}) { + t.Errorf("skipped %v", done.Skipped) + } + }) + + t.Run("nothing missing and no boot builder are both no-ops", func(t *testing.T) { + d := &startingDriver{} + if done := bootBinding(d).ReconcileMissing(t.Context(), nil, nil, boot); done.Recreated != nil { + t.Errorf("an empty reconciliation did something: %+v", done) + } + // A nil builder is a programming error upstream, and starting a + // machine from a zero Boot would be worse than doing nothing: it would + // boot an image nobody asked for. + if done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{absent("a")}, nil, nil); len(done.Recreated) != 0 { + t.Errorf("a nil boot builder started something: %+v", done) + } + if len(d.started) != 0 { + t.Errorf("commands were emitted: %v", d.started) + } + }) + t.Run("a machine the host still holds is left alone", func(t *testing.T) { + // This is where the decision moved to, and why. The core used to pick + // the missing records itself, with a "feint-" prefix: a provider's + // vocabulary in the neutral layer, and broad enough to match a test's + // own bookkeeping stamp, which it then tried to boot. The binding + // knows its RuntimeKey, so it decides. + d := &startingDriver{} + res := absent("a") + done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{res}, []string{"feint-acme-a"}, boot) + if len(d.started) != 0 { + t.Errorf("a machine the host holds was started again: %v", d.started) + } + if len(done.Recreated) != 0 || len(done.Failed) != 0 || len(done.Skipped) != 0 { + t.Errorf("reported %+v for a record that needed nothing", done) + } + }) + + t.Run("a record whose key names no machine is not a candidate", func(t *testing.T) { + d := &startingDriver{} + res := absent("a") + res.Runtime = map[string]string{"address": "10.0.0.5"} + if done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{res}, nil, boot); len(done.Recreated) != 0 { + t.Errorf("a record with no machine name was started: %+v", done) + } + if len(d.started) != 0 { + t.Errorf("commands were emitted: %v", d.started) + } + }) +} diff --git a/internal/core/machine/surface.go b/internal/core/machine/surface.go index 6e296971..3c09b261 100644 --- a/internal/core/machine/surface.go +++ b/internal/core/machine/surface.go @@ -126,6 +126,7 @@ func PackSurface() map[string]string { "Binding.PowerOff": "stop the machine behind a resource", "Binding.Destroy": "destroy the machine behind a resource", "Binding.ReconcileGone": "take down the machines of records a reset removed (#853), in one shared gesture", + "Binding.ReconcileMissing": "start again the machines of records a reset brought back and the host no longer holds (#853), with the Boot this pack builds", "Reconciled": "what a reconciliation did: what it destroyed, and what it could not act on", "Binding.RefreshIfRunning": "re-read what the host holds for a machine the store calls running", "Binding.RuntimeKey": "the pack's own Runtime key, to read the machine name it is about to hand to Unroute", diff --git a/internal/providers/exoscale/machines.go b/internal/providers/exoscale/machines.go index 5ac45266..93e94a8a 100644 --- a/internal/providers/exoscale/machines.go +++ b/internal/providers/exoscale/machines.go @@ -296,3 +296,20 @@ func (p *Pack) addressOf(res *resource.Resource) string { func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { return p.binding().ReconcileGone(ctx, gone) } + +// ReconcileMissing starts again the machines of records a reset brought back +// and the host no longer holds (#853). +// +// One line, for the reason ReconcileGone is two: the loop, the ownership path +// and the honest reporting are shared, and what varies is the Boot this pack +// builds from its own catalogue. Handing the builder rather than a Boot is +// what keeps it that way. +// +// emulator.TestAResetStartsAgainWhatTheHostNoLongerHolds drives this through +// the server; machine.TestReconcileStartsOnlyTheMachinesThatAreMissing holds +// what it delegates to. +func (p *Pack) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + return p.binding().ReconcileMissing(ctx, records, held, p.bootOf) +} diff --git a/internal/providers/outscale/machines.go b/internal/providers/outscale/machines.go index c0df98f0..e0c36092 100644 --- a/internal/providers/outscale/machines.go +++ b/internal/providers/outscale/machines.go @@ -289,3 +289,20 @@ func (p *Pack) attachmentOf(res *resource.Resource) []machine.Attachment { func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { return p.binding().ReconcileGone(ctx, gone) } + +// ReconcileMissing starts again the machines of records a reset brought back +// and the host no longer holds (#853). +// +// One line, for the reason ReconcileGone is two: the loop, the ownership path +// and the honest reporting are shared, and what varies is the Boot this pack +// builds from its own catalogue. Handing the builder rather than a Boot is +// what keeps it that way. +// +// emulator.TestAResetStartsAgainWhatTheHostNoLongerHolds drives this through +// the server; machine.TestReconcileStartsOnlyTheMachinesThatAreMissing holds +// what it delegates to. +func (p *Pack) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + return p.binding().ReconcileMissing(ctx, records, held, p.bootOf) +} diff --git a/internal/providers/scaleway/machines.go b/internal/providers/scaleway/machines.go index 85b8125c..7a865205 100644 --- a/internal/providers/scaleway/machines.go +++ b/internal/providers/scaleway/machines.go @@ -350,3 +350,20 @@ func (p *Pack) gatewayPushesDefaultRoute(privateNetworkID string) bool { func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { return p.binding().ReconcileGone(ctx, gone) } + +// ReconcileMissing starts again the machines of records a reset brought back +// and the host no longer holds (#853). +// +// One line, for the reason ReconcileGone is two: the loop, the ownership path +// and the honest reporting are shared, and what varies is the Boot this pack +// builds from its own catalogue. Handing the builder rather than a Boot is +// what keeps it that way. +// +// emulator.TestAResetStartsAgainWhatTheHostNoLongerHolds drives this through +// the server; machine.TestReconcileStartsOnlyTheMachinesThatAreMissing holds +// what it delegates to. +func (p *Pack) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + return p.binding().ReconcileMissing(ctx, records, held, p.bootOf) +} diff --git a/tools/conformance/reset-host.sh b/tools/conformance/reset-host.sh index 27e20d6f..0482b0d8 100755 --- a/tools/conformance/reset-host.sh +++ b/tools/conformance/reset-host.sh @@ -106,4 +106,28 @@ EOF [ -z "$missing" ] || fail "the reset took a container it does not own:$missing" ok "every container feint did not create is still there" +# --------------------------------------------------------------------------- +# The other half: a machine the checkpoint HOLDS that the host has lost. +# +# The removal is out of band on purpose, and it is the only way to produce the +# state this measures — the record is there, the machine is not — without +# touching the store. A reset that only removed would leave an operator with a +# control plane describing servers that answer nothing. +echo "-- the host loses a machine the checkpoint holds" +"$FEINT" checkpoint reset-back --addr "$ADDR" >/dev/null || fail "the second checkpoint was refused" +held="$(ours | head -1)" +[ -n "$held" ] || fail "no machine to lose, so this half would measure nothing" +incus delete --force "$held" >/dev/null 2>&1 || fail "could not take the machine away out of band" +[ -z "$(ours)" ] || fail "the machine is still there, so the reset below would have nothing to do" +ok "the machine is gone from the host while the record remains" + +"$FEINT" reset reset-back --addr "$ADDR" || fail "the reset reported that the lab did not converge" +printf '%s\n' "$(ours)" | grep -qx "$held" \ + || fail "the reset did not start the machine again: [$(ours | tr '\n' ' ')]" +ok "the machine the checkpoint holds is back, under the same name" + +# The name is the assertion, not the count: a reset that started SOME +# container would satisfy a count while leaving every record naming one that +# does not exist, and the next reset would report it missing again, forever. + echo "conformance: the host half of a reset measured under $mode" diff --git a/tools/falsify/specs/a-reset-is-not-a-restore.json b/tools/falsify/specs/a-reset-is-not-a-restore.json index 2f9751f9..69347c3b 100644 --- a/tools/falsify/specs/a-reset-is-not-a-restore.json +++ b/tools/falsify/specs/a-reset-is-not-a-restore.json @@ -3,10 +3,10 @@ "note": "The subject is #853's own line: \"reset is not successful because JSON was restored\". `feint snapshot load` and `PUT /_feint/state` restore the control plane verbatim and touch nothing else, so a resource that disappears leaves its container running while the API no longer mentions it. The issue's last closing criterion asks for exactly this spec: \"a falsification that restores only the store but not runtime makes the test fail\".\n\nThe mutations are therefore all of the same family: each one turns the reset back into a restore, by a different route. The route matters, because each is a shape that would look reasonable in review — skipping the reconciliation loop, computing the difference by count instead of identity, handing a pack the resources that SURVIVED, or taking the \"before\" picture after the restore, when it is already gone.\n\nThe last one is the one worth reading twice. Reading the store after the restore instead of before is a one-line reordering that compiles, runs, reports a clean reset, and reconciles nothing at all — because by then there is no difference left to see.", "mutations": [ { - "label": "the reconciliation loop is skipped, so a reset restores the control plane and leaves every machine the checkpoint does not hold running. This is the gap #853 names, reproduced exactly: the API no longer mentions the container and nobody takes it down", + "label": "the destroying half stops being called, so a reset restores the control plane and leaves every machine the checkpoint does not hold running. This is the gap #853 names, reproduced exactly: the API no longer mentions the container and nobody takes it down", "file": "internal/core/emulator/checkpoint.go", - "find": "\t\treconciler, ok := pack.(Reconciler)\n\t\tif !ok {\n\t\t\tcontinue\n\t\t}", - "replace": "\t\treconciler, ok := pack.(Reconciler)\n\t\tif !ok || true {\n\t\t\t_ = reconciler\n\t\t\tcontinue\n\t\t}", + "find": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 {", + "replace": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 && false {", "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" }, { @@ -17,10 +17,10 @@ "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" }, { - "label": "each pack is handed the resources that SURVIVED the restore rather than the ones that did not, so a reset destroys exactly the machines the checkpoint expects to keep. The opposite error of the first mutation, and the expensive one: it takes down what the operator asked to come back", + "label": "each pack is handed ANOTHER pack's resources, so a reset destroys exactly the machines the checkpoint expects to keep and leaves the ones it does not. The opposite error of the first mutation and the expensive one: it takes down what the operator asked to come back", "file": "internal/core/emulator/checkpoint.go", - "find": "\t\t\tif res.Tenant.Provider == pack.Name() {\n\t\t\t\tmine = append(mine, res)\n\t\t\t}", - "replace": "\t\t\tif res.Tenant.Provider != pack.Name() {\n\t\t\t\tmine = append(mine, res)\n\t\t\t}", + "find": "\t\tif res.Tenant.Provider == provider {\n\t\t\tout = append(out, res)\n\t\t}", + "replace": "\t\tif res.Tenant.Provider != provider {\n\t\t\tout = append(out, res)\n\t\t}", "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" }, { @@ -43,6 +43,21 @@ "find": "\tif !validCheckpointName(name) {", "replace": "\tif !validCheckpointName(name) && false {", "test": "TestACheckpointTakesBothHalvesOrNeither" + }, + { + "label": "a host nobody can survey is taken for an empty one, so a reset offers every record for recreation and starts a second container beside each one already running. The defect is trusting the store about a question only the runtime can answer: the store is exactly what just changed", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif !asked || err != nil {\n\t\treturn nil, false\n\t}", + "replace": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif (!asked || err != nil) && false {\n\t\treturn nil, false\n\t}", + "test": "TestAResetAsksNothingOfAHostNobodyCanSurvey" + }, + { + "label": "the pack is handed the records and asked to start them without being told what the host holds, so every record looks missing. This is the version of the code that shipped for an hour: the core picked the missing records itself with a \"feint-\" prefix, which put a provider's vocabulary in the neutral layer and matched a test's own bookkeeping stamp, which it then tried to boot", + "file": "internal/core/machine/reconcile.go", + "find": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || slices.Contains(held, name) {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || (slices.Contains(held, name) && false) {\n\t\t\tcontinue\n\t\t}", + "test": "TestReconcileStartsOnlyTheMachinesThatAreMissing", + "package": "./internal/core/machine/" } ] } diff --git a/tools/testplan/rules.go b/tools/testplan/rules.go index 6847b22c..7f6a1c7b 100644 --- a/tools/testplan/rules.go +++ b/tools/testplan/rules.go @@ -504,8 +504,9 @@ var rules = []rule{ Why: "the host half of a reset, which only a real runtime can answer for", Runs: []string{"FEINT_VM=incus-ovn mise run conformance:leg -- runtime"}, Unproven: "the reset suite runs on the runtime leg, which refuses without a runtime, " + - "and measures one machine kept against one removed: a reset interrupted part-way, " + - "a checkpoint whose machine died on its own, and more than two machines are not covered", + "and measures one machine kept, one removed and one lost out of band and started " + + "again: a reset interrupted part-way, a machine whose image left the catalogue " + + "between checkpoint and reset, and more than two machines are not covered", Cites: []claim{{ About: "the reset suite runs on the runtime leg, which refuses without a runtime", In: "tools/conformance/leg.sh",