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

Five log-only startup reporters in Fleetd.main have unpinned call sites — deleting any one ships a green build #407

New Issue
Open
opened 2026-09-10 04:01:31 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 04:01:31 +02:00
Owner
Copy Link
Copy Source

Split out of #398 so the number is right. #398's follow-up closes the validators; this is the other half of the same shape and it is not covered by that work.

Measured

Fleetd.main calls five reporters whose only effect is a log line:

FleetConfig cfg = FleetConfig.load(configPath);
reportRequiredSecrets(cfg);
reportGitHostShape(cfg);
reportMemberTrustModel(cfg);
reportMemberCredentialsGap(cfg);
...
reportExhaustedPatternGap(cfg);      // added by #395, merged as 7180b1a

I mutated one of them directly. Deleting reportExhaustedPatternGap(cfg); from Fleetd.java:140:

Tests run: 1472, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Every existing test calls the reporter method directly — for reportExhaustedPatternGap, ExhaustedPatternGapReportTest:93 and :142 — and no test boots main. So each reporter's behaviour is pinned and its invocation is not. A refactor can delete any of these five lines and ship green with the capability completely dead.

I only ran the mutation on reportExhaustedPatternGap. The other four have the same structure — a static void reportXxx(FleetConfig) called once from main, with tests that call it directly — so I expect the same result, but I have not measured the other four and am not claiming them.

Why this is not fixed by #398's validateAll()

FleetConfig.validateAll() reflectively sweeps public no-arg void methods on FleetConfig named validateXxx. These five are none of those things: they live on Fleetd, not FleetConfig, and they are named reportXxx. The sweep cannot reach them, and it should not be widened to try — a reporter and a validator are different contracts.

The one real difference from a validator

