fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Sweep: find every invariant kept at N sites but asserted at fewer than N #577

New Issue
Closed
opened 2026-09-12 14:09:14 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 14:09:14 +02:00
Owner
Copy Link
Copy Source

Sweep, not a fix. Change no production code. The output is a ranked list of findings.

Why

Three instances of one shape landed today, all in MessageService.java, all found the same way:

ticket the invariant sites asserted the survivor
#561 both halves of a composed TurnListener must run 2 1 bothMustRunKeepingSecondResult
#572 answer() releases the session lock on every exit 2 1 lock.unlock() in answer()'s finally
#575 a rendezvous waiter and its asyncTasksByWaiter entry are always cleaned up 3 2 the hand-rolled STALE_TURN pair

The rule that finds them: count assertions per site, not per invariant. One invariant kept at N places needs N assertions. The thing that hides the gap is the non-zero total — mutate one site, 23 tests go red, everyone concludes the invariant is covered, and the identical sibling two hundred lines down has zero tests on it.

A shared helper makes this worse, not better. Extracting the duplicated lines into one method does not reduce the number of call sites that must maintain the invariant; it only reduces how many of them you can see.

Three instances in one file in one day is not three accidents. Sweep the rest of the package.

Scope

fleetd/src/main/java/dev/ltms/fleet/ — all of it. Prioritise, in this order:

  1. msg/ (where all three instances were found — but not the three already fixed)
  2. inject/
  3. mcp/ and rest/
  4. everything else

How to find a candidate

