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

Three more unpinned constructor arguments in FleetdAssembly — the same shape as #670 and #672 #675

New Issue
Closed
opened 2026-10-03 20:55:09 +02:00 by ltms · 1 comment
ltms commented 2026-10-03 20:55:09 +02:00
Owner
Copy Link
Copy Source

Filed from the sweep the #672 worker was asked to run and not to fix. Two instances of this shape were found and closed on 2026-10-03 (#670, #672); the sweep says the file holds at least three more.

The shape: a production call site in FleetdAssembly supplies a constant that encodes a real decision, and the only test touching that parameter constructs the collaborator itself with its own value. The test covers the seam, not the producer, so the production literal can be changed with the suite still green.

I have not re-measured these three myself. The ranking and the file:line references below are the worker's, reported honestly as a sweep rather than as verified defects. Re-measure before acting: mutate the literal, run the build, and only call it a gap if nothing goes red.

The three, ranked by blast radius

1. FleetdAssembly.java:376-377 — the ReplyPushLoop fallbacks

int maxReminders = cfg.primary() != null ? cfg.primary().remindersOrDefault() : 5;
long backoffMs   = cfg.primary() != null ? cfg.primary().backoffMsOrDefault()  : 15_000L;

These are what a daemon booted with no primary: block at all gives ReplyPushLoop. Reported as unobserved: FleetdAssemblyReleaseCleanupBehaviouralTest only reaches in for the primaryRegistry field, and FleetdBackendErrorSinkTest builds its own ReplyPushLoop with its own literals (3, 50).

If wrong, a daemon with no primary: block nudges the primary at a broken cadence, or stops nudging it. That is a quiet functional loss, not a security hole — but async ticket nudges are the mechanism a lead relies on to learn a worker finished.

2. FleetdAssembly.java:118 (declaration) / :499 (use) — LEAD_COORD_INTERVAL_MS = 3_000L

How often LeadCoordLoop polls the shared broker for peer-lead mail. Reported as: FleetdAssemblyCoordinatorLifecycleTest asserts only that LeadCoordLoop is constructed non-null when a coordinator: block is present, never the interval.

If wrong, cross-host lead coordination either lags badly or hammers the broker. Both fleets share one LavinMQ, so the second failure mode reaches the other fleet too.

3. FleetdAssembly.java:350 — Injector.POLL_INTERVAL_MILLIS into StatusPoller

The constant is well covered where it is declared, but no assembly-level test pins that this constant rather than some other value is what reaches the real StatusPoller. Lowest priority of the three: a gross error here shows up quickly as fleet-wide delivery lag, unlike a silent authz gap.

Ruled out by the same sweep, and worth recording so nobody re-checks them

  • FleetHealthMonitor.coverage(true, …) at :431 — the true sits inside the branch that already requires it true, so it is not an independent decision.
  • CallerResolver.withLeadsAndMembers(identity, true/false, …) at :464/:468 — both the token-mode and loopback-trust branches are already driven behaviourally against the real assembled graph by FleetdQuarantineOutageDualWindowAssemblyTest and FleetdListReportingSourcesAssemblyTest.
  • new PaneLocator(herdr, memberHerdr) at :453 — argument order already pinned by FleetdAssemblyConnectionIdentityTest.
  • FleetdAssembly.java:490 — cfg.coordinator() == null ? List.of() : cfg.coordinator().peers(). FleetMcp's constructor already null-safes peers, so a wrong value here has nowhere to do damage.

Acceptance criteria

Per item, and written as a property rather than as "a test named X exists":

  1. Mutating the literal in FleetdAssembly turns at least one test red. Report the mutant output, not only the green run — a surviving mutant and a build that never reached the test phase produce the same empty failure list, so pair it with the surefire report-file count as a control.
  2. The test observes the object assembleAndStart built. A test that constructs the collaborator itself reproduces the exact blind spot this ticket is about.
  3. Name the failing test from the mutant run and say whether any unrelated test also reddened. An extra red is information: it may mean the site was already pinned elsewhere.
  4. Do not change the production literals. All three current values are believed correct; this ticket adds tests.