A validator throws; a reporter only logs. That changes how you pin it. FleetdStartupValidationTest (added in #398's follow-up) proves a startup call ran by asserting Fleetd.main throws IllegalStateException on a config that fails exactly one validator. There is no exception here — the observable effect is a log event.

So pinning these needs a log appender attached to the Fleetd logger, the same technique ExhaustedPatternGapReportTest:42 already uses:

Logger logger = (Logger) LoggerFactory.getLogger(Fleetd.class);

The difference is that the assertion has to run around a call to the real Fleetd.main, not around a direct call to the reporter.

A complication worth knowing before starting

FleetdStartupValidationTest gets to call Fleetd.main safely only because its configs are invalid — validateAll() throws before main opens the herdr socket or binds Javalin. A reporter test needs the opposite: a config valid enough that the reporters run and produce their log lines. That config would then carry main past validation and into real side effects.

Options, in the order I would try them:

  1. Keep the config invalid, and assert on the log lines the reporters emitted before the throw. All five reporters run before validateAll(), so a config that fails a validator still exercises every one of them. This looks like the cheapest correct answer and needs no new seam.
  2. Extract the reporter block into one package-private method that main calls, and pin that one call the way validateAll() is pinned. This trades a test-visible seam for a simpler assertion, and it makes the same "one call, not five" collapse #398 made for validators.

Option 1 is preferable if it works, because it adds no production code at all.

Acceptance

  • Deleting any one of the five reportXxx(cfg); lines from Fleetd.main fails a test.
  • The test drives the real Fleetd.main, not the reporter methods directly. A test that calls reportXxx(cfg) cannot catch this — that is the entire defect.
  • The test must not open a socket, bind a port, spawn a member, or write outside a @TempDir.
  • Report the mutation result for each of the five separately, with exact failing test names. If any one of them turns out to be pinned already, say so rather than assuming all five behave alike.

Related

Same family as #398 (validators) and the general rule in this repo's notes: a test on the seam does not prove the caller. #395's field was reviewed by me and merged with this gap recorded in its merge commit and its Features wiki entry.

Split out of #398 so the number is right. #398's follow-up closes the **validators**; this is the other half of the same shape and it is not covered by that work. ## Measured `Fleetd.main` calls five reporters whose only effect is a log line: ```java FleetConfig cfg = FleetConfig.load(configPath); reportRequiredSecrets(cfg); reportGitHostShape(cfg); reportMemberTrustModel(cfg); reportMemberCredentialsGap(cfg); ... reportExhaustedPatternGap(cfg); // added by #395, merged as 7180b1a ``` I mutated one of them directly. Deleting `reportExhaustedPatternGap(cfg);` from `Fleetd.java:140`: ``` Tests run: 1472, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` Every existing test calls the reporter method directly — for `reportExhaustedPatternGap`, `ExhaustedPatternGapReportTest:93` and `:142` — and no test boots `main`. So each reporter's behaviour is pinned and its invocation is not. A refactor can delete any of these five lines and ship green with the capability completely dead. I only ran the mutation on `reportExhaustedPatternGap`. The other four have the same structure — a `static void reportXxx(FleetConfig)` called once from `main`, with tests that call it directly — so I expect the same result, but **I have not measured the other four** and am not claiming them. ## Why this is not fixed by #398's `validateAll()` `FleetConfig.validateAll()` reflectively sweeps public no-arg `void` methods on `FleetConfig` named `validateXxx`. These five are none of those things: they live on `Fleetd`, not `FleetConfig`, and they are named `reportXxx`. The sweep cannot reach them, and it should not be widened to try — a reporter and a validator are different contracts. ## The one real difference from a validator **A validator throws; a reporter only logs.** That changes how you pin it. `FleetdStartupValidationTest` (added in #398's follow-up) proves a startup call ran by asserting `Fleetd.main` throws `IllegalStateException` on a config that fails exactly one validator. There is no exception here — the observable effect is a log event. So pinning these needs a log appender attached to the `Fleetd` logger, the same technique `ExhaustedPatternGapReportTest:42` already uses: ```java Logger logger = (Logger) LoggerFactory.getLogger(Fleetd.class); ``` The difference is that the assertion has to run around a call to the real `Fleetd.main`, not around a direct call to the reporter. ## A complication worth knowing before starting `FleetdStartupValidationTest` gets to call `Fleetd.main` safely **only because its configs are invalid** — `validateAll()` throws before `main` opens the herdr socket or binds Javalin. A reporter test needs the opposite: a config valid enough that the reporters run and produce their log lines. That config would then carry `main` past validation and into real side effects. Options, in the order I would try them: 1. Keep the config invalid, and assert on the log lines the reporters emitted **before** the throw. All five reporters run before `validateAll()`, so a config that fails a validator still exercises every one of them. This looks like the cheapest correct answer and needs no new seam. 2. Extract the reporter block into one package-private method that `main` calls, and pin that one call the way `validateAll()` is pinned. This trades a test-visible seam for a simpler assertion, and it makes the same "one call, not five" collapse #398 made for validators. Option 1 is preferable if it works, because it adds no production code at all. ## Acceptance - Deleting **any one** of the five `reportXxx(cfg);` lines from `Fleetd.main` fails a test. - The test drives the real `Fleetd.main`, not the reporter methods directly. A test that calls `reportXxx(cfg)` cannot catch this — that is the entire defect. - The test must not open a socket, bind a port, spawn a member, or write outside a `@TempDir`. - Report the mutation result for each of the five separately, with exact failing test names. If any one of them turns out to be pinned already, say so rather than assuming all five behave alike. ## Related Same family as #398 (validators) and the general rule in this repo's notes: a test on the seam does not prove the caller. #395's field was reviewed by me and merged with this gap recorded in its merge commit and its Features wiki entry.
ltms commented 2026-09-10 14:47:14 +02:00
Author
Owner
Copy Link
Copy Source

A sixth instance of this exact shape, found today, measured rather than assumed. Adding it here because it widens the ticket from "five reporters" to "unpinned call sites in Fleetd.main", which is the real scope.

The sixth site: the quarantine wiring

#466 (PR #470) makes BackendQuarantine escalate instead of retrying flat. Its production wiring is one line in Fleetd.main:

BackendQuarantine quarantine = BackendQuarantine.withEscalation(System::nanoTime,
        TimeUnit.SECONDS.toNanos(cfg.quarantineCooldownSeconds()));

I mutated it on the merge commit, putting main back on the flat two-argument constructor:

Tests run: 1608, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS   rc=0

Fully green. The factory and its seven new tests stay perfect while the daemon reverts to retrying a weekly subscription limit about 336 times a week. Measured on the same tree: exactly 1 test file names withEscalation, and it is the factory's own unit test.

So this is not only the reportXxx family. It is the same structure with a different verb: a well-tested static on another class, called once from main, with every test calling it directly and nothing asserting the call.

What this changes about the ticket

Your acceptance criteria are right and I am not proposing to widen them mid-flight. But two things are worth recording:

  1. The count is a floor, not a total. Five reporters plus this constructor is six, and I found the sixth only because I happened to mutate that line. Nobody has swept Fleetd.main for the shape. Worth doing as part of this ticket or as a follow-up: for each statement in main, ask whether deleting it fails a test.
  2. Your option 1 does not generalise to this one. Asserting on log lines emitted before validateAll() throws works beautifully for the five reporters, because their whole observable effect is a log event. The quarantine wiring emits nothing — the difference between the two constructors is invisible until a credential is exhausted twice. So a log-appender test cannot reach it.

For that sixth site I asked for the cheapest instrument that can: a source-reading assertion on Fleetd.java, the FleetMcpAuthzTest technique. 10 test files in this repo already read Fleetd.java as source text, so it needs no daemon, no socket and no new idiom.

I also asked for the vacuity proof, because that is the weakness of the technique: a test that scrapes for a string passes silently once its anchor moves. So the worker has to show it fails loudly when the anchored name is renamed with behaviour unchanged — not just that it fails when the wiring changes. And its javadoc has to say what it proves and what it does not: the text at that call site, not that the call runs.

That is the honest limit of source reading, and it is why it is a partial answer rather than the answer. Recording it here so this ticket does not later adopt the technique without the caveat.

Why the family keeps recurring

Three separate times now, the same wall: extracting a value or a factory and testing it moves the untested surface up to the call site rather than shrinking it. #446 hit it twice (its survivor is on #460), and #466 hit it once. The useful question on any extraction is which of three things a test now reaches — the value, the call site, or the selection between values. Extraction answers only the first, which is why "we added a test" and "the behaviour is pinned" keep coming apart here.

Measured on merge commit tree ab937ad (#466's merge), and on origin/main = 1fb6176 for the test-file counts.

A sixth instance of this exact shape, found today, measured rather than assumed. Adding it here because it widens the ticket from "five reporters" to "unpinned call sites in `Fleetd.main`", which is the real scope. ## The sixth site: the quarantine wiring #466 (PR #470) makes `BackendQuarantine` escalate instead of retrying flat. Its production wiring is one line in `Fleetd.main`: ```java BackendQuarantine quarantine = BackendQuarantine.withEscalation(System::nanoTime, TimeUnit.SECONDS.toNanos(cfg.quarantineCooldownSeconds())); ``` I mutated it on the merge commit, putting `main` back on the flat two-argument constructor: ``` Tests run: 1608, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS rc=0 ``` Fully green. The factory and its seven new tests stay perfect while the daemon reverts to retrying a weekly subscription limit about 336 times a week. Measured on the same tree: exactly **1** test file names `withEscalation`, and it is the factory's own unit test. So this is not only the `reportXxx` family. It is the same structure with a different verb: a well-tested static on another class, called once from `main`, with every test calling it directly and nothing asserting the call. ## What this changes about the ticket Your acceptance criteria are right and I am not proposing to widen them mid-flight. But two things are worth recording: 1. **The count is a floor, not a total.** Five reporters plus this constructor is six, and I found the sixth only because I happened to mutate that line. Nobody has swept `Fleetd.main` for the shape. Worth doing as part of this ticket or as a follow-up: for each statement in `main`, ask whether deleting it fails a test. 2. **Your option 1 does not generalise to this one.** Asserting on log lines emitted before `validateAll()` throws works beautifully for the five reporters, because their whole observable effect is a log event. The quarantine wiring emits nothing — the difference between the two constructors is invisible until a credential is exhausted twice. So a log-appender test cannot reach it. For that sixth site I asked for the cheapest instrument that can: a source-reading assertion on `Fleetd.java`, the `FleetMcpAuthzTest` technique. **10 test files in this repo already read `Fleetd.java` as source text**, so it needs no daemon, no socket and no new idiom. I also asked for the vacuity proof, because that is the weakness of the technique: a test that scrapes for a string passes silently once its anchor moves. So the worker has to show it fails loudly when the anchored name is renamed with behaviour unchanged — not just that it fails when the wiring changes. And its javadoc has to say what it proves and what it does not: the **text** at that call site, not that the call runs. That is the honest limit of source reading, and it is why it is a partial answer rather than the answer. Recording it here so this ticket does not later adopt the technique without the caveat. ## Why the family keeps recurring Three separate times now, the same wall: extracting a value or a factory and testing it moves the untested surface **up** to the call site rather than shrinking it. #446 hit it twice (its survivor is on #460), and #466 hit it once. The useful question on any extraction is which of three things a test now reaches — the value, the call site, or the selection between values. Extraction answers only the first, which is why "we added a test" and "the behaviour is pinned" keep coming apart here. Measured on merge commit tree `ab937ad` (#466's merge), and on `origin/main` = `1fb6176` for the test-file counts.
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#407
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?