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

MemberRegistry: profileForSlot, nameForSlot and isSlot were made live with no test pinning any of them #431

New Issue
Open
opened 2026-09-10 07:56:06 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 07:56:06 +02:00
Owner
Copy Link
Copy Source

Follow-up to #424, merged as 7667727.

#424 changed MemberRegistry.slots() to re-read fleet: through a supplier on every call. Five methods read it. The PR's own tests pin roleForSlot well — the worker mutated it three ways plus a mirror, and each killed exactly the right test.

The other three are not pinned at all. I mutated each one to read a snapshot flattened once in the constructor, one method at a time. Each mutation printed its own method name and line, and the count of live slots() readers dropped 5 -> 4 to prove only one site moved:

Mutation Result
profileForSlot reads a frozen snapshot 1535 tests, 0 failures
nameForSlot reads a frozen snapshot 1535 tests, 0 failures
isSlot reads a frozen snapshot 1535 tests, 0 failures

All three passed. Nothing in the suite can tell the live version from the frozen one.

Why it matters

profileForSlot is the one with teeth. The spawn lifecycle reads it to choose which backend an architect slot stands up on. If it were frozen — which is exactly the state #424 just fixed it out of — then an operator who repoints a slot to another profile and reloads would get a spawn on the old profile, and the whole suite would stay green. That is #424's defect in a second consumer.

isSlot gates bind/reserve. It is public, and it can answer "yes, that is a configured architect slot" about a slot the operator deleted, with no test objecting. The four reload-direction tests in MemberRegistryLiveTest pass through reserve/requireSlotFor, so they never reach isSlot directly.

nameForSlot is the least risky of the three, but it is the same gap.

This is the pattern from #404 and #425: a key is classified correctly and then only one of its consumers is checked. The class-level ConfigRef coverage tests compare the classification against the config record, never against a consumer, so they cannot catch this.

Scope

One test per method, each driving a real ConfigRef.reload() against a @TempDir file and asserting Outcome.applied() — the pattern MemberRegistryLiveTest already uses. Do not compare two frozen registries in memory.

  1. profileForSlot: a reload that repoints an architect slot to a different profile makes profileForSlot return the new profile. Assert through whatever the spawn lifecycle actually calls, not only the accessor, if a seam exists for it.
  2. isSlot: a reload that removes a slot makes isSlot return false; a reload that adds one makes it return true. Both directions.
  3. nameForSlot: a reload that renames the configured name behind a qualified key is reflected.

Acceptance

For each of the three, a mutation proof that freezes that method alone and kills that test alone. Print the method name, the before/after occurrence count, and the count of every other slots() reader in the file — a bare 5 -> 4 does not say which one moved.

Follow-up to #424, merged as `7667727`. #424 changed `MemberRegistry.slots()` to re-read `fleet:` through a supplier on every call. Five methods read it. The PR's own tests pin `roleForSlot` well — the worker mutated it three ways plus a mirror, and each killed exactly the right test. The other three are not pinned at all. I mutated each one to read a snapshot flattened once in the constructor, one method at a time. Each mutation printed its own method name and line, and the count of live `slots()` readers dropped 5 -> 4 to prove only one site moved: | Mutation | Result | |---|---| | `profileForSlot` reads a frozen snapshot | **1535 tests, 0 failures** | | `nameForSlot` reads a frozen snapshot | **1535 tests, 0 failures** | | `isSlot` reads a frozen snapshot | **1535 tests, 0 failures** | All three passed. Nothing in the suite can tell the live version from the frozen one. ## Why it matters `profileForSlot` is the one with teeth. The spawn lifecycle reads it to choose which backend an architect slot stands up on. If it were frozen — which is exactly the state #424 just fixed it out of — then an operator who repoints a slot to another profile and reloads would get a spawn on the **old** profile, and the whole suite would stay green. That is #424's defect in a second consumer. `isSlot` gates `bind`/`reserve`. It is public, and it can answer "yes, that is a configured architect slot" about a slot the operator deleted, with no test objecting. The four reload-direction tests in `MemberRegistryLiveTest` pass through `reserve`/`requireSlotFor`, so they never reach `isSlot` directly. `nameForSlot` is the least risky of the three, but it is the same gap. This is the pattern from #404 and #425: a key is classified correctly and then only one of its consumers is checked. The class-level `ConfigRef` coverage tests compare the classification against the config **record**, never against a consumer, so they cannot catch this. ## Scope One test per method, each driving a real `ConfigRef.reload()` against a `@TempDir` file and asserting `Outcome.applied()` — the pattern `MemberRegistryLiveTest` already uses. Do not compare two frozen registries in memory. 1. `profileForSlot`: a reload that repoints an architect slot to a different profile makes `profileForSlot` return the new profile. Assert through whatever the spawn lifecycle actually calls, not only the accessor, if a seam exists for it. 2. `isSlot`: a reload that removes a slot makes `isSlot` return false; a reload that adds one makes it return true. Both directions. 3. `nameForSlot`: a reload that renames the configured name behind a qualified key is reflected. ## Acceptance For each of the three, a mutation proof that freezes **that** method alone and kills **that** test alone. Print the method name, the before/after occurrence count, and the count of every other `slots()` reader in the file — a bare `5 -> 4` does not say which one moved.
ltms commented 2026-09-10 08:18:02 +02:00
Author
Owner
Copy Link
Copy Source

Correction — my severity ranking in this ticket was wrong in three places

The worker on PR #432 grepped for profileForSlot and found no caller. That contradicted my own ticket text, so I measured it myself. The worker is right and I was wrong. Here is what I ran, in the tree at a196d34, from fleetd/src:

$ grep -rn '\.profileForSlot(\|::profileForSlot' main --include='*.java'
(no output)
$ grep -rn '\.resolve(' main --include='*.java' | wc -l
27                # control: the grep itself works