FleetdAssemblyAuthorizationModeTest (merged for #672) and FleetdAssemblyLeadTabScannerExclusionTest (#671) are both working models for driving assembleAndStart with a fake ResourcePorts that never binds a port.

One more thing worth deciding

Three tickets in two days on one file says the problem is the file, not the three literals. It may be worth one test that walks FleetdAssembly's wiring generically rather than a fourth hand-written pin — but note that a reflective checker paired with a hand-maintained list is exactly the shape that failed in #668, so a generic sweep needs to be reachability-proving, not shape-proving.

Filed from the sweep the #672 worker was asked to run and not to fix. Two instances of this shape were found and closed on 2026-10-03 (#670, #672); the sweep says the file holds at least three more. **The shape:** a production call site in `FleetdAssembly` supplies a constant that encodes a real decision, and the only test touching that parameter constructs the collaborator itself with its own value. The test covers the seam, not the producer, so the production literal can be changed with the suite still green. I have **not** re-measured these three myself. The ranking and the file:line references below are the worker's, reported honestly as a sweep rather than as verified defects. **Re-measure before acting**: mutate the literal, run the build, and only call it a gap if nothing goes red. ## The three, ranked by blast radius ### 1. `FleetdAssembly.java:376-377` — the `ReplyPushLoop` fallbacks ```java int maxReminders = cfg.primary() != null ? cfg.primary().remindersOrDefault() : 5; long backoffMs = cfg.primary() != null ? cfg.primary().backoffMsOrDefault() : 15_000L; ``` These are what a daemon booted with **no `primary:` block at all** gives `ReplyPushLoop`. Reported as unobserved: `FleetdAssemblyReleaseCleanupBehaviouralTest` only reaches in for the `primaryRegistry` field, and `FleetdBackendErrorSinkTest` builds its own `ReplyPushLoop` with its own literals (`3`, `50`). If wrong, a daemon with no `primary:` block nudges the primary at a broken cadence, or stops nudging it. That is a quiet functional loss, not a security hole — but async ticket nudges are the mechanism a lead relies on to learn a worker finished. ### 2. `FleetdAssembly.java:118` (declaration) / `:499` (use) — `LEAD_COORD_INTERVAL_MS = 3_000L` How often `LeadCoordLoop` polls the shared broker for peer-lead mail. Reported as: `FleetdAssemblyCoordinatorLifecycleTest` asserts only that `LeadCoordLoop` is constructed non-null when a `coordinator:` block is present, never the interval. If wrong, cross-host lead coordination either lags badly or hammers the broker. Both fleets share one LavinMQ, so the second failure mode reaches the other fleet too. ### 3. `FleetdAssembly.java:350` — `Injector.POLL_INTERVAL_MILLIS` into `StatusPoller` The constant is well covered where it is declared, but no assembly-level test pins that *this* constant rather than some other value is what reaches the real `StatusPoller`. Lowest priority of the three: a gross error here shows up quickly as fleet-wide delivery lag, unlike a silent authz gap. ## Ruled out by the same sweep, and worth recording so nobody re-checks them - `FleetHealthMonitor.coverage(true, …)` at `:431` — the `true` sits inside the branch that already requires it true, so it is not an independent decision. - `CallerResolver.withLeadsAndMembers(identity, true/false, …)` at `:464`/`:468` — both the token-mode and loopback-trust branches are already driven behaviourally against the real assembled graph by `FleetdQuarantineOutageDualWindowAssemblyTest` and `FleetdListReportingSourcesAssemblyTest`. - `new PaneLocator(herdr, memberHerdr)` at `:453` — argument order already pinned by `FleetdAssemblyConnectionIdentityTest`. - `FleetdAssembly.java:490` — `cfg.coordinator() == null ? List.of() : cfg.coordinator().peers()`. `FleetMcp`'s constructor already null-safes `peers`, so a wrong value here has nowhere to do damage. ## Acceptance criteria Per item, and written as a property rather than as "a test named X exists": 1. Mutating the literal in `FleetdAssembly` turns at least one test **red**. Report the mutant output, not only the green run — a surviving mutant and a build that never reached the test phase produce the same empty failure list, so pair it with the surefire report-file count as a control. 2. The test observes the object `assembleAndStart` built. A test that constructs the collaborator itself reproduces the exact blind spot this ticket is about. 3. Name the failing test from the mutant run and say whether any unrelated test also reddened. An extra red is information: it may mean the site was already pinned elsewhere. 4. Do not change the production literals. All three current values are believed correct; this ticket adds tests. `FleetdAssemblyAuthorizationModeTest` (merged for #672) and `FleetdAssemblyLeadTabScannerExclusionTest` (#671) are both working models for driving `assembleAndStart` with a fake `ResourcePorts` that never binds a port. ## One more thing worth deciding Three tickets in two days on one file says the problem is the file, not the three literals. It may be worth one test that walks `FleetdAssembly`'s wiring generically rather than a fourth hand-written pin — but note that a reflective checker paired with a hand-maintained list is exactly the shape that failed in #668, so a generic sweep needs to be reachability-proving, not shape-proving.
ltms commented 2026-10-03 22:30:19 +02:00
Author
Owner
Copy Link
Copy Source

Fixed by PR #688, merged as 2eb2d61 on main (pushed).

FleetdAssemblyTimingDefaultsTest now observes the real ReplyPushLoop, LeadCoordLoop and
StatusPoller built by assembleAndStart, and pins the three constructor args this ticket named.
Test-only; no production change. Merged tree: 1942 tests, 0 failures, BUILD SUCCESS.

The three args are pinned by literal value. The fourth assertion, on StatusPoller, pins the
wiring instead — it compares the field to Injector.POLL_INTERVAL_MILLIS, so both sides move
together if that constant changes. I measured this: mutating the argument to 1000L goes RED
(expected: <250> but was: <1000>), while mutating the constant from 250 to 777 stays green.

That is the right property for that argument and the assertion message says so, but it means the
value 250 is not pinned by this test. Noting it so nobody later assumes otherwise. Closing.

Fixed by PR #688, merged as `2eb2d61` on `main` (pushed). `FleetdAssemblyTimingDefaultsTest` now observes the real `ReplyPushLoop`, `LeadCoordLoop` and `StatusPoller` built by `assembleAndStart`, and pins the three constructor args this ticket named. Test-only; no production change. Merged tree: **1942 tests, 0 failures, BUILD SUCCESS**. The three args are pinned by literal value. The fourth assertion, on `StatusPoller`, pins the **wiring** instead — it compares the field to `Injector.POLL_INTERVAL_MILLIS`, so both sides move together if that constant changes. I measured this: mutating the argument to `1000L` goes RED (`expected: <250> but was: <1000>`), while mutating the constant from `250` to `777` stays green. That is the right property for that argument and the assertion message says so, but it means the value `250` is not pinned by this test. Noting it so nobody later assumes otherwise. Closing.
ltms closed this issue 2026-10-03 22:30:21 +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#675
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?