From 2862e8c968d68a29d18f7f6974666af819e09999 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 14:12:46 +0100 Subject: [PATCH 1/5] fix(cli): the reset verdict compares bytes, and a broken promise fails the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from an external audit of merged code, each verified against the source before being corrected, and three of them were real false greens of mine. ## The verdict compared nothing about an object's content `observable.go` took an object's digest from ListObjectsV2's ETag. `internal/s3/list.go` emits no ETag at all, so the field was ALWAYS empty, and the comparison key was the door, the bucket and the key — the size it had already read was dropped too. Two different contents under one key compared equal, and a reset could report convergence over bytes that had changed. The commit message that shipped it claimed objects were compared by digest. What let it through is the shape of the test beside it: TestADifferenceIsNamedWhereverItIs builds observedObject values BY HAND, with digests in them, so it exercised the comparison and never the observation. A fake emitting an ETag would have hidden it just as well. Each object is now FETCHED and hashed by the observer. Not the store's record of it and not the door's checksum header: what is judged is whether a client gets the same bytes back, and a digest supplied by the subject cannot answer that. The delivered size and the listed size travel separately, so an inventory that disagrees with the bytes is a difference rather than a silent one. The audit's decisive test exists — TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge — and it mounts a REAL door over a REAL store, because that is the only arrangement in which an absent field is visible. It also asserts the same bytes twice compare EQUAL, so it cannot be satisfied by a comparison that always differs. Writing through the door rather than behind it was not a preference either: a bucket created straight on the store carries no creation metadata and the door refuses to list it with a 501. That cost three runs of blaming the reader. ## `feint run` could return 0 with a broken promise Only "did not converge" failed the run. A reset the emulator refused, one that could not be read back, and one with no checkpoint to return to all left the exit code at the child's, so this was a possible CI run: terraform test success feint reset failed feint run exit 0 The same held for a lab this command owned and could not stop: `cleanup: failed` reached the receipt and nothing else. The rule is now the general one, and it is what a wrapper is for: the child's code travels untouched when the wrapper kept its promises, and the wrapper fails when it did not. `deliveredWhatWasAsked` lists the answers that are NOT a success rather than the ones that are, so a verdict word added later cannot default to "fine" — and the test covers the full cross product including two words nobody has written yet. ## The checkpoint's "both halves or neither" was a sentence The bytes were saved before the control-plane snapshot, so a failure in the second step left an earlier checkpoint of that name holding its OLD store beside the NEW object references. objectstore publishes its half atomically by renaming a temporary file, which is what makes the fix a reordering: snapshot into memory first, save the bytes, publish the store half last. A failure at any step now leaves the previous checkpoint consistent with itself. **The first test for this measured nothing**, and the harness said so: closing the byte store makes BOTH orders refuse at the same step, so the mutation that reverses the order stayed green. The scenario that separates them is the one the audit described — the byte half succeeds and the control-plane half fails — so the store is made unmarshallable and the assertion is on the BYTES a later reset brings back. ## Named checkpoints overwrote each other The emulator holds any number; the CLI wrote one file per instance. So `checkpoint A` then `checkpoint B` made `reset A` impossible, and the refusal that fired was honest while the feature was missing. Records are indexed by name now, with a `last` pointer so `feint reset` with no argument still means something. One of my own tests then passed for the wrong reason — it found a record a neighbouring subtest had written — which the per-name index changed underneath it. It has its own runtime directory now and asserts that the other run's record is left alone. ## What this does NOT fix, and why The audit's third P0: a firewall rule modified in place keeps its rule-set name, so `Survey` returning names cannot see it and a reset could report convergence while packets are still blocked. Its premise is correct — Survey answers names — but the audit says it deduced this from the code rather than reproducing it, and so have I. I am not writing a fix without a test that bites first; it needs a real TCP flow before, after and after the reset, on Incus or OVN. Also untouched: the concurrency question the audit raises at the end. A reset is not a global transaction, and nothing yet proves what a create arriving mid-reset does. /falsify: 11 mutations on a-reset-is-not-a-restore.json and 16 on a-run-owns-its-lab.json, all biting, including the two that reproduce the audit's own P0s and the one that reverses the checkpoint's order. mise run prepush green. Assisted-by: Claude Code (claude-opus-5) --- internal/cli/checkpoint.go | 84 +++++--- internal/cli/checkpoint_test.go | 70 +++++-- internal/cli/exit_contract_test.go | 64 ++++++ internal/cli/observable.go | 91 ++++++++- internal/cli/observable_bytes_test.go | 191 ++++++++++++++++++ internal/cli/reset_after_test.go | 18 +- internal/cli/run.go | 56 ++++- .../core/emulator/atomic_checkpoint_test.go | 110 ++++++++++ internal/core/emulator/checkpoint.go | 44 ++-- .../specs/a-reset-is-not-a-restore.json | 25 ++- tools/falsify/specs/a-run-owns-its-lab.json | 18 +- 11 files changed, 706 insertions(+), 65 deletions(-) create mode 100644 internal/cli/exit_contract_test.go create mode 100644 internal/cli/observable_bytes_test.go create mode 100644 internal/core/emulator/atomic_checkpoint_test.go diff --git a/internal/cli/checkpoint.go b/internal/cli/checkpoint.go index d3d7fdf7..7edf2c63 100644 --- a/internal/cli/checkpoint.go +++ b/internal/cli/checkpoint.go @@ -165,25 +165,16 @@ func resetCommand(args []string, stdout, stderr io.Writer) int { } endpoint := "http://" + *addr - recorded, err := readRecordedCheckpoint(*addr) + // The name chooses the record, and an empty one means the last taken. + // Before the audit this read one file per instance, so a second + // checkpoint made the first unresettable. + recorded, err := readRecordedCheckpoint(*addr, asked) 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 { @@ -306,18 +297,45 @@ func callCheckpoint(method, url string) (map[string]any, error) { 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) { +// checkpointPath is where one checkpoint's observed state lives: beside the +// instance record, because it belongs to that emulator and goes when it does. +// +// Indexed by NAME, which the first version was not. An external audit caught +// it: the emulator holds any number of named checkpoints and the CLI wrote a +// single checkpoint.json, so `checkpoint A` followed by `checkpoint B` made +// `reset A` impossible — the observation it would have been judged against had +// been overwritten. The refusal that fired was honest and the feature was +// still missing. +// +// The name has already been through checkpointArgName or is a session +// identity this command built, so it carries no separator. It is cleaned here +// anyway, because a path built from a name is a path whoever reads this file +// next will assume was checked. +// +// TestTwoNamedCheckpointsDoNotOverwriteEachOther fails without this. +func checkpointPath(addr, name string) (string, error) { + dir, err := instanceDir(addr) + if err != nil { + return "", err + } + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) { + return "", fmt.Errorf("%q is not a checkpoint name", name) + } + return filepath.Join(dir, "checkpoints", filepath.Base(name)+".json"), nil +} + +// lastCheckpointPath records which name was taken most recently, so that +// `feint reset` with no argument has something to mean. +func lastCheckpointPath(addr string) (string, error) { dir, err := instanceDir(addr) if err != nil { return "", err } - return filepath.Join(dir, "checkpoint.json"), nil + return filepath.Join(dir, "checkpoints", "last"), nil } func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error { - path, err := checkpointPath(addr) + path, err := checkpointPath(addr, recorded.Name) if err != nil { return err } @@ -328,17 +346,37 @@ func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error { if err != nil { return err } - return os.WriteFile(path, append(body, '\n'), 0o600) + if err := os.WriteFile(path, append(body, '\n'), 0o600); err != nil { + return err + } + // Written after the record, so a failure above leaves `last` pointing at + // a checkpoint that exists. + last, err := lastCheckpointPath(addr) + if err != nil { + return err + } + return os.WriteFile(last, []byte(recorded.Name+"\n"), 0o600) } -func readRecordedCheckpoint(addr string) (recordedCheckpoint, error) { - path, err := checkpointPath(addr) +func readRecordedCheckpoint(addr, name string) (recordedCheckpoint, error) { + if name == "" { + last, err := lastCheckpointPath(addr) + if err != nil { + return recordedCheckpoint{}, err + } + body, err := os.ReadFile(filepath.Clean(last)) + if err != nil { + return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr) + } + name = strings.TrimSpace(string(body)) + } + path, err := checkpointPath(addr, name) 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) + return recordedCheckpoint{}, fmt.Errorf("no checkpoint named %q recorded for %s", name, addr) } var recorded recordedCheckpoint if err := json.Unmarshal(body, &recorded); err != nil { @@ -386,7 +424,7 @@ func takeSessionCheckpoint(endpoint, session, addr string) error { // // TestAFailedResetAfterIsReportedAndNotRaised fails without this. func resetToSession(endpoint, session, addr string, stderr io.Writer) (string, []string) { - recorded, err := readRecordedCheckpoint(addr) + recorded, err := readRecordedCheckpoint(addr, session) 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 diff --git a/internal/cli/checkpoint_test.go b/internal/cli/checkpoint_test.go index 48d21217..ae8f5814 100644 --- a/internal/cli/checkpoint_test.go +++ b/internal/cli/checkpoint_test.go @@ -55,15 +55,16 @@ func TestACheckpointNameIsTakenBeforeTheFlags(t *testing.T) { } } -// TestAResetRefusesToJudgeAgainstAnotherCheckpointsState is the other half of -// the verdict's honesty. +// TestAResetNamesTheCheckpointItCouldNotFind replaces a refusal that lost its +// subject to the audit's second finding. // -// 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) { +// Before, the CLI kept one observation per instance, so asking to reset a +// name it had not recorded last meant comparing one checkpoint's restore to +// another's observation — and the command refused, honestly, while the +// feature was missing. Observations are indexed by name now, so an unknown +// name is simply unknown, and what matters is that the refusal says WHICH +// name and how to make one. +func TestAResetNamesTheCheckpointItCouldNotFind(t *testing.T) { t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) const addr = "127.0.0.1:4599" if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ @@ -74,9 +75,9 @@ func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { 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) + t.Fatalf("a reset of a name nobody recorded was allowed (%d)", code) } - for _, want := range []string{"baseline", "something-else", "compare the wrong states"} { + for _, want := range []string{"something-else", "feint checkpoint"} { if !strings.Contains(stderr.String(), want) { t.Errorf("the refusal does not mention %q: %s", want, stderr.String()) } @@ -94,6 +95,51 @@ func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { } } +// TestTwoNamedCheckpointsDoNotOverwriteEachOther is cited by checkpointPath, +// and it is the audit's second finding. +// +// The emulator holds any number of named checkpoints; the CLI wrote a single +// file per instance. So `checkpoint A` then `checkpoint B` made `reset A` +// impossible, because the observation A would have been judged against had +// been overwritten by B's. +func TestTwoNamedCheckpointsDoNotOverwriteEachOther(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + + for _, name := range []string{"alpha", "beta"} { + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: name, + State: observed{Machines: []string{"feint-scw-" + name}}, + }); err != nil { + t.Fatalf("record %s: %v", name, err) + } + } + + // Both are still readable BY NAME, which is what the single file broke. + for _, name := range []string{"alpha", "beta"} { + back, err := readRecordedCheckpoint(addr, name) + if err != nil { + t.Fatalf("read %s back: %v", name, err) + } + if back.Name != name { + t.Errorf("reading %q answered %q", name, back.Name) + } + if len(back.State.Machines) != 1 || back.State.Machines[0] != "feint-scw-"+name { + t.Errorf("%s carries the wrong observation: %v", name, back.State.Machines) + } + } + + // And an empty name means the last taken, so `feint reset` with no + // argument has something to mean. + last, err := readRecordedCheckpoint(addr, "") + if err != nil { + t.Fatalf("read the last: %v", err) + } + if last.Name != "beta" { + t.Errorf("the last taken reads as %q", last.Name) + } +} + // TestARecordedCheckpointRefusesAVersionItCannotRead is the forward // compatibility half. // @@ -109,7 +155,7 @@ func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) { }); err != nil { t.Fatalf("record a checkpoint: %v", err) } - if _, err := readRecordedCheckpoint(addr); err == nil { + 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) @@ -123,7 +169,7 @@ func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) { }); err != nil { t.Fatalf("record a checkpoint: %v", err) } - back, err := readRecordedCheckpoint(addr) + back, err := readRecordedCheckpoint(addr, "") if err != nil { t.Fatalf("read it back: %v", err) } diff --git a/internal/cli/exit_contract_test.go b/internal/cli/exit_contract_test.go new file mode 100644 index 00000000..4c03e1a2 --- /dev/null +++ b/internal/cli/exit_contract_test.go @@ -0,0 +1,64 @@ +package cli + +import ( + "testing" +) + +// TestAGuaranteeThatWasNotDeliveredFailsTheRun is cited by runCommand's exit +// code, and it closes a contract error an external audit found in merged code. +// +// The first version failed the run only on "did not converge". So this was a +// possible CI run: +// +// terraform test success +// feint reset failed +// feint run exit 0 +// +// and the same held for a lab this command owned and could not stop: the +// receipt said `cleanup: failed` and the exit code stayed at the child's. +// +// The table is written as the full cross product on purpose. Two of the +// verdict words — "unverified" and "not attempted" — are the ones the first +// version let through, and a test that only covered the word it already +// handled would have agreed with the defect. +func TestAGuaranteeThatWasNotDeliveredFailsTheRun(t *testing.T) { + for _, probe := range []struct { + name string + reset string + cleanup string + delivered bool + }{ + // No reset was asked for, so there is nothing to have failed. + {"nothing asked, clean teardown", "", "clean", true}, + {"nothing asked, lab not ours", "", "not owned", true}, + {"nothing asked, kept on purpose", "", "kept", true}, + + // A reset that was asked for and delivered. + {"reset converged", "converged", "clean", true}, + + // Every way a reset can fail to deliver. The first version of this + // guard treated the last three as success. + {"reset did not converge", "did not converge", "clean", false}, + {"the emulator refused the reset", "failed", "clean", false}, + {"the reset could not be read back", "unverified", "clean", false}, + {"there was no checkpoint to return to", "not attempted", "clean", false}, + + // A teardown this command owed and did not perform. The receipt said + // so and the exit code did not. + {"the lab could not be stopped", "", "failed", false}, + {"converged, and the lab leaked anyway", "converged", "failed", false}, + + // A word nobody has written yet must not default to success: that is + // the whole reason the function lists the failures rather than the + // successes. + {"an unknown verdict word", "something-new", "clean", false}, + {"an unknown cleanup word", "", "something-new", false}, + } { + t.Run(probe.name, func(t *testing.T) { + if got := deliveredWhatWasAsked(probe.reset, probe.cleanup); got != probe.delivered { + t.Errorf("deliveredWhatWasAsked(%q, %q) = %v, want %v", + probe.reset, probe.cleanup, got, probe.delivered) + } + }) + } +} diff --git a/internal/cli/observable.go b/internal/cli/observable.go index 7c607f62..02be169d 100644 --- a/internal/cli/observable.go +++ b/internal/cli/observable.go @@ -2,6 +2,8 @@ package cli import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "encoding/xml" "fmt" @@ -36,6 +38,31 @@ import ( // restoring the store and the host leaves bytes from a later test in a bucket // the checkpoint had empty. // +// ## Why the digest is computed here and not read from the listing +// +// The first version of this file took the digest from ListObjectsV2's ETag, +// and `internal/s3/list.go` emits no ETag at all: the field was always empty, +// so two different contents under one key compared EQUAL and a reset could +// report convergence over bytes that had changed. The commit message claimed +// objects were compared by digest. They were not. +// +// It survived review and a passing test because the test built observedObject +// values by hand, with digests in them. A comparison is only worth the +// observation under it, and an observation nobody made is a field with a zero +// value. Found by an external audit of the merged code, not here. +// +// So each object is FETCHED and hashed by the observer. Not the store's own +// recorded digest and not the checksum header the door offers: what is being +// judged is whether a client gets the same bytes back, and a digest supplied +// by the subject cannot answer that. The size travels with it as a second, +// independent signal and is part of the comparison key, which the first +// version also dropped. +// +// It costs one GET per object. That is acceptable for a verdict on a local +// store with a byte budget, and the alternative measured nothing. +// +// TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge fails without this. +// // ## 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 @@ -88,8 +115,16 @@ type observedObject struct { Door string `json:"door"` Bucket string `json:"bucket"` Key string `json:"key"` + // Digest is a SHA-256 this observer computed over the bytes a GET + // delivered. Never the store's own record of it, and never the door's + // checksum header: a digest supplied by the subject cannot answer whether + // the subject serves the bytes it claims. Digest string `json:"digest"` - Size int64 `json:"size"` + // Size is what the GET actually delivered, and Listed is what the + // inventory said. They are compared separately so an inventory that + // disagrees with the bytes is a difference rather than a silent one. + Size int64 `json:"size"` + Listed int64 `json:"listed"` } // observeLab reads the three halves and normalises them. @@ -234,9 +269,18 @@ func observeObjects(ctx context.Context, endpoint string) ([]observedObject, err return nil, fmt.Errorf("%s%s: %w", door.Path, bucket, err) } for _, object := range objects { + digest, size, err := fetchDigest(ctx, client, base+bucket+"/"+object.Key) + if err != nil { + return nil, fmt.Errorf("%s%s/%s: %w", door.Path, bucket, object.Key, err) + } + // The listing's size travels too, and a disagreement between + // it and what the GET delivered is itself a difference: a + // door whose inventory and bytes disagree has not converged + // whatever the digests say. out = append(out, observedObject{ Door: strings.Trim(door.Path, "/"), Bucket: bucket, - Key: object.Key, Digest: object.ETag, Size: object.Size, + Key: object.Key, Digest: digest, Size: size, + Listed: object.Size, }) } } @@ -255,7 +299,6 @@ type bucketListing struct { type objectListing struct { Contents []struct { Key string `xml:"Key"` - ETag string `xml:"ETag"` Size int64 `xml:"Size"` } `xml:"Contents"` IsTruncated bool `xml:"IsTruncated"` @@ -277,13 +320,11 @@ func listBuckets(ctx context.Context, client *http.Client, base string) ([]strin 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 @@ -337,7 +378,11 @@ func (o observed) diff(other observed) []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 + // Every field, and the size twice. The first version compared the + // door, the bucket, the key and a digest that was always empty, so it + // compared nothing about the content at all. + return fmt.Sprintf("%s/%s/%s digest=%s bytes=%d listed=%d", + ob.Door, ob.Bucket, ob.Key, ob.Digest, ob.Size, ob.Listed) } out = append(out, compareSets("resource", mapAll(o.Resources, resourceKey), mapAll(other.Resources, resourceKey))...) @@ -371,3 +416,37 @@ func compareSets(kind string, want, got []string) []string { } return out } + +// fetchDigest reads an object through its door and answers what arrived. +// +// The hash is computed over the response body as it streams, so nothing the +// door says about the bytes is taken on trust, and the byte count comes from +// the hash's own writer rather than from a header. +func fetchDigest(ctx context.Context, client *http.Client, url string) (string, int64, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return "", 0, err + } + res, err := client.Do(req) + if err != nil { + return "", 0, err + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return "", 0, fmt.Errorf("the door answered %d", res.StatusCode) + } + sum := sha256.New() + size, err := io.Copy(sum, io.LimitReader(res.Body, maxObservedObjectBytes)) + if err != nil { + return "", 0, err + } + return hex.EncodeToString(sum.Sum(nil)), size, nil +} + +// maxObservedObjectBytes bounds one object a verdict will read. +// +// Generous rather than tight: the store's own per-object limit is 256 MiB by +// default, and a verdict that silently truncated at a smaller bound would +// compare the first N bytes of two objects and call them equal — the same +// class of defect as the empty digest this replaces. +const maxObservedObjectBytes = 512 << 20 diff --git a/internal/cli/observable_bytes_test.go b/internal/cli/observable_bytes_test.go new file mode 100644 index 00000000..4d20eca9 --- /dev/null +++ b/internal/cli/observable_bytes_test.go @@ -0,0 +1,191 @@ +package cli + +import ( + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/objectstore" + "github.com/stephrobert/feint/internal/s3" +) + +// The defect an external audit found in merged code, and the test that would +// have stopped it. +// +// The observable state took an object's digest from ListObjectsV2's ETag, and +// internal/s3/list.go emits no ETag: the field was always empty, so two +// different contents under one key compared EQUAL. A reset could report +// convergence over bytes that had changed, and the commit message claimed +// objects were compared by digest. +// +// What let it through is the shape of the test beside it: +// TestADifferenceIsNamedWhereverItIs built observedObject values BY HAND, with +// digests in them, so it exercised the comparison and never the observation. +// A fake that emitted an ETag would have hidden it just as well. +// +// So this one mounts a REAL door over a REAL object store. It is the only +// arrangement in which the absent field is visible at all. + +// realDoorLab serves one scaleway/fr-par door over a byte store, plus the +// three endpoints observeLab reads. Nothing here is canned: the listing, the +// bytes and the report all come from internal/s3. +func realDoorLab(t *testing.T) (*httptest.Server, *objectstore.Store) { + t.Helper() + + root := t.TempDir() + if err := os.Chmod(root, 0o700); err != nil { + t.Fatal(err) + } + store, err := objectstore.OpenWithLimit(t.Context(), filepath.Join(root, "objects"), 2<<20) + if err != nil { + t.Fatalf("open the byte store: %v", err) + } + t.Cleanup(func() { + if err := store.Close(); err != nil { + t.Error(err) + } + }) + + control := http.NewServeMux() + control.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"count":0,"resources":[]}`)) + }) + control.HandleFunc("/_feint/health", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"machines":"none","capabilities":null,"enforced":{}}`)) + }) + + doors, err := s3.Mount(control, store, []s3.Profile{ + // The profile the Scaleway pack actually declares, not a minimal one: + // a door without BucketConfiguration answers 501 to the bucket listing, + // and the reader would then observe nothing at all. Copied from the + // pack so this test measures the door an operator gets. + {Provider: "scaleway", Region: "fr-par", MissingBucket: s3.ScalewayMissingBucket, + BucketConfiguration: true, ObjectDiscovery: true, ListObjectsV1: true, + PrivateACLWrite: true}, + }, 1<<20, slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatalf("mount the door: %v", err) + } + // The report the reader validates, served from the doors' own snapshot so + // the test cannot disagree with the code about what is mounted. + mounted := http.NewServeMux() + mounted.HandleFunc(s3.ReportPath, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + writeReportJSON(t, w, doors) + }) + mounted.Handle("/", doors) + + server := httptest.NewServer(mounted) + t.Cleanup(server.Close) + return server, store +} + +func writeReportJSON(t *testing.T, w http.ResponseWriter, doors *s3.Doors) { + t.Helper() + if err := json.NewEncoder(w).Encode(doors.Snapshot()); err != nil { + t.Errorf("serve the S3 report: %v", err) + } +} + +// TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge is cited by the +// observable state's doc, and it is the audit's own decisive case: two objects +// of the same key and the same size, with different bytes, must produce a +// different verdict. +func TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge(t *testing.T) { + lab, _ := realDoorLab(t) + const bucket, key = "images", "disk.img" + base := lab.URL + s3.DoorPath("scaleway", "fr-par") + + // Everything goes THROUGH the door, which is what a client does and what + // this test is about. It also earned its place: a bucket created behind + // the door, straight on the store, carries no creation metadata and the + // door then refuses to list it with a 501 — "legacy bucket listing has no + // measured creation metadata". The first version of this test did that + // and spent three runs blaming the reader. + doorPut(t, base+bucket, "") + + // Same length, different content. A comparison on size alone, or on an + // absent digest, calls these two states equal. + const before, after = "AAAAAAAAAAAAAAAA", "BBBBBBBBBBBBBBBB" + if len(before) != len(after) { + t.Fatal("the two payloads must be the same length or this test proves nothing") + } + + put := func(body string) { + t.Helper() + doorPut(t, base+bucket+"/"+key, body) + } + + put(before) + checkpoint, err := observeLab(t.Context(), lab.URL, machine.Runtime{}) + if err != nil { + t.Fatalf("observe the checkpoint: %v", err) + } + if len(checkpoint.Objects) != 1 { + t.Fatalf("the observation found %d object(s); the door or the reader is not wired", + len(checkpoint.Objects)) + } + // The digest must be REAL. An empty one is how the defect looked. + if len(checkpoint.Objects[0].Digest) != 64 { + t.Fatalf("the observation carries digest %q; an empty or short digest compares everything equal", + checkpoint.Objects[0].Digest) + } + if checkpoint.Objects[0].Size != int64(len(before)) { + t.Errorf("the observation read %d bytes", checkpoint.Objects[0].Size) + } + + put(after) + changed, err := observeLab(t.Context(), lab.URL, machine.Runtime{}) + if err != nil { + t.Fatalf("observe the change: %v", err) + } + + differences := checkpoint.diff(changed) + if len(differences) == 0 { + t.Fatal("two objects of one key and one size with different bytes compared EQUAL; " + + "a reset would report convergence over content that changed") + } + joined := strings.Join(differences, "\n") + if !strings.Contains(joined, key) { + t.Errorf("the verdict does not name the object that changed:\n%s", joined) + } + + // And the same bytes twice compare equal, so this cannot be satisfied by + // a comparison that always differs — which would make every reset fail + // and teach an operator to ignore the verdict. + put(before) + back, err := observeLab(t.Context(), lab.URL, machine.Runtime{}) + if err != nil { + t.Fatalf("observe the restoration: %v", err) + } + if differences := checkpoint.diff(back); len(differences) != 0 { + t.Errorf("the same bytes compared differently:\n%s", strings.Join(differences, "\n")) + } +} + +// doorPut writes through the door, so that what the listing and the GET see +// is what a client would have made. +func doorPut(t *testing.T, url, body string) { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodPut, url, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + req.ContentLength = int64(len(body)) + res, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT %s: %v", url, err) + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + answer, _ := io.ReadAll(io.LimitReader(res.Body, 1<<12)) + t.Fatalf("PUT %s answered %d: %s", url, res.StatusCode, answer) + } +} diff --git a/internal/cli/reset_after_test.go b/internal/cli/reset_after_test.go index 309c0ba4..8523fa51 100644 --- a/internal/cli/reset_after_test.go +++ b/internal/cli/reset_after_test.go @@ -103,8 +103,15 @@ func TestAFailedResetAfterIsReportedAndNotRaised(t *testing.T) { 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. + // state. Since the audit, records are indexed by name, so the + // isolation is the lookup itself: a run asks for ITS session and + // finds nothing when only another run's was recorded. + // + // A fresh runtime directory, because the subtests above recorded + // their own sessions here and the first version of this one found + // one of them — the test passing for the wrong reason, which is what + // the per-name index changed underneath it. + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ SchemaVersion: checkpointStateVersion, Name: "feint-lab-OTHER", }); err != nil { @@ -115,6 +122,11 @@ func TestAFailedResetAfterIsReportedAndNotRaised(t *testing.T) { if verdict != "not attempted" { t.Errorf("verdict %q for another run's session", verdict) } + // And the other run's record is untouched: this one did not reset it + // and did not delete it either. + if back, err := readRecordedCheckpoint(addr, "feint-lab-OTHER"); err != nil || back.Name != "feint-lab-OTHER" { + t.Errorf("another run's record was disturbed: %+v %v", back, err) + } }) } @@ -136,7 +148,7 @@ func TestASessionCheckpointIsNamedAfterItsRun(t *testing.T) { if len(asked) != 1 || asked[0] != "/_feint/checkpoint/feint-lab-4021" { t.Fatalf("the emulator was asked for %v", asked) } - recorded, err := readRecordedCheckpoint(addr) + recorded, err := readRecordedCheckpoint(addr, "") if err != nil { t.Fatalf("read the record back: %v", err) } diff --git a/internal/cli/run.go b/internal/cli/run.go index 8e41a8ed..0a06db49 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -278,11 +278,29 @@ 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 { + // A guarantee this command was ASKED for and did not deliver is this + // command's failure, even when the child passed. + // + // The first version only failed on "did not converge", which an external + // audit read correctly as a contract error: a reset that could not be + // attempted, that the emulator refused, or that could not be read back + // left the exit code at the child's, so + // + // terraform test success + // feint reset failed + // feint run exit 0 + // + // was a possible CI run. The same held for a lab this command owned and + // could not stop: `cleanup = "failed"` reached the receipt and nothing + // else, so a leaked port and a leaked directory reported success. + // + // So the rule is the general one, and it is worth stating because it is + // what a wrapper is for: the child's code travels untouched when the + // wrapper kept its promises, and the wrapper fails when it did not. Which + // of the two happened is in the receipt, because "the test failed" and + // "the lab leaked" send a reader to different places. + // TestAGuaranteeThatWasNotDeliveredFailsTheRun fails without this. + if code == exitOK && !deliveredWhatWasAsked(resetVerdict, cleanup) { return exitError } // The child's code, not this command's: a wrapper that swallowed a @@ -307,6 +325,34 @@ func keepsTheLab(code int, runErr error, asked bool) bool { return code != exitOK || runErr != nil } +// deliveredWhatWasAsked reports whether this command kept the promises it was +// given, which is a different question from whether the child passed. +// +// Written as a list of the answers that are NOT a success rather than the ones +// that are: a verdict word added later would otherwise default to "fine", and +// the one thing this function exists to prevent is a new way of failing that +// nobody notices because the switch did not name it. +func deliveredWhatWasAsked(resetVerdict, cleanup string) bool { + switch resetVerdict { + case "", "converged": + // Empty means --reset-after was never asked for, so there is nothing + // to have failed. + default: + // "did not converge", "failed", "unverified" and "not attempted" are + // all a reset the caller asked for and did not get. + return false + } + switch cleanup { + case "clean", "kept", "not owned": + // Kept is a success: --keep-on-failure asked for exactly that. + // Not owned is this command declining to claim a teardown it did not + // perform. + default: + return false + } + return true +} + // labSession names an owned lab, and deliberately nothing for an attached // one: its identity belongs to whoever started it, and inventing a second name // for it would put two names on one emulator. diff --git a/internal/core/emulator/atomic_checkpoint_test.go b/internal/core/emulator/atomic_checkpoint_test.go new file mode 100644 index 00000000..95828e1a --- /dev/null +++ b/internal/core/emulator/atomic_checkpoint_test.go @@ -0,0 +1,110 @@ +package emulator_test + +import ( + "io" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/objectstore" + "github.com/stephrobert/feint/internal/core/resource" +) + +// TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole is cited by +// handleCheckpointSave, and it is the audit's finding about my own comment. +// +// The first version saved the bytes first and the control plane second while +// promising "both halves or neither". A failure in the second step then left +// an earlier checkpoint of that name holding its OLD store beside the NEW +// object references, and a later reset restored one against the other. +// +// ## Why this test looks the way it does +// +// A first attempt made the failure by closing the byte store, and it measured +// NOTHING: with the bytes failing first, both orders refuse at the same step +// and the earlier checkpoint survives either way. The mutation that reverses +// the order stayed green, which is the harness catching the author. +// +// The scenario that separates the two orders is the one the audit actually +// described: the byte half SUCCEEDS and the control-plane half fails. So the +// store is made unmarshallable — a resource carrying a value encoding/json +// cannot write — and the assertion is on the BYTES a later reset brings back. +// +// Under the shipped order: the snapshot fails first, the bytes are never +// touched, and the reset returns the original content. Under the reversed +// order: the new bytes are published, the snapshot then fails, and the reset +// returns content that was never checkpointed. +func TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole(t *testing.T) { + root := t.TempDir() + if err := os.Chmod(root, 0o700); err != nil { + t.Fatal(err) + } + store, err := objectstore.OpenWithLimit(t.Context(), filepath.Join(root, "objects"), 2<<20) + if err != nil { + t.Fatalf("open the byte store: %v", err) + } + t.Cleanup(func() { + if err := store.Close(); err != nil { + t.Error(err) + } + }) + server, held := labServerWithBytes(t, store, stubPack{name: "acme"}) + + const bucket, key = "acme/images", "disk.img" + const original, later = "the bytes the checkpoint holds", "bytes nobody checkpointed" + if err := store.CreateBucket(t.Context(), bucket); err != nil { + t.Fatalf("create the bucket: %v", err) + } + write := func(body string) { + t.Helper() + if _, err := store.PutObject(t.Context(), bucket, key, + strings.NewReader(body), objectstore.PutOptions{Size: int64(len(body))}); err != nil { + t.Fatalf("put: %v", err) + } + } + + write(original) + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("the first checkpoint: %d %v", code, body) + } + + // The lab moves on, and the control-plane half is then made to fail while + // the byte half would succeed. + write(later) + unmarshallable := resource.New("broken", "instance/server", + resource.Tenant{Provider: "acme", Project: "p", Zone: "z"}, "running", + time.Unix(1700000000, 0).UTC()) + unmarshallable.Attrs = map[string]any{"cannot": make(chan int)} + held.Put(unmarshallable) + + code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline") + if code == http.StatusOK { + t.Fatalf("a checkpoint whose control-plane half could not be taken reported success: %v", body) + } + if message, _ := body["error"].(string); !strings.Contains(message, "control plane") { + t.Errorf("the refusal does not name the half that failed: %v", body["error"]) + } + + // The earlier checkpoint's BYTES are intact. This is the assertion the + // reversed order fails: it would have published `later` under the name + // `baseline` before refusing. + if err := store.ResetCheckpoint(t.Context(), "baseline"); err != nil { + t.Fatalf("reset the byte checkpoint: %v", err) + } + _, reader, err := store.GetObject(t.Context(), bucket, key) + if err != nil { + t.Fatalf("read the object back: %v", err) + } + defer func() { _ = reader.Close() }() + back, err := io.ReadAll(reader) + if err != nil { + t.Fatalf("read: %v", err) + } + if string(back) != original { + t.Errorf("the checkpoint's bytes are %q, not the %q it held: a half-taken checkpoint "+ + "published bytes the control plane never saw", back, original) + } +} diff --git a/internal/core/emulator/checkpoint.go b/internal/core/emulator/checkpoint.go index b149039a..cbb6c138 100644 --- a/internal/core/emulator/checkpoint.go +++ b/internal/core/emulator/checkpoint.go @@ -120,13 +120,28 @@ func (c *checkpoints) drop(name string) bool { // 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. +// Both halves or neither, and the ORDER is what makes that true rather than a +// sentence. An external audit caught the first version claiming it: the bytes +// were saved first, so a failure in the store snapshot left a previously taken +// checkpoint of the same name holding its OLD control plane beside the NEW +// object references. A later reset would have restored one against the other, +// and the comment promising otherwise is the defect this repository names most +// often. // -// TestACheckpointTakesBothHalvesOrNeither fails without this. +// The order that delivers it: +// +// 1. take the control-plane snapshot into memory. Nothing durable is touched, +// and a failure here leaves the previous checkpoint whole; +// 2. save the bytes, which objectstore publishes by renaming a temporary +// file: it either replaces the old checkpoint or leaves it, never a +// half of one; +// 3. publish the control-plane half only now. +// +// So a failure at any step leaves the previous checkpoint consistent with +// itself, which is the only promise worth making. +// +// TestACheckpointTakesBothHalvesOrNeither and +// TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole fail without this. func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { name := r.PathValue("name") if !validCheckpointName(name) { @@ -135,6 +150,13 @@ func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { }) 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 + } if s.env.Objects != nil { if err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil { writeJSON(w, http.StatusInternalServerError, map[string]any{ @@ -143,13 +165,9 @@ func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { 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 - } + // Published last, so that a byte half that failed above leaves any earlier + // checkpoint of this name holding its own control plane rather than a + // newer one. s.checkpoints.put(name, state.Bytes()) writeJSON(w, http.StatusOK, map[string]any{ "schema_version": CheckpointSchemaVersion, 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 69347c3b..036950db 100644 --- a/tools/falsify/specs/a-reset-is-not-a-restore.json +++ b/tools/falsify/specs/a-reset-is-not-a-restore.json @@ -1,6 +1,6 @@ { "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.", + "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.\n\nSince an external audit of the merged code: the verdict's object comparison is in here too. It took the digest from ListObjectsV2's ETag, which internal/s3/list.go does not emit, so the field was always empty and two different contents under one key compared equal. The mutation that reproduces it is below, and the test that bites mounts a REAL door over a REAL store — the test that let the defect through built observedObject values by hand, with digests in them, so it exercised the comparison and never the observation.", "mutations": [ { "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", @@ -58,6 +58,29 @@ "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/" + }, + { + "label": "the object comparison goes back to the door, the bucket and the key, dropping the digest and both sizes. This IS the defect an audit found in merged code: with no content in the key, a reset reports convergence over bytes that changed, and the commit message claimed objects were compared by digest", + "file": "internal/cli/observable.go", + "find": "\t\treturn fmt.Sprintf(\"%s/%s/%s digest=%s bytes=%d listed=%d\",\n\t\t\tob.Door, ob.Bucket, ob.Key, ob.Digest, ob.Size, ob.Listed)", + "replace": "\t\treturn fmt.Sprintf(\"%s/%s/%s\",\n\t\t\tob.Door, ob.Bucket, ob.Key) + fmt.Sprint(ob.Digest, ob.Size, ob.Listed)[:0]", + "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", + "package": "./internal/cli/" + }, + { + "label": "the digest stops being computed over the bytes that arrived and becomes the empty string, which is exactly what reading an absent ETag produced. A verdict whose digest is constant compares every content equal, and the run reports a clean reset", + "file": "internal/cli/observable.go", + "find": "\treturn hex.EncodeToString(sum.Sum(nil)), size, nil", + "replace": "\treturn hex.EncodeToString(sum.Sum(nil))[:0], size, nil", + "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", + "package": "./internal/cli/" + }, + { + "label": "the two halves are taken in the order an audit caught: the bytes first, the control plane second. A failure in the second step then leaves an earlier checkpoint of that name holding its OLD store beside the NEW object references, and a later reset restores one against the other — while the comment above promises both halves or neither. The order IS the guard here, so the mutation reorders rather than neutralises", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}\n\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}", + "replace": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}\n\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}", + "test": "TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole" } ] } diff --git a/tools/falsify/specs/a-run-owns-its-lab.json b/tools/falsify/specs/a-run-owns-its-lab.json index eff81685..9b9d1131 100644 --- a/tools/falsify/specs/a-run-owns-its-lab.json +++ b/tools/falsify/specs/a-run-owns-its-lab.json @@ -96,9 +96,23 @@ { "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 {", + "find": "\tif code == exitOK && !deliveredWhatWasAsked(resetVerdict, cleanup) {", + "replace": "\tif code == exitOK && !deliveredWhatWasAsked(resetVerdict, cleanup) && false {", "test": "TestARunWhoseResetDidNotConvergeFails" + }, + { + "label": "only a non-converging reset fails the run, which is the contract error an audit found: a reset the emulator refused, one that could not be read back, and one with no checkpoint to return to all left the exit code at the child's, so a CI run could read `terraform test success / feint reset failed / feint run exit 0`", + "file": "internal/cli/run.go", + "find": "\tcase \"\", \"converged\":", + "replace": "\tcase \"\", \"converged\", \"failed\", \"unverified\", \"not attempted\":", + "test": "TestAGuaranteeThatWasNotDeliveredFailsTheRun" + }, + { + "label": "a lab this command owned and could not stop stops failing the run, so a leaked port and a leaked directory report success with `cleanup: failed` in a receipt nobody reads. The receipt said it and the exit code did not, which is the half that reaches CI", + "file": "internal/cli/run.go", + "find": "\tswitch cleanup {\n\tcase \"clean\", \"kept\", \"not owned\":", + "replace": "\tswitch cleanup {\n\tcase \"clean\", \"kept\", \"not owned\", \"failed\":", + "test": "TestAGuaranteeThatWasNotDeliveredFailsTheRun" } ] } From dce97886131dd77fc2f0675a4f6db65854492113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 14:38:58 +0100 Subject: [PATCH 2/5] fix(reset): a reset puts the rule back on the NIC, not only in the store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit's third P0, reproduced before being believed, then fixed — plus two defects the reproduction itself surfaced, one of them mine. ## The finding, and why it waited The audit deduced from the code that a reset could report convergence while a firewall rule was still missing from the runtime, and said plainly that it had not reproduced it. Neither had anybody here, so the previous commit shipped without a fix and said so. The premise was right. `Survey` answers NAMES; a rule set whose content changed after the checkpoint keeps its name, so nothing in the observation can see the difference. `tools/conformance/scaleway/network.sh` now takes a checkpoint with port 80 open, lets the suite revoke the rule, resets, and asks a TCP connection: - revoking it closes it again ok: port 80 refused again - and a reset puts the rule back on the NIC, not only in the store FAIL: the reset reported convergence and port 80 is still refused: the rule came back in the store and not on the NIC That is the test that bites. The fix came after it. ## The fix: a third optional half `emulator.RuleReconciler`, beside `Reconciler` and `MissingReconciler` and separate from them for the reason MissingReconciler already documents — one interface carrying two methods turned every pack into a non-implementer in silence, and the call still answered 200. The gesture is shared: `machine.GroupSync.Reapply` translates the restored group, writes the set and replays it onto the wearers, which is the same sequence for the three packs. What varies is one word — which restored record a provider calls a group — so each pack writes a loop and a kind. The firewall reconciliation was written once in a pack before, and the two others ran with zero ACLs while their API described a closed port (#475). Ordered last of the three, because a set attaches to the interfaces of the machines that wear it: re-applying before ReconcileMissing has put those machines back would attach nothing. ## Two more, from the same leg **Mine, immediately.** The first version re-applied every restored group, and the runtime leg reported two rule sets appearing on the host that the checkpoint never held — left over from earlier suites whose machines were long swept. A reset puts back what the checkpoint had; it does not furnish the host with objects it did not. Only a group somebody wears is re-applied now, which is also exactly the group the audit's case is about. **Older than the audit.** A record keeps its machine name after the machine is gone — that is how ReconcileGone finds what to destroy — so "names a machine the host does not hold" also describes, word for word, a server the checkpoint says is stopped. A reset was starting containers for terminated VMs and writing `running` over the state it had been asked to return to, and the verdict then named the resource the reset had itself changed: resource present and not in the checkpoint: vm i-a3f536ec state=running resource in the checkpoint and not present: vm i-a3f536ec state=terminated machine present and not in the checkpoint: feint-osc-i-a3f536ec The comparison is in the shared layer, against `Binding.RunningState`: the word varies, the gesture does not, and the Binding already carries that field because PowerOn writes it. Measured as pre-existing before being attributed — the leg was run with these changes stashed, and failed on those three lines alone. ## Proof `FEINT_VM=incus mise run conformance:leg -- runtime`, 94 s, green end to end, including the new reset step and `reset-host.sh`, which the stashed baseline failed. /falsify: 15 mutations on a-reset-is-not-a-restore.json, all biting, including the four added here. Both directions of the firewall fix are mutated, because a guard that refuses everything passes the attack and breaks the product. mise run prepush green. The fixed sleep this adds carries its `waits on silence:` reason, which TestEveryFixedSleepInARuntimeSuiteSaysWhatItWaitsFor demanded before it would pass. ## The debt that remains, with its number The concurrency question the audit raises at the end: a reset is not a transaction, and nothing measures what a create or a delete arriving mid-reset does. It is now #870, with the three paths read from the code, what would close it, and the statement that none of it is reproduced yet. It is not a reason to hold #853 open. Assisted-by: Claude Code (claude-opus-5) --- internal/cli/reconcile_rules_test.go | 51 +++++ internal/core/emulator/checkpoint.go | 46 +++- internal/core/emulator/reapply_test.go | 91 ++++++++ internal/core/machine/groupsync.go | 77 ++++++- .../core/machine/groupsync_reapply_test.go | 85 ++++++++ internal/core/machine/reconcile.go | 20 ++ .../core/machine/reconcile_stopped_test.go | 73 +++++++ internal/core/machine/surface.go | 1 + internal/providers/exoscale/firewall.go | 16 ++ internal/providers/outscale/firewall.go | 16 ++ internal/providers/scaleway/firewall.go | 16 ++ tools/conformance/scaleway/network.sh | 50 +++++ .../specs/a-reset-is-not-a-restore.json | 198 ++++++++++-------- 13 files changed, 652 insertions(+), 88 deletions(-) create mode 100644 internal/cli/reconcile_rules_test.go create mode 100644 internal/core/emulator/reapply_test.go create mode 100644 internal/core/machine/groupsync_reapply_test.go create mode 100644 internal/core/machine/reconcile_stopped_test.go diff --git a/internal/cli/reconcile_rules_test.go b/internal/cli/reconcile_rules_test.go new file mode 100644 index 00000000..9d79f71d --- /dev/null +++ b/internal/cli/reconcile_rules_test.go @@ -0,0 +1,51 @@ +package cli + +import ( + "testing" + + "github.com/stephrobert/feint/internal/core/emulator" +) + +// TestAPackThatEnforcesRulesReappliesThemOnReset is cited by +// emulator.RuleReconciler. +// +// A pack that pushes its rules onto the runtime owns a second promise it is +// easy not to notice: bringing them BACK. A reset restores the control plane +// and asks the runtime what it holds, and Survey answers names. A rule set +// whose content changed after the checkpoint keeps its name, so the +// observation sees no difference, the reset reports convergence, and the +// runtime goes on enforcing the rules of a state nobody asked to return to. +// The network suite measured that with a TCP connection before this interface +// existed. +// +// The subject is read from the pack — "does it declare that it enforces" — +// rather than from a list of provider names, so a fourth pack inherits the +// obligation without anyone editing this file. That is rule 5 applied to a +// test, and the same shape as its neighbour +// TestAPackThatDrivesMachinesReconcilesBothWays. +func TestAPackThatEnforcesRulesReappliesThemOnReset(t *testing.T) { + srv, _, err := newServer(nil) + if err != nil { + t.Fatalf("build the emulator: %v", err) + } + + examined := 0 + for _, pack := range srv.Packs() { + enforcer, declares := pack.(emulator.FirewallEnforcer) + if !declares || !enforcer.EnforcesFirewall() { + // A pack whose rules stay metadata has nothing to put back. + continue + } + examined++ + if _, reapplies := pack.(emulator.RuleReconciler); !reapplies { + t.Errorf("%s pushes its rules onto the runtime and cannot put them back: a reset "+ + "would report convergence while the runtime still enforces the rules of a "+ + "state the checkpoint does not describe", pack.Name()) + } + } + + // A denominator, because a loop over nothing passes silently. + if examined < 3 { + t.Fatalf("only %d pack(s) declare that they enforce rules; this build has three", examined) + } +} diff --git a/internal/core/emulator/checkpoint.go b/internal/core/emulator/checkpoint.go index cbb6c138..090e4213 100644 --- a/internal/core/emulator/checkpoint.go +++ b/internal/core/emulator/checkpoint.go @@ -83,6 +83,28 @@ type MissingReconciler interface { ReconcileMissing(ctx context.Context, records []*resource.Resource, held []string) machine.Reconciled } +// RuleReconciler is the third optional half, and separate from the other two +// for the reason MissingReconciler spells out: a pack gains one when it is +// ready, and the ones it already had keep working. Merging them would turn +// every pack into a non-implementer in silence. +// +// It exists because a reset that only restores the store reports convergence +// over a firewall the runtime is still enforcing from before the checkpoint. +// Survey answers names, a modified rule set keeps its name, so nothing in the +// observation can see the difference — and machine.GroupSync.Reapply carries +// the measurement that turned that deduction into a defect. +// +// tools/conformance/scaleway/network.sh and internal/cli's +// TestAPackThatEnforcesRulesReappliesThemOnReset fail without it. +type RuleReconciler interface { + // ReconcileRules writes this pack's restored rule sets to the runtime + // again. It takes the pack's own records, because which of them is a + // group is the pack's vocabulary — "instance/security-group", + // "securitygroup" and "security-group" for the three — and the core + // guessing at that is the mistake ReconcileMissing documents. + ReconcileRules(ctx context.Context, records []*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 @@ -254,11 +276,15 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { // asked would start containers beside ones already running. held, surveyed := heldOnHost(r.Context(), s.env) reconciled := map[string]any{} - var destroyed, skipped, recreated, failed []string + var destroyed, skipped, recreated, failed, reapplied []string for _, pack := range s.packs { mine := ownedBy(gone, pack.Name()) records := ownedBy(restored, pack.Name()) - if len(mine) == 0 && (!surveyed || len(records) == 0) { + // Records alone are enough to have work to do, with no survey: rule + // sets live on the runtime whether or not a machine wears one, and an + // earlier form of this guard skipped the whole pack when nothing could + // be surveyed, which would have skipped the firewall half with it. + if len(mine) == 0 && len(records) == 0 { continue } done := machine.Reconciled{} @@ -279,15 +305,26 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { s.env.Store.Put(res) } } + // Last of the three, and the order is the point: a set is attached to + // the interfaces of the machines that wear it, so re-applying before + // ReconcileMissing has put those machines back would attach nothing. + if rules, ok := pack.(RuleReconciler); ok && len(records) != 0 { + back := rules.ReconcileRules(r.Context(), records) + done.Reapplied = append(done.Reapplied, back.Reapplied...) + done.Skipped = append(done.Skipped, back.Skipped...) + } destroyed = append(destroyed, done.Destroyed...) + reapplied = append(reapplied, done.Reapplied...) 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, + "reapplied": done.Reapplied, } } + sort.Strings(reapplied) sort.Strings(destroyed) sort.Strings(skipped) sort.Strings(recreated) @@ -300,6 +337,11 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { "destroyed": destroyed, "skipped": skipped, "recreated": recreated, + // Named in the receipt rather than folded into recreated: nothing was + // started, a rule set the runtime already held was overwritten with + // the restored rules. An operator reading "recreated" would look for + // a machine that never moved. + "reapplied": reapplied, // 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. diff --git a/internal/core/emulator/reapply_test.go b/internal/core/emulator/reapply_test.go new file mode 100644 index 00000000..f6882943 --- /dev/null +++ b/internal/core/emulator/reapply_test.go @@ -0,0 +1,91 @@ +package emulator_test + +import ( + "context" + "net/http" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" +) + +// reapplyingPack implements the third half and records what it was handed. +type reapplyingPack struct { + stubPack + asked [][]string +} + +func (p *reapplyingPack) ReconcileRules(_ context.Context, records []*resource.Resource) machine.Reconciled { + var groups []string + for _, res := range records { + if res.Kind == "instance/security-group" { + groups = append(groups, res.ID) + } + } + p.asked = append(p.asked, groups) + return machine.Reconciled{Reapplied: groups} +} + +// TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore is cited by +// emulator.RuleReconciler and by machine.GroupSync.Reapply. +// +// ## The defect it closes +// +// An external audit deduced it and tools/conformance/scaleway/network.sh then +// measured it under --vm incus, with the only instrument whose verdict cannot +// be argued with: +// +// - and a reset puts the rule back on the NIC, not only in the store +// FAIL: the reset reported convergence and port 80 is still refused: +// the rule came back in the store and not on the NIC +// +// A reset restores the control plane and asks the runtime what it HOLDS, and +// Survey answers NAMES. The rule set of a group whose rules changed after the +// checkpoint keeps its name, so the observation sees no difference at all: the +// reset reports convergence while the runtime goes on enforcing a state the +// checkpoint does not describe. +// +// ## The two properties this asserts, and why each would have been missed +// +// 1. the group is handed over although it is UNCHANGED by the restore. It is +// not in `gone` and its machine is not missing, so both existing halves +// skip it — and the whole point is that the record is identical and the +// runtime is not; +// 2. it is handed over with NO surveyable runtime. Rule sets live on the +// runtime whether or not any machine wears one, and the handler's guard +// used to skip a pack entirely when nothing could be surveyed, which would +// have taken this half with it. +func TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore(t *testing.T) { + acme := &reapplyingPack{stubPack: stubPack{name: "acme"}} + // The zero runtime, deliberately: see property 2. + server, held := labServer(t, acme) + + put(t, held, "acme", "instance/security-group", "open-80") + put(t, held, "acme", "instance/server", "web") + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + + // Nothing is added or removed between the checkpoint and the reset: the + // rules of the group changed, which the store round-trips and the runtime + // does not. + code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") + if code != http.StatusOK { + t.Fatalf("reset: %d %v", code, body) + } + if len(acme.asked) != 1 { + t.Fatalf("the pack was asked to re-apply its rules %d time(s); a reset must ask exactly once", + len(acme.asked)) + } + if len(acme.asked[0]) != 1 || acme.asked[0][0] != "open-80" { + t.Fatalf("the pack was handed %v; the restored group is what its rule set is translated from", + acme.asked[0]) + } + // And the receipt says so, rather than leaving a reader to infer it from a + // quiet success. Named apart from recreated: nothing was started. + back, _ := body["reapplied"].([]any) + if len(back) != 1 || back[0] != "open-80" { + t.Errorf("the receipt reports reapplied=%v, so an operator cannot tell the rules were put back", + body["reapplied"]) + } +} diff --git a/internal/core/machine/groupsync.go b/internal/core/machine/groupsync.go index 902cab96..7af27ae6 100644 --- a/internal/core/machine/groupsync.go +++ b/internal/core/machine/groupsync.go @@ -264,12 +264,23 @@ func (s GroupSync) spec(group, fresh *resource.Resource) FirewallSpec { // fresh, when non-nil, wins over the store's copy of the same resource, in the // wearer replay as in the expansion — SpecOf says why. func (s GroupSync) SyncGroup(ctx context.Context, group, fresh *resource.Resource) { + _ = s.syncGroup(ctx, group, fresh) +} + +// syncGroup is SyncGroup with its verdict kept. +// +// The bool exists for Reapply alone: every other caller acts on a change it +// just made and has nothing to report, while a reconciliation answers an +// operator who asked whether the lab came back. false means the runtime was +// not wired or refused the set — in both cases the rules are metadata again, +// and saying so is the point. +func (s GroupSync) syncGroup(ctx context.Context, group, fresh *resource.Resource) bool { if !s.wired() { - return + return false } fw := s.enforcer() if !s.Binding.SyncRuleSet(ctx, fw, s.spec(group, fresh)) { - return + return false } for _, wearer := range s.Wearers(group) { if fresh != nil && wearer.ID == fresh.ID { @@ -277,6 +288,68 @@ func (s GroupSync) SyncGroup(ctx context.Context, group, fresh *resource.Resourc } s.applyMachine(ctx, wearer, fresh) } + return true +} + +// Reapply writes every one of these groups to the runtime again and replays +// each onto its wearers. +// +// ## Why a reset needs this at all +// +// An external audit deduced it from the code and this repository then measured +// it, which is the order that makes it a defect rather than a worry. A reset +// restores the control plane and asks the runtime what it HOLDS, and Survey +// answers NAMES. A rule revoked after the checkpoint leaves its rule set in +// place under the same name, carrying the revoked content: nothing in the +// observation differs, the reset reports convergence, and the port stays +// closed. The suite reproduced exactly that under --vm incus, with a TCP +// connection as the instrument: +// +// FAIL: the reset reported convergence and port 80 is still refused: +// the rule came back in the store and not on the NIC +// +// Destroying and recreating the set would be the wrong repair: it is a +// reconfiguration, not a resurrection, and SyncRuleSet already replaces a set +// of the same name with the restored translation. +// +// ## Why it is shared and not three copies +// +// The sequence is identical for the three packs — translate the restored +// group, write the set, replay it onto the wearers — and only the KIND of +// record a pack calls a group differs. That is the repository's own test: a +// line that could be written identically for another provider belongs here. +// The firewall reconciliation was written once in a pack before, and the two +// others ran with zero ACLs while their API described a closed port (#475). +// +// tools/conformance/scaleway/network.sh fails without this. +func (s GroupSync) Reapply(ctx context.Context, groups []*resource.Resource) Reconciled { + var done Reconciled + for _, group := range groups { + // Only a group somebody WEARS. A reset puts back what the checkpoint + // held; it does not furnish the host with objects the checkpoint never + // had, and a rule set attached to no interface enforces nothing by + // definition. The first version wrote every restored group, and the + // runtime leg caught it at once: two rule sets left over from earlier + // suites, whose machines were long swept, appeared on the host and the + // verdict reported them as differences the reset had created. + // + // This is also why the audit's case is still covered: the group whose + // rule was revoked is worn by the two servers the suite started, so it + // is precisely the one re-applied. + if len(s.Wearers(group)) == 0 { + done.Skipped = append(done.Skipped, group.ID) + continue + } + if s.syncGroup(ctx, group, nil) { + done.Reapplied = append(done.Reapplied, group.ID) + continue + } + // Not Failed: a runtime that enforces nothing is the documented + // degraded mode, not an error, and the caller already knows whether + // one was asked. Skipped is the answer for "nobody could act on it". + done.Skipped = append(done.Skipped, group.ID) + } + return done } // ApplyMachine writes every set one resource wears and attaches them to its diff --git a/internal/core/machine/groupsync_reapply_test.go b/internal/core/machine/groupsync_reapply_test.go new file mode 100644 index 00000000..b3414c8b --- /dev/null +++ b/internal/core/machine/groupsync_reapply_test.go @@ -0,0 +1,85 @@ +package machine + +import ( + "slices" + "testing" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// TestReapplyWritesTheWornSetsAndOnlyThose is cited by GroupSync.Reapply. +// +// ## What Reapply is for +// +// A reset restores the control plane and asks the runtime what it HOLDS, and +// Survey answers NAMES. A rule set whose content changed after the checkpoint +// keeps its name, so the observation cannot see the difference: the reset +// reports convergence while the runtime goes on enforcing rules the checkpoint +// does not describe. tools/conformance/scaleway/network.sh measured exactly +// that with a TCP connection, and this is the shared gesture that answers it. +// +// ## Why the wearer filter is asserted, and in the same test +// +// The first version wrote every restored group, and the runtime leg caught it +// in one pass: rule sets left over from earlier suites, whose machines were +// long swept, appeared on the host and the verdict reported them as +// differences the reset had created. A reset puts back what the checkpoint +// held; it does not furnish the host with objects the checkpoint never had, +// and a set attached to no interface enforces nothing by definition. +// +// The two halves are asserted together on purpose. A guard that refused every +// group would pass the second half alone, and would silently turn the whole +// re-application off — which is the defect this file exists to close. +func TestReapplyWritesTheWornSetsAndOnlyThose(t *testing.T) { + b := newGroupSyncBench() + worn := b.group("worn", "") + orphan := b.group("orphan", "") + b.machine("m-web", "10.42.0.9", "worn") + + done := b.sync().Reapply(t.Context(), []*resource.Resource{worn, orphan}) + + // The worn set reached the runtime, with its rules. + spec := ensuredSpec(t, b.rec, FirewallName("bench", "worn")) + if len(spec.Rules) == 0 { + t.Errorf("the re-applied set carries no rule: %+v", spec) + } + if !slices.Equal(done.Reapplied, []string{"worn"}) { + t.Errorf("reapplied %v; the group a machine wears is the one to put back", done.Reapplied) + } + + // And the orphan set did not, which is the half the host pays for. + for _, e := range b.rec.Events() { + if e.Kind == "EnsureFirewall" && e.Resource == FirewallName("bench", "orphan") { + t.Fatalf("a reset created a rule set nobody wears: the checkpoint never held it, "+ + "and the verdict above reports it as a difference the reset itself made (%v)", e) + } + } + if !slices.Equal(done.Skipped, []string{"orphan"}) { + t.Errorf("skipped %v; a group nobody could act on is reported, never passed over in silence", + done.Skipped) + } +} + +// TestReapplyOnNoRuntimeSaysSoRatherThanClaimingSuccess holds the degraded +// mode, which is the DEFAULT here: --vm off is what an ordinary run uses. +// +// Reporting these as Reapplied would have a reset tell an operator that rules +// are enforced again on a runtime that enforces nothing, which is the exact +// lie this emulator exists to avoid. Failed would be wrong too: nothing +// refused anything, and docs/limits.md documents the mode. +func TestReapplyOnNoRuntimeSaysSoRatherThanClaimingSuccess(t *testing.T) { + b := newGroupSyncBench() + group := b.group("worn", "") + b.machine("m-web", "10.42.0.9", "worn") + + sync := b.sync() + sync.Binding = Binding{Provider: "bench", Prefix: "feint-bench-", RuntimeKey: "machine"} + + done := sync.Reapply(t.Context(), []*resource.Resource{group}) + if len(done.Reapplied) != 0 || len(done.Failed) != 0 { + t.Errorf("a binding with no runtime reported %+v", done) + } + if !slices.Equal(done.Skipped, []string{"worn"}) { + t.Errorf("skipped %v", done.Skipped) + } +} diff --git a/internal/core/machine/reconcile.go b/internal/core/machine/reconcile.go index 38049bec..968d94bd 100644 --- a/internal/core/machine/reconcile.go +++ b/internal/core/machine/reconcile.go @@ -51,6 +51,11 @@ type Reconciled struct { // and a record nobody could act on need different answers from the // operator. Failed []string + // Reapplied names the rule sets written to the runtime again because the + // restore brought back the records they are translated from. Separate + // from Recreated: nothing was started, a set that the runtime already + // held under the same name was overwritten with the restored rules. + Reapplied []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. @@ -168,6 +173,21 @@ func (b Binding) ReconcileMissing(ctx context.Context, records []*resource.Resou if name == "" || slices.Contains(held, name) { continue } + // And the checkpoint must describe it as UP. A record keeps its + // machine name after the machine is gone — that is how ReconcileGone + // finds what to destroy — so "has a name and the host does not hold + // it" is also, word for word, the description of a server the + // checkpoint says is stopped. The runtime leg measured it: a reset + // returned a terminated VM to running and started a container for it, + // and the verdict then named the resource it had itself changed. + // + // The word is the pack's and the comparison is shared, which is the + // field this binding already carries for PowerOn to write. + // + // TestAResetLeavesAStoppedRecordStopped fails without this. + if res.State != b.RunningState { + continue + } if b.driver == nil { // Metadata only: PowerOn would answer running without starting // anything, which is the documented degraded mode and not diff --git a/internal/core/machine/reconcile_stopped_test.go b/internal/core/machine/reconcile_stopped_test.go new file mode 100644 index 00000000..17ae2f91 --- /dev/null +++ b/internal/core/machine/reconcile_stopped_test.go @@ -0,0 +1,73 @@ +package machine + +import ( + "slices" + "testing" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// TestAResetLeavesAStoppedRecordStopped is cited by ReconcileMissing. +// +// ## The defect it closes, as it was measured +// +// A record keeps its machine name after the machine is gone — that is how +// ReconcileGone finds what to destroy — so "names a machine and the host does +// not hold it" is also, word for word, the description of a server the +// checkpoint says is stopped. The first version of this half read only those +// two facts, so a reset started a container for a terminated VM and then wrote +// `running` over the state the checkpoint held. +// +// The runtime leg said it plainly, and the shape of the complaint is worth +// keeping: the verdict reported, as differences the lab had failed to +// reconcile, the very resource the reset had itself changed. +// +// resource present and not in the checkpoint: vm i-a3f536ec state=running +// resource in the checkpoint and not present: vm i-a3f536ec state=terminated +// machine present and not in the checkpoint: feint-osc-i-a3f536ec +// +// ## Why the comparison is here and not in the packs +// +// Each cloud spells a running machine its own way, and the Binding already +// carries that word as a field because PowerOn writes it. So the word varies +// and the gesture does not, which is this repository's own test for where a +// line belongs. Written in each pack, the next pack would have been the one +// that forgot. +// +// The accepting half is asserted too: a guard that refused every record would +// pass the failing case above and silently turn the whole recreating half off. +func TestAResetLeavesAStoppedRecordStopped(t *testing.T) { + boot := func(res *resource.Resource) Boot { + return Boot{Image: "img", User: "root", Hostname: res.ID} + } + + stopped := absent("a") + stopped.State = "terminated" + running := absent("b") + + d := &startingDriver{} + done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{stopped, running}, nil, boot) + + // The refusing half: nothing was started for the stopped record, which is + // the assertion the host pays for. + if slices.Contains(d.started, "feint-acme-a") { + t.Errorf("a reset started a container for a record the checkpoint describes as stopped: %v", + d.started) + } + if stopped.State != "terminated" { + t.Errorf("the record says %q; a reset that rewrites the state it was asked to return to "+ + "then reports its own change as a difference", stopped.State) + } + if slices.Contains(done.Recreated, "feint-acme-a") { + t.Errorf("a stopped record was reported recreated: %+v", done) + } + + // The accepting half, in the same pass so neither can be satisfied alone. + if !slices.Contains(d.started, "feint-acme-b") { + t.Errorf("the running record was not started again: %v", d.started) + } + if !slices.Contains(done.Recreated, "feint-acme-b") { + t.Errorf("recreated %v", done.Recreated) + } +} diff --git a/internal/core/machine/surface.go b/internal/core/machine/surface.go index 3c09b261..d586a06f 100644 --- a/internal/core/machine/surface.go +++ b/internal/core/machine/surface.go @@ -184,6 +184,7 @@ func PackSurface() map[string]string { "GroupSync": "the firewall orchestrator, built from the pack's own translation of its groups", "GroupSync.SyncGroup": "write one group's rule set and replay it onto every wearer", "GroupSync.ApplyMachine": "write every set one resource wears and attach them in one call", + "GroupSync.Reapply": "write a restored group's rule set to the runtime again, so a reset puts the rule back on the NIC", "GroupSync.SyncReferrers": "re-expand the groups whose rules name the groups this resource wears", "GroupSync.Drop": "remove a deleted group's rule set from the runtime", "GroupSync.PlanOf": "declare the plan the scope of the groups is read from, the pack's own", diff --git a/internal/providers/exoscale/firewall.go b/internal/providers/exoscale/firewall.go index e78ecb5f..618ee834 100644 --- a/internal/providers/exoscale/firewall.go +++ b/internal/providers/exoscale/firewall.go @@ -257,3 +257,19 @@ func (p *Pack) removeFirewall(ctx context.Context, group *resource.Resource) { // internal/cli's TestEveryPackThatWiresTheFirewallSaysSo is what notices if // it does not. func (p *Pack) EnforcesFirewall() bool { return true } + +// ReconcileRules implements emulator.RuleReconciler: after a reset restores +// the control plane, this pack's groups are written to the runtime again. +// +// One line of its own, and the rest is machine.GroupSync.Reapply, shared with +// the two other packs. What varies is this: which restored record this +// provider calls a group. +func (p *Pack) ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled { + groups := make([]*resource.Resource, 0, len(records)) + for _, res := range records { + if res.Kind == kindSecurityGroup { + groups = append(groups, res) + } + } + return p.groupSync().Reapply(ctx, groups) +} diff --git a/internal/providers/outscale/firewall.go b/internal/providers/outscale/firewall.go index 37b38809..06546a3d 100644 --- a/internal/providers/outscale/firewall.go +++ b/internal/providers/outscale/firewall.go @@ -246,3 +246,19 @@ func (p *Pack) removeFirewall(ctx context.Context, group *resource.Resource) { // internal/cli's TestEveryPackThatWiresTheFirewallSaysSo is what notices if // it does not. func (p *Pack) EnforcesFirewall() bool { return true } + +// ReconcileRules implements emulator.RuleReconciler: after a reset restores +// the control plane, this pack's groups are written to the runtime again. +// +// One line of its own, and the rest is machine.GroupSync.Reapply, shared with +// the two other packs. What varies is this: which restored record this +// provider calls a group. +func (p *Pack) ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled { + groups := make([]*resource.Resource, 0, len(records)) + for _, res := range records { + if res.Kind == kindSecurityGroup { + groups = append(groups, res) + } + } + return p.groupSync().Reapply(ctx, groups) +} diff --git a/internal/providers/scaleway/firewall.go b/internal/providers/scaleway/firewall.go index bb5ebfb7..78370c5e 100644 --- a/internal/providers/scaleway/firewall.go +++ b/internal/providers/scaleway/firewall.go @@ -243,3 +243,19 @@ func (p *Pack) removeFirewall(ctx context.Context, group *resource.Resource) { // The day this pack stops handing rules over, this line is what has to change, // and TestEnforcementNamesOnlyThePacksThatWireIt is what notices if it does not. func (p *Pack) EnforcesFirewall() bool { return true } + +// ReconcileRules implements emulator.RuleReconciler: after a reset restores +// the control plane, this pack's groups are written to the runtime again. +// +// One line of its own, and the rest is machine.GroupSync.Reapply, shared with +// the two other packs. What varies is this: which restored record this +// provider calls a group. +func (p *Pack) ReconcileRules(ctx context.Context, records []*resource.Resource) machine.Reconciled { + groups := make([]*resource.Resource, 0, len(records)) + for _, res := range records { + if res.Kind == kindSecurityGroup { + groups = append(groups, res) + } + } + return p.groupSync().Reapply(ctx, groups) +} diff --git a/tools/conformance/scaleway/network.sh b/tools/conformance/scaleway/network.sh index 63ae6777..e7476009 100755 --- a/tools/conformance/scaleway/network.sh +++ b/tools/conformance/scaleway/network.sh @@ -272,6 +272,28 @@ rule_id="$(api -X POST "$ENDPOINT/instance/v1/zones/$ZONE/security_groups/$sg_id wait_until 30 reach || fail "port 80 is still refused after the rule was authorised" ok "port 80 reachable" +# ---- The audit's third P0, reproduced before being believed ----------------- +# An external audit deduced from the code that a reset could report convergence +# while a firewall rule was still missing from the runtime: `Survey` answers +# machine, network and rule-set NAMES, and a rule modified in place keeps its +# rule set's name, so nothing in the observation would see the difference. +# +# It said so as a hypothesis and had not reproduced it. Neither had anybody +# here. This measures it, with the only instrument whose verdict cannot be +# argued with: a TCP connection. +# +# The port is open at this point, so the checkpoint is taken with the rule in +# place. The suite then revokes the rule below and proves the port closes; the +# reset step after that asks whether the rule comes BACK. +fw_checkpoint=0 +if command -v "${FEINT_BIN:-./feint}" >/dev/null 2>&1; then + if "${FEINT_BIN:-./feint}" checkpoint firewall-reset --addr "${ENDPOINT#http://}" >/dev/null; then + fw_checkpoint=1 + else + skip "the checkpoint was refused; the reset step below is skipped" + fi +fi + echo "- revoking it closes it again" api -X DELETE "$ENDPOINT/instance/v1/zones/$ZONE/security_groups/$sg_id/rules/$rule_id" >/dev/null # waits on silence: the verdict below is drawn from a connection that does NOT @@ -283,6 +305,34 @@ sleep 3 if reach; then fail "port 80 is still open after the rule was revoked"; fi ok "port 80 refused again" +echo "- and a reset puts the rule back on the NIC, not only in the store" +if [ "${fw_checkpoint:-0}" = 1 ]; then + "${FEINT_BIN:-./feint}" reset firewall-reset --addr "${ENDPOINT#http://}" >/dev/null \ + || fail "the reset reported that the lab did not converge" + # The store has the rule again by construction: it was restored verbatim. + # What this asks is whether the PACKETS flow, which is the question `Survey` + # cannot answer and the one an operator has. + if wait_until 30 reach; then + ok "port 80 reachable again after the reset" + # The reset put the AUTHORISING rule back, which is exactly what it was + # asked for. Everything below measures a group that DROPS inbound, so the + # rule is revoked once more and the suite goes on from the state it + # expected. Without this the flexible-IP section reads the reset's own + # success as a firewall gap. + api -X DELETE "$ENDPOINT/instance/v1/zones/$ZONE/security_groups/$sg_id/rules/$rule_id" >/dev/null + # waits on silence: the verdict below is drawn from a connection that + # does NOT open, exactly as above the first revocation, and polling + # until the port closes would end at the first failed connection — + # which is also what a port that has not reopened yet looks like. + sleep 3 + if reach; then fail "port 80 is still open after the restored rule was revoked again"; fi + else + fail "the reset reported convergence and port 80 is still refused: the rule came back in the store and not on the NIC" + fi +else + skip "no checkpoint was taken, so the reset step measures nothing" +fi + # ---- Flexible IP ------------------------------------------------------------ # A public address a cloud hands out is routed to the machine holding it. Here it # comes from 203.0.113.0/24, which RFC 5737 reserves for documentation: it reads 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 036950db..de0c462c 100644 --- a/tools/falsify/specs/a-reset-is-not-a-restore.json +++ b/tools/falsify/specs/a-reset-is-not-a-restore.json @@ -1,86 +1,116 @@ { - "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.\n\nSince an external audit of the merged code: the verdict's object comparison is in here too. It took the digest from ListObjectsV2's ETag, which internal/s3/list.go does not emit, so the field was always empty and two different contents under one key compared equal. The mutation that reproduces it is below, and the test that bites mounts a REAL door over a REAL store — the test that let the defect through built observedObject values by hand, with digests in them, so it exercised the comparison and never the observation.", - "mutations": [ - { - "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\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 {", - "replace": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 && false {", - "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 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\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" - }, - { - "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" - }, - { - "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/" - }, - { - "label": "the object comparison goes back to the door, the bucket and the key, dropping the digest and both sizes. This IS the defect an audit found in merged code: with no content in the key, a reset reports convergence over bytes that changed, and the commit message claimed objects were compared by digest", - "file": "internal/cli/observable.go", - "find": "\t\treturn fmt.Sprintf(\"%s/%s/%s digest=%s bytes=%d listed=%d\",\n\t\t\tob.Door, ob.Bucket, ob.Key, ob.Digest, ob.Size, ob.Listed)", - "replace": "\t\treturn fmt.Sprintf(\"%s/%s/%s\",\n\t\t\tob.Door, ob.Bucket, ob.Key) + fmt.Sprint(ob.Digest, ob.Size, ob.Listed)[:0]", - "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", - "package": "./internal/cli/" - }, - { - "label": "the digest stops being computed over the bytes that arrived and becomes the empty string, which is exactly what reading an absent ETag produced. A verdict whose digest is constant compares every content equal, and the run reports a clean reset", - "file": "internal/cli/observable.go", - "find": "\treturn hex.EncodeToString(sum.Sum(nil)), size, nil", - "replace": "\treturn hex.EncodeToString(sum.Sum(nil))[:0], size, nil", - "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", - "package": "./internal/cli/" - }, - { - "label": "the two halves are taken in the order an audit caught: the bytes first, the control plane second. A failure in the second step then leaves an earlier checkpoint of that name holding its OLD store beside the NEW object references, and a later reset restores one against the other — while the comment above promises both halves or neither. The order IS the guard here, so the mutation reorders rather than neutralises", - "file": "internal/core/emulator/checkpoint.go", - "find": "\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}\n\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}", - "replace": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}\n\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}", - "test": "TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole" - } - ] + "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.\n\nSince an external audit of the merged code: the verdict's object comparison is in here too. It took the digest from ListObjectsV2's ETag, which internal/s3/list.go does not emit, so the field was always empty and two different contents under one key compared equal. The mutation that reproduces it is below, and the test that bites mounts a REAL door over a REAL store — the test that let the defect through built observedObject values by hand, with digests in them, so it exercised the comparison and never the observation.\n\nThird round, same audit: its P0-3, the firewall. The audit deduced from the code that a reset could report convergence while a rule was still missing from the runtime, and said so as a hypothesis it had not reproduced. The network suite reproduced it under --vm incus, with a TCP connection as the instrument, before a line of the fix was written: \"the reset reported convergence and port 80 is still refused: the rule came back in the store and not on the NIC\". Survey answers NAMES, and a rule set whose content changed keeps its name, so nothing in the observation can see the difference.\n\nThe first fix then over-corrected, and the same leg caught that too: writing every restored group put rule sets nobody wears back on the host, and the verdict reported them as differences the reset itself had made. Both directions are mutated below, because a guard that refuses everything passes the attack and breaks the product.\n\nThe fourth mutation is a defect the leg surfaced on the way, older than the audit: a record keeps its machine name after the machine is gone, so \"names a machine the host does not hold\" also describes a server the checkpoint says is stopped. A reset was starting containers for terminated VMs and then writing `running` over the state it had been asked to return to.", + "mutations": [ + { + "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\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 {", + "replace": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 && false {", + "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 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\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" + }, + { + "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" + }, + { + "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/" + }, + { + "label": "the object comparison goes back to the door, the bucket and the key, dropping the digest and both sizes. This IS the defect an audit found in merged code: with no content in the key, a reset reports convergence over bytes that changed, and the commit message claimed objects were compared by digest", + "file": "internal/cli/observable.go", + "find": "\t\treturn fmt.Sprintf(\"%s/%s/%s digest=%s bytes=%d listed=%d\",\n\t\t\tob.Door, ob.Bucket, ob.Key, ob.Digest, ob.Size, ob.Listed)", + "replace": "\t\treturn fmt.Sprintf(\"%s/%s/%s\",\n\t\t\tob.Door, ob.Bucket, ob.Key) + fmt.Sprint(ob.Digest, ob.Size, ob.Listed)[:0]", + "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", + "package": "./internal/cli/" + }, + { + "label": "the digest stops being computed over the bytes that arrived and becomes the empty string, which is exactly what reading an absent ETag produced. A verdict whose digest is constant compares every content equal, and the run reports a clean reset", + "file": "internal/cli/observable.go", + "find": "\treturn hex.EncodeToString(sum.Sum(nil)), size, nil", + "replace": "\treturn hex.EncodeToString(sum.Sum(nil))[:0], size, nil", + "test": "TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge", + "package": "./internal/cli/" + }, + { + "label": "the two halves are taken in the order an audit caught: the bytes first, the control plane second. A failure in the second step then leaves an earlier checkpoint of that name holding its OLD store beside the NEW object references, and a later reset restores one against the other — while the comment above promises both halves or neither. The order IS the guard here, so the mutation reorders rather than neutralises", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}\n\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}", + "replace": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {\n\t\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\t\"error\": \"the bytes could not be checkpointed: \" + err.Error(),\n\t\t\t})\n\t\t\treturn\n\t\t}\n\t}\n\tvar state bytes.Buffer\n\tif err := s.env.Store.Snapshot(&state); err != nil {\n\t\twriteJSON(w, http.StatusInternalServerError, map[string]any{\n\t\t\t\"error\": \"the control plane could not be checkpointed: \" + err.Error(),\n\t\t})\n\t\treturn\n\t}", + "test": "TestAFailedControlPlaneHalfLeavesTheEarlierBytesWhole" + }, + { + "label": "the firewall half stops being called, which is the audit's third P0 exactly: the store gets its rule back, the runtime keeps enforcing the one from after the checkpoint, and the verdict sees no difference because Survey answers names", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif rules, ok := pack.(RuleReconciler); ok && len(records) != 0 {", + "replace": "\t\tif rules, ok := pack.(RuleReconciler); ok && len(records) != 0 && false {", + "test": "TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore" + }, + { + "label": "the pack is only offered its records when something could be surveyed, which is the guard as it stood before the firewall half existed. Rule sets live on the runtime whether or not any machine wears one, so this skips the whole third half on a host nobody can survey", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif len(mine) == 0 && len(records) == 0 {", + "replace": "\t\tif len(mine) == 0 && (!surveyed || len(records) == 0) {", + "test": "TestAResetPutsTheRuleBackOnTheRuntimeAndNotOnlyInTheStore" + }, + { + "label": "the wearer filter goes, and a reset furnishes the host with rule sets the checkpoint never held. The runtime leg reported them as differences the reset had itself created, which is the over-correction half of the same fix", + "package": "./internal/core/machine/", + "file": "internal/core/machine/groupsync.go", + "find": "\t\tif len(s.Wearers(group)) == 0 {", + "replace": "\t\tif len(s.Wearers(group)) == 0 && false {", + "test": "TestReapplyWritesTheWornSetsAndOnlyThose" + }, + { + "label": "a reset starts again the machine of a record the checkpoint describes as stopped, then writes `running` over the state it was asked to return to. The verdict that follows names, as a difference it failed to reconcile, the very resource it changed", + "package": "./internal/core/machine/", + "file": "internal/core/machine/reconcile.go", + "find": "\t\tif res.State != b.RunningState {", + "replace": "\t\tif res.State != b.RunningState && false {", + "test": "TestAResetLeavesAStoppedRecordStopped" + } + ] } From 5c769896fbc4aa1af26498c82a89630b4bb4ef9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 16:09:41 +0100 Subject: [PATCH 3/5] fix(release): the tracker agrees with the tags, and a gate keeps it that way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three documents and the issue tracker each claimed something the tags disproved. The tracker was corrected by hand; this commit is about the part that stops it happening again, plus the two sentences that were simply false. ## What was measured v0.14.0 and v0.15.0 are published tags. Both carried an OPEN milestone titled after them, holding 19 and 10 open issues, and neither described what its release had shipped: 0.14.0 shipped drift triage and block import, 0.15.0 shipped `feint run` and the S3 data plane. It was not the practice slipping once. Every minor from 0.9.0 to 0.13.0 has a closed, empty milestone — 0.9.0 with 63, 0.11.0 with 79, 0.12.0 with 45 — so the convention was real and these two broke it. What let it through is that `CONTRIBUTING.md` still said milestones were "the six waves of the roadmap's sequence", stale since 0.9.0: the sentence that would have caught it was itself out of date. ## The gate, and why it is narrower than the first draft `tools/release/milestone-gate.sh` asks two questions before a tag: does an open milestone claim a version git has ALREADY tagged, and does the version being tagged hold open issues under its own name. The first draft asked "does any open milestone carry a version number", which is wrong in a way worth recording: it would have refused every tag from the day a `1.0.0` milestone was created. A milestone for work still AHEAD is legitimate, and is the one case where the tag should wait for the milestone rather than the milestone being emptied after the tag. A guard that refuses everything passes its own attack test and breaks the product, so the falsify spec mutates both directions and the accepting half is its own test. Three exit codes, for the reason preflight.sh already carries at length: a failed measurement and an absent result are different answers, and only one of them is about the repository. **The test harness lied to me twice, and the comments say so rather than claiming otherwise.** The `gh` stub first returned the raw fixture instead of applying the `--jq` expression, so the gate read JSON where it expected lines, matched nothing, and reported the repository coherent on the very payload that asked for the gate — green for the only reason that makes a green worthless. And the accepting fixture had the tagged version's milestone CLOSED, which the gate skips before reaching the exception, so the mutation deleting that exception reported STILL GREEN. Both times the fix was the test. The test also builds its own git repository with the tags the fixtures name. A shallow CI checkout has no tags, and reading the real repository's would have turned this into a measurement of the checkout depth. ## Two sentences that were false **`README.md`: "Object storage is out of scope for now."** On the day the release whose headline is object storage. It sits after the generated `coverage:end` marker, so `feint docs --check` could not reach it. It now says what shipped, why it took a year, and what is still not certified (#856, #858). **The generated coverage headline named the wrong population.** It read "Of the 548 operations upstream declares" while `coverage/scaleway-baseline.json` scopes the Scaleway scan to 11 of the 56 products in that SDK — so 548 is the surface of a third of the provider, printed as the provider's surface, with the scoping stated only at `docs/limits.md:3316`. A reader with `scaleway_rdb_instance` could not learn their stack was outside the scan, and `docs/adoption.md` says what then happens: the client reaches the real endpoint with whatever credentials the shell holds. The headline now names the product count, uniformly for the three, so nothing has to decide which provider deserves a caveat. `docs/roadmap.md` also stopped saying "The order 0.11.0 is being worked in" in the present tense, three releases later. ## The tracker, corrected by hand in the same pass Eight milestones lost their version prefix; nothing else about them changed. Nine issues left a milestone that contradicted another document — #191, #192, #193, #194, #480 and #482 were in three places at once while `docs/roadmap.md:818` says that work carries no milestone at all; #763 planned a Kubernetes API that `docs/limits.md:692` refuses under #283; #695 and #679 are a defect and harness work rather than contract text. #755 was retitled off a number that moved without it. A `1.0.0` milestone now holds #847, #848, #851, #852, #854, #405 and the three adoption P0s. Its description is a view: #847 holds the graph, #854 the acceptance, `docs/roadmap.md` the ordering. /falsify: 4 mutations on a-milestone-cannot-claim-a-version.json, all biting. The coverage headline has none, deliberately, and the spec says why: the clause is prose inside a format string, every mutation removing it removes names the harness requires to survive, and contriving one would measure the contrivance. mise run prepush green. The gate answers exit 0 on the live repository, which is the measurement that the hand corrections landed. Assisted-by: Claude Code (claude-opus-5) --- CONTRIBUTING.md | 22 +- README.md | 39 +-- docs/roadmap.md | 17 +- internal/cli/coverage_population_test.go | 51 ++++ internal/cli/docs.go | 18 +- tools/ci/preflight_milestones_test.go | 238 ++++++++++++++++++ .../a-milestone-cannot-claim-a-version.json | 34 +++ tools/release/milestone-gate.sh | 120 +++++++++ tools/release/preflight.sh | 20 ++ 9 files changed, 536 insertions(+), 23 deletions(-) create mode 100644 internal/cli/coverage_population_test.go create mode 100644 tools/ci/preflight_milestones_test.go create mode 100644 tools/falsify/specs/a-milestone-cannot-claim-a-version.json create mode 100755 tools/release/milestone-gate.sh diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 087b639e..ef66398b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -312,8 +312,26 @@ the subject: `core`, `machine`, `conformance`. State: `blocked`, with the blocker named in the body. There is no `wontfix` label: the project's word for out of scope is a decline in the pack, with a reason. -**Milestones** are the six waves of the roadmap's sequence — an order, not a -schedule, so they carry no due dates. +**Milestones** are themes, not versions, and they carry no due dates: an order, +never a schedule. That sentence used to say "the six waves of the roadmap's +sequence", which stopped being true at 0.9.0 and was still there two years of +releases later — which is how two milestones came to be titled after published +tags, `0.14.0` and `0.15.0`, while holding 19 and 10 open issues between them. + +So the rule is now checkable rather than described, and it is narrower than it +first looks: **an open milestone never claims a version that is already +tagged.** A milestone for work still ahead may carry its number — `1.0.0` is +one, and it is the case where the tag should wait for the milestone rather than +the reverse. What is refused is a statement about the past that the tags +disprove. The second half is its mirror: the version being tagged must hold no +open issue under its own name. + +`tools/release/milestone-gate.sh` asks both before a tag, and says so when it +could not measure instead of refusing for a reason that is not about the +repository. `tools/ci`'s `TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone` +is driven by the payload that asked for it, and +`TestTheMilestoneGateAcceptsTheShippedShape` keeps the accepting half honest: a +gate that refused every tag would pass the first test and teach `--no-verify`. `tools/issues/setup.sh` creates the labels and milestones (`--dry-run` shows what it would do); `tools/issues/batches/` holds the eighteen batch diff --git a/README.md b/README.md index 17b7a64f..b934db68 100644 --- a/README.md +++ b/README.md @@ -943,8 +943,8 @@ what this emulator serves, declines on purpose, or has not triaged yet. #### Scaleway -204 routes mounted. Of the 548 operations upstream declares: 37% served, -62% declined on purpose, 0% untriaged. +204 routes mounted. Of the 548 operations upstream declares in the 11 product(s) +this scan covers: 37% served, 62% declined on purpose, 0% untriaged. | Group | Served | Declined | Untriaged | Upstream | |---|--:|--:|--:|--:| @@ -961,8 +961,8 @@ what this emulator serves, declines on purpose, or has not triaged yet. #### Exoscale -105 routes mounted. Of the 374 operations upstream declares: 28% served, -71% declined on purpose, 0% untriaged. +105 routes mounted. Of the 374 operations upstream declares in the 1 product(s) +this scan covers: 28% served, 71% declined on purpose, 0% untriaged. | Group | Served | Declined | Untriaged | Upstream | |---|--:|--:|--:|--:| @@ -979,8 +979,8 @@ what this emulator serves, declines on purpose, or has not triaged yet. #### Outscale -100 routes mounted. Of the 263 operations upstream declares: 38% served, -61% declined on purpose, 0% untriaged. +100 routes mounted. Of the 263 operations upstream declares in the 2 product(s) +this scan covers: 38% served, 61% declined on purpose, 0% untriaged. | Group | Served | Declined | Untriaged | Upstream | |---|--:|--:|--:|--:| @@ -1024,14 +1024,25 @@ conformance run. The sequence and the state of every batch live in measurements that cut them (2026-07-30) are archived under [docs/history/](docs/history/), where a snapshot cannot read as the present. -**Object storage is out of scope for now.** The Scaleway Terraform provider -hardcodes `https://s3..scw.cloud`, so it needs DNS interception and TLS -rather than an endpoint setting — and that is not theory: a surveyed stack's -`CreateBucket` went to the real endpoint and got a 403 while everything else it -did ran here (#262). Exoscale's `sos-endpoint` is served empty for the -same reason: an invented address is a promise the emulator cannot keep. See -[docs/limits.md](docs/limits.md), which states every limit that was measured -rather than assumed. +**Object storage is served since 0.15.0, and it is opt-in.** The reason it was +out of scope for a year is worth keeping, because it is what the feature had to +answer: the Scaleway Terraform provider hardcodes +`https://s3..scw.cloud`, so an endpoint setting cannot redirect it — a +surveyed stack's `CreateBucket` went to the real endpoint and got a 403 while +everything else it did ran here (#262). + +What changed is that `feint run` terminates that host itself, with an authority +it generates for the run alone, and **refuses any host it was not told about +rather than tunnelling it**. On the published provider 2.82.0 that path reads +139,264 identical bytes back out of the local byte store by sha256, with an +empty second plan. It is opt-in because interception is a side effect on the +machine that runs it, and because a client carrying real credentials is refused +unless you say otherwise. + +Normal Exoscale bucket creation is still not certified (#858), and the +multipart states are #856. [docs/confidence.md](docs/confidence.md) carries the +row with its client versions, and [docs/limits.md](docs/limits.md) states every +limit that was measured rather than assumed. --- diff --git a/docs/roadmap.md b/docs/roadmap.md index 287faa86..3f129137 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -9,11 +9,18 @@ What is *measured* rather than planned lives in [limits.md](limits.md), and how the pieces fit together in [architecture.md](architecture.md). -## The order 0.11.0 is being worked in - -Frozen 2026-08-22. Thirteen issues remain, and they are not independent: two of -them move the numbers the other eleven quote. The order below is the dependency, -not a preference. +## The order 0.11.0 was worked in + +Frozen 2026-08-22, and kept in the past tense deliberately: 0.11.0 shipped, and +so did 0.12.0 to 0.15.0 after it. This heading read "is being worked in" for +three releases, which is the same failure as a milestone titled after a +published tag — a sentence nobody could notice was stale because nothing +measured it. **The order being worked now is the 1.0 one, at the end of this +page.** + +Thirteen issues remained at the freeze, and they were not independent: two of +them moved the numbers the other eleven quoted. The order below is the +dependency, not a preference. **1 — Make the measurement trustworthy. Blocking.** diff --git a/internal/cli/coverage_population_test.go b/internal/cli/coverage_population_test.go new file mode 100644 index 00000000..25290994 --- /dev/null +++ b/internal/cli/coverage_population_test.go @@ -0,0 +1,51 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/stephrobert/feint/internal/drift" +) + +// TestTheCoverageHeadlineNamesItsPopulation is cited by renderCoverage. +// +// The generated headline read "Of the 548 operations upstream declares", and +// the Scaleway scan covers 11 of the 56 products in that SDK +// (`coverage/scaleway-baseline.json`). So 548 is the surface of a third of the +// provider, printed on the README's front page as the provider's surface, with +// the scoping stated only at `docs/limits.md:3316`. +// +// That is this repository's cardinal failure — a verdict that does not name its +// population — and it had been shipping in the one document most readers see +// first. A reader with `scaleway_rdb_instance` could not learn from it that +// their stack is outside the scan entirely, and `docs/adoption.md` says what +// then happens: the client reaches the real endpoint with whatever credentials +// the shell holds. +// +// The assertion is on the sentence rather than on a number, because the numbers +// move with every upstream scan and the population clause must not. +func TestTheCoverageHeadlineNamesItsPopulation(t *testing.T) { + reports := []drift.CoverageFile{{ + Provider: "scaleway", + Total: 548, + Implemented: 204, + Declined: 344, + Products: []drift.ProductView{ + {Product: "instance"}, {Product: "vpc"}, {Product: "lb"}, + }, + }} + + out := renderCoverage(reports, map[string]map[string]int{ + "scaleway": {"instance": 120, "vpc": 50, "lb": 34}, + }) + + if !strings.Contains(out, "in the 3 product(s)\nthis scan covers") { + t.Errorf("the headline does not name the population it counted:\n%s", out) + } + // And the bare form must be gone, not merely supplemented: a sentence that + // still says "upstream declares: 37% served" one line above the scoped one + // leaves the misreading available. + if strings.Contains(out, "operations upstream declares:") { + t.Errorf("the unscoped headline is still printed:\n%s", out) + } +} diff --git a/internal/cli/docs.go b/internal/cli/docs.go index 540e8f42..ba7db46d 100644 --- a/internal/cli/docs.go +++ b/internal/cli/docs.go @@ -788,8 +788,22 @@ func renderCoverage(reports []drift.CoverageFile, routes map[string]map[string]i // most of the gap is operations somebody decided to refuse. The // distinction between a refusal and an absence is the one these tables // exist to make, so the headline makes it before the rows do. - fmt.Fprintf(&b, "%d routes mounted. Of the %d operations upstream declares: %s served,\n%s declined on purpose, %s untriaged.\n\n", - routeTotal(byProduct), rep.Total, + // The product count is part of the sentence, and that is not padding. + // It read "Of the 548 operations upstream declares" while the Scaleway + // scan covers 11 of the 56 products in that SDK: a reader took 548 for + // Scaleway's whole API, and the scoping was stated only at + // docs/limits.md:3316. A verdict that does not name its population is + // the one failure this repository exists to refuse, and it was printed + // on the front page. + // + // Uniform across the three on purpose: Outscale and Exoscale are + // scanned whole, so the count simply says so, and nothing has to + // decide which provider deserves a caveat. + // + // internal/cli's TestTheCoverageHeadlineNamesItsPopulation fails + // without this. + fmt.Fprintf(&b, "%d routes mounted. Of the %d operations upstream declares in the %d product(s)\nthis scan covers: %s served, %s declined on purpose, %s untriaged.\n\n", + routeTotal(byProduct), rep.Total, len(rep.Products), percent(rep.Implemented, rep.Total), percent(rep.Declined, rep.Total), percent(rep.Unknown, rep.Total)) named, rest, foldedGroups := foldSmallGroups(groupRows(rep), rep.Total) b.WriteString("| Group | Served | Declined | Untriaged | Upstream |\n") diff --git a/tools/ci/preflight_milestones_test.go b/tools/ci/preflight_milestones_test.go new file mode 100644 index 00000000..a7197ae5 --- /dev/null +++ b/tools/ci/preflight_milestones_test.go @@ -0,0 +1,238 @@ +package ci + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// milestoneGhStub answers one call — the milestones query — from a fixture, and +// refuses anything else loudly. A stub that answered every call would let a +// change to the query pass unnoticed, which is the failure the gate's own +// comment describes happening twice to preflight.sh's CI query. +// +// It APPLIES the --jq expression the gate passes, with the real jq, rather than +// returning the raw fixture. The first version did not, and it is worth saying +// why: the gate then read a JSON array where it expected tab-separated lines, +// matched nothing, and reported the repository coherent on the very payload +// that asked for the gate. The test went green for the only reason that makes a +// green worthless — the instrument, not the subject. Emulating gh's own +// transform is what makes the expression part of what is measured. +const milestoneGhStub = `#!/usr/bin/env bash +set -eu +case "$1" in + api) + case "$*" in + *"/milestones"*) + expr='.' + prev="" + for a in "$@"; do + if [ "${prev}" = "--jq" ]; then expr="${a}"; fi + prev="${a}" + done + jq -r "${expr}" "${GH_STUB_MILESTONES}" + ;; + *) + echo "unexpected gh api call: $*" >&2 + exit 64 + ;; + esac + ;; + *) + echo "unexpected gh call: $*" >&2 + exit 64 + ;; +esac +` + +// runMilestoneGate drives tools/release/milestone-gate.sh against one fixture +// of the repository's milestones and returns its exit code and its output. +// +// It builds a throwaway git repository carrying the tags the fixtures talk +// about, and runs the gate there. Reading the real repository's tags would +// have been shorter and wrong twice over: a shallow CI checkout has no tags, +// so the red fixture would stop producing findings and this test would start +// measuring the checkout depth; and the day v0.16.0 is tagged for real, the +// accepting fixture would silently become a red one. +func runMilestoneGate(t *testing.T, version, milestones string) (int, string) { + t.Helper() + + dir := t.TempDir() + stub := filepath.Join(dir, "gh") + if err := os.WriteFile(stub, []byte(milestoneGhStub), 0o755); err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "milestones.json") + if err := os.WriteFile(fixture, []byte(milestones), 0o644); err != nil { + t.Fatal(err) + } + + script, err := filepath.Abs(filepath.Join("..", "release", "milestone-gate.sh")) + if err != nil { + t.Fatal(err) + } + + repo := filepath.Join(dir, "repo") + if err := os.Mkdir(repo, 0o755); err != nil { + t.Fatal(err) + } + // Every GIT_* and PRE_COMMIT_* variable is dropped rather than inherited. + // This test passed standalone and failed under the pre-push hook, which + // exports GIT_DIR, GIT_INDEX_FILE and friends: the throwaway repository + // then pointed at the real one, and `git commit` ran the project's hooks. + // An inherited environment is the harness measuring whoever ran it. + clean := make([]string, 0, len(os.Environ())) + for _, kv := range os.Environ() { + if strings.HasPrefix(kv, "GIT_") || strings.HasPrefix(kv, "PRE_COMMIT") { + continue + } + clean = append(clean, kv) + } + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = repo + cmd.Env = append(append([]string{}, clean...), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.invalid", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.invalid") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + // An empty template directory, so no hook of the operator's own is copied + // into a repository this test is about to commit in. + git("-c", "init.templateDir=", "init", "--quiet") + git("commit", "--allow-empty", "--quiet", "-m", "root") + // The published history these fixtures are about. v0.16.0 is deliberately + // absent: it is the version being tagged. + for _, tag := range []string{"v0.12.0", "v0.13.0", "v0.14.0", "v0.15.0"} { + git("tag", tag) + } + + cmd := exec.Command("bash", script, version) + cmd.Dir = repo + cmd.Env = append(append([]string{}, clean...), + "PATH="+dir+string(os.PathListSeparator)+os.Getenv("PATH"), + "GH_STUB_MILESTONES="+fixture, + ) + out, err := cmd.CombinedOutput() + code := 0 + var exit *exec.ExitError + if err != nil { + if ok := asExitError(err, &exit); !ok { + t.Fatalf("running the gate: %v\n%s", err, out) + } + code = exit.ExitCode() + } + return code, string(out) +} + +// The payload the repository actually answered on 2026-11-11, trimmed to the +// rows that matter. This is the red fixture: it is the measurement that asked +// for the gate, so the gate is tested against the state it was written for +// rather than against a shape invented here. +const milestonesOn20261111 = `[ + {"state":"open","open_issues":19,"title":"0.14.0 — The topology outranks its history"}, + {"state":"open","open_issues":10,"title":"0.15.0 — The cloud lab lives in the repository"}, + {"state":"open","open_issues":9,"title":"0.16.0 — Make every claim inspectable"}, + {"state":"closed","open_issues":0,"title":"0.13.0 — Failure is part of the API"}, + {"state":"closed","open_issues":0,"title":"0.12.0 — One contract, same working cloud"}, + {"state":"open","open_issues":5,"title":"1.0 — the local cloud lab"} +]` + +// The shape a coherent repository has at the moment preflight runs: themes with +// no number, and the tagged version's own milestone emptied but **still open**, +// because it is closed after the tag and this check happens before it. +// +// That "still open" is load-bearing, and the first version of this fixture had +// it closed. The gate skips a closed milestone before it reaches the exception, +// so the exception was never exercised: the mutation that deletes it reported +// STILL GREEN. The harness caught the test, which is the only reason this +// comment is not a claim. +const milestonesShipped = `[ + {"state":"open","open_issues":14,"title":"Runtime Contract v1 — the topology outranks its history"}, + {"state":"open","open_issues":0,"title":"0.16.0 — Make every claim inspectable"}, + {"state":"closed","open_issues":0,"title":"0.13.0 — Failure is part of the API"} +]` + +// TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone is cited by +// milestone-gate.sh. +// +// Both findings are asserted, not just the count: the gate must name WHICH +// milestone and how many issues it holds, because a gate that refuses a tag +// without saying what to fix teaches `--no-verify`, which this repository has +// written down as worse than no gate at all. +func TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone(t *testing.T) { + code, out := runMilestoneGate(t, "v0.16.0", milestonesOn20261111) + if code != 1 { + t.Fatalf("the gate exited %d on the state that asked for it; 1 is incoherent\n%s", code, out) + } + for _, want := range []string{ + `"0.14.0 — The topology outranks its history" claims v0.14.0, which is already tagged, and holds 19`, + `"0.15.0 — The cloud lab lives in the repository" claims v0.15.0, which is already tagged, and holds 10`, + } { + if !strings.Contains(out, want) { + t.Errorf("the refusal does not name %q:\n%s", want, out) + } + } + // The version being tagged has its own open milestone here, and that is a + // second, distinct finding: a release cannot ship with open work filed + // under its own number. + if !strings.Contains(out, "named after the version being tagged and holds 9") { + t.Errorf("the gate did not notice open work under the version being tagged:\n%s", out) + } + // And a milestone for work still AHEAD is not a finding. This is the + // narrowing that makes the gate usable: a rule reading "no version number + // on an open milestone" would have refused every tag from the day the 1.0 + // milestone was created, which is the gate-blocks-the-release shape + // preflight.sh already carries a scar from. + if strings.Contains(out, "1.0 — the local cloud lab") { + t.Errorf("the gate refused a milestone for work that has not shipped, so it would block every tag:\n%s", out) + } + // A closed milestone carrying a number is the CONVENTION, never a finding. + if strings.Contains(out, "0.13.0") || strings.Contains(out, "0.12.0") { + t.Errorf("the gate complained about a closed milestone, which is how a shipped release is recorded:\n%s", out) + } +} + +// TestTheMilestoneGateAcceptsTheShippedShape is the accepting half, in its own +// test so neither can be satisfied alone. +// +// A gate that refused everything would pass the test above and refuse every +// tag — the shape this repository names as the one that teaches `--no-verify`. +// It also pins the narrow exception: the tagged version's milestone may carry +// its number once it is closed and empty. +func TestTheMilestoneGateAcceptsTheShippedShape(t *testing.T) { + code, out := runMilestoneGate(t, "v0.16.0", milestonesShipped) + if code != 0 { + t.Fatalf("the gate exited %d on the shape 0.9.0 to 0.13.0 all had; it would refuse every tag\n%s", + code, out) + } + if !strings.Contains(out, "no open milestone claims a version that has already shipped") { + t.Errorf("the gate passed without saying what it checked:\n%s", out) + } +} + +// TestTheMilestoneGateSaysWhenItCouldNotMeasure keeps the third exit code +// honest. An absent `gh` and a repository in a bad state are different answers, +// and exit 2 is what lets preflight.sh print "not checked" instead of refusing +// a tag for a reason that is not about the repository. +func TestTheMilestoneGateSaysWhenItCouldNotMeasure(t *testing.T) { + script, err := filepath.Abs(filepath.Join("..", "release", "milestone-gate.sh")) + if err != nil { + t.Fatal(err) + } + // An empty PATH removes gh without removing bash, which is run by name. + cmd := exec.Command("bash", script, "v0.16.0") + cmd.Env = []string{"PATH=" + t.TempDir()} + out, err := cmd.CombinedOutput() + var exit *exec.ExitError + if err == nil || !asExitError(err, &exit) || exit.ExitCode() != 2 { + t.Fatalf("a gate that cannot measure must exit 2, got err=%v\n%s", err, out) + } + if !strings.Contains(string(out), "gh is not installed") { + t.Errorf("the gate did not say why it could not measure:\n%s", out) + } +} diff --git a/tools/falsify/specs/a-milestone-cannot-claim-a-version.json b/tools/falsify/specs/a-milestone-cannot-claim-a-version.json new file mode 100644 index 00000000..a15c3ecb --- /dev/null +++ b/tools/falsify/specs/a-milestone-cannot-claim-a-version.json @@ -0,0 +1,34 @@ +{ + "package": "./tools/ci/", + "note": "The subject is the tracker agreeing with the tags. On 2026-11-11, with v0.14.0 and v0.15.0 both published, two OPEN milestones were titled after them and held 19 and 10 open issues; neither described what its release had shipped — 0.14.0 shipped drift triage and block import, 0.15.0 shipped `feint run` and the S3 data plane. Every minor from 0.9.0 to 0.13.0 has a closed, empty milestone, so the convention was real and these two broke it. `CONTRIBUTING.md` still said milestones were \"the six waves of the roadmap's sequence\", stale since 0.9.0, which is why nobody noticed: the sentence that would have caught it was itself out of date.\n\nThe titles were corrected by hand the same day. That fixes the state and not the cause, so these mutations are about the GATE: each is a shape that would look reasonable in review and would let the next release recreate it.\n\nThe third is the one worth reading twice, and it is the over-correction rather than the defect. The rule was first written as \"no version number on an open milestone\", which would have refused every tag from the day a 1.0 milestone was created — a milestone for work still AHEAD is legitimate, and is the one case where the tag should wait for the milestone rather than the reverse. A guard that refuses everything passes the attack and breaks the product, so both directions are mutated.\n\nThe coverage headline fixed in the same commit — it read \"Of the 548 operations upstream declares\" while the Scaleway scan covers 11 of that SDK's 56 products — has NO mutation here, deliberately. The clause is prose inside a format string, and every mutation that removes it also removes names this harness requires to survive; contriving one that satisfies the rule would measure the contrivance. What holds it is internal/cli's TestTheCoverageHeadlineNamesItsPopulation, which asserts the scoped phrase is present AND the unscoped one is gone, plus `feint docs --check`, which refuses a README the generator would not produce.", + "mutations": [ + { + "label": "an open milestone claiming an already-published version stops being a finding, which restores the state of the repository on the day this gate was written: two releases out, two milestones claiming them, 29 open issues between them", + "file": "tools/release/milestone-gate.sh", + "find": " if [ -n \"$claimed\" ] && git rev-parse \"v$claimed\" >/dev/null 2>&1; then", + "replace": " if [ -n \"$claimed\" ] && git rev-parse \"v$claimed\" >/dev/null 2>&1 && false; then", + "test": "TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone" + }, + { + "label": "open work under the version being tagged stops being a finding, so a release ships while issues filed under its own number are still open. The narrower half of the same defect, and the one a renaming cannot fix", + "file": "tools/release/milestone-gate.sh", + "find": " if printf '%s' \"$title\" | grep -Fq \"$NUMBER\" && [ \"$open\" != 0 ]; then", + "replace": " if printf '%s' \"$title\" | grep -Fq \"$NUMBER\" && [ \"$open\" = 0 ]; then", + "test": "TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone" + }, + { + "label": "the tag check stops mattering, so ANY version-shaped title is refused whether or not it has shipped. This is the first draft of the rule, and it would have refused every tag from the day the 1.0 milestone was created: a gate that blocks the release it exists to authorise, which preflight.sh already carries a scar from", + "file": "tools/release/milestone-gate.sh", + "find": " if [ -n \"$claimed\" ] && git rev-parse \"v$claimed\" >/dev/null 2>&1; then", + "replace": " if [ -n \"$claimed\" ] && (git rev-parse \"v$claimed\" >/dev/null 2>&1 || true); then", + "test": "TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone" + }, + { + "label": "a gate that cannot measure reports the repository incoherent instead of saying so, so an absent `gh` refuses a tag for a reason that is not about the repository. preflight.sh paid for this distinction twice on its CI query, and the third exit code is the lesson", + "file": "tools/release/milestone-gate.sh", + "find": "if ! command -v gh >/dev/null 2>&1; then\n echo \"gh is not installed, so no milestone was read\"\n exit 2\nfi", + "replace": "if ! command -v gh >/dev/null 2>&1; then\n echo \"gh is not installed, so no milestone was read\"\n exit 1\nfi", + "test": "TestTheMilestoneGateSaysWhenItCouldNotMeasure" + } + ] +} diff --git a/tools/release/milestone-gate.sh b/tools/release/milestone-gate.sh new file mode 100755 index 00000000..e9daad68 --- /dev/null +++ b/tools/release/milestone-gate.sh @@ -0,0 +1,120 @@ +#!/usr/bin/env bash +# What shipped is measured by the tags, so no open milestone may claim a version. +# +# ## The measurement that asked for this +# +# On 2026-11-11, with v0.14.0 and v0.15.0 both tagged and published, the +# repository carried two OPEN milestones titled `0.14.0 — The topology outranks +# its history` (19 open issues) and `0.15.0 — The cloud lab lives in the +# repository` (10 open). Neither described what its release had shipped: 0.14.0 +# shipped drift triage and block import, 0.15.0 shipped `feint run` and the S3 +# data plane. A reader of the tracker could only conclude that two published +# releases were unfinished. +# +# It was not the practice slipping once. Every minor from 0.9.0 to 0.13.0 has a +# CLOSED milestone with zero open issues — 0.9.0 with 63, 0.11.0 with 79, +# 0.12.0 with 45 — so the convention was real and these two broke it. And +# `CONTRIBUTING.md` still said "Milestones are the six waves of the roadmap's +# sequence", stale since 0.9.0, which is how nobody noticed: the sentence that +# would have caught it was itself out of date. +# +# ## Why a gate and not a tidy-up +# +# The titles were corrected by hand the same day. That fixes the state and not +# the cause: the next release that ships by triage rather than by theme +# recreates it, and the only person who would notice is the one tagging. So the +# tag is what asks the question. +# +# The rule is narrower than "no version number on an open milestone", and the +# narrowing was not a softening. A milestone for work still AHEAD is +# legitimate — `1.0.0 — the local cloud lab` is one, and it is the case where +# the tag should wait for the milestone rather than the reverse. What is +# refused is a milestone claiming a version git has already tagged, because +# that is a statement about the past that the tags disprove. +# +# Both halves are therefore asked: +# +# 1. an OPEN milestone whose version is already tagged: it describes a +# release that shipped without it; +# 2. the version being tagged right now holding open issues, whatever its +# title's shape: the release would ship with work filed under its own +# number still open. +# +# ## Exit codes, and why three +# +# 0 coherent, 1 incoherent, 2 could not measure. A failed measurement and an +# absent result are different answers and only one of them is about the +# repository — the lesson preflight.sh paid for twice on its CI query, written +# at length there. +# +# tools/ci's TestTheMilestoneGateRefusesAVersionOnAnOpenMilestone and +# TestTheMilestoneGateAcceptsTheShippedShape fail without this. +set -u + +VERSION="${1:-}" +if [ -z "$VERSION" ]; then + echo "usage: milestone-gate.sh vX.Y.Z" >&2 + exit 2 +fi +# The tag carries a leading v and a milestone title does not. +NUMBER="${VERSION#v}" + +if ! command -v gh >/dev/null 2>&1; then + echo "gh is not installed, so no milestone was read" + exit 2 +fi + +# state=all, because the milestone of the version being tagged may legitimately +# be closed already, and a query restricted to open ones could not tell that +# from absent. +if ! milestones="$(gh api 'repos/{owner}/{repo}/milestones?state=all&per_page=100' \ + --paginate --jq '.[] | "\(.state)\t\(.open_issues)\t\(.title)"' 2>&1)"; then + printf 'could not read the milestones: %s\n' "$milestones" >&2 + exit 2 +fi + +findings=0 + +# A version number at the START of an open milestone's title. Anchored, because +# a theme is free to mention a version in passing ("Runtime Contract v1"), and +# only the leading position is the claim this refuses. +while IFS=$'\t' read -r state open title; do + [ -n "${state:-}" ] || continue + case "$state" in open) ;; *) continue ;; esac + + # The subject is a milestone claiming a version that ALREADY SHIPPED, which + # is what was measured: v0.14.0 and v0.15.0 were published tags while + # milestones titled after them stayed open. + # + # Not "any version number", and the difference matters. A milestone for work + # still ahead — `1.0.0 — the local cloud lab` — is legitimate and is the one + # case where the tag should wait for the milestone rather than the reverse. + # A rule written as "no number at all" would have refused every tag from the + # day that milestone was created, which is the gate-that-blocks-the-release + # shape preflight.sh already carries a scar from. + # + # So the question is answered by git, offline: does a tag for this title's + # version exist? + claimed="$(printf '%s' "$title" | grep -Eo '^[0-9]+\.[0-9]+(\.[0-9]+)?' || true)" + if [ -n "$claimed" ] && git rev-parse "v$claimed" >/dev/null 2>&1; then + printf 'the open milestone "%s" claims v%s, which is already tagged, and holds %s open issue(s)\n' \ + "$title" "$claimed" "$open" + findings=$((findings + 1)) + fi + + # And the version being tagged must not have open work under its own name, + # whatever the title's shape. + if printf '%s' "$title" | grep -Fq "$NUMBER" && [ "$open" != 0 ]; then + printf 'the milestone "%s" is named after the version being tagged and holds %s open issue(s)\n' \ + "$title" "$open" + findings=$((findings + 1)) + fi +done </dev/null 2>&1; then + # The status is captured rather than read from $? after the fact: the three + # branches are the whole point of this gate, and a check written as + # `if ...; elif [ $? = 2 ]` reads the status of whichever test ran last. + gate_status=0 + gate="$("$(dirname "$0")/milestone-gate.sh" "$VERSION" 2>&1)" || gate_status=$? + case "$gate_status" in + 0) ok "$gate" ;; + 2) printf " %s·%s milestones not checked (%s)\n" "$DIM" "$OFF" "$gate" ;; + *) ko "the tracker disagrees with the tags" "$gate" ;; + esac +else + printf " %s·%s milestones not checked (no remote, or gh is not installed)\n" "$DIM" "$OFF" +fi + echo if [ "$failures" -gt 0 ]; then printf "%s%d check(s) failed; nothing was tagged.%s\n" "$RED" "$failures" "$OFF" >&2 From 64a9d959ac545aaa4a8bdf1b735e40a389024c73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 16:24:15 +0100 Subject: [PATCH 4/5] docs(limits): the interception cost was paid, and not by the route this section costed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `limits:check` flagged two sections the moment #850 and #849 closed, which is the tripwire doing its job: a closed issue is not a lifted limit, and somebody has to look. Both were read. The Object Storage section is accurate — its bounds are the product's, and shipping the doors lifts none of them — so it is re-dated and nothing else. The interception section costed a route the product did not take. It still opened on "the DNS redirect is the hard, dangerous one" with no mention that `feint run` now pays the cost through a scoped forward proxy instead: CONNECT accepted, TLS terminated with a per-run authority, every unknown host refused rather than tunnelled, measured on the host network with no sudo and no namespace. The DNS analysis stays, because a reader comparing the two approaches needs it and because the namespace correction in it is a measurement error worth not repeating — but it is no longer the reason anything is refused. Only these two lines are re-dated. Eleven sections stay flagged: that is #857, and `tools/docs/limits-acks.py refresh` would have re-dated every one of them and buried it. Assisted-by: Claude Code (claude-opus-5) --- docs/limits-acks.json | 8 ++++---- docs/limits.md | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/docs/limits-acks.json b/docs/limits-acks.json index 312bcf51..33881280 100644 --- a/docs/limits-acks.json +++ b/docs/limits-acks.json @@ -1,5 +1,4 @@ { - "comment": "When each section of docs/limits.md was last confirmed against the issues it cites. A citation is a signal, never a proof: most closed issues are decisions whose limits hold. Re-date a line only after looking at the section. See tools/docs/limits-acks.py and #558.", "acknowledged": { "A Scaleway VPC's Network ACL is a record, and nothing at the edge applies it": "2026-08-27", "A Scaleway load balancer and public gateway record their configuration; nothing forwards packets": "2026-08-27", @@ -21,7 +20,7 @@ "Identifiers are not checked against anything": "2026-09-02", "Lifecycle transitions are immediate, unless you ask for otherwise": "2026-09-03", "Managed Kubernetes is not emulated, and a CRUD-only version is refused (#283)": "2026-08-27", - "Object Storage: simple explicit doors, not full client workflows": "2026-11-07", + "Object Storage: simple explicit doors, not full client workflows": "2026-11-11", "Outscale and Exoscale trail Scaleway, and the generated tables say by how much": "2026-08-27", "Placement is recorded, never enforced (#285)": "2026-08-27", "ReadTags does not list an internet service, because upstream names no type for one": "2026-08-27", @@ -30,7 +29,7 @@ "The Scaleway gateway's `GET /metadata` is served, and what it answers is three constants (#776)": "2026-09-15", "The catalogue is a whitelist, and its values are measured": "2026-08-27", "The contracts do not guarantee the same thing": "2026-08-27", - "The cost of DNS/TLS interception, measured (#76)": "2026-08-27", + "The cost of DNS/TLS interception, measured (#76)": "2026-11-11", "The firewall enforces, within stated bounds": "2026-08-28", "The guest's DHCP client is not how a published address reaches a machine (#587)": "2026-08-29", "The per-parameter half: 18 Scaleway list operations, 72 parameters, each served or refused (#277)": "2026-08-29", @@ -40,5 +39,6 @@ "`feint images resolve` prints only what boots, and names what it left out (#476)": "2026-08-30", "`lb/v1` refusals carry an envelope the real one does not": "2026-09-03", "`scw` 2.56.3 prints a recovered panic on every successful `lb acl delete`, and the defect is upstream (#505)": "2026-08-28" - } + }, + "comment": "When each section of docs/limits.md was last confirmed against the issues it cites. A citation is a signal, never a proof: most closed issues are decisions whose limits hold. Re-date a line only after looking at the section. See tools/docs/limits-acks.py and #558." } diff --git a/docs/limits.md b/docs/limits.md index 65af05a3..d2e1932d 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -242,6 +242,22 @@ interception plus a certificate the provider will accept"*, as if both were the hard part. The certificate is the easy, safe half; the DNS redirect is the hard, dangerous one. +**Since 0.15.0 the cost is paid, and not by the route this section costed.** +`feint run` (#849) stands a scoped forward proxy in front of the child: it +accepts the `CONNECT`, terminates TLS with an authority generated for that run +alone, and **refuses any host it was not told about rather than tunnelling +it**. Measured on the published Scaleway provider 2.82.0, on the host network, +with no sudo and no namespace: apply complete, 139,264 identical bytes read +back from the local byte store by sha256, an empty second plan, a complete +teardown. The authority is owner-only and never installed durably, and a client +carrying real credentials is refused unless `--unsafe-real-credentials` says +otherwise. + +What follows is therefore the cost of the **DNS** route, kept because it is +what a reader comparing the two approaches needs, and because the namespace +correction below is a measurement error worth not repeating. It is no longer +the reason anything is refused. + ### 1. How many endpoints are actually hardcoded: one product, one client The blocker is *"an endpoint built in code that no setting overrides"*. Swept From b66a187065da4a96c30ed86b93225ce6034aa8c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Wed, 11 Nov 2026 16:43:23 +0100 Subject: [PATCH 5/5] docs(limits): the Outscale peering transcript is not committed, and that is a decision (#438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #438 asks for a corpus that was recorded and never committed. Closing it needed the reason written where a reader meets it, because a refusal living only in an issue's history is the debt this repository forbids. The surface itself is not in question: the five peering operations each read `driven`, `shape: observed` and `negative: true` in `coverage/evidence.json`, from `shapes/outscale.json`. What is refused is committing a SECOND proof of an already-proven surface, whose replay cascades from one unreproduced `CreateNet` conflict — and `corpus/accepted.json` cannot hold it either, since an entry that excuses nothing fails that gate by its own rule. The recorder findings from that run are not lost: they are the `REDACTED-10` tag-key family, already filed against the instrument as #384 and already recorded in `corpus/accepted.json` as NOT THE EMULATOR. The section names what lifts it, which is what separates a limit from an excuse: the `CreateNet` conflict reproduced, or a client this project drives exercising peering where `shapes/` cannot reach. Both READMEs move with it, and that is the banner doing its job rather than noise: it counts the limits sections it vouches for, so a new section is a regeneration. `feint docs --check` refused the commit until they agreed. Assisted-by: Claude Code (claude-opus-5) --- README.fr.md | 2 +- README.md | 2 +- docs/limits-acks.json | 1 + docs/limits.md | 27 +++++++++++++++++++++++++++ 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/README.fr.md b/README.fr.md index e1c40178..06b1bd8c 100644 --- a/README.fr.md +++ b/README.fr.md @@ -31,7 +31,7 @@ > > **Prouvé** : 384 des 409 opérations montées sont pilotées par un vrai client, à chaque pull request. `scw`, `octl`, `exo`, Terraform et OpenTofu tournent contre l'émulateur en CI, et les machines démarrent réellement : connexion ssh sur le compte par défaut de chaque provider, subnets isolés, pare-feu qui filtre. La chaîne complète est décrite dans [docs/conformance.md](docs/conformance.md). > -> **Pas prouvé** : quotas, prix, capacité réelle, validation des identifiants, authentification, cohérence à terme. Les 59 sections de [docs/limits.md](docs/limits.md) disent chacune ce qu'elle coûte. Un émulateur avec un seul compte implicite et aucune grille tarifaire devrait inventer ces chiffres, et quelqu'un agirait dessus. +> **Pas prouvé** : quotas, prix, capacité réelle, validation des identifiants, authentification, cohérence à terme. Les 60 sections de [docs/limits.md](docs/limits.md) disent chacune ce qu'elle coûte. Un émulateur avec un seul compte implicite et aucune grille tarifaire devrait inventer ces chiffres, et quelqu'un agirait dessus. > > **Inconnu** : 25 opérations sont montées et n'ont jamais été pilotées par un client. Chacune dit pourquoi aucun client officiel ne l'atteint, à la route et dans [docs/routes.md](docs/routes.md). Elles sont comptées plutôt qu'escamotées, une par une, dans [coverage/evidence.json](coverage/evidence.json). > diff --git a/README.md b/README.md index b934db68..8401dcf5 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,7 @@ > > **Proven**: 384 of the 409 mounted operations are driven by a real client, on every pull request: each pack's own official CLI, and an infrastructure engine wherever a pack admits one, all of them running against the emulator in CI, and machines really boot: an ssh login on each provider's own default account, isolated subnets, a firewall that filters. The whole chain is described in [docs/conformance.md](docs/conformance.md). > -> **Not proven**: quotas, prices, real capacity, identifier validation, authentication, eventual consistency. The 59 sections of [docs/limits.md](docs/limits.md) each say what one costs. An emulator with a single implicit account and no price list would have to invent those figures, and somebody would act on them. +> **Not proven**: quotas, prices, real capacity, identifier validation, authentication, eventual consistency. The 60 sections of [docs/limits.md](docs/limits.md) each say what one costs. An emulator with a single implicit account and no price list would have to invent those figures, and somebody would act on them. > > **Unknown**: 25 operations are mounted and have never been driven by a client. Every one of them states why no official client reaches it, at the route and in [docs/routes.md](docs/routes.md). They are counted rather than glossed, one by one, in [coverage/evidence.json](coverage/evidence.json). > diff --git a/docs/limits-acks.json b/docs/limits-acks.json index 33881280..a6599b40 100644 --- a/docs/limits-acks.json +++ b/docs/limits-acks.json @@ -26,6 +26,7 @@ "ReadTags does not list an internet service, because upstream names no type for one": "2026-08-27", "The Exoscale Terraform provider is refused, and why": "2026-08-27", "The Exoscale stack's second plan is not empty: two per-id outputs read back null at apply time (#520)": "2026-08-29", + "The Outscale Net-peering surface is proven by `shapes/`, and its transcript is not committed (#438)": "2026-11-11", "The Scaleway gateway's `GET /metadata` is served, and what it answers is three constants (#776)": "2026-09-15", "The catalogue is a whitelist, and its values are measured": "2026-08-27", "The contracts do not guarantee the same thing": "2026-08-27", diff --git a/docs/limits.md b/docs/limits.md index d2e1932d..5b8d7fc3 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -3097,6 +3097,33 @@ the `provider-pack-author` skill), not a redesign. The lifting condition is the tables', never this paragraph's: the day the counts converge, the generated artefacts say so without anyone rewriting a sentence here. +## The Outscale Net-peering surface is proven by `shapes/`, and its transcript is not committed (#438) + +The five peering operations are proven: `CreateNetPeering`, `AcceptNetPeering`, +`RejectNetPeering`, `DeleteNetPeering` and `ReadNetPeerings` each read +`driven: true`, `shape: observed` and `negative: true` in +`coverage/evidence.json`, from `shapes/outscale.json`. Nothing about that +surface is unmeasured. + +What is **not** committed is a second proof of the same surface: the recorded +transcript was never added to `corpus/outscale/`, which holds five +`oapi-cli-*.jsonl` files and none for peering. Its replay produces a cascade +from a single `CreateNet` conflict that nobody has reproduced, and +`corpus/accepted.json` forbids the only shape that would let it in anyway: *an +entry that excuses nothing fails the gate*, so an exemption whose reason is +"the root is not known" cannot be written there. + +So the choice is deliberate rather than pending. A corpus trimmed until it +passes is the outcome that file exists to make impossible, and a second proof +of an already-proven surface does not buy enough to justify committing a +replay whose failure nobody understands. The recorder findings that run +surfaced are not lost: they are the `REDACTED-10` tag-key family, filed +against the instrument as #384, which `corpus/accepted.json` already records +as "NOT THE EMULATOR". + +This limit lifts the day the `CreateNet` conflict is reproduced, or the day a +client this project drives exercises peering in a way `shapes/` cannot cover. + ## The contracts do not guarantee the same thing Every response the emulator writes is checked against the provider's own API