Skip to content

fix(reset): the verdict compares bytes, a broken promise fails the run, and the rule comes back on the NIC - #871

Merged
stephrobert merged 2 commits into
mainfrom
fix/audit-the-reset-verdict-compares-bytes
Nov 11, 2026
Merged

stephrobert merged 2 commits into
mainfrom
fix/audit-the-reset-verdict-compares-bytes

Conversation

@stephrobert

Copy link
Copy Markdown
Owner

Summary

Five findings from an external audit of merged code, each verified against the source before being corrected — and two more defects that the verification of the fifth surfaced on its own.

The audit's own framing matters here: it marked its third P0 as deduced from the code, not reproduced. So the first commit fixed the four it could demonstrate and said in writing that the fifth was untouched, with the reason. The second commit reproduced the fifth, with a TCP connection under --vm incus, and only then fixed it.

What was wrong

The reset verdict compared nothing about an object's content. observable.go took an object's digest from ListObjectsV2's ETag, and internal/s3/list.go emits no ETag at all — so the field was always empty, and the size it had already read was dropped from the key too. Two different contents under one key compared equal, and the commit that shipped it claimed objects were compared by digest. Each object is now fetched and hashed by the observer, because a digest supplied by the subject cannot answer whether a client gets the same bytes back.

feint run could return 0 with a broken promise. Only "did not converge" failed the run, so terraform test: success + feint reset: failed + feint run: exit 0 was a possible CI run. deliveredWhatWasAsked now lists the answers that are not a success, so a verdict word added later cannot default to "fine".

The checkpoint's "both halves or neither" was a sentence. The bytes were saved before the control-plane snapshot, so a failure in the second step left an earlier checkpoint of that name holding its old store beside the new object references. The fix is a reordering, because objectstore publishes its half atomically by renaming a temporary file.

Named checkpoints overwrote each other. The emulator holds any number; the CLI wrote one file per instance, so checkpoint A then checkpoint B made reset A impossible.

A reset put the firewall rule back in the store and not on the NIC. Survey answers names, and a rule set whose content changed after the checkpoint keeps its name, so nothing in the observation could see the difference. Fixed with a third optional pack half, emulator.RuleReconciler, whose gesture is shared (machine.GroupSync.Reapply) and whose only varying part is which restored record a provider calls a group.

Two the reproduction surfaced. My own over-correction — re-applying every restored group furnished the host with rule sets the checkpoint never held — and one older than the audit: a reset started containers for records the checkpoint describes as stopped, then wrote running over the state it had been asked to return to. That one was measured as pre-existing by replaying the leg with these changes stashed.

How each was proven

Every fix has a test that fails without it, and three of those tests exist because the first version of them measured nothing. The repository's harness said so each time rather than me noticing:

  • the byte comparison needed a real door over a real store, because an absent field is invisible to a test that builds its own values by hand — which is exactly the shape of the test that let the defect through;
  • the checkpoint ordering: closing the byte store makes both orders refuse at the same step, so the reordering mutation stayed green. The scenario that separates them is the one the audit described, where the byte half succeeds;
  • the firewall: the suite reproduced the failure before a line of the fix was written.
- revoking it closes it again
  ok: port 80 refused again
- and a reset puts the rule back on the NIC, not only in the store
FAIL: the reset reported convergence and port 80 is still refused:
      the rule came back in the store and not on the NIC

Type of change

  • A route added or changed
  • Bug fix
  • Refactor
  • Documentation
  • Chore / tooling
  • Security / supply chain

Checklist

Always

  • mise run check passes
  • No new external Go dependency
  • internal/core still knows no provider. The third half takes the pack's own records and the pack decides which of them is a group, for the same reason ReconcileMissing already documents: the core guessing with a feint- prefix was wrong twice
  • Nothing in the diff could be written identically for another provider. The re-application sequence is in machine.GroupSync; each pack writes a loop and its own kind. The stopped-record comparison is in the Binding, against the RunningState field it already carried
  • mise run testplan was run, and what it printed was run — the full list of 20 runs, in its
    order, all green. The two that cost: conformance:leg -- fields (118 s), the only leg where the
    omission gate judges, and FEINT_VM=incus-ovn mise run conformance:leg -- runtime (1433 s).
    Eleven falsification specs are in that list because the diff touches the files they mutate