An invariant kept at N sites usually looks like one of these:

  • The same one or two statements repeated in more than one finally. unlock(), close(), remove(), release(), a try/finally pair copy-pasted into a sibling method.
  • A finally's work also written out by hand before an early return. That is #575 exactly: two copies, one protected, one not.
  • The same guard repeated at more than one entry point — a null check, a permission check, a state check.
  • A try that opens after the resource it is supposed to clean up was acquired. Measure the gap between the acquire line and the try { line; anything above zero is a candidate.
  • Two methods that do the same thing for different callers — send/sendAsync, answer/reply, a sync and an async variant of one operation.

How to confirm one — this is the whole job

For each candidate site, one at a time:

  1. Locate the line fresh. Never reuse a line number from this ticket or from anything you read earlier. MessageService.java moved a target by 13 lines between one ticket being written and its fix landing; a sed anchored to a stale number mutates the wrong line and produces a meaningless green.
  2. Count the anchor before you mutate, by exact whole-line match: grep -Fxc '<the exact line>' <file>. -F is literal, -x is whole line. Never awk -v — it backslash-escape-processes the assigned value, so an anchor holding a tab silently counts 0, which reads as "mutation not applied". A pristine count of 0 for a line you just read out of that file is impossible; if you see one, your counting command is broken, not the file.
  3. Mutate with a line-anchored sed. Never a regex or perl substitution across the file — the same line usually appears more than once.
  4. Count again. The count must drop by exactly one. 2→1 is a valid proof. 1→0 is a valid proof. "unchanged" means the mutation did not apply — stop and fix your command, do not build.
  5. Build: mvn -o clean install from fleetd/ (there is no POM at the repo root). Exit code, and never mvn -q — it suppresses the test count, so you get exit 0 with no evidence.
  6. Restore the file and prove it: shasum -a 256 must match the pristine value byte for byte.

A site is a finding only if the mutated build is green. Report the test count for every run, both the mutated one and the pristine control.

A survivor has three causes, and only one is a finding

Before you report any survivor, rule out the other two:

  1. The line never executes. Name at least one test, by class and method, that provably drives that line — or say plainly that you could not find one, which is itself the finding.
  2. It executes but nothing asserts the consequence. This is the real finding.
  3. The covering tests were excluded from the profile you ran. fleetd/pom.xml:264 sets excludedGroups=contract in the default profile. The default run is about 1766 tests; -Pcontract adds roughly 30 more, and those need Docker or AMQP_URI. Name the profile every number came from. A survivor that dies under -Pcontract is not a survivor.

One trap that has already false-passed here

ReentrantLock is reentrant. A probe issued by the same thread that holds the lock re-enters it for free, so the probe passes whether or not the lock was released. That test proves reentrancy, not release, and it stays green under mutation. Any probe for a released lock must run on a different thread. The same caution applies to anything else whose owner-thread gets a free pass.

What to report

fleet_reply with a ranked list. For each finding:

  • file and the fresh line number, plus the exact anchor line
  • the invariant, in one sentence
  • how many sites keep it, and how many assertions each has
  • the mutation you ran, the anchor count before and after, and the test result (Tests run: N, Failures: F, Errors: E)
  • which of the three causes it is, with the evidence that rules out the other two
  • the profile the numbers came from

Rank by how bad the consequence is if the invariant breaks, not by how easy the fix is. Do not fix anything. Findings only. I will file the ones worth fixing as their own tickets.

If you find nothing, say so and show the sites you mutated and the kills you got. A clean sweep with evidence is a real result; a clean sweep with no mutations is not.

Related: #561, #572, #575 (the three instances), #512 (one symbol carrying two states).

Sweep, not a fix. **Change no production code.** The output is a ranked list of findings. ## Why Three instances of one shape landed today, all in `MessageService.java`, all found the same way: | ticket | the invariant | sites | asserted | the survivor | |---|---|---|---|---| | #561 | both halves of a composed `TurnListener` must run | 2 | 1 | `bothMustRunKeepingSecondResult` | | #572 | `answer()` releases the session lock on every exit | 2 | 1 | `lock.unlock()` in `answer()`'s finally | | #575 | a rendezvous waiter and its `asyncTasksByWaiter` entry are always cleaned up | 3 | 2 | the hand-rolled `STALE_TURN` pair | The rule that finds them: **count assertions per site, not per invariant.** One invariant kept at N places needs N assertions. The thing that hides the gap is the **non-zero total** — mutate one site, 23 tests go red, everyone concludes the invariant is covered, and the identical sibling two hundred lines down has zero tests on it. A shared helper makes this **worse**, not better. Extracting the duplicated lines into one method does not reduce the number of *call sites* that must maintain the invariant; it only reduces how many of them you can see. Three instances in one file in one day is not three accidents. Sweep the rest of the package. ## Scope `fleetd/src/main/java/dev/ltms/fleet/` — all of it. Prioritise, in this order: 1. `msg/` (where all three instances were found — but **not** the three already fixed) 2. `inject/` 3. `mcp/` and `rest/` 4. everything else ## How to find a candidate An invariant kept at N sites usually looks like one of these: - **The same one or two statements repeated in more than one `finally`.** `unlock()`, `close()`, `remove()`, `release()`, a `try`/`finally` pair copy-pasted into a sibling method. - **A `finally`'s work also written out by hand before an early `return`.** That is #575 exactly: two copies, one protected, one not. - **The same guard repeated at more than one entry point** — a null check, a permission check, a state check. - **A `try` that opens *after* the resource it is supposed to clean up was acquired.** Measure the gap between the acquire line and the `try {` line; anything above zero is a candidate. - **Two methods that do the same thing for different callers** — `send`/`sendAsync`, `answer`/`reply`, a sync and an async variant of one operation. ## How to confirm one — this is the whole job For **each** candidate site, one at a time: 1. **Locate the line fresh.** Never reuse a line number from this ticket or from anything you read earlier. `MessageService.java` moved a target by 13 lines between one ticket being written and its fix landing; a `sed` anchored to a stale number mutates the wrong line and produces a meaningless green. 2. **Count the anchor before you mutate**, by exact whole-line match: `grep -Fxc '<the exact line>' <file>`. `-F` is literal, `-x` is whole line. **Never `awk -v`** — it backslash-escape-processes the assigned value, so an anchor holding a tab silently counts `0`, which reads as "mutation not applied". A pristine count of `0` for a line you just read out of that file is impossible; if you see one, your counting command is broken, not the file. 3. **Mutate with a line-anchored `sed`.** Never a regex or `perl` substitution across the file — the same line usually appears more than once. 4. **Count again. The count must drop by exactly one.** 2→1 is a valid proof. 1→0 is a valid proof. "unchanged" means the mutation did not apply — stop and fix your command, do not build. 5. **Build:** `mvn -o clean install` from `fleetd/` (there is no POM at the repo root). Exit code, and **never `mvn -q`** — it suppresses the test count, so you get exit 0 with no evidence. 6. **Restore the file and prove it:** `shasum -a 256` must match the pristine value byte for byte. A site is a **finding** only if the mutated build is **green**. Report the test count for every run, both the mutated one and the pristine control. ## A survivor has three causes, and only one is a finding Before you report any survivor, rule out the other two: 1. **The line never executes.** Name at least one test, by class and method, that provably drives that line — or say plainly that you could not find one, which is itself the finding. 2. **It executes but nothing asserts the consequence.** This is the real finding. 3. **The covering tests were excluded from the profile you ran.** `fleetd/pom.xml:264` sets `excludedGroups=contract` in the default profile. The default run is about 1766 tests; `-Pcontract` adds roughly 30 more, and those need Docker or `AMQP_URI`. **Name the profile every number came from.** A survivor that dies under `-Pcontract` is not a survivor. ## One trap that has already false-passed here `ReentrantLock` is **reentrant**. A probe issued by the *same thread* that holds the lock re-enters it for free, so the probe passes whether or not the lock was released. That test proves reentrancy, not release, and it stays green under mutation. **Any probe for a released lock must run on a different thread.** The same caution applies to anything else whose owner-thread gets a free pass. ## What to report `fleet_reply` with a ranked list. For each finding: - file and the **fresh** line number, plus the exact anchor line - the invariant, in one sentence - how many sites keep it, and how many assertions each has - the mutation you ran, the anchor count before and after, and the test result (`Tests run: N, Failures: F, Errors: E`) - which of the three causes it is, with the evidence that rules out the other two - the profile the numbers came from Rank by how bad the consequence is if the invariant breaks, not by how easy the fix is. **Do not fix anything.** Findings only. I will file the ones worth fixing as their own tickets. If you find nothing, say so and show the sites you mutated and the kills you got. A clean sweep with evidence is a real result; a clean sweep with no mutations is not. Related: #561, #572, #575 (the three instances), #512 (one symbol carrying two states).
ltms commented 2026-09-12 15:08:01 +02:00
Author
Owner
Copy Link
Copy Source

Sweep collected. Two findings, both filed. Closing this ticket — but read the scope note, because most of the tree was never swept.

Findings filed as separate tickets, each carrying its own mutation proof:

  • #581 — CompletionResolver.inFlight: the compare-and-remove invariant is maintained at 9 sites and asserted at 2. Severity 1. Now delegated.
  • #582 — AmqpReplyInbox / LeadMailbox: the pendingByMsgId half of the dual-map cleanup is unasserted at every error-path site, 5 sites in each of two files. Severity 2. Now delegated.

The work was read-only and honest: no production file changed, every mutation restored and checked byte-for-byte with sha256, git status --short clean afterward.

What makes #581 the sharpest instance of this rule so far

Every earlier instance (#561, #572, #575) looked like an invariant somebody had forgotten. This one is not. The class javadoc names the exact bug it exists to prevent, and each of the three covering tests has a comment spelling out why the compare-and-remove form matters — "a one-arg remove(target) here would evict it even though the map no longer holds turnA".

The authors understood the danger precisely, wrote it down twice, and then guarded 2 of 9 sites. So understanding the invariant is not what produces coverage of it. Nothing in the process counts the sites, which is the actual gap, and it is why this sweep was worth running.

The count is now five

Known instances of the per-site shape: #561, #572, #575, the LeadMailbox.inspect() fix in 3f8c38f (merged as #567), and now #581/#582. The worker checked git history for prior fixes of this shape before counting anything as new, which is the right instinct and found the 3f8c38f one.

SCOPE NOT SWEPT — do not read this ticket as "the tree is clean"

The worker ran out of turn budget after msg/ and inject/, and said so plainly rather than implying full coverage. Not opened at all:

  • mcp/ and rest/
  • auth, config, guard, health, herdr, lead, logging, member, metrics, peer, placement, power, session
  • inside inject/, only CompletionResolver.java was deep-dived

One unexplored candidate, flagged but not investigated: Injector.java has several per-target reset lines — t.turnObserved = false;, t.injectableSincePostTurnPickup = 0;, t.awaitingPostTurnPickup = false;, t.awaitingPickup = false;, t.awaitingCompletion = false; — each repeated 3-4 times across the file. Whether that is the same invariant shape or coincidental duplication is unknown; it was found by a duplicate-line scan, not by reading. It is recorded here as unexplored, not as a finding, and it is a good first target for the next sweep.

A follow-up sweep over the untouched packages is worth filing when there is capacity. Two findings from two packages is a hit rate that justifies continuing.

## Sweep collected. Two findings, both filed. Closing this ticket — but read the scope note, because most of the tree was never swept. Findings filed as separate tickets, each carrying its own mutation proof: - **#581** — `CompletionResolver.inFlight`: the compare-and-remove invariant is maintained at 9 sites and asserted at 2. Severity 1. Now delegated. - **#582** — `AmqpReplyInbox` / `LeadMailbox`: the `pendingByMsgId` half of the dual-map cleanup is unasserted at every error-path site, 5 sites in each of two files. Severity 2. Now delegated. The work was read-only and honest: no production file changed, every mutation restored and checked byte-for-byte with `sha256`, `git status --short` clean afterward. ### What makes #581 the sharpest instance of this rule so far Every earlier instance (#561, #572, #575) looked like an invariant somebody had forgotten. This one is not. The class javadoc names the exact bug it exists to prevent, and each of the three covering tests has a comment spelling out why the compare-and-remove form matters — *"a one-arg `remove(target)` here would evict it even though the map no longer holds turnA"*. **The authors understood the danger precisely, wrote it down twice, and then guarded 2 of 9 sites.** So understanding the invariant is not what produces coverage of it. Nothing in the process counts the sites, which is the actual gap, and it is why this sweep was worth running. ### The count is now five Known instances of the per-site shape: #561, #572, #575, the `LeadMailbox.inspect()` fix in `3f8c38f` (merged as #567), and now #581/#582. The worker checked git history for prior fixes of this shape before counting anything as new, which is the right instinct and found the `3f8c38f` one. ### SCOPE NOT SWEPT — do not read this ticket as "the tree is clean" The worker ran out of turn budget after `msg/` and `inject/`, and said so plainly rather than implying full coverage. Not opened at all: - `mcp/` and `rest/` - `auth`, `config`, `guard`, `health`, `herdr`, `lead`, `logging`, `member`, `metrics`, `peer`, `placement`, `power`, `session` - inside `inject/`, only `CompletionResolver.java` was deep-dived **One unexplored candidate, flagged but not investigated:** `Injector.java` has several per-target reset lines — `t.turnObserved = false;`, `t.injectableSincePostTurnPickup = 0;`, `t.awaitingPostTurnPickup = false;`, `t.awaitingPickup = false;`, `t.awaitingCompletion = false;` — each repeated 3-4 times across the file. Whether that is the same invariant shape or coincidental duplication is **unknown**; it was found by a duplicate-line scan, not by reading. It is recorded here as unexplored, not as a finding, and it is a good first target for the next sweep. A follow-up sweep over the untouched packages is worth filing when there is capacity. Two findings from two packages is a hit rate that justifies continuing.
ltms closed this issue 2026-09-12 15:08:09 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#577
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?