diff --git a/.github/workflows/runtime-proof.yml b/.github/workflows/runtime-proof.yml index d78eae0a..2f5b377a 100644 --- a/.github/workflows/runtime-proof.yml +++ b/.github/workflows/runtime-proof.yml @@ -437,6 +437,25 @@ jobs: if: always() run: sudo ./feint stop --addr 127.0.0.1:4599 || true + # The closing doorstep (#521), which this workflow did not have and paid + # for on 2026-08-27 and 2026-08-28: the leg's own suites left four objects + # on the host, the next step asked the doorstep question at its start, and + # the answer came back as "a previous run left …" on a runner nothing had + # ever touched. Both halves were wrong to leave alone — a leak reported by + # the *next* thing to run, and a sentence naming a run that never existed. + # + # So the same form `mise run conformance` and conformance:leg.sh already + # carry: stop, then ask, in the closing spelling. Whatever the suites left + # reddens the leg that left it, and the report says "this run". + # + # Not `if: always()`, deliberately. A leg whose suite already failed left + # its machines by accident and not by leak; a second red there would say + # "this run leaked" about a run that simply died, and the finding this + # gate exists for would be buried under it. On the failure path the state + # is printed by the step above instead. + - name: What this leg left on the host + run: sudo tools/conformance/guard.sh leftovers-after "${{ matrix.mode }}" + # The dataplane witness gate (#486) needs Terraform: it applies the # example stacks through `feint up`. Same pinned version and upstream # checksums as conformance.yml's TERRAFORM_VERSION — one client, two @@ -504,11 +523,14 @@ jobs: # 2026-08-28, reproducing the leg: the three ssh suites each exit 0 and # each leave something standing — `scw-…` and `exo-…` rule sets no client # can delete, `fnt-default` and `feint-uplink` no resource owns — and - # `feint stop` sweeps none of them. That is why the witness gate below - # failed at its own doorstep on the one night it has been reached since it - # landed — 2026-08-27, "this host still holds what an earlier run left", - # naming feint-uplink and fnt-default; the night after never got that far. - # A job of its own starts on a runner nothing has touched. + # `feint stop` swept none of them. That is why the witness gate below + # failed at its own doorstep on the two nights it has been reached since + # it landed, "this host still holds what an earlier run left", naming + # objects the same job had made minutes earlier. The four are released by + # the graceful exit now (machine.PlumbingReleaser) and the leg asks the + # closing doorstep after its stop, so the leg reddens itself if that ever + # stops being true — but a job of its own still starts on a runner nothing + # has touched, which is one fewer thing to be wrong about. # - Three passes is half an hour. Added to that leg it would push a 45-minute # job onto its own timeout, and a job that times out is a verdict nobody # wrote. diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index e87e3496..e5e224a6 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -38,6 +38,38 @@ change ni l'un ni l'autre a sa place dans `git log`. ### Corrigé +- **Un arrêt propre rend toute la plomberie qu'aucun client ne peut supprimer, + et l'exécution qui fuit est celle qui rougit (#521).** La jambe incus-ovn de + `runtime-proof.yml` a échoué au portillon de l'étape suivante sur un runner + neuf : *a previous run left 0 machine(s) and 2 network(s) on this host*, en + nommant `feint-uplink`, `fnt-default` et les jeux de règles des groupes de + sécurité par défaut de deux providers. Il n'y avait pas d'exécution + précédente : les trois suites ssh de la jambe avaient créé les quatre, en + sortant chacune à 0. + + Quatre objets, une propriété : **aucun appel client ne peut en retirer un + seul**, donc les laisser ne mesurait rien sur les suites. `fnt-default` est + le réseau sur lequel démarre une machine sans attachement à elle, créé ici + par une Vm Outscale hors Net et possédé par aucune ressource ; + `feint-uplink` était relâché depuis #521 et restait parce que `fnt-default` + y puisait encore ; les jeux `scw-*` et `exo-*` appartiennent à des groupes + par défaut qu'un client ne peut pas supprimer, et celui de Scaleway est tiré + à chaque exécution, donc une ACL s'accumulait par session. L'arrêt rend + désormais les quatre, dans le seul ordre que le runtime accepte, et chaque + restitution pose les deux questions — est-ce à nous, et quelque chose y + puise-t-il encore — de sorte qu'un `feint stop` sans `--cleanup` laisse leur + pare-feu aux machines qu'il laisse délibérément tourner. + + La seconde moitié est la phrase. `feint clean --check --closing` et + `tools/conformance/guard.sh leftovers-after` posent la même question et + nomment **cette** exécution, `runtime-proof.yml` la pose après son propre + arrêt au lieu de laisser l'étape suivante rencontrer le reste, et le + portillon nomme désormais les jeux de règles sur lesquels il ne refuse *pas* + au lieu de passer dessus en silence. La colonne d'attribution du registre a + été corrigée avec eux : un réseau est trouvé par l'étiquette que lit + `Survey`, pas par le préfixe `fnt-` que la colonne annonçait — et le premier + réseau nommé par la jambe en échec, `feint-uplink`, ne porte pas ce préfixe. + - **Le portillon de sortie de la porte des stacks pose enfin la question que son propre commentaire annonçait, et la porte cesse de choisir son runtime en silence (#504).** Elle se terminait sur diff --git a/CHANGELOG.md b/CHANGELOG.md index a091e625..a8ec37e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,36 @@ what this project is judged on: **a response shape a client can observe**, and ### Fixed +- **A graceful exit gives back every piece of host plumbing no client can + delete, and a run that leaks is the run that goes red (#521).** The incus-ovn + leg of `runtime-proof.yml` failed at the doorstep of the next step on a + GitHub runner nothing had ever touched: *a previous run left 0 machine(s) and + 2 network(s) on this host*, naming `feint-uplink`, `fnt-default` and the rule + sets of two providers' default security groups. There was no previous run — + the leg's own three ssh suites had made all four, each exiting 0. + + Four objects, one property: **no client call can remove any of them**, so + leaving them measured nothing about the suites. `fnt-default` is the network + a machine with no attachment of its own boots on, created here by an Outscale + Vm outside a Net and owned by no resource; `feint-uplink` had been released + since #521 and stayed because `fnt-default` still drew from it; the `scw-*` + and `exo-*` rule sets belong to default security groups a client cannot + delete, and Scaleway's is minted per run, so one host ACL accumulated per + session. The exit now releases all four, in the only order the runtime + accepts, and each release answers both questions — is it ours, and is + anything still drawing from it — so a `feint stop` without `--cleanup` leaves + the firewall on the machines it deliberately leaves running. + + The second half is the sentence. `feint clean --check --closing` and + `tools/conformance/guard.sh leftovers-after` ask the identical question and + name **this** run, `runtime-proof.yml` asks it after its own stop instead of + letting the next step meet the residue, and the doorstep now names the rule + sets it does *not* refuse on rather than passing over them in silence. The + ledger's attribution column was corrected with them: a network is found by + the label `Survey` reads, not by the `fnt-` prefix the column claimed — and + the first network the failing leg reported, `feint-uplink`, carries no such + prefix. + - **The stack gate's closing doorstep asks the question its own comment claimed, and the gate stops choosing its runtime in silence (#504).** It closed with `guard_leftovers_for "$RUNTIME" "the end of the run"` under a diff --git a/internal/cli/clean.go b/internal/cli/clean.go index 5c8af5c6..0cd8675e 100644 --- a/internal/cli/clean.go +++ b/internal/cli/clean.go @@ -26,6 +26,7 @@ func clean(args []string, stdout io.Writer) error { check := fs.Bool("check", false, "report what this user cannot remove and remove nothing; exit 1 if anything is stuck") format := fs.String("format", "text", "output format: text, or json for one aggregatable line per object found") doorstep := fs.Bool("doorstep", false, "also refuse a machine or network of an earlier run; only true before a run starts, because a run in flight owns both") + closing := fs.Bool("closing", false, "the same refusal, asked once this run's emulator has stopped: what is found is then this run's own leak, and the report says so") force := fs.Bool("force", false, "also clear what no ordinary command of the runtime reaches: the peering rows a deleted network left behind, named one by one before they go") if err := fs.Parse(args); err != nil { return err @@ -40,9 +41,24 @@ func clean(args []string, stdout io.Writer) error { if *check && *force { return fmt.Errorf("--check removes nothing and --force removes what nothing else can: ask for one of the two") } + // The two moments name opposite culprits — "a previous run" and "this run" — + // and a caller who asked for both wants a sentence that cannot be true. The + // refusal is identical either way, so guessing here would cost nothing at + // the gate and everything at the report, which is the whole subject of the + // leg that made this flag exist. + if *doorstep && *closing { + return fmt.Errorf("--doorstep asks what an earlier run left and --closing what this one did: ask for one of the two") + } + moment := momentInFlight + switch { + case *doorstep: + moment = momentDoorstep + case *closing: + moment = momentClosing + } led := newLedger(stdout, *format == "json", time.Now()) if *check { - return reportStuckLeftovers(stdout, led, *vm, *doorstep) + return reportStuckLeftovers(stdout, led, *vm, moment) } // The state directories go first, and deliberately before the runtime is @@ -207,7 +223,7 @@ var ( // // TestCleanCheckRefusesAHostWhoseLeftoverThisUserCannotEnd and // TestCleanCheckPassesWhenTheSweepItselfWouldClearThem fail without it. -func reportStuckLeftovers(stdout io.Writer, led *ledger, vm string, doorstep bool) error { +func reportStuckLeftovers(stdout io.Writer, led *ledger, vm string, moment leftoverMoment) error { // The runtime half first, and #426 is why it exists at all. // // Before this, the doorstep asked one question — is there a DHCP service @@ -253,8 +269,8 @@ func reportStuckLeftovers(stdout io.Writer, led *ledger, vm string, doorstep boo return fmt.Errorf("could not ask the %s runtime what a previous run left: %w", vm, err) } - if doorstep { - if err := refuseRuntimeLeftovers(stdout, led, vm, rt); err != nil { + if moment.asks() { + if err := refuseRuntimeLeftovers(stdout, led, vm, rt, moment); err != nil { return err } } diff --git a/internal/cli/clean_ledger.go b/internal/cli/clean_ledger.go index 6c869f53..b16f893c 100644 --- a/internal/cli/clean_ledger.go +++ b/internal/cli/clean_ledger.go @@ -100,8 +100,51 @@ const ( const ( stageDoorstep = "doorstep" stageSweep = "sweep" + // stageClosing is the same question as the doorstep, asked once a run's + // emulator has stopped rather than before it started (#521). It is a stage + // of its own and not a flavour of the doorstep because the ledger exists to + // name *which mechanism* produces waste: "a previous run left this" and + // "the run that just ended left this" are the two different findings, and + // counting them together loses the only one an operator can act on today. + stageClosing = "closing" ) +// leftoverMoment says when in a run the machine-and-network question is being +// asked. It changes no verdict — the same objects refuse at both moments — and +// it changes every sentence, because the wrong sentence sends the reader to the +// wrong run. +// +// Measured on 2026-08-28: the incus-ovn leg of runtime-proof.yml failed on a +// GitHub runner nothing had ever touched with "a previous run left 0 machine(s) +// and 2 network(s) on this host", naming objects the very same job had created +// eight steps earlier. There was no previous run. A reader who believed the +// sentence would have gone looking at the schedule instead of at the ssh suites. +type leftoverMoment string + +const ( + // momentInFlight is a run asking about itself, which is why the question is + // not asked at all: see reportStuckLeftovers. + momentInFlight leftoverMoment = "" + // momentDoorstep is before anything of this run has started, so anything + // found belongs to somebody else's run. + momentDoorstep leftoverMoment = "doorstep" + // momentClosing is after this run's emulator has stopped, so anything found + // is this run's own leak — the form #521 gave `mise run conformance`, which + // runtime-proof.yml had not been given. + momentClosing leftoverMoment = "closing" +) + +// asks reports whether this moment asks the machine-and-network question. +func (m leftoverMoment) asks() bool { return m == momentDoorstep || m == momentClosing } + +// stage names the ledger stage of this moment. +func (m leftoverMoment) stage() string { + if m == momentClosing { + return stageClosing + } + return stageDoorstep +} + // newRunID identifies one invocation. Random rather than a counter because two // runs may append to one file from two shells, and time alone collides. func newRunID(now time.Time) string { @@ -184,13 +227,24 @@ func (l *ledger) recordAll(left machine.Leftovers, stage, why, action string) { names []string attribution string }{ - // Machines and rule sets carry a label the emulator wrote; a network is - // recognised by the prefix this code derives. Saying which is which is - // the "who produced it" column, and it is also the answer to whether - // this run was ever entitled to touch the object. + // Machines and networks carry a label the emulator wrote; a rule set + // carries no config at all, so it is recognised by the description + // EnsureFirewall writes. Saying which is which is the "who produced it" + // column, and it is also the answer to whether this run was ever + // entitled to touch the object. + // + // The network line said "name-prefix:fnt-" until 2026-08-28, and it was + // a column lying about its own subject: Incus.Survey selects networks + // by `user.feint.provider` exactly as it does machines, and the first + // network the leg of that day reported was `feint-uplink`, which does + // not carry the prefix the column claimed had identified it. A ledger + // whose attribution column can name a mark the object does not carry is + // a ledger that cannot be used to decide what may be touched. + // TestTheLedgerAttributesEachObjectToTheMarkItWasFoundBy fails without + // the correction. {"machine", left.Machines, "label:" + machine.LabelKey}, - {"network", left.Networks, "name-prefix:" + machine.NetworkPrefix + "-"}, - {"rule-set", left.Firewalls, "label:" + machine.LabelKey}, + {"network", left.Networks, "label:" + machine.LabelKey}, + {"rule-set", left.Firewalls, "description:" + machine.FirewallDescription}, } { names := append([]string(nil), group.names...) sort.Strings(names) @@ -224,10 +278,11 @@ func (l *ledger) recordAll(left machine.Leftovers, stage, why, action string) { // The driver is resolved by the caller and passed in, since the check now asks // this runtime two questions rather than one and resolving it twice would let // them disagree about which host they are talking about. -func refuseRuntimeLeftovers(out io.Writer, led *ledger, vm string, rt machine.Runtime) error { +func refuseRuntimeLeftovers(out io.Writer, led *ledger, vm string, rt machine.Runtime, moment leftoverMoment) error { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() + stage := moment.stage() left, surveyable, err := surveyRuntime(ctx, rt) if !surveyable { // --vm off, and every driver that cannot be asked. Said out loud rather @@ -238,28 +293,88 @@ func refuseRuntimeLeftovers(out io.Writer, led *ledger, vm string, rt machine.Ru } if err != nil { led.record(leftoverRecord{Kind: "survey", Name: rt.Name(), Attribution: "none", - Stage: stageDoorstep, Why: whyUnreadable, Action: actionNone}) + Stage: stage, Why: whyUnreadable, Action: actionNone}) return fmt.Errorf("could not look at what the %s runtime holds, so this host cannot be called clean: %w", rt.Name(), err) } if len(left.Machines) == 0 && len(left.Networks) == 0 { - led.prose("no machine or network of an earlier run is left on this runtime\n") + reportRuleSetsLeftBehind(out, led, vm, left, stage) + led.prose("no machine or network %s is left on this runtime\n", moment.whose()) return nil } - led.recordAll(left, stageDoorstep, whyUnswept, actionReported) + led.recordAll(left, stage, whyUnswept, actionReported) if !led.asJSON { - fmt.Fprintf(out, "\na previous run left %d machine(s) and %d network(s) on this host.\n", - len(left.Machines), len(left.Networks)) - fmt.Fprintf(out, "They hold their address blocks, and the next run asks for those blocks under new\n"+ - "names, so it fails on \"Address already in use\" thirty steps in instead of here.\n") + fmt.Fprintf(out, "\n%s left %d machine(s) and %d network(s) on this host.\n", + moment.subject(), len(left.Machines), len(left.Networks)) + fmt.Fprintf(out, "%s\n", moment.consequence()) // One command, nothing to copy out of the text above it. #375 measured // what the other shape costs: a remedy that needs a pid retyped out of a // log did not get run for three consecutive failures. fmt.Fprintf(out, "\nRun: feint clean --vm %s\n", vm) } - return fmt.Errorf("%d machine(s) and %d network(s) of an earlier run still hold this host", - len(left.Machines), len(left.Networks)) + return fmt.Errorf("%d machine(s) and %d network(s) %s still hold this host", + len(left.Machines), len(left.Networks), moment.whose()) +} + +// reportRuleSetsLeftBehind names the rule sets a run left when nothing refuses +// on them, which is every time: refuseRuntimeLeftovers refuses on machines and +// networks alone, because a rule set holds no address block and refusing on one +// would fire on a host nothing was going to fail on (unkillable-dhcp-orphan's +// mutation 7 holds that). +// +// Not refusing is not the same as not saying. Until 2026-08-28 a host holding +// nothing but rule sets of this emulator was reported "no machine or network of +// an earlier run is left" and the rule sets were never printed at all — they +// appeared in the ledger only when some *other* object had already made the +// check refuse. So the one artefact that accumulates one object per session on +// an operator's station (a Scaleway project default group is minted per run) +// was invisible precisely on the hosts where it was the only thing left. +// +// An expected remainder is admitted with its reason written, never in silence. +// TestTheDoorstepNamesRuleSetsItDoesNotRefuseOn fails without this. +func reportRuleSetsLeftBehind(out io.Writer, led *ledger, vm string, left machine.Leftovers, stage string) { + if len(left.Firewalls) == 0 { + return + } + led.recordAll(machine.Leftovers{Firewalls: left.Firewalls}, stage, whyUnswept, actionReported) + if led.asJSON { + return + } + fmt.Fprintf(out, "\n%d rule set(s) of this emulator are on this host, and nothing here refuses on them:\n", + len(left.Firewalls)) + fmt.Fprintf(out, "they hold no address block, so no run fails for their sake. They are still waste —\n"+ + "`feint clean --vm %s` removes them, and a graceful `feint stop` gives back the ones\n"+ + "nothing uses, so a rule set surviving one is a finding rather than housekeeping.\n", vm) +} + +// subject names who left what this moment found, in the sentence's own words. +func (m leftoverMoment) subject() string { + if m == momentClosing { + return "this run" + } + return "a previous run" +} + +// whose is the same fact in the possessive, for the error a caller reads. +func (m leftoverMoment) whose() string { + if m == momentClosing { + return "of this run" + } + return "of an earlier run" +} + +// consequence is why it matters, and it differs because the reader's next step +// differs: before a run, the leftovers are what will make it fail later; after +// one, they are what this run failed to clean up. +func (m leftoverMoment) consequence() string { + if m == momentClosing { + return "Its clients were supposed to delete what they created, and the emulator gives its own\n" + + "plumbing back on the way out (machine.PlumbingReleaser). What is named above is neither,\n" + + "so it is this run's leak — and the next run would have been blamed for it." + } + return "They hold their address blocks, and the next run asks for those blocks under new\n" + + "names, so it fails on \"Address already in use\" thirty steps in instead of here." } // survivors returns the objects present in both surveys: the ones a sweep was diff --git a/internal/cli/clean_ledger_test.go b/internal/cli/clean_ledger_test.go index ac1aed50..f1b54b9e 100644 --- a/internal/cli/clean_ledger_test.go +++ b/internal/cli/clean_ledger_test.go @@ -108,7 +108,7 @@ func TestTheDoorstepRefusesAHostHoldingAPreviousRunsNetwork(t *testing.T) { withDriver(t, machine.Use(held)) var out bytes.Buffer - err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", true) + err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", momentDoorstep) if err == nil { t.Fatal("the doorstep accepted a host still holding a network of an earlier run: " + "the run starts, takes minutes, and dies on \"Address already in use\" for that block (#426)") @@ -126,7 +126,7 @@ func TestTheDoorstepRefusesAHostHoldingAPreviousRunsNetwork(t *testing.T) { // The accepting half, on the same path. withDriver(t, machine.Use(&sweptDriver{})) var clean bytes.Buffer - if err := reportStuckLeftovers(&clean, newLedger(&clean, false, time.Now()), "incus", true); err != nil { + if err := reportStuckLeftovers(&clean, newLedger(&clean, false, time.Now()), "incus", momentDoorstep); err != nil { t.Fatalf("the doorstep refused a runtime holding nothing: %v (%q)", err, clean.String()) } } @@ -142,7 +142,7 @@ func TestTheDoorstepSaysItCouldNotLookRatherThanCallingTheHostClean(t *testing.T var out bytes.Buffer led := newLedger(&out, true, time.Now()) - if err := reportStuckLeftovers(&out, led, "incus", true); err == nil { + if err := reportStuckLeftovers(&out, led, "incus", momentDoorstep); err == nil { t.Fatal("a runtime that could not be surveyed was reported as a clean host") } if why := whyOfLines(t, out.String()); !why[whyUnreadable] { @@ -188,7 +188,9 @@ func TestTheSweepNamesWhatSurvivedItsOwnSuccessfulDelete(t *testing.T) { if survived.Kind != "network" { t.Errorf("kind %q, want network", survived.Kind) } - if !strings.HasPrefix(survived.Attribution, "name-prefix:") { + // A network is found by the label Survey reads, not by the name prefix this + // line used to claim — see TestTheLedgerAttributesEachObjectToTheMarkItWasFoundBy. + if !strings.HasPrefix(survived.Attribution, "label:") { t.Errorf("attribution %q says nothing about how this run knows the object is ours", survived.Attribution) } if survived.Stage != stageSweep { @@ -358,7 +360,7 @@ func TestTheLeftoverCheckMidRunIgnoresTheRunsOwnObjects(t *testing.T) { withDriver(t, machine.Use(live)) var out bytes.Buffer - if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", false); err != nil { + if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", momentInFlight); err != nil { t.Fatalf("the mid-run check refused a run for owning the machines and networks it had just created: %v\n%s", err, out.String()) } @@ -372,7 +374,7 @@ func TestTheLeftoverCheckMidRunIgnoresTheRunsOwnObjects(t *testing.T) { // The same runtime at the doorstep must refuse, or this test is passing // because nothing looks at all. var door bytes.Buffer - if err := reportStuckLeftovers(&door, newLedger(&door, false, time.Now()), "incus", true); err == nil { + if err := reportStuckLeftovers(&door, newLedger(&door, false, time.Now()), "incus", momentDoorstep); err == nil { t.Fatal("the doorstep accepted the same runtime, so the mid-run pass above proves nothing") } } diff --git a/internal/cli/clean_report_test.go b/internal/cli/clean_report_test.go new file mode 100644 index 00000000..02e2532f --- /dev/null +++ b/internal/cli/clean_report_test.go @@ -0,0 +1,160 @@ +package cli + +import ( + "bytes" + "strings" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/machine" +) + +// The three properties the incus-ovn leg of runtime-proof.yml demanded on +// 2026-08-28, when it failed at its own witness gate on a runner nothing had +// ever touched. Each is a sentence the report was getting wrong, and a report +// that names the wrong culprit, the wrong mark, or nothing at all, is an +// instrument rather than a finding. +// +// The two rule-set names below are the ones the job printed, digests of the +// providers' default security groups: a Scaleway project default, minted per +// run, and Exoscale's account default, whose identifier is fixed. +const ( + leakedScalewaySet = "scw-" + "45e6b24f03f" + leakedExoscaleSet = "exo-" + "11e594f4819" +) + +// TestTheClosingCheckBlamesTheRunThatLeaked: the same objects, the same +// refusal, and the culprit named as this run rather than a previous one. +// +// "a previous run left 0 machine(s) and 2 network(s) on this host" is what the +// job printed about objects its own ssh suites had created eight steps earlier. +// The refusal was right; the sentence sent the reader to a run that never +// existed. +func TestTheClosingCheckBlamesTheRunThatLeaked(t *testing.T) { + quietDHCP(t) + withDriver(t, machine.Use(&sweptDriver{ + left: machine.Leftovers{Networks: []string{"fnt-default", "feint-uplink"}}, + })) + + var closing bytes.Buffer + err := reportStuckLeftovers(&closing, newLedger(&closing, false, time.Now()), "incus-ovn", momentClosing) + if err == nil { + t.Fatal("the closing check accepted a host this run left two networks on") + } + if strings.Contains(closing.String(), "previous run") || strings.Contains(err.Error(), "earlier run") { + t.Errorf("the closing check blames a run that does not exist:\n%s\n%v", closing.String(), err) + } + if !strings.Contains(closing.String(), "this run") { + t.Errorf("the closing check never says whose leak it found:\n%s", closing.String()) + } + + // The witness: the doorstep must keep saying the opposite, or this test + // passes on code that simply renamed the sentence everywhere. + withDriver(t, machine.Use(&sweptDriver{ + left: machine.Leftovers{Networks: []string{"fnt-default", "feint-uplink"}}, + })) + var door bytes.Buffer + if err := reportStuckLeftovers(&door, newLedger(&door, false, time.Now()), "incus-ovn", momentDoorstep); err == nil { + t.Fatal("the doorstep accepted the same host") + } + if !strings.Contains(door.String(), "previous run") { + t.Errorf("the doorstep stopped naming the earlier run it exists to name:\n%s", door.String()) + } +} + +// TestTheDoorstepNamesRuleSetsItDoesNotRefuseOn: a host holding nothing but +// rule sets of this emulator is not refused — they hold no address block — and +// it must still be told about them. +// +// Until this, they were printed only when some *other* object had already made +// the check refuse, so the one artefact that accumulates once per session on an +// operator's station was invisible exactly on the hosts where it was all that +// was left. +func TestTheDoorstepNamesRuleSetsItDoesNotRefuseOn(t *testing.T) { + quietDHCP(t) + withDriver(t, machine.Use(&sweptDriver{ + left: machine.Leftovers{Firewalls: []string{leakedScalewaySet, leakedExoscaleSet}}, + })) + + var out bytes.Buffer + if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus-ovn", momentDoorstep); err != nil { + t.Fatalf("a rule set holds no address block, so it must not refuse a run: %v", err) + } + for _, name := range []string{leakedScalewaySet, leakedExoscaleSet} { + if !strings.Contains(out.String(), name) { + t.Errorf("the check found %s and said nothing about it:\n%s", name, out.String()) + } + } + if !strings.Contains(out.String(), "feint clean") { + t.Errorf("the remainder is admitted without naming what removes it:\n%s", out.String()) + } + + // The witness: a host holding none must not grow the paragraph, or every + // clean run would read as one holding waste. + withDriver(t, machine.Use(&sweptDriver{})) + var clean bytes.Buffer + if err := reportStuckLeftovers(&clean, newLedger(&clean, false, time.Now()), "incus-ovn", momentDoorstep); err != nil { + t.Fatalf("the check refused a runtime holding nothing: %v", err) + } + if strings.Contains(clean.String(), "rule set(s) of this emulator") { + t.Errorf("a host holding no rule set was told it holds some:\n%s", clean.String()) + } +} + +// TestTheLedgerAttributesEachObjectToTheMarkItWasFoundBy: the column that says +// how this run knows an object is the emulator's must name a mark the object +// actually carries. +// +// It did not. Networks were recorded as `name-prefix:fnt-` while Survey selects +// them by `user.feint.provider`, exactly as it does machines — and the first +// network the leg of 2026-08-28 reported was `feint-uplink`, which carries no +// such prefix. A column that can name a mark the object does not carry cannot +// be used to decide what may be touched, which is the only thing it is for. +func TestTheLedgerAttributesEachObjectToTheMarkItWasFoundBy(t *testing.T) { + quietDHCP(t) + withDriver(t, machine.Use(&sweptDriver{ + left: machine.Leftovers{ + Machines: []string{"feint-scw-a"}, + Networks: []string{"feint-uplink"}, + Firewalls: []string{leakedScalewaySet}, + }, + })) + + var out bytes.Buffer + if err := reportStuckLeftovers(&out, newLedger(&out, true, time.Now()), "incus-ovn", momentDoorstep); err == nil { + t.Fatal("the doorstep accepted a host holding a machine and a network") + } + want := map[string]string{ + "feint-scw-a": "label:" + machine.LabelKey, + "feint-uplink": "label:" + machine.LabelKey, + leakedScalewaySet: "description:" + machine.FirewallDescription, + } + for _, rec := range ledgerLines(t, out.String()) { + expected, known := want[rec.Name] + if !known { + continue + } + if rec.Attribution != expected { + t.Errorf("%s is attributed to %q, and it carries %q", rec.Name, rec.Attribution, expected) + } + delete(want, rec.Name) + } + for name := range want { + t.Errorf("%s never appeared in the ledger at all", name) + } +} + +// TestTheTwoMomentsCannotBeAskedAtOnce: --doorstep and --closing name opposite +// culprits, so a caller who typed both asked for a sentence that cannot be +// true. Resolving it in either direction would print a confident lie, which is +// the whole subject of the leg that made the flag exist. +func TestTheTwoMomentsCannotBeAskedAtOnce(t *testing.T) { + var out bytes.Buffer + err := clean([]string{"--check", "--doorstep", "--closing", "--vm", "incus"}, &out) + if err == nil { + t.Fatal("clean accepted --doorstep and --closing together and picked one of the two sentences") + } + if !strings.Contains(err.Error(), "one of the two") { + t.Errorf("the refusal does not tell the caller what to do: %v", err) + } +} diff --git a/internal/cli/clean_traps_test.go b/internal/cli/clean_traps_test.go index 04543001..028979fa 100644 --- a/internal/cli/clean_traps_test.go +++ b/internal/cli/clean_traps_test.go @@ -59,7 +59,7 @@ func checkAgainst(t *testing.T, driver machine.Runtime) (string, error) { withDriver(t, driver) var out bytes.Buffer - err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", false) + err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", momentInFlight) return out.String(), err } diff --git a/internal/cli/cli.go b/internal/cli/cli.go index d1142b0e..aaba78bd 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -199,11 +199,22 @@ const ( // needs the API that is about to stop. Two verbs added; nothing was removed, no // exit code moved, and a pipeline keyed on version 17 keeps working. // +// Version 19 adds `clean --closing` (#521, and the leg of 2026-08-28): the +// doorstep refusal, asked once a run's emulator has stopped rather than before +// it started. The refusal is identical — a machine or a network nothing here +// owns — and the sentence is not, because the culprit is not: the incus-ovn leg +// of runtime-proof.yml failed on a GitHub runner nothing had ever touched with +// "a previous run left 0 machine(s) and 2 network(s) on this host", naming +// objects the same job had created eight steps earlier. A reader who believed +// it would have gone looking at the schedule instead of at the ssh suites. An +// addition to one existing verb; nothing was removed, no exit code moved, and a +// pipeline keyed on version 18 keeps working. +// // The surface itself is frozen in testdata/frozen/cli.json, compared by // TestTheFrozenSurfacesStillMatchTheirFixture, and a fixture regenerated // without bumping this constant fails TestASurfaceChangeDemandsItsVersionBump. // The procedure for a deliberate change is in RELEASING.md ("Frozen surfaces"). -const cliSurfaceVersion = 18 +const cliSurfaceVersion = 19 // Run executes one command and returns the process exit code. func Run(args []string, stdout, stderr io.Writer) int { @@ -555,8 +566,8 @@ Usage: feint catalog [--format json] Print the emulated inventory a client reads before creating. - feint clean [--vm incus|incus-vm|incus-ovn] [--check] [--doorstep] [--force] - [--format text|json] + feint clean [--vm incus|incus-vm|incus-ovn] [--check] [--doorstep|--closing] + [--force] [--format text|json] Remove every machine, network and rule set the emulator created. Labelled resources only; nothing else is touched. --check removes nothing: it names what a run left behind @@ -565,6 +576,12 @@ Usage: doorstep instead of dying on the block minutes in. It also names the states no ordinary command of the runtime can leave, and exits 1 on those. + --doorstep and --closing add the same refusal — a machine + or a network of a run that is not this one — and differ in + the sentence they print: before a run starts what is found + belongs to a previous one, after its emulator stopped it is + that run's own leak. Naming the wrong culprit sends the + reader to the wrong run, which a nightly job measured. --force clears those states, which today means the peering rows a deleted network left behind. It reaches the runtime's own database through "incus admin sql", touches only rows @@ -1052,17 +1069,21 @@ func serve(args []string, stdout io.Writer) error { } // shutdownSweep is what a graceful exit leaves the host as: --cleanup prunes -// everything this run created, and the uplink goes in every case where it is -// this process's own and nothing draws from it any more (#521). +// everything this run created, and the host plumbing goes in every case where +// it is this process's own and nothing draws from it any more (#521). // // The release is not gated on --cleanup, deliberately. --cleanup is a sweep, // and running it by default would hide a client that leaks — the suites are -// supposed to delete what they create, and the doorstep of the next run is the -// instrument that says whether they did. The uplink is different in kind: no +// supposed to delete what they create, and the doorstep after the stop is the +// instrument that says whether they did. The plumbing is different in kind: no // client owns it and no client's delete will ever remove it, so leaving it is // not a measurement of anything, it is a leftover by construction — the one -// that made two green conformance runs fail their successor's doorstep. -// TestAGracefulExitReleasesTheUplink fails without the call. +// that made two green conformance runs fail their successor's doorstep, and +// then the incus-ovn leg of runtime-proof.yml fail its own witness gate with +// four objects rather than one. machine.PlumbingReleaser lists them and says +// what each is. +// TestAGracefulExitReleasesTheUplink fails without the call, and +// TestAGracefulExitNamesEveryPieceOfPlumbingItGaveBack without the names. func shutdownSweep(rt machine.Runtime, cleanup bool, stdout io.Writer) { if cleanup { if pruned, asked, err := rt.Prune(context.Background()); asked { @@ -1073,15 +1094,20 @@ func shutdownSweep(rt machine.Runtime, cleanup bool, stdout io.Writer) { } } } - if released, asked, err := rt.ReleaseUplink(context.Background()); asked { - switch { - case err != nil: - // Said rather than swallowed: an uplink this exit could not judge - // is one the next run's doorstep will refuse, and the operator + if released, asked, err := rt.ReleasePlumbing(context.Background()); asked { + if len(released) > 0 { + // Named, not counted. What was given back is the difference between + // a host this run left clean and one the next doorstep refuses, and + // an operator reading "released 3 object(s)" cannot tell which + // three. + fmt.Fprintf(stdout, "released the plumbing this run held: %s\n", + strings.Join(released, ", ")) + } + if err != nil { + // Said rather than swallowed: plumbing this exit could not judge is + // what the doorstep after the stop will refuse, and the operator // should hear it from the run that caused it. - fmt.Fprintf(stdout, "uplink: %v\n", err) - case released: - fmt.Fprintf(stdout, "released the uplink; no network of this run holds the host\n") + fmt.Fprintf(stdout, "plumbing: %v\n", err) } } } diff --git a/internal/cli/dhcp_leftover_test.go b/internal/cli/dhcp_leftover_test.go index 40665864..a837069a 100644 --- a/internal/cli/dhcp_leftover_test.go +++ b/internal/cli/dhcp_leftover_test.go @@ -213,7 +213,7 @@ func TestCleanCheckRefusesAHostWhoseLeftoverThisUserCannotEnd(t *testing.T) { swapProbeSeam(t, func(machine.DHCPLeftover) error { return fmt.Errorf("signal: %w", os.ErrPermission) }) var out bytes.Buffer - err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", false) + err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", momentInFlight) if err == nil { t.Fatal("a host whose block is held by a process nobody here may end was reported as ready") } @@ -242,7 +242,7 @@ func TestCleanCheckPassesWhenTheSweepItselfWouldClearThem(t *testing.T) { swapProbeSeam(t, func(machine.DHCPLeftover) error { return nil }) var out bytes.Buffer - if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", false); err != nil { + if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", momentInFlight); err != nil { t.Fatalf("a leftover this user can end refused the run: %v\n%s", err, out.String()) } report := out.String() @@ -265,7 +265,7 @@ func TestCleanCheckSaysSoOnAHostWithNothingLeftBehind(t *testing.T) { swapProbeSeam(t, func(machine.DHCPLeftover) error { t.Fatal("a check probed a process it never found"); return nil }) var out bytes.Buffer - if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", false); err != nil { + if err := reportStuckLeftovers(&out, newLedger(&out, false, time.Now()), "incus", momentInFlight); err != nil { t.Fatalf("a clean host was refused: %v", err) } if !strings.Contains(out.String(), "no DHCP service") { diff --git a/internal/cli/driver_surface_test.go b/internal/cli/driver_surface_test.go index 9e6c4970..18d79c35 100644 --- a/internal/cli/driver_surface_test.go +++ b/internal/cli/driver_surface_test.go @@ -699,7 +699,7 @@ var mustStayOutside = []string{ // them. "Capable", "Waiter", "ImageBuilder", "ImageLister", "Pruner", "Repairer", "Surveyor", "Watcher", - "UplinkReleaser", + "PlumbingReleaser", // The handle itself, and its door. It is the emulator's and the CLI's // spelling of a runtime; a pack that names it has gone looking for the // value #511 took out of its reach. diff --git a/internal/cli/shutdown_sweep_test.go b/internal/cli/shutdown_sweep_test.go index 59389c4f..ca0ae1bd 100644 --- a/internal/cli/shutdown_sweep_test.go +++ b/internal/cli/shutdown_sweep_test.go @@ -3,29 +3,33 @@ package cli import ( "bytes" "context" + "errors" "strings" "testing" "github.com/stephrobert/feint/internal/core/machine" ) -// The serve half of #521: the driver knows how to release the uplink, and a -// graceful exit that never asks leaves the one network no client's delete can +// The serve half of #521: the driver knows how to release the host plumbing, +// and a graceful exit that never asks leaves the objects no client's delete can // remove — which is what two green conformance runs left, and what the next -// run's doorstep refused. These tests hold the wiring through a fake releaser, -// the way leftovers_test.go holds the startup notice; the driver's own -// behaviour lives with the driver, in incus_uplink_release_test.go. +// run's doorstep refused. The same defect came back on 2026-08-28 with three +// more objects (the default machine network and two default-group rule sets), +// which is why the exit now names what it gave back instead of announcing one. +// These tests hold the wiring through a fake releaser, the way leftovers_test.go +// holds the startup notice; the driver's own behaviour lives with the driver, in +// incus_release_test.go and incus_uplink_release_test.go. // releasingDriver is a metadata-only driver that records whether the exit -// asked it to release the uplink. +// asked it to release its plumbing. type releasingDriver struct { machine.Noop asked bool - released bool + released []string err error } -func (d *releasingDriver) ReleaseUplink(context.Context) (bool, error) { +func (d *releasingDriver) ReleasePlumbing(context.Context) ([]string, error) { d.asked = true return d.released, d.err } @@ -34,24 +38,42 @@ func (d *releasingDriver) ReleaseUplink(context.Context) (bool, error) { // a release that happened is said out loud rather than logged nowhere. func TestAGracefulExitReleasesTheUplink(t *testing.T) { var buf bytes.Buffer - driver := &releasingDriver{released: true} + driver := &releasingDriver{released: []string{"feint-uplink"}} shutdownSweep(machine.Use(driver), false, &buf) if !driver.asked { - t.Fatal("the exit never asked the driver to release the uplink; the next run's doorstep refuses what stays (#521)") + t.Fatal("the exit never asked the driver to release its plumbing; the next run's doorstep refuses what stays (#521)") } - if !strings.Contains(buf.String(), "released the uplink") { + if !strings.Contains(buf.String(), "feint-uplink") { t.Errorf("a release that happened was not said:\n%s", buf.String()) } } -// TestAnExitSaysNothingWhenTheUplinkIsNotItsToRelease: an uplink kept — held -// by another run, or still in use — is the doorstep's story to tell, and a -// line here on every runtime-free exit would be noise nobody reads. +// TestAGracefulExitNamesEveryPieceOfPlumbingItGaveBack is the half the leg of +// 2026-08-28 asked for: four objects were left on that host and the exit's +// line spoke of one. A count would not have told the operator which of them +// went, and the whole value of this line is that the next doorstep's silence +// is explained by it. +func TestAGracefulExitNamesEveryPieceOfPlumbingItGaveBack(t *testing.T) { + var buf bytes.Buffer + driver := &releasingDriver{released: []string{"scw-45e6b24f03f", "fnt-default", "feint-uplink"}} + + shutdownSweep(machine.Use(driver), false, &buf) + + for _, name := range driver.released { + if !strings.Contains(buf.String(), name) { + t.Errorf("the exit released %s and did not name it:\n%s", name, buf.String()) + } + } +} + +// TestAnExitSaysNothingWhenTheUplinkIsNotItsToRelease: plumbing kept — held by +// another run, or still in use — is the doorstep's story to tell, and a line +// here on every runtime-free exit would be noise nobody reads. func TestAnExitSaysNothingWhenTheUplinkIsNotItsToRelease(t *testing.T) { var buf bytes.Buffer - driver := &releasingDriver{released: false} + driver := &releasingDriver{} shutdownSweep(machine.Use(driver), false, &buf) @@ -63,14 +85,28 @@ func TestAnExitSaysNothingWhenTheUplinkIsNotItsToRelease(t *testing.T) { } } -// TestAnExitWithoutAReleaserStaysAnExit: --vm off and every driver without an -// uplink must shut down exactly as before. +// TestAnExitThatCouldNotReleaseSaysSo: a release that failed is what the next +// doorstep will refuse on, and the operator must hear it from the run that +// caused it rather than from the run that meets it. +func TestAnExitThatCouldNotReleaseSaysSo(t *testing.T) { + var buf bytes.Buffer + driver := &releasingDriver{err: errors.New("rule set scw-45e6b24f03f: in use by something")} + + shutdownSweep(machine.Use(driver), false, &buf) + + if !strings.Contains(buf.String(), "scw-45e6b24f03f") { + t.Errorf("a failed release was swallowed:\n%s", buf.String()) + } +} + +// TestAnExitWithoutAReleaserStaysAnExit: --vm off and every driver without +// plumbing must shut down exactly as before. func TestAnExitWithoutAReleaserStaysAnExit(t *testing.T) { var buf bytes.Buffer shutdownSweep(machine.Use(machine.Noop{}), false, &buf) if buf.Len() != 0 { - t.Errorf("a driver with no uplink produced output:\n%s", buf.String()) + t.Errorf("a driver with no plumbing produced output:\n%s", buf.String()) } } diff --git a/internal/cli/testdata/frozen/cli.json b/internal/cli/testdata/frozen/cli.json index 4b8446cf..ab3511ea 100644 --- a/internal/cli/testdata/frozen/cli.json +++ b/internal/cli/testdata/frozen/cli.json @@ -2978,6 +2978,206 @@ ] } } + }, + { + "schema_version": 19, + "content": { + "exit_codes": { + "drift": 2, + "error": 1, + "ok": 0 + }, + "verbs": { + "catalog": [ + "--format" + ], + "clean": [ + "--check", + "--closing", + "--doorstep", + "--force", + "--format", + "--vm" + ], + "corpus": [ + "--accepted", + "--against-cloud", + "--bind", + "--check", + "--credential", + "--dir", + "--dry-run", + "--endpoint", + "--file", + "--format", + "--mark-stale", + "--timeout" + ], + "coverage": [ + "--artefact", + "--axis", + "--baseline", + "--contract", + "--evidence", + "--fail-on-unknown", + "--format", + "--gaps", + "--observed", + "--products", + "--provider", + "--sdk", + "--write-baseline" + ], + "docs": [ + "--check", + "--client-pins", + "--confidence", + "--contracts", + "--coverage", + "--file", + "--install", + "--limits", + "--proved", + "--routes", + "--screenshots", + "--ui-manifest", + "--workflow" + ], + "doctor": [ + "--addr", + "--vm" + ], + "down": [ + "--file", + "--keep-emulator" + ], + "env": [ + "--client", + "--endpoint", + "--shell", + "--unset" + ], + "evidence": [ + "--allow-narrowing", + "--contracts", + "--endpoint", + "--join", + "--out", + "--reshape", + "--shapes", + "--suites" + ], + "images": [ + "--check", + "--only", + "--vm" + ], + "logs": [ + "--addr", + "-f", + "-n" + ], + "probe": [ + "--contracts", + "--endpoint", + "--provider" + ], + "proxy": [ + "--addr", + "--expose-to-network", + "--forward", + "--intercept", + "--max-body", + "--provider", + "--queue", + "--record", + "--upstream" + ], + "replay": [ + "--endpoint", + "--format", + "--refusals-only", + "--timeout" + ], + "restart": [ + "--addr", + "--timeout" + ], + "serve": [ + "--addr", + "--cleanup", + "--contracts", + "--coverage", + "--expose-to-network", + "--log-level", + "--shapes", + "--state", + "--vm" + ], + "shapes": [ + "--check", + "--dir", + "--dry-run", + "--profile", + "--provider", + "--record" + ], + "snapshot list": [ + "--format" + ], + "snapshot load": [ + "--addr" + ], + "snapshot save": [ + "--addr", + "--force" + ], + "start": [ + "--addr", + "--cleanup", + "--contracts", + "--detach", + "--foreground", + "--log-level", + "--state", + "--timeout", + "--vm" + ], + "status": [ + "--addr", + "--coverage", + "--format" + ], + "stop": [ + "--addr", + "--timeout" + ], + "transcript": [ + "--against", + "--contract", + "--format", + "--sanitise", + "--shape" + ], + "ui": [ + "--addr", + "--print" + ], + "up": [ + "--file", + "--no-iac", + "--runtime", + "--timeout" + ], + "version": [ + "--check" + ], + "wait": [ + "--addr", + "--timeout" + ] + } + } } ] } diff --git a/internal/core/machine/incus_balancer.go b/internal/core/machine/incus_balancer.go index 7e9ce36e..6f231db4 100644 --- a/internal/core/machine/incus_balancer.go +++ b/internal/core/machine/incus_balancer.go @@ -30,7 +30,7 @@ import ( // balancerDescription marks a balancer as the emulator's own. A load balancer // carries no user config that a sweep could key on, so the description is what -// ownership is read from — exactly as aclDescription is for a rule set. +// ownership is read from — exactly as FirewallDescription is for a rule set. const balancerDescription = "feint load balancer" type lbBackend struct { diff --git a/internal/core/machine/incus_firewall.go b/internal/core/machine/incus_firewall.go index 0ad936e6..6db3b839 100644 --- a/internal/core/machine/incus_firewall.go +++ b/internal/core/machine/incus_firewall.go @@ -29,9 +29,13 @@ import ( // A rule that names another group has to reach this layer already expanded into // blocks; there is nothing this driver can do about it. -// aclDescription marks a rule set as the emulator's own. Rule sets carry no +// FirewallDescription marks a rule set as the emulator's own. Rule sets carry no // user config, so this is what a sweep recognises them by. -const aclDescription = "feint security group" +// +// Exported because the report an operator reads names the mark each object was +// found by, and a report that names a mark the object does not carry cannot be +// used to decide what may be touched (internal/cli/clean_ledger.go). +const FirewallDescription = "feint security group" // mustOwnACL refuses to touch a rule set the emulator did not create. // @@ -57,7 +61,7 @@ func (d *Incus) mustOwnACL(ctx context.Context, name string) error { if err := json.Unmarshal(out, &acl); err != nil { return fmt.Errorf("read the description of firewall %s: %w", name, err) } - if !strings.HasPrefix(acl.Description, aclDescription) { + if !strings.HasPrefix(acl.Description, FirewallDescription) { return fmt.Errorf("firewall %s was not created by the emulator; refusing to delete it", name) } return nil @@ -117,7 +121,7 @@ func (d *Incus) EnsureFirewall(ctx context.Context, spec FirewallSpec) error { } body := aclBody{ - Description: aclDescription, + Description: FirewallDescription, Ingress: []aclRule{}, Egress: []aclRule{}, Config: map[string]string{}, diff --git a/internal/core/machine/incus_isolate.go b/internal/core/machine/incus_isolate.go index d081e87a..ebb91e1e 100644 --- a/internal/core/machine/incus_isolate.go +++ b/internal/core/machine/incus_isolate.go @@ -159,7 +159,7 @@ func (d *Incus) IsolateNetwork(ctx context.Context, network string, foreign []st // gets an immediate answer instead of a timeout, which is the difference // between an obvious topology mistake and a puzzling hang. body := aclBody{ - Description: aclDescription, + Description: FirewallDescription, Ingress: []aclRule{}, Egress: []aclRule{}, Config: map[string]string{}, diff --git a/internal/core/machine/incus_ovn.go b/internal/core/machine/incus_ovn.go index 0e9d21fd..377c5af8 100644 --- a/internal/core/machine/incus_ovn.go +++ b/internal/core/machine/incus_ovn.go @@ -1242,8 +1242,8 @@ func (d *Incus) dropUplinkRoutes(ctx context.Context, routes []string) error { return nil } -// ReleaseUplink implements UplinkReleaser: the graceful exit takes the one -// object no client's delete will ever remove (#521). Two green conformance +// releaseUplink is the uplink half of ReleasePlumbing: the graceful exit takes +// the object no client's delete will ever remove (#521). Two green conformance // runs each left `feint-uplink` standing — every resource had been deleted by // the clients that made it, the closing `feint stop` pruned nothing, and the // next run's own doorstep refused the host on exactly that network. @@ -1258,11 +1258,13 @@ func (d *Incus) dropUplinkRoutes(ctx context.Context, routes []string) error { // An uplink that networks still draw from stays, silently: those networks are // this run's leftovers, the doorstep and the sweep name them, and a release // that hid them behind a forced teardown would be the sweep this path -// deliberately is not. +// deliberately is not. It is also why ReleasePlumbing gives the default machine +// network back first — that network is what kept this one standing on the leg +// of 2026-08-28. // TestAShutdownReleaseTakesTheUnusedUplinkOfThisProcess fails without the // release; TestAReleaseNeverTouchesAnUplinkThisProcessDoesNotHold holds the // refusing half. -func (d *Incus) ReleaseUplink(ctx context.Context) (bool, error) { +func (d *Incus) releaseUplink(ctx context.Context) (bool, error) { if !d.OVN { return false, nil } diff --git a/internal/core/machine/incus_prune.go b/internal/core/machine/incus_prune.go index 38bca664..f52cca85 100644 --- a/internal/core/machine/incus_prune.go +++ b/internal/core/machine/incus_prune.go @@ -237,7 +237,7 @@ func (d *Incus) ownedACLs(ctx context.Context) []string { // to the description check. coreOwnedACL matches the emulator's own // network prefix (and the permissive posture set's fixed name) rather // than a bare iso-, which used to claim an operator's rule sets too. - if strings.HasPrefix(acl.Description, aclDescription) || coreOwnedACL(acl.Name) { + if strings.HasPrefix(acl.Description, FirewallDescription) || coreOwnedACL(acl.Name) { names = append(names, acl.Name) } } diff --git a/internal/core/machine/incus_release.go b/internal/core/machine/incus_release.go new file mode 100644 index 00000000..be96b32f --- /dev/null +++ b/internal/core/machine/incus_release.go @@ -0,0 +1,210 @@ +package machine + +import ( + "context" + "encoding/json" + "fmt" + "strings" +) + +// What a graceful exit gives back to the host, and why it is three objects and +// not one (#521, then the measurement of 2026-08-28). +// +// #521 named the family with one member: the uplink. Every emulated resource +// goes when a client deletes it, so a run whose clients cleaned up after +// themselves still left `feint-uplink` standing, and the next run's doorstep +// refused the host on it. The exit learned to release it. +// +// The incus-ovn leg of runtime-proof.yml then failed at the same doorstep with +// four objects rather than one, on a runner nothing had touched, and the +// reproduction on this station (2026-08-28) named them and their causes: +// +// feint-uplink host plumbing; released since #521, and it stayed +// because `fnt-default` still drew from it +// fnt-default host plumbing: DefaultMachineNetwork, created the first +// time a machine boots with no attachment of its own (an +// Outscale Vm outside a Net), owned by no resource, so no +// client's delete ever reaches it +// scw- the rule set of the Scaleway project's default security +// group — permissive, therefore attached to nothing +// (EnforcesNothing), and undeletable by a client: the API +// answers "the default security group of a project cannot +// be deleted". Its identifier is minted per run, so one +// such rule set accumulates on the host per session +// exo- the same for the Exoscale account's default group, whose +// identifier is fixed, so it survives rather than piles up +// +// The common property is what makes them one family and what makes releasing +// them honest rather than tidy: **no client call can remove any of them**, so +// leaving them measures nothing about the suites. That is exactly the sentence +// #521 wrote for the uplink, and it is why this is not `--cleanup`: a machine +// or a network a client was supposed to delete and did not is a leak, it stays, +// and the doorstep after the stop fails the run that leaked it. +// +// Everything here answers both questions of cli.go before it issues a +// destructive command — is the object the emulator's own, and is anything still +// drawing from it — because the second is what separates this from the +// `network unset security.acls` primitive an audit used to disarm a firewall. +// A machine left running by a stop without --cleanup keeps its rule set for +// exactly that reason. + +// ReleasePlumbing implements PlumbingReleaser. +// +// The order is the one the runtime imposes and is not cosmetic: a rule set +// attached to a network keeps that network alive, and a network drawing from +// the uplink keeps the uplink alive. Released the other way round, every step +// fails "in use" and the exit gives back nothing — which is precisely what was +// measured before this existed, `feint-uplink` refusing to go because +// `fnt-default` sat on it. +// +// Failures are collected rather than returned at the first one: a rule set the +// runtime will not part with must not stop the network behind it from going. +// TestAGracefulExitReleasesEveryPieceOfPlumbingItHolds fails without the two +// new members; TestTheReleaseOrderIsRuleSetsThenNetworkThenUplink fails when +// the order is reversed. +func (d *Incus) ReleasePlumbing(ctx context.Context) ([]string, error) { + var released []string + var failures []string + + sets, err := d.releaseUnusedRuleSets(ctx) + released = append(released, sets...) + if err != nil { + failures = append(failures, err.Error()) + } + + if gone, err := d.releaseDefaultNetwork(ctx); err != nil { + failures = append(failures, err.Error()) + } else if gone { + released = append(released, DefaultMachineNetwork) + } + + if gone, err := d.releaseUplink(ctx); err != nil { + failures = append(failures, err.Error()) + } else if gone { + released = append(released, d.uplinkName()) + } + + if len(failures) > 0 { + return released, fmt.Errorf("could not release %d piece(s) of plumbing: %s", + len(failures), strings.Join(failures, "; ")) + } + return released, nil +} + +// releaseUnusedRuleSets removes the rule sets this emulator wrote that nothing +// on the host references any more. +// +// Why they are the exit's business at all. A rule set is written for a security +// group, and a group a client deletes takes its rule set with it +// (GroupSync.Drop). What no client deletes is a *default* group: Scaleway +// refuses it by precondition, Exoscale's is seeded before any call, and both +// are what a machine wears when the client named no group — which is what every +// ssh suite does. Their rule sets therefore outlive every client and every run, +// and the store that gave them meaning dies with this process. +// +// Why `used_by` is read first rather than the delete simply being attempted. +// The runtime does refuse a delete of a referenced rule set ("Cannot delete an +// ACL that is in use", measured on this station on 2026-08-28), so the read is +// not what makes this safe. What it makes is the difference between asking a +// question and issuing a destructive command against a machine's live firewall +// and being saved by the answer. The in-use tolerance below stays as the race +// window's net: something may attach between the read and the delete. +// +// TestAReleaseKeepsTheRuleSetOfAMachineLeftRunning fails without the check. +func (d *Incus) releaseUnusedRuleSets(ctx context.Context) ([]string, error) { + out, err := d.run(ctx, "query", "/1.0/network-acls?recursion=1") + if err != nil { + // Could not look. Never an empty list on a failed read: "there is + // nothing to release" and "nobody could tell" are different facts, and + // reporting the first as the second is how an inventory once called a + // live account empty. + return nil, fmt.Errorf("list the rule sets to release: %w", err) + } + var acls []struct { + Name string `json:"name"` + Description string `json:"description"` + UsedBy []string `json:"used_by"` + } + if err := json.Unmarshal(out, &acls); err != nil { + return nil, fmt.Errorf("decode the rule sets to release: %w", err) + } + + var released []string + var failures []string + for _, acl := range acls { + // Ours, by the same two marks ownedACLs sweeps by: the description this + // driver writes, or the isolation names it derives. An operator's rule + // set carries neither and is never named here, whatever it is called. + if !strings.HasPrefix(acl.Description, FirewallDescription) && !coreOwnedACL(acl.Name) { + continue + } + if len(acl.UsedBy) > 0 { + continue + } + // RemoveFirewall rather than a bare delete, so ownership is re-derived + // at the moment of the command and not taken from the listing above: a + // name that made a round trip is an input, not a permission. + if err := d.RemoveFirewall(ctx, acl.Name); err != nil { + if strings.Contains(strings.ToLower(err.Error()), "in use") { + continue + } + failures = append(failures, fmt.Sprintf("rule set %s: %v", acl.Name, err)) + continue + } + released = append(released, acl.Name) + } + if len(failures) > 0 { + return released, fmt.Errorf("%s", strings.Join(failures, "; ")) + } + return released, nil +} + +// releaseDefaultNetwork removes DefaultMachineNetwork when it is this +// emulator's and nothing sits on it. +// +// No holder pid is recorded on this one, unlike the uplink, and `used_by` is +// what stands in for it: two emulators sharing a host share this network, and +// the one exiting first must not take it from under the other's machines. A +// machine attached to it makes `used_by` non-empty, which is the same fact a +// pid would have carried and one the runtime maintains itself. +// +// TestAReleaseLeavesTheDefaultNetworkAMachineStillSitsOn and +// TestAReleaseNeverTouchesAnUnlabelledNetworkUnderTheDefaultName fail without +// the two refusals; TestAGracefulExitReleasesEveryPieceOfPlumbingItHolds fails +// without the accepting half. +func (d *Incus) releaseDefaultNetwork(ctx context.Context) (bool, error) { + name := DefaultMachineNetwork + out, err := d.run(ctx, "query", "/1.0/networks/"+name) + if err != nil { + if isNotFound(err) { + return false, nil + } + return false, fmt.Errorf("inspect default network %s: %w", name, err) + } + var existing struct { + Config map[string]string `json:"config"` + UsedBy []string `json:"used_by"` + } + if err := json.Unmarshal(out, &existing); err != nil { + return false, fmt.Errorf("decode default network %s: %w", name, err) + } + if existing.Config["user."+LabelKey] == "" { + // An operator's own network under this name. ensureDefaultNetwork + // already refuses to reuse one; the exit refuses to remove one, which + // is the same refusal read backwards. + return false, nil + } + if len(existing.UsedBy) > 0 { + return false, nil + } + if _, err := d.run(ctx, "network", "delete", name); err != nil { + if isNotFound(err) { + return false, nil + } + if strings.Contains(strings.ToLower(err.Error()), "in use") { + return false, nil + } + return false, fmt.Errorf("release default network %s: %w", name, err) + } + return true, nil +} diff --git a/internal/core/machine/incus_release_test.go b/internal/core/machine/incus_release_test.go new file mode 100644 index 00000000..518962cd --- /dev/null +++ b/internal/core/machine/incus_release_test.go @@ -0,0 +1,267 @@ +package machine + +import ( + "context" + "errors" + "os" + "strconv" + "strings" + "testing" +) + +// The exit half of #521, one measurement later. +// +// On 2026-08-28 the incus-ovn leg of runtime-proof.yml failed at the doorstep +// of its own witness gate, on a runner nothing had touched, naming four +// objects the leg itself had left: `feint-uplink`, `fnt-default` and the rule +// sets of two providers' default security groups. The station reproduced it +// exactly. None of the four is removable by any client call, which is the +// property that makes releasing them honest rather than tidy — and the +// refusing halves matter as much, because an exit that deleted a rule set a +// machine still wears would be the audit's own firewall-disarming primitive +// with a friendlier name. + +// releaseHost is the shape of a host holding one of everything this release +// gives back. Each answer is keyed on a fragment that matches exactly one +// command, because fakeRuntime scans its map in random order. +func releaseHost(aclUsedBy, defaultUsedBy string, defaultConfig string) map[string]string { + self := strconv.Itoa(os.Getpid()) + if defaultConfig == "" { + defaultConfig = `"user.` + LabelKey + `":"feint"` + } + return map[string]string{ + "network-acls?recursion=1": `[ + {"name":"acltest","description":"","used_by":[]}, + {"name":"scw-aaa","description":"feint security group","used_by":[` + aclUsedBy + `]}, + {"name":"exo-bbb","description":"feint security group","used_by":[]} + ]`, + "network-acls/scw-aaa": `{"description":"feint security group"}`, + "network-acls/exo-bbb": `{"description":"feint security group"}`, + "networks/fnt-default": `{"type":"ovn","config":{` + defaultConfig + `},"used_by":[` + defaultUsedBy + `]}`, + "networks/feint-uplink": ourUplinkJSON(self, ""), + "network-acls/probe-none": `{"description":""}`, + } +} + +// TestAGracefulExitReleasesEveryPieceOfPlumbingItHolds is the accepting half, +// and it is the one that fails on the code as it stood on 2026-08-28: the exit +// released the uplink alone, and the uplink is precisely the object that could +// not go while `fnt-default` still drew from it. +func TestAGracefulExitReleasesEveryPieceOfPlumbingItHolds(t *testing.T) { + f := &fakeRuntime{answers: releaseHost("", "", "")} + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("release: %v", err) + } + for _, want := range []string{"scw-aaa", "exo-bbb", DefaultMachineNetwork, "feint-uplink"} { + if !containsName(released, want) { + t.Errorf("the exit did not give back %s; the next run's doorstep refuses exactly that\nreleased: %v\ncommands:\n%s", + want, released, strings.Join(f.commands(), "\n")) + } + } +} + +// TestTheReleaseOrderIsRuleSetsThenNetworkThenUplink holds the order the +// runtime imposes. A rule set keeps its network alive and a network keeps the +// uplink alive, so released the other way round every step answers "in use" +// and the host keeps all three — which is the state the leg was measured in. +func TestTheReleaseOrderIsRuleSetsThenNetworkThenUplink(t *testing.T) { + f := &fakeRuntime{answers: releaseHost("", "", "")} + d := newFakeDriver(f) + d.OVN = true + + if _, err := d.ReleasePlumbing(context.Background()); err != nil { + t.Fatalf("release: %v", err) + } + acl := indexOf(f.commands(), "network acl delete exo-bbb") + network := indexOf(f.commands(), "network delete "+DefaultMachineNetwork) + uplink := indexOf(f.commands(), "network delete feint-uplink") + if acl < 0 || network < 0 || uplink < 0 { + t.Fatalf("one of the three deletes never happened:\n%s", strings.Join(f.commands(), "\n")) + } + if acl >= network || network >= uplink { + t.Fatalf("the release order is rule sets, then the default network, then the uplink; got acl=%d network=%d uplink=%d:\n%s", + acl, network, uplink, strings.Join(f.commands(), "\n")) + } +} + +// TestAReleaseKeepsTheRuleSetOfAMachineLeftRunning is the refusal that keeps +// this from being a firewall teardown. `feint stop` without --cleanup leaves +// the run's machines up on purpose; a machine still wearing a rule set makes +// the runtime report it in use, and that rule set must not see a delete at all. +func TestAReleaseKeepsTheRuleSetOfAMachineLeftRunning(t *testing.T) { + f := &fakeRuntime{answers: releaseHost(`"/1.0/instances/feint-scw-x"`, "", "")} + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("release: %v", err) + } + if containsName(released, "scw-aaa") { + t.Fatal("the release claims a rule set a running machine still wears") + } + if deletes := f.matching("network acl delete scw-aaa"); len(deletes) != 0 { + t.Fatalf("a delete was issued against the rule set of a machine left running:\n%s", + strings.Join(f.commands(), "\n")) + } +} + +// TestAReleaseNeverTouchesARuleSetTheEmulatorDidNotWrite: an operator's rule +// set carries neither the description this driver writes nor a name it +// derives, and an unused one is exactly the shape a sweep is tempted by. +func TestAReleaseNeverTouchesARuleSetTheEmulatorDidNotWrite(t *testing.T) { + f := &fakeRuntime{answers: releaseHost("", "", "")} + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("release: %v", err) + } + if containsName(released, "acltest") { + t.Fatal("the release claims a rule set the operator created") + } + if deletes := f.matching("acltest"); len(deletes) != 0 { + t.Fatalf("a command was issued against an operator's rule set:\n%s", + strings.Join(f.commands(), "\n")) + } +} + +// TestAReleaseLeavesTheDefaultNetworkAMachineStillSitsOn: two emulators share +// one host and one default network, and `used_by` is what a holder pid would +// have said. The one exiting first must not take it from under the other's +// machines. +func TestAReleaseLeavesTheDefaultNetworkAMachineStillSitsOn(t *testing.T) { + f := &fakeRuntime{answers: releaseHost("", `"/1.0/instances/feint-osc-y"`, "")} + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("release: %v", err) + } + if containsName(released, DefaultMachineNetwork) { + t.Fatal("the release claims a default network a machine still sits on") + } + if deletes := f.matching("network delete " + DefaultMachineNetwork); len(deletes) != 0 { + t.Fatalf("a delete was issued against a default network still in use:\n%s", + strings.Join(f.commands(), "\n")) + } +} + +// TestAReleaseNeverTouchesAnUnlabelledNetworkUnderTheDefaultName: the name is +// short and ordinary enough for an operator to have typed it, and +// ensureDefaultNetwork already refuses to reuse one. This is that refusal read +// backwards, on the destructive side where it costs more. +func TestAReleaseNeverTouchesAnUnlabelledNetworkUnderTheDefaultName(t *testing.T) { + f := &fakeRuntime{answers: releaseHost("", "", `"ipv4.address":"10.0.0.1/24"`)} + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("release: %v", err) + } + if containsName(released, DefaultMachineNetwork) { + t.Fatal("the release claims a network the emulator never labelled") + } + if deletes := f.matching("network delete " + DefaultMachineNetwork); len(deletes) != 0 { + t.Fatalf("a delete was issued against an operator's own network:\n%s", + strings.Join(f.commands(), "\n")) + } +} + +// TestAReleaseThatCouldNotLookSaysSoAndKeepsGoing: a listing that failed is not +// an empty host, and it must not stop the network and the uplink behind it from +// going — one stuck rule set keeping the uplink standing is the shape this +// whole file exists to end. +func TestAReleaseThatCouldNotLookSaysSoAndKeepsGoing(t *testing.T) { + f := &fakeRuntime{ + answers: releaseHost("", "", ""), + fail: map[string]error{ + "network-acls?recursion=1": errors.New("connection refused"), + }, + } + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err == nil { + t.Fatal("a rule-set listing that failed was reported as a host with nothing to release") + } + if !containsName(released, DefaultMachineNetwork) || !containsName(released, "feint-uplink") { + t.Fatalf("a failed rule-set listing stopped the rest of the release: %v", released) + } +} + +// TestAReleaseAsksNothingOfARuntimeWithNoPlumbing: everything absent is the +// state this release exists to reach, and reaching it must be silent rather +// than an error — `serve --cleanup` already pruned the lot. +func TestAReleaseAsksNothingOfARuntimeWithNoPlumbing(t *testing.T) { + f := &fakeRuntime{ + answers: map[string]string{"network-acls?recursion=1": `[]`}, + fail: map[string]error{ + "networks/fnt-default": errors.New("Error: Network not found"), + "networks/feint-uplink": errors.New("Error: Network not found"), + }, + } + d := newFakeDriver(f) + d.OVN = true + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("an empty host is the outcome asked for, not an error: %v", err) + } + if len(released) != 0 { + t.Fatalf("the release claims objects that were never there: %v", released) + } + if deletes := f.matching("delete"); len(deletes) != 0 { + t.Fatalf("a delete was issued on a host holding nothing:\n%s", strings.Join(f.commands(), "\n")) + } +} + +func containsName(names []string, want string) bool { + for _, name := range names { + if name == want { + return true + } + } + return false +} + +func indexOf(commands []string, want string) int { + for i, cmd := range commands { + if cmd == want { + return i + } + } + return -1 +} + +// TestAReleaseGivesBackTheDefaultNetworkUnderABridgeToo: the default machine +// network exists in both modes — ensureDefaultNetwork creates a managed bridge +// when the driver is not OVN — so the bridged leg leaks it exactly as the OVN +// one did. Only the uplink is OVN's alone. +func TestAReleaseGivesBackTheDefaultNetworkUnderABridgeToo(t *testing.T) { + f := &fakeRuntime{answers: releaseHost("", "", "")} + d := newFakeDriver(f) + + released, err := d.ReleasePlumbing(context.Background()) + if err != nil { + t.Fatalf("release: %v", err) + } + if !containsName(released, DefaultMachineNetwork) { + t.Errorf("the bridged exit kept the default network: %v", released) + } + if containsName(released, "feint-uplink") { + t.Error("a bridged runtime has no uplink, and the release claims one") + } + if asked := f.matching("networks/feint-uplink"); len(asked) != 0 { + t.Errorf("bridge mode asked the runtime about an uplink it does not have:\n%s", + strings.Join(f.commands(), "\n")) + } +} diff --git a/internal/core/machine/incus_uplink_release_test.go b/internal/core/machine/incus_uplink_release_test.go index a816c689..178c20fb 100644 --- a/internal/core/machine/incus_uplink_release_test.go +++ b/internal/core/machine/incus_uplink_release_test.go @@ -27,7 +27,7 @@ func TestAShutdownReleaseTakesTheUnusedUplinkOfThisProcess(t *testing.T) { d := newFakeDriver(f) d.OVN = true - released, err := d.ReleaseUplink(context.Background()) + released, err := d.releaseUplink(context.Background()) if err != nil { t.Fatalf("release: %v", err) } @@ -52,7 +52,7 @@ func TestAReleaseLeavesAnUplinkStillInUse(t *testing.T) { d := newFakeDriver(f) d.OVN = true - released, err := d.ReleaseUplink(context.Background()) + released, err := d.releaseUplink(context.Background()) if err != nil { t.Fatalf("an uplink still in use is the outcome asked for, not an error: %v", err) } @@ -82,7 +82,7 @@ func TestAReleaseNeverTouchesAnUplinkThisProcessDoesNotHold(t *testing.T) { d := newFakeDriver(f) d.OVN = true - released, err := d.ReleaseUplink(context.Background()) + released, err := d.releaseUplink(context.Background()) if err != nil { t.Fatalf("%s: release: %v", name, err) } @@ -102,7 +102,7 @@ func TestAReleaseDoesNothingOffOVN(t *testing.T) { f := &fakeRuntime{} d := newFakeDriver(f) - released, err := d.ReleaseUplink(context.Background()) + released, err := d.releaseUplink(context.Background()) if err != nil || released { t.Fatalf("released=%v err=%v off OVN, want false and nil", released, err) } @@ -122,7 +122,7 @@ func TestAReleaseTreatsAMissingUplinkAsAlreadyGone(t *testing.T) { d := newFakeDriver(f) d.OVN = true - released, err := d.ReleaseUplink(context.Background()) + released, err := d.releaseUplink(context.Background()) if err != nil { t.Fatalf("a missing uplink is the outcome asked for, not an error: %v", err) } diff --git a/internal/core/machine/prune.go b/internal/core/machine/prune.go index a81e1e4e..e54b347a 100644 --- a/internal/core/machine/prune.go +++ b/internal/core/machine/prune.go @@ -59,20 +59,33 @@ type Surveyor interface { Survey(ctx context.Context) (Leftovers, error) } -// UplinkReleaser is the optional half of a driver whose networks share one -// piece of host plumbing no resource delete will ever remove: the uplink. -// Every emulated resource goes when a client deletes it, so a run whose -// clients cleaned up after themselves still leaves exactly one labelled -// network standing — and the doorstep of the next run refuses exactly that -// (#521, measured twice after two green conformance runs). The process that -// set the uplink up is the one that takes it down, on its way out. -type UplinkReleaser interface { - // ReleaseUplink removes the uplink when, and only when, this process is - // the one holding it and nothing draws from it any more. It reports - // whether the uplink went; an uplink another run left, one an operator - // named, or one that networks still sit on is left standing, so the sweep - // and the doorstep keep naming what this path must not hide. - ReleaseUplink(ctx context.Context) (bool, error) +// PlumbingReleaser is the optional half of a driver that creates host objects +// no emulated resource owns and no client's delete will ever reach. Every +// emulated resource goes when a client deletes it, so a run whose clients +// cleaned up after themselves still leaves those standing — and the doorstep +// of the next run refuses exactly that (#521, measured twice after two green +// conformance runs, and again on 2026-08-28 with three more objects). The +// process that set them up is the one that takes them down, on its way out. +// +// Which objects those are is the driver's own knowledge and is deliberately +// not spelled here; what is shared is the rule. On the Incus driver they are +// the uplink every OVN network draws from, the default network a machine with +// no attachment boots on (DefaultMachineNetwork), and the rule sets it wrote +// for security groups a client cannot delete — a provider's default group +// exists before any client call and outlives every one of them, so its rule +// set is nobody's to remove but this process's. +type PlumbingReleaser interface { + // ReleasePlumbing removes those objects, when and only when this process + // is the one holding them and nothing on the host draws from them any + // more. It names what went, in the order it went, so a graceful exit can + // say it out loud. + // + // What it must never do is force. An object another run left, one an + // operator named, or one something still sits on is left standing, so the + // sweep and the doorstep keep naming what this path must not hide: a + // release that tore down a machine's live rule set would be the audit's + // own `network unset security.acls` primitive, dressed as tidiness. + ReleasePlumbing(ctx context.Context) ([]string, error) } // A leftover the sweep can still remove is untidy. A leftover no ordinary diff --git a/internal/core/machine/runtime.go b/internal/core/machine/runtime.go index a67c9b56..24f5ac4b 100644 --- a/internal/core/machine/runtime.go +++ b/internal/core/machine/runtime.go @@ -165,15 +165,16 @@ func (r Runtime) Prune(ctx context.Context) (pruned Pruned, asked bool, err erro return pruned, true, err } -// ReleaseUplink gives back the host plumbing no resource delete removes, when -// and only when this process is the one holding it. asked is false for a -// runtime with no uplink to give back. -func (r Runtime) ReleaseUplink(ctx context.Context) (released, asked bool, err error) { - u, ok := r.backing().(UplinkReleaser) +// ReleasePlumbing gives back the host objects no resource delete removes, when +// and only when this process is the one holding them and nothing draws from +// them. It names what went. asked is false for a runtime with no plumbing to +// give back. +func (r Runtime) ReleasePlumbing(ctx context.Context) (released []string, asked bool, err error) { + u, ok := r.backing().(PlumbingReleaser) if !ok { - return false, false, nil + return nil, false, nil } - released, err = u.ReleaseUplink(ctx) + released, err = u.ReleasePlumbing(ctx) return released, true, err } diff --git a/mise.toml b/mise.toml index c1c224d1..b7fdbd91 100644 --- a/mise.toml +++ b/mise.toml @@ -631,7 +631,7 @@ FEINT_FIELD_GATE=1 tools/conformance/score.sh "http://$FEINT_ADDR" # that leaks now fails the run that leaked, not the next one. # TestTheConformanceTaskEndsOnItsOwnDoorstep fails without these two lines. ./feint stop --addr $FEINT_ADDR -tools/conformance/guard.sh leftovers "${FEINT_VM:-off}" +tools/conformance/guard.sh leftovers-after "${FEINT_VM:-off}" """ [tasks.falsify] diff --git a/tools/conformance/guard.sh b/tools/conformance/guard.sh index cfec27c3..d934cf2d 100755 --- a/tools/conformance/guard.sh +++ b/tools/conformance/guard.sh @@ -242,12 +242,25 @@ guard_leftovers() { # guard_leftovers_for is the same refusal for a caller that has no emulator to # ask yet: `mise run conformance` runs it before it starts anything, from the -# mode it is about to pass to `feint start`. +# mode it is about to pass to `feint start`, and again once that emulator has +# stopped. +# +# Three scopes, and the third exists because of a sentence rather than a +# verdict. `doorstep` and `closing` refuse identically; what differs is who is +# named. On 2026-08-28 the incus-ovn leg of runtime-proof.yml failed on a +# GitHub runner nothing had ever touched with "this host still holds what an +# earlier run left", about four objects its own ssh suites had made eight steps +# earlier. There was no earlier run. A reader who believed it would have gone +# looking at the schedule instead of at the suites, and that reader is who this +# distinction is for. guard_leftovers_for() { - local machines="${1:-off}" scope="${2:-inflight}" binary doorstep="" - # Only the caller that runs before `feint start` may ask what an earlier run + local machines="${1:-off}" scope="${2:-inflight}" binary moment="" + # Only the callers that run outside a serving emulator may ask what a run # left standing: see guard_leftovers above for what happens otherwise. - [ "$scope" = "doorstep" ] && doorstep="--doorstep" + case "$scope" in + doorstep) moment="--doorstep" ;; + closing) moment="--closing" ;; + esac # With no machine runtime nothing will take an address block, so the question # does not apply. Said out loud rather than returned in silence, per the rule @@ -274,7 +287,26 @@ EOF exit 1 fi - if ! "$binary" clean --check $doorstep --vm "$machines" >&2; then + if ! "$binary" clean --check $moment --vm "$machines" >&2; then + if [ "$scope" = "closing" ]; then + cat >&2 <&2 <" >&2 + echo "usage: tools/conformance/guard.sh leftovers (before a run starts)" >&2 + echo " tools/conformance/guard.sh leftovers-after (after its emulator stopped)" >&2 echo " (the other guards take an endpoint and are sourced, not run)" >&2 exit 2 ;; esac diff --git a/tools/conformance/guard_test.go b/tools/conformance/guard_test.go index 71665eb4..c5b8933a 100644 --- a/tools/conformance/guard_test.go +++ b/tools/conformance/guard_test.go @@ -369,8 +369,13 @@ func TestTheConformanceTaskEndsOnItsOwnDoorstep(t *testing.T) { if trimmed == "./feint stop --addr $FEINT_ADDR" { stopAt = i } + // `leftovers-after`, not `leftovers`: the same refusal, and the + // sentence that names this run rather than a previous one. The + // doorstep spelling here would print "a previous run left…" about + // objects the task had just made, which is exactly what the incus-ovn + // leg of runtime-proof.yml printed on a runner nothing had touched. if stopAt >= 0 && i > stopAt && - strings.HasPrefix(trimmed, "tools/conformance/guard.sh leftovers ") { + strings.HasPrefix(trimmed, "tools/conformance/guard.sh leftovers-after ") { askedAfterStop = true } } @@ -379,8 +384,9 @@ func TestTheConformanceTaskEndsOnItsOwnDoorstep(t *testing.T) { "can ask the doorstep question after the run — the residue waits for the next run's refusal (#521)") } if !askedAfterStop { - t.Error("the conformance task never re-asks the doorstep after its stop: a green run may " + - "leave what its own doorstep refuses, and the next run pays for it (#521)") + t.Error("the conformance task never re-asks the doorstep after its stop, in its closing " + + "form: a green run may leave what its own doorstep refuses, and the next run pays " + + "for it and is blamed for it (#521)") } } @@ -456,4 +462,47 @@ func TestOnlyTheDoorstepAsksWhatAnEarlierRunLeft(t *testing.T) { if !strings.Contains(doorstep, "--doorstep") { t.Errorf("the doorstep never asked what an earlier run left, which is the whole of #426:\n%s", doorstep) } + + // And the closing form, which asks the same question of the same runtime + // and must name this run rather than a previous one. Both flags at once is + // refused by the binary itself (internal/cli's + // TestTheTwoMomentsCannotBeAskedAtOnce), so reading which one was passed is + // reading the whole answer. + code, closing := bashGuard(t, `. "$1"; guard_leftovers_for "$2" closing`, "incus", stubBinary(t, 0)) + if code != 0 { + t.Fatalf("the closing check refused a host the stub reports clean (exit %d):\n%s", code, closing) + } + if !strings.Contains(closing, "--closing") { + t.Errorf("the closing check asked the doorstep's question, so a leg that leaks would blame "+ + "a run that never existed:\n%s", closing) + } + if strings.Contains(closing, "--doorstep") { + t.Errorf("the closing check still carries --doorstep:\n%s", closing) + } +} + +// TestTheClosingRefusalBlamesThisRunAndNotAnEarlierOne is the sentence half. +// The flag above decides the binary's report; this decides the shell's, and it +// is the one an operator reads first in a job log. +func TestTheClosingRefusalBlamesThisRunAndNotAnEarlierOne(t *testing.T) { + code, closing := bashGuard(t, `. "$1"; guard_leftovers_for "$2" closing`, "incus", stubBinary(t, 1)) + if code == 0 { + t.Fatalf("the closing check passed on a host the stub reports dirty:\n%s", closing) + } + if strings.Contains(closing, "an earlier run") { + t.Errorf("the closing refusal blames an earlier run for what this one left:\n%s", closing) + } + if !strings.Contains(closing, "this run left") { + t.Errorf("the closing refusal never names whose leak it found:\n%s", closing) + } + + // The witness: the doorstep must keep its own sentence, or this passes on + // code that renamed both. + code, doorstep := bashGuard(t, `. "$1"; guard_leftovers_for "$2" doorstep`, "incus", stubBinary(t, 1)) + if code == 0 { + t.Fatalf("the doorstep passed on a host the stub reports dirty:\n%s", doorstep) + } + if !strings.Contains(doorstep, "an earlier run") { + t.Errorf("the doorstep stopped naming the earlier run it exists to name:\n%s", doorstep) + } } diff --git a/tools/conformance/leg.sh b/tools/conformance/leg.sh index c273ee0e..4ba26910 100755 --- a/tools/conformance/leg.sh +++ b/tools/conformance/leg.sh @@ -166,4 +166,4 @@ fi # leg that left a machine or a network behind is the leg that fails. The trap # above stays the safety net for a leg that dies mid-flight. ./feint stop --addr "$addr" -tools/conformance/guard.sh leftovers "$vm" +tools/conformance/guard.sh leftovers-after "$vm" diff --git a/tools/conformance/runtimeproofclose_test.go b/tools/conformance/runtimeproofclose_test.go new file mode 100644 index 00000000..12da655d --- /dev/null +++ b/tools/conformance/runtimeproofclose_test.go @@ -0,0 +1,86 @@ +package conformance + +import ( + "path/filepath" + "strings" + "testing" +) + +// The runtime proof ends on the host state its own doorstep accepts (#521). +// +// It did not, and it is what the incus-ovn leg failed on both nights it got far +// enough to be asked. The leg's suites left four objects on the host, `feint +// stop` gave none of them back, and the *next* step — the witness gate, whose +// subject is something else entirely — met them at its own doorstep and +// reported "a previous run left …" on a GitHub runner nothing had ever touched. +// Two defects in one line: the run that leaked was not the run that went red, +// and the sentence named a run that never existed. +// +// `mise run conformance` and `tools/conformance/leg.sh` had carried the fixed +// form since #521 — stop on a line of its own, then the question — and the +// workflow had not. This holds all three to it, and holds the workflow to the +// closing spelling in particular: `leftovers`, the doorstep form, would refuse +// on exactly the same objects and blame the wrong run for them, which is a +// green-looking half-fix. +func TestTheRuntimeProofEndsOnItsOwnDoorstep(t *testing.T) { + root := repoRoot(t) + body := readFile(t, filepath.Join(root, ".github", "workflows", "runtime-proof.yml")) + lines := strings.Split(body, "\n") + + stopAt, askedAt := -1, -1 + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "run:") && strings.Contains(trimmed, "feint stop --addr 127.0.0.1:4599") { + stopAt = i + } + if stopAt >= 0 && i > stopAt && strings.Contains(trimmed, "guard.sh leftovers-after") { + askedAt = i + break + } + } + if stopAt < 0 { + t.Fatal("the runtime proof never stops its emulator on a step of its own, so nothing can " + + "ask what the leg left once the emulator is gone (#521)") + } + if askedAt < 0 { + t.Fatalf("the runtime proof never asks the closing doorstep after its stop: whatever its "+ + "suites leave is met by the next step instead, which reports it as a previous run's "+ + "and reddens the wrong subject. It stops at line %d and asks nothing after it.", stopAt+1) + } + + // And the step must be able to go red on the path it judges. `if: always()` + // here would fire on a leg that had already failed, where the leftovers are + // the wreck of a crash and not a leak, and bury the finding under noise — + // the same reason `mise run conformance` puts this after its trap rather + // than inside it. + if condition := stepConditionAbove(lines, askedAt); condition != "" { + t.Errorf("the closing doorstep carries `%s`: it then reports a failed leg's wreckage as a "+ + "leak, and the run that really leaked is buried under a second red", condition) + } + + // The doorstep spelling at the same position would be the half-fix: same + // refusal, wrong culprit named. + for i := stopAt; i < askedAt; i++ { + trimmed := strings.TrimSpace(lines[i]) + if strings.Contains(trimmed, "guard.sh leftovers ") { + t.Errorf("line %d asks the doorstep form after the stop, which blames a previous run "+ + "for what this leg left: %s", i+1, trimmed) + } + } +} + +// stepConditionAbove returns the `if:` of the step containing the given line, +// or "" when the step carries none. Steps start at a `- name:` entry, so the +// search walks back to it. +func stepConditionAbove(lines []string, at int) string { + for i := at; i >= 0; i-- { + trimmed := strings.TrimSpace(lines[i]) + if strings.HasPrefix(trimmed, "if:") { + return trimmed + } + if strings.HasPrefix(trimmed, "- name:") { + return "" + } + } + return "" +} diff --git a/tools/falsify/specs/a-run-ends-where-it-could-start.json b/tools/falsify/specs/a-run-ends-where-it-could-start.json index 3dee983a..72e638fa 100644 --- a/tools/falsify/specs/a-run-ends-where-it-could-start.json +++ b/tools/falsify/specs/a-run-ends-where-it-could-start.json @@ -31,29 +31,142 @@ "test": "TestAShutdownReleaseTakesTheUnusedUplinkOfThisProcess" }, { - "label": "serve stops asking the driver on its way out, so the uplink outlives every graceful stop again and the next run's doorstep refuses the host", + "label": "serve stops asking the driver on its way out, so the uplink, the default machine network and every unused rule set outlive each graceful stop and the closing doorstep refuses the host", "file": "internal/cli/cli.go", - "find": "\tif released, asked, err := rt.ReleaseUplink(context.Background()); asked {", - "replace": "\tif released, asked, err := rt.ReleaseUplink(context.Background()); asked && false {", + "find": "\tif released, asked, err := rt.ReleasePlumbing(context.Background()); asked {", + "replace": "\tif released, asked, err := rt.ReleasePlumbing(context.Background()); asked && false {", "test": "TestAGracefulExitReleasesTheUplink", "package": "./internal/cli/" }, { "label": "the conformance task stops re-asking the doorstep after its stop, so a green run may again leave what its own doorstep refuses and the next run pays for it", "file": "mise.toml", - "find": "./feint stop --addr $FEINT_ADDR\ntools/conformance/guard.sh leftovers \"${FEINT_VM:-off}\"", - "replace": "./feint stop --addr $FEINT_ADDR\ntrue || tools/conformance/guard.sh leftovers \"${FEINT_VM:-off}\"", + "find": "./feint stop --addr $FEINT_ADDR\ntools/conformance/guard.sh leftovers-after \"${FEINT_VM:-off}\"", + "replace": "./feint stop --addr $FEINT_ADDR\ntrue || tools/conformance/guard.sh leftovers-after \"${FEINT_VM:-off}\"", "test": "TestTheConformanceTaskEndsOnItsOwnDoorstep", "package": "./tools/conformance/" }, { "label": "the explicit stop goes back into the trap alone, where the doorstep cannot follow it — bash keeps the pre-trap exit status, so a red doorstep there reddens nothing", "file": "mise.toml", - "find": "./feint stop --addr $FEINT_ADDR\ntools/conformance/guard.sh leftovers \"${FEINT_VM:-off}\"", - "replace": "true || ./feint stop --addr $FEINT_ADDR\ntools/conformance/guard.sh leftovers \"${FEINT_VM:-off}\"", + "find": "./feint stop --addr $FEINT_ADDR\ntools/conformance/guard.sh leftovers-after \"${FEINT_VM:-off}\"", + "replace": "true || ./feint stop --addr $FEINT_ADDR\ntools/conformance/guard.sh leftovers-after \"${FEINT_VM:-off}\"", "test": "TestTheConformanceTaskEndsOnItsOwnDoorstep", "package": "./tools/conformance/" + }, + { + "label": "the exit counts what it gave back instead of naming it, so an operator reading \"released 3 object(s)\" cannot tell which three, and the next doorstep's silence explains nothing", + "file": "internal/cli/cli.go", + "find": "\t\t\tfmt.Fprintf(stdout, \"released the plumbing this run held: %s\\n\",\n\t\t\t\tstrings.Join(released, \", \"))", + "replace": "\t\t\tfmt.Fprintf(stdout, \"released %d piece(s) of plumbing\\n\",\n\t\t\t\tlen(strings.Join(released, \", \")))", + "test": "TestAGracefulExitNamesEveryPieceOfPlumbingItGaveBack", + "package": "./internal/cli/" + }, + { + "label": "the exit stops giving back the default machine network, which is the object that kept the uplink standing on the leg of 2026-08-28 — one release short and both networks survive", + "file": "internal/core/machine/incus_release.go", + "find": "func (d *Incus) releaseDefaultNetwork(ctx context.Context) (bool, error) {\n\tname := DefaultMachineNetwork", + "replace": "func (d *Incus) releaseDefaultNetwork(ctx context.Context) (bool, error) {\n\tif true {\n\t\treturn false, nil\n\t}\n\tname := DefaultMachineNetwork", + "test": "TestAGracefulExitReleasesEveryPieceOfPlumbingItHolds" + }, + { + "label": "the exit stops giving back the rule sets nothing uses, so a default security group no client can delete leaves one host ACL per session behind it", + "file": "internal/core/machine/incus_release.go", + "find": "func (d *Incus) releaseUnusedRuleSets(ctx context.Context) ([]string, error) {\n\tout, err := d.run(ctx, \"query\", \"/1.0/network-acls?recursion=1\")", + "replace": "func (d *Incus) releaseUnusedRuleSets(ctx context.Context) ([]string, error) {\n\tif true {\n\t\treturn nil, nil\n\t}\n\tout, err := d.run(ctx, \"query\", \"/1.0/network-acls?recursion=1\")", + "test": "TestAGracefulExitReleasesEveryPieceOfPlumbingItHolds" + }, + { + "label": "the release takes the uplink before the network that draws from it, so the runtime answers \"in use\" and the exit gives back nothing — the measured state, reintroduced by an ordering nobody would call a defect", + "file": "internal/core/machine/incus_release.go", + "find": "\tif gone, err := d.releaseDefaultNetwork(ctx); err != nil {\n\t\tfailures = append(failures, err.Error())\n\t} else if gone {\n\t\treleased = append(released, DefaultMachineNetwork)\n\t}\n\n\tif gone, err := d.releaseUplink(ctx); err != nil {\n\t\tfailures = append(failures, err.Error())\n\t} else if gone {\n\t\treleased = append(released, d.uplinkName())\n\t}", + "replace": "\tif gone, err := d.releaseUplink(ctx); err != nil {\n\t\tfailures = append(failures, err.Error())\n\t} else if gone {\n\t\treleased = append(released, d.uplinkName())\n\t}\n\n\tif gone, err := d.releaseDefaultNetwork(ctx); err != nil {\n\t\tfailures = append(failures, err.Error())\n\t} else if gone {\n\t\treleased = append(released, DefaultMachineNetwork)\n\t}", + "test": "TestTheReleaseOrderIsRuleSetsThenNetworkThenUplink" + }, + { + "label": "the release stops asking whether anything sits on the default network, so the emulator exiting first takes it from under a second one's machines", + "file": "internal/core/machine/incus_release.go", + "find": "\tif len(existing.UsedBy) > 0 {\n\t\treturn false, nil\n\t}", + "replace": "\tif false && len(existing.UsedBy) > 0 {\n\t\treturn false, nil\n\t}", + "test": "TestAReleaseLeavesTheDefaultNetworkAMachineStillSitsOn" + }, + { + "label": "the release stops reading the label on the default network, so an operator's own network under that short and ordinary name is deleted on the way out", + "file": "internal/core/machine/incus_release.go", + "find": "\tif existing.Config[\"user.\"+LabelKey] == \"\" {", + "replace": "\tif false && existing.Config[\"user.\"+LabelKey] == \"\" {", + "test": "TestAReleaseNeverTouchesAnUnlabelledNetworkUnderTheDefaultName" + }, + { + "label": "the release stops asking whether a rule set is in use, so a stop without --cleanup tears the firewall off the machines it deliberately left running — the audit's own `network unset security.acls`, dressed as tidiness", + "file": "internal/core/machine/incus_release.go", + "find": "\t\tif len(acl.UsedBy) > 0 {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tif false && len(acl.UsedBy) > 0 {\n\t\t\tcontinue\n\t\t}", + "test": "TestAReleaseKeepsTheRuleSetOfAMachineLeftRunning" + }, + { + "label": "the release stops asking whether a rule set is ours, so an operator's own ACL is named and reached for on every graceful exit", + "file": "internal/core/machine/incus_release.go", + "find": "\t\tif !strings.HasPrefix(acl.Description, FirewallDescription) && !coreOwnedACL(acl.Name) {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tif false && !strings.HasPrefix(acl.Description, FirewallDescription) && !coreOwnedACL(acl.Name) {\n\t\t\tcontinue\n\t\t}", + "test": "TestAReleaseNeverTouchesARuleSetTheEmulatorDidNotWrite" + }, + { + "label": "the closing check speaks the doorstep's sentence, so a leg that leaks blames a previous run that never existed — the half of the 2026-08-28 failure a green gate would have hidden", + "file": "internal/cli/clean_ledger.go", + "find": "func (m leftoverMoment) subject() string {\n\tif m == momentClosing {\n\t\treturn \"this run\"\n\t}", + "replace": "func (m leftoverMoment) subject() string {\n\tif m == momentClosing && false {\n\t\treturn \"this run\"\n\t}", + "test": "TestTheClosingCheckBlamesTheRunThatLeaked", + "package": "./internal/cli/" + }, + { + "label": "the doorstep goes back to saying nothing about the rule sets it does not refuse on, so the one artefact that accumulates once per session is invisible exactly on the hosts where it is all that is left", + "file": "internal/cli/clean_ledger.go", + "find": "\tif len(left.Firewalls) == 0 {\n\t\treturn\n\t}", + "replace": "\tif len(left.Firewalls) == 0 || true {\n\t\treturn\n\t}", + "test": "TestTheDoorstepNamesRuleSetsItDoesNotRefuseOn", + "package": "./internal/cli/" + }, + { + "label": "the ledger goes back to attributing a network to a name prefix it does not carry — `feint-uplink` was reported as found by `fnt-` — so the column that decides what may be touched names a mark nobody read", + "file": "internal/cli/clean_ledger.go", + "find": "\t\t{\"network\", left.Networks, \"label:\" + machine.LabelKey},", + "replace": "\t\t{\"network\", left.Networks, \"name-prefix:\" + machine.NetworkPrefix + \"-\" + machine.LabelKey[:0]},", + "test": "TestTheLedgerAttributesEachObjectToTheMarkItWasFoundBy", + "package": "./internal/cli/" + }, + { + "label": "clean accepts --doorstep and --closing together and picks one of the two sentences, printing a confident lie about which run left what", + "file": "internal/cli/clean.go", + "find": "\tif *doorstep && *closing {", + "replace": "\tif *doorstep && *closing && false {", + "test": "TestTheTwoMomentsCannotBeAskedAtOnce", + "package": "./internal/cli/" + }, + { + "label": "the shell refusal after a run speaks about an earlier run, sending the reader of a job log to a run that never existed", + "file": "tools/conformance/guard.sh", + "find": " if [ \"$scope\" = \"closing\" ]; then", + "replace": " if [ \"$scope\" = \"closing\" ] && false; then", + "test": "TestTheClosingRefusalBlamesThisRunAndNotAnEarlierOne", + "package": "./tools/conformance/" + }, + { + "label": "the closing scope passes --doorstep to the binary, so the report names a previous run for what this one left", + "file": "tools/conformance/guard.sh", + "find": " closing) moment=\"--closing\" ;;", + "replace": " closing) moment=\"--doorstep\" ;;", + "test": "TestOnlyTheDoorstepAsksWhatAnEarlierRunLeft", + "package": "./tools/conformance/" + }, + { + "label": "the runtime proof's closing doorstep runs on the failure path too, so a leg that already died reports its wreckage as a leak and buries the finding under a second red", + "file": ".github/workflows/runtime-proof.yml", + "find": " - name: What this leg left on the host\n run: sudo tools/conformance/guard.sh leftovers-after \"${{ matrix.mode }}\"", + "replace": " - name: What this leg left on the host\n if: always()\n run: sudo tools/conformance/guard.sh leftovers-after \"${{ matrix.mode }}\"", + "test": "TestTheRuntimeProofEndsOnItsOwnDoorstep", + "package": "./tools/conformance/" } ], - "note": "The subject is #521: two full `FEINT_VM=incus-ovn mise run conformance` runs, both green, each left the host holding `feint-uplink` and one detached `osc-*` rule set at used_by 0 — and the suite's own doorstep refused the next launch on exactly that, `0 machine(s) and 1 network(s) of an earlier run still hold this host`. Five manual `feint clean --vm incus-ovn` were paid in one day, in four different work streams, for a sweep the task never mentions.\n\nTwo residues, two causes, and neither is the suites' fault. The `osc-*` rule set is the default security group a Net is born with: DeleteSecurityGroup refuses it by design (`the default security group of ... cannot be deleted`), so a client that tears down Vms, Subnet, Net — balancer.sh's cleanup, or any terraform destroy — has no call that could ever drop its host ACL; only the DeleteNet cascade can, and the cascade deleted the store record without touching the runtime. The uplink is process plumbing: no resource owns it, so no resource's delete removes it, and the closing `feint stop` pruned nothing.\n\nThe fix lands the end of the run where the start would accept it: the DeleteNet cascade drops the default group's rule set (the store delete and the runtime delete were asymmetric — deleteSecurityGroup had both since #475, the cascade had one); a graceful serve exit releases the uplink when this process holds it and nothing draws from it; and the conformance task stops explicitly, then re-asks its own doorstep, so a suite that leaks fails the run that leaked rather than the next one.\n\nThe refusing halves matter as much as the release: mutations 2 and 3 are the #455 family one object up — an exit that deletes an uplink held by another live run, or an operator's bridge under the uplink's name because a pid claim outranked the label. Mutation 4 is the lying-instrument family: released=true over an uplink still standing reads exactly like the fix working." + "note": "The subject is #521: two full `FEINT_VM=incus-ovn mise run conformance` runs, both green, each left the host holding `feint-uplink` and one detached `osc-*` rule set at used_by 0 — and the suite's own doorstep refused the next launch on exactly that, `0 machine(s) and 1 network(s) of an earlier run still hold this host`. Five manual `feint clean --vm incus-ovn` were paid in one day, in four different work streams, for a sweep the task never mentions.\n\nTwo residues, two causes, and neither is the suites' fault. The `osc-*` rule set is the default security group a Net is born with: DeleteSecurityGroup refuses it by design (`the default security group of ... cannot be deleted`), so a client that tears down Vms, Subnet, Net — balancer.sh's cleanup, or any terraform destroy — has no call that could ever drop its host ACL; only the DeleteNet cascade can, and the cascade deleted the store record without touching the runtime. The uplink is process plumbing: no resource owns it, so no resource's delete removes it, and the closing `feint stop` pruned nothing.\n\nThe fix lands the end of the run where the start would accept it: the DeleteNet cascade drops the default group's rule set (the store delete and the runtime delete were asymmetric — deleteSecurityGroup had both since #475, the cascade had one); a graceful serve exit releases the uplink when this process holds it and nothing draws from it; and the conformance task stops explicitly, then re-asks its own doorstep, so a suite that leaks fails the run that leaked rather than the next one.\n\nThe refusing halves matter as much as the release: mutations 2 and 3 are the #455 family one object up — an exit that deletes an uplink held by another live run, or an operator's bridge under the uplink's name because a pid claim outranked the label. Mutation 4 is the lying-instrument family: released=true over an uplink still standing reads exactly like the fix working.\n\n2026-08-28, the same family measured again and larger. The incus-ovn leg of runtime-proof.yml failed at the doorstep of its own witness gate on a GitHub runner nothing had ever touched: `a previous run left 0 machine(s) and 2 network(s) on this host`, naming feint-uplink, fnt-default and the rule sets of two providers' default security groups. The station reproduced it exactly — the three ssh suites each exit 0 and each leave something standing.\n\nFour objects, one property: no client call can remove any of them. fnt-default is DefaultMachineNetwork, created the first time a machine boots with no attachment of its own (an Outscale Vm outside a Net) and owned by no resource; feint-uplink was already released by #521 and stayed because fnt-default still drew from it; the scw-* and exo-* rule sets belong to default security groups a client cannot delete, and the Scaleway one is minted per run, so it accumulates one host ACL per session. So the graceful exit gives all four back (machine.PlumbingReleaser), in the only order the runtime accepts, and each release answers both questions — ours, and used by nothing — because a release that took a rule set off a machine a --cleanup-less stop deliberately left running would be the firewall-disarming primitive an audit already demonstrated.\n\nThe second half is the sentence. The refusal was right and its subject was not: there was no previous run. `clean --closing` and `guard.sh leftovers-after` ask the identical question and name this run, and runtime-proof.yml now asks it after its own stop instead of letting the next step meet the residue." } diff --git a/tools/falsify/specs/run-leaves-nothing.json b/tools/falsify/specs/run-leaves-nothing.json index c639bbda..63520bff 100644 --- a/tools/falsify/specs/run-leaves-nothing.json +++ b/tools/falsify/specs/run-leaves-nothing.json @@ -44,8 +44,8 @@ { "label": "the doorstep finds the previous run's networks and lets the run start anyway, which is the state that fails thirty steps later on a message naming only the block", "file": "internal/cli/clean.go", - "find": "\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt); err != nil {", - "replace": "\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt); err != nil && false {", + "find": "\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt, moment); err != nil {", + "replace": "\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt, moment); err != nil && false {", "test": "TestTheDoorstepRefusesAHostHoldingAPreviousRunsNetwork", "package": "./internal/cli/" }, @@ -76,8 +76,8 @@ { "label": "the doorstep question is asked at every moment, so a run is refused for owning the machines and networks it just created", "file": "internal/cli/clean.go", - "find": "\tif doorstep {\n\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt); err != nil {", - "replace": "\tif doorstep || true {\n\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt); err != nil {", + "find": "\tif moment.asks() {\n\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt, moment); err != nil {", + "replace": "\tif moment.asks() || true {\n\t\tif err := refuseRuntimeLeftovers(stdout, led, vm, rt, moment); err != nil {", "test": "TestTheLeftoverCheckMidRunIgnoresTheRunsOwnObjects", "package": "./internal/cli/" }, diff --git a/tools/falsify/specs/unkillable-dhcp-orphan.json b/tools/falsify/specs/unkillable-dhcp-orphan.json index ca6c6add..b4fd9957 100644 --- a/tools/falsify/specs/unkillable-dhcp-orphan.json +++ b/tools/falsify/specs/unkillable-dhcp-orphan.json @@ -41,8 +41,8 @@ { "label": "the shell refusal never fires, so the leg starts on a host whose block is held and dies in the sweep twelve steps later", "file": "tools/conformance/guard.sh", - "find": " if ! \"$binary\" clean --check $doorstep --vm \"$machines\" >&2; then", - "replace": " if ! \"$binary\" clean --check $doorstep --vm \"$machines\" >&2 && false; then", + "find": " if ! \"$binary\" clean --check $moment --vm \"$machines\" >&2; then", + "replace": " if ! \"$binary\" clean --check $moment --vm \"$machines\" >&2 && false; then", "test": "TestTheLeftoverGuardRefusesAHostItCannotClean", "package": "./tools/conformance/" },