What the ticket claimed, and what is true

1. profileForSlot — I wrote "the one with teeth. The spawn lifecycle reads it to choose which backend an architect slot stands up on."

False. It has no caller in src/main in either form, .profileForSlot( or ::profileForSlot. I took that sentence from the method's own javadoc without checking it:

/** The strong-model profile a slot runs under — what the spawn lifecycle reads. */
public String profileForSlot(String slotName) {     // MemberRegistry.java:185

That javadoc describes a caller that does not exist. So the accessor is dead in production, and the javadoc is a second, separate defect — it is the kind of prose a future session will read as a fact about the code.

2. nameForSlot — I called it the least risky of the three.

Also wrong, and wrong in the other direction. It is wired:

members == null ? null : members::roleForSlot,      // CallerResolver.java:136
members == null ? null : members::nameForSlot);     // CallerResolver.java:137

My first audit missed this because I only grepped the .nameForSlot( form. A method reference is invisible to that pattern. nameForSlot sits on the same identity path as roleForSlot, which is the method #424 was about, so it is the most wired of the three — not the least.

3. isSlot — I described it as public API.

It is reachable, but not that way. It has no external caller; it is called inside the class, at MemberRegistry.java:235 (in bind) and :376 (in the bind(SlotReservation, …) overload). So it is reached through bind, and a test that drives bind covers it. Pinning it directly is still right, because bind is what refuses an unknown slot.

Corrected ranking

method production reach why the live read matters
nameForSlot wired at CallerResolver.java:137 on the architect identity path, beside roleForSlot
isSlot internal, :235 and :376 inside bind bind refuses an unknown slot through it
profileForSlot none nothing reads it; pinning it protects the accessor only

The four tests in PR #432 are still all worth having. The ranking was wrong; the scope was not.

Two follow-ups this opens, which are not part of #432

  • The profileForSlot javadoc says the spawn lifecycle reads it. Nothing does. Either wire it or delete both the method and the sentence.
  • A separate report from the #424 worker says CallerResolver.members() also has no production caller. I have not checked that one myself.

I will file these as their own ticket rather than widen a test-only PR.

One more correction, to an audit inside this comment's own method

My first pass at this audit was a broken command. I ran grep -rn "…" "$R" --include=*.java under zsh, which expands *.java before grep sees it, so grep never ran and every method reported zero callers. That reads exactly like "nothing is wired". Every count above comes from the re-run with the pattern quoted, and with the .resolve( control in the same pass to prove the grep fires at all.

## Correction — my severity ranking in this ticket was wrong in three places The worker on PR #432 grepped for `profileForSlot` and found no caller. That contradicted my own ticket text, so I measured it myself. The worker is right and I was wrong. Here is what I ran, in the tree at `a196d34`, from `fleetd/src`: ``` $ grep -rn '\.profileForSlot(\|::profileForSlot' main --include='*.java' (no output) $ grep -rn '\.resolve(' main --include='*.java' | wc -l 27 # control: the grep itself works ``` ### What the ticket claimed, and what is true **1. `profileForSlot` — I wrote "the one with teeth. The spawn lifecycle reads it to choose which backend an architect slot stands up on."** False. It has no caller in `src/main` in either form, `.profileForSlot(` or `::profileForSlot`. I took that sentence from the method's own javadoc without checking it: ```java /** The strong-model profile a slot runs under — what the spawn lifecycle reads. */ public String profileForSlot(String slotName) { // MemberRegistry.java:185 ``` That javadoc describes a caller that does not exist. So the accessor is dead in production, and the javadoc is a second, separate defect — it is the kind of prose a future session will read as a fact about the code. **2. `nameForSlot` — I called it the least risky of the three.** Also wrong, and wrong in the other direction. It is wired: ```java members == null ? null : members::roleForSlot, // CallerResolver.java:136 members == null ? null : members::nameForSlot); // CallerResolver.java:137 ``` My first audit missed this because I only grepped the `.nameForSlot(` form. A method reference is invisible to that pattern. `nameForSlot` sits on the same identity path as `roleForSlot`, which is the method #424 was about, so it is the *most* wired of the three — not the least. **3. `isSlot` — I described it as public API.** It is reachable, but not that way. It has no external caller; it is called inside the class, at `MemberRegistry.java:235` (in `bind`) and `:376` (in the `bind(SlotReservation, …)` overload). So it is reached through `bind`, and a test that drives `bind` covers it. Pinning it directly is still right, because `bind` is what refuses an unknown slot. ### Corrected ranking | method | production reach | why the live read matters | |---|---|---| | `nameForSlot` | wired at `CallerResolver.java:137` | on the architect identity path, beside `roleForSlot` | | `isSlot` | internal, `:235` and `:376` inside `bind` | `bind` refuses an unknown slot through it | | `profileForSlot` | **none** | nothing reads it; pinning it protects the accessor only | The four tests in PR #432 are still all worth having. The ranking was wrong; the scope was not. ### Two follow-ups this opens, which are not part of #432 - The `profileForSlot` javadoc says the spawn lifecycle reads it. Nothing does. Either wire it or delete both the method and the sentence. - A separate report from the #424 worker says `CallerResolver.members()` also has no production caller. I have not checked that one myself. I will file these as their own ticket rather than widen a test-only PR. ### One more correction, to an audit inside this comment's own method My first pass at this audit was a broken command. I ran `grep -rn "…" "$R" --include=*.java` under zsh, which expands `*.java` before grep sees it, so grep never ran and every method reported zero callers. That reads exactly like "nothing is wired". Every count above comes from the re-run with the pattern quoted, and with the `.resolve(` control in the same pass to prove the grep fires at all.
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#431
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?