When a model wrote a substantive part of this

  • An Assisted-by: trailer names the tool and the model version
  • The conformance legs the plan named were run here, not assumed. The full mise run conformance pass belongs to the assembled batch; what this pull request played is written above and below
  • No provider field name is added or changed by this diff: it touches /_feint/*, the shared machine layer and three one-method pack halves

When a route is added or changed

N/A — no provider route is added or changed.

When behaviour a client can observe changes

  • docs/limits.md unchanged: no documented limit moves. The degraded mode is reported rather than claimed — a group on a runtime that enforces nothing comes back as skipped, never as reapplied
  • No generated table changes

When .github/workflows/ is touched

N/A.

When a machine runtime is involved

  • Measured under FEINT_VM=incus and under FEINT_VM=incus-ovn, never only with --vm off.
    Both modes answer the new assertion the same way — ok: port 80 reachable again after the reset — and reset-host.sh reports convergence under OVN, where the stashed baseline
    failed on three lines
  • Nothing the emulator did not create was touched. The re-application goes through GroupSync, which goes through the Binding and the driver's ownership probes
  • The run leaves nothing behind — checked after each pass with feint clean, which reported nothing left on the runtime, and the station's own container untouched

Related issues

Closes #853.
Follows the audit of main@91d5a46.
Opens #870 — a reset is not a transaction, and nothing measures what a create or a delete arriving mid-reset does. The three paths are read from the code and none is reproduced, which the issue says in those words.

🤖 Generated with Claude Code

…s the run

Four findings from an external audit of merged code, each verified against the
source before being corrected, and three of them were real false greens of
mine.

## The verdict compared nothing about an object's content

`observable.go` took an object's digest from ListObjectsV2's ETag.
`internal/s3/list.go` emits no ETag at all, so the field was ALWAYS empty, and
the comparison key was the door, the bucket and the key — the size it had
already read was dropped too. Two different contents under one key compared
equal, and a reset could report convergence over bytes that had changed. The
commit message that shipped it claimed objects were compared by digest.

What let it through is the shape of the test beside it:
TestADifferenceIsNamedWhereverItIs builds observedObject values BY HAND, with
digests in them, so it exercised the comparison and never the observation. A
fake emitting an ETag would have hidden it just as well.

Each object is now FETCHED and hashed by the observer. Not the store's record
of it and not the door's checksum header: what is judged is whether a client
gets the same bytes back, and a digest supplied by the subject cannot answer
that. The delivered size and the listed size travel separately, so an
inventory that disagrees with the bytes is a difference rather than a silent
one.

The audit's decisive test exists —
TestTwoObjectsOfOneSizeAndDifferentBytesDoNotConverge — and it mounts a REAL
door over a REAL store, because that is the only arrangement in which an
absent field is visible. It also asserts the same bytes twice compare EQUAL, so
it cannot be satisfied by a comparison that always differs.

Writing through the door rather than behind it was not a preference either: a
bucket created straight on the store carries no creation metadata and the door
refuses to list it with a 501. That cost three runs of blaming the reader.

## `feint run` could return 0 with a broken promise

Only "did not converge" failed the run. A reset the emulator refused, one that
could not be read back, and one with no checkpoint to return to all left the
exit code at the child's, so this was a possible CI run:

    terraform test   success
    feint reset      failed
    feint run        exit 0

The same held for a lab this command owned and could not stop: `cleanup:
failed` reached the receipt and nothing else.

The rule is now the general one, and it is what a wrapper is for: the child's
code travels untouched when the wrapper kept its promises, and the wrapper
fails when it did not. `deliveredWhatWasAsked` lists the answers that are NOT
a success rather than the ones that are, so a verdict word added later cannot
default to "fine" — and the test covers the full cross product including two
words nobody has written yet.

## The checkpoint's "both halves or neither" was a sentence

The bytes were saved before the control-plane snapshot, so a failure in the
second step left an earlier checkpoint of that name holding its OLD store
beside the NEW object references. objectstore publishes its half atomically by
renaming a temporary file, which is what makes the fix a reordering: snapshot
into memory first, save the bytes, publish the store half last. A failure at
any step now leaves the previous checkpoint consistent with itself.

**The first test for this measured nothing**, and the harness said so: closing
the byte store makes BOTH orders refuse at the same step, so the mutation that
reverses the order stayed green. The scenario that separates them is the one
the audit described — the byte half succeeds and the control-plane half fails —
so the store is made unmarshallable and the assertion is on the BYTES a later
reset brings back.

## Named checkpoints overwrote each other

The emulator holds any number; the CLI wrote one file per instance. So
`checkpoint A` then `checkpoint B` made `reset A` impossible, and the refusal
that fired was honest while the feature was missing. Records are indexed by
name now, with a `last` pointer so `feint reset` with no argument still means
something.

One of my own tests then passed for the wrong reason — it found a record a
neighbouring subtest had written — which the per-name index changed underneath
it. It has its own runtime directory now and asserts that the other run's
record is left alone.

## What this does NOT fix, and why

The audit's third P0: a firewall rule modified in place keeps its rule-set
name, so `Survey` returning names cannot see it and a reset could report
convergence while packets are still blocked. Its premise is correct — Survey
answers names — but the audit says it deduced this from the code rather than
reproducing it, and so have I. I am not writing a fix without a test that
bites first; it needs a real TCP flow before, after and after the reset, on
Incus or OVN.

Also untouched: the concurrency question the audit raises at the end. A reset
is not a global transaction, and nothing yet proves what a create arriving
mid-reset does.

/falsify: 11 mutations on a-reset-is-not-a-restore.json and 16 on
a-run-owns-its-lab.json, all biting, including the two that reproduce the
audit's own P0s and the one that reverses the checkpoint's order.

mise run prepush green.

Assisted-by: Claude Code (claude-opus-5)
The audit's third P0, reproduced before being believed, then fixed — plus two
defects the reproduction itself surfaced, one of them mine.

## The finding, and why it waited

The audit deduced from the code that a reset could report convergence while a
firewall rule was still missing from the runtime, and said plainly that it had
not reproduced it. Neither had anybody here, so the previous commit shipped
without a fix and said so.

The premise was right. `Survey` answers NAMES; a rule set whose content changed
after the checkpoint keeps its name, so nothing in the observation can see the
difference. `tools/conformance/scaleway/network.sh` now takes a checkpoint with
port 80 open, lets the suite revoke the rule, resets, and asks a TCP connection:

    - revoking it closes it again
      ok: port 80 refused again
    - and a reset puts the rule back on the NIC, not only in the store
    FAIL: the reset reported convergence and port 80 is still refused:
          the rule came back in the store and not on the NIC

That is the test that bites. The fix came after it.

## The fix: a third optional half

`emulator.RuleReconciler`, beside `Reconciler` and `MissingReconciler` and
separate from them for the reason MissingReconciler already documents — one
interface carrying two methods turned every pack into a non-implementer in
silence, and the call still answered 200.

The gesture is shared: `machine.GroupSync.Reapply` translates the restored
group, writes the set and replays it onto the wearers, which is the same
sequence for the three packs. What varies is one word — which restored record
a provider calls a group — so each pack writes a loop and a kind. The firewall
reconciliation was written once in a pack before, and the two others ran with
zero ACLs while their API described a closed port (#475).

Ordered last of the three, because a set attaches to the interfaces of the
machines that wear it: re-applying before ReconcileMissing has put those
machines back would attach nothing.

## Two more, from the same leg

**Mine, immediately.** The first version re-applied every restored group, and
the runtime leg reported two rule sets appearing on the host that the
checkpoint never held — left over from earlier suites whose machines were long
swept. A reset puts back what the checkpoint had; it does not furnish the host
with objects it did not. Only a group somebody wears is re-applied now, which
is also exactly the group the audit's case is about.

**Older than the audit.** A record keeps its machine name after the machine is
gone — that is how ReconcileGone finds what to destroy — so "names a machine
the host does not hold" also describes, word for word, a server the checkpoint
says is stopped. A reset was starting containers for terminated VMs and writing
`running` over the state it had been asked to return to, and the verdict then
named the resource the reset had itself changed:

    resource present and not in the checkpoint: vm i-a3f536ec state=running
    resource in the checkpoint and not present: vm i-a3f536ec state=terminated
    machine present and not in the checkpoint:  feint-osc-i-a3f536ec

The comparison is in the shared layer, against `Binding.RunningState`: the word
varies, the gesture does not, and the Binding already carries that field
because PowerOn writes it. Measured as pre-existing before being attributed —
the leg was run with these changes stashed, and failed on those three lines
alone.

## Proof

`FEINT_VM=incus mise run conformance:leg -- runtime`, 94 s, green end to end,
including the new reset step and `reset-host.sh`, which the stashed baseline
failed.

/falsify: 15 mutations on a-reset-is-not-a-restore.json, all biting, including
the four added here. Both directions of the firewall fix are mutated, because
a guard that refuses everything passes the attack and breaks the product.

mise run prepush green. The fixed sleep this adds carries its `waits on
silence:` reason, which TestEveryFixedSleepInARuntimeSuiteSaysWhatItWaitsFor
demanded before it would pass.

## The debt that remains, with its number

The concurrency question the audit raises at the end: a reset is not a
transaction, and nothing measures what a create or a delete arriving mid-reset
does. It is now #870, with the three paths read from the code, what would close
it, and the statement that none of it is reproduced yet. It is not a reason to
hold #853 open.

Assisted-by: Claude Code (claude-opus-5)
@stephrobert
stephrobert merged commit 567e9cf into main Nov 11, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1.0 — feint checkpoint / feint reset: transaction-like test isolation on top of snapshots

1 participant