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

Revoking an architect slot does not revoke it: MemberRegistry freezes fleet.architects, and the reload says it applied #424

New Issue
Closed
opened 2026-09-10 06:45:35 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 06:45:35 +02:00
Owner
Copy Link
Copy Source

This is the reverse of #404 / #416 / #400, which is what #417 went looking for: a site frozen at the boot snapshot for a key ConfigRef classifies as hot. Found by a #417 sweep and verified independently here, line by line, before filing.

The defect

MemberRegistry is built once from the boot snapshot and never re-reads the config:

Fleetd.java:121   FleetConfig cfg = FleetConfig.load(configPath);
Fleetd.java:355   MemberRegistry members = new MemberRegistry(cfg.fleet());

Its constructor flattens every role pool into a map at construction time (MemberRegistry.java:64-76), and its own class javadoc calls slots "A read-only snapshot taken at construction." I checked for any live read: grep -c 'config\.get()\|Supplier' MemberRegistry.java returns 0. There is no supplier and no re-read anywhere in the class.

But ConfigRef's class javadoc says role pools are hot (ConfigRef.java:29-37):

Most of fleet: — every role pool (architects/developers/reviewers), charters, and tabLabel — is read the same live way, through the same supplier (() -> config.get().fleet()).

That sentence is true of CompositePeerLauncher, which really does read the pools live for placement. It is false of MemberRegistry, which reads fleet.architects frozen for identity. Two consumers, one key, opposite behaviour — so fleet.architects is itself a split key sitting inside the already-split fleet: key, and nothing anywhere records that.

Why this is the dangerous direction

Only the ARCHITECT role reads the snapshot again at runtime. requireSlotFor (MemberRegistry.java:258-278) and reserve (:281-295) both return immediately for DEV and REVIEWER, then match a spawn's profile against slotsFor(MemberRole.ARCHITECT) — the frozen map.

So the operator removes an architect slot, or repoints it at a different profile, intending to revoke it. The config reloads. And a later fleet_spawn{role: "architect", profile: "<the removed one>"} is still granted, because requireSlotFor is checking the pre-edit list. An architect is materially more privileged than a worker — it may send turns and delegate — so a live privilege the operator believes is closed stays open, with no warning anywhere. That is the same overstate-a-capability direction as #404 and #416, one hop further from ConfigRef.

The other direction happens too and is merely annoying, but it misdiagnoses itself. Add a slot, and the spawn throws no architect slot for profile '<X>' — ... fleet.architects carries profiles: <stale set>. That message reads as an operator typo, so it sends someone to re-check YAML they already got right, instead of to a restart.

What the operator is told, which is the part that makes this a filing

There are two false statements, and the second is worse because the daemon asserts it rather than merely omitting it.

1. Edit only fleet.architects. No branch in ConfigRef names architects outside its javadoc, so changedColdKeys / changedDeferredKeys / changedSplitKeys all add nothing. Outcome.summary() reports a bare "config reloaded" and applied() is true. Nothing deferred, nothing split, nothing to read. The operator has every reason to think it took effect.

2. Edit fleet.leaders as well. Now the split branch fires, and its message says this, verbatim (ConfigRef.java:543-551):

the rest of fleet: (architects, developers, reviewers, charters, tabLabel) is read live through the supplier on CompositePeerLauncher and already applied

For fleet.architects that is not true. The daemon is telling the operator the change applied, at the exact moment they are most likely to be reading reload output carefully.

Note the sentence is also accurate about the placement path. That is what makes it hard to spot, and it is the #404 lesson again: a claim that is correct about one consumer of a key, copied onto a key with two consumers.

Reachability

Fully reachable, no unusual configuration needed:

  1. Daemon running, configReload: enabled, fleet.architects declaring at least one slot.
  2. Operator edits fleet.architects — removes a slot, or changes a slot's profile.
  3. Reload reports success, with nothing deferred (case 1) or an explicit "already applied" (case 2).
  4. fleet_spawn{role: "architect", profile: <the old one>} is still granted.

Why nothing catches it

ConfigRefTopLevelCoverageTest, ConfigRefTopLevelReportingCoverageTest and ConfigRefProfileCoverageTest all prove ConfigRef's own key sets are internally consistent with FleetConfig's record shape. None of them touches MemberRegistry — a second, independent consumer in a different package that ConfigRef has no way to learn about. The checkers verify the classification against the config record, never against the consumers, so a consumer that disagrees with the classification is invisible to all three.

That is the gap worth fixing beyond this one instance: a key's class is a claim about every site that reads it, and nothing tests it that way.

What is wanted

State the goal, not the mechanism — pick the implementation yourself.

The architect slot check must see an edit to fleet.architects without a restart, or the reload must stop claiming it applied. Those are the only two honest end states. Either is acceptable, and the choice is a real design call:

  • Making it hot is the better outcome, and matches what the docs already promise. The care needed: MemberRegistry also owns terminal bindings, which are live mutable state it created, not config. A rebuild must not drop or invalidate an existing architect's binding, and a slot that is currently bound and then removed from config needs a defined answer. Say what you chose.
  • Making the report honest is the smaller change: fleet.architects moves into the frozen half of the fleet: split key, the "already applied" sentence is corrected, and a branch is added so that editing only the architect pool is reported at all rather than silently.

Do not do half of the first option. A rebuild that fixes requireSlotFor while leaving reserve reading a stale map would be a one-way gate, and both are on the spawn path.

Acceptance

  • A test that performs a real ConfigRef.reload() removing an architect slot, then asserts the spawn-side check refuses that profile. Assert applied() on the reload, so the test proves the reload really happened rather than passing because nothing changed.
  • The mirror test: a slot added by reload becomes usable, or the reload says a restart is needed. Whichever end state you chose, both directions get a test — one direction alone would pass on a registry that refuses everything.
  • If you chose "make it hot": a test that an already-bound architect survives the rebuild.
  • If you chose "correct the report": editing only fleet.architects must produce a non-empty report. Today it produces nothing at all.
  • Mutation proof: break your fix on purpose and quote the failing test name and assertion for each direction. A mutation that leaves the suite green means that half is not pinned.

Out of scope

  • fleet.developers / fleet.reviewers in this registry are dead data, and this ticket does not change them. requireSlotFor and reserve both return early for those roles — see the comment at MemberRegistry.java:247-250: those pools are placement candidates only, never a live identity binding. Do not "fix" them; do not delete them either without a separate ticket.
  • The frozen default-profile problem in the same sweep is filed separately. Same root key, different consumer, different fix.
  • Do not add a ConfigRef exclusion to make a coverage test green.

Found by the #417 reverse-mismatch sweep. Verified here: Fleetd.java:121/355, MemberRegistry.java:20-28/64-76/247-250/258-295, ConfigRef.java:29-37/119/532/543-551, and the zero-hit live-read grep above.

This is the **reverse** of #404 / #416 / #400, which is what #417 went looking for: a site frozen at the boot snapshot for a key `ConfigRef` classifies as **hot**. Found by a #417 sweep and verified independently here, line by line, before filing. ## The defect `MemberRegistry` is built once from the boot snapshot and never re-reads the config: ``` Fleetd.java:121 FleetConfig cfg = FleetConfig.load(configPath); Fleetd.java:355 MemberRegistry members = new MemberRegistry(cfg.fleet()); ``` Its constructor flattens every role pool into a map at construction time (`MemberRegistry.java:64-76`), and its own class javadoc calls `slots` "A read-only snapshot taken at construction." I checked for any live read: `grep -c 'config\.get()\|Supplier' MemberRegistry.java` returns **0**. There is no supplier and no re-read anywhere in the class. But `ConfigRef`'s class javadoc says role pools are **hot** (`ConfigRef.java:29-37`): > Most of `fleet:` — every role pool (`architects`/`developers`/`reviewers`), `charters`, and `tabLabel` — is read the same live way, through the same supplier (`() -> config.get().fleet()`). That sentence is true of `CompositePeerLauncher`, which really does read the pools live for **placement**. It is false of `MemberRegistry`, which reads `fleet.architects` frozen for **identity**. Two consumers, one key, opposite behaviour — so `fleet.architects` is itself a split key sitting inside the already-split `fleet:` key, and nothing anywhere records that. ## Why this is the dangerous direction Only the ARCHITECT role reads the snapshot again at runtime. `requireSlotFor` (`MemberRegistry.java:258-278`) and `reserve` (`:281-295`) both return immediately for DEV and REVIEWER, then match a spawn's `profile` against `slotsFor(MemberRole.ARCHITECT)` — the frozen map. So the operator removes an architect slot, or repoints it at a different profile, intending to **revoke** it. The config reloads. And a later `fleet_spawn{role: "architect", profile: "<the removed one>"}` is still granted, because `requireSlotFor` is checking the pre-edit list. An architect is materially more privileged than a worker — it may send turns and delegate — so **a live privilege the operator believes is closed stays open, with no warning anywhere.** That is the same overstate-a-capability direction as #404 and #416, one hop further from `ConfigRef`. The other direction happens too and is merely annoying, but it misdiagnoses itself. Add a slot, and the spawn throws `no architect slot for profile '<X>' — ... fleet.architects carries profiles: <stale set>`. That message reads as an operator typo, so it sends someone to re-check YAML they already got right, instead of to a restart. ## What the operator is told, which is the part that makes this a filing There are two false statements, and the second is worse because the daemon asserts it rather than merely omitting it. **1. Edit only `fleet.architects`.** No branch in `ConfigRef` names `architects` outside its javadoc, so `changedColdKeys` / `changedDeferredKeys` / `changedSplitKeys` all add nothing. `Outcome.summary()` reports a bare "config reloaded" and `applied()` is true. Nothing deferred, nothing split, nothing to read. The operator has every reason to think it took effect. **2. Edit `fleet.leaders` as well.** Now the split branch fires, and its message says this, verbatim (`ConfigRef.java:543-551`): > the rest of fleet: (architects, developers, reviewers, charters, tabLabel) is read live through the supplier on CompositePeerLauncher and **already applied** For `fleet.architects` that is not true. The daemon is telling the operator the change applied, at the exact moment they are most likely to be reading reload output carefully. Note the sentence is *also* accurate about the placement path. That is what makes it hard to spot, and it is the #404 lesson again: a claim that is correct about one consumer of a key, copied onto a key with two consumers. ## Reachability Fully reachable, no unusual configuration needed: 1. Daemon running, `configReload:` enabled, `fleet.architects` declaring at least one slot. 2. Operator edits `fleet.architects` — removes a slot, or changes a slot's `profile`. 3. Reload reports success, with nothing deferred (case 1) or an explicit "already applied" (case 2). 4. `fleet_spawn{role: "architect", profile: <the old one>}` is still granted. ## Why nothing catches it `ConfigRefTopLevelCoverageTest`, `ConfigRefTopLevelReportingCoverageTest` and `ConfigRefProfileCoverageTest` all prove `ConfigRef`'s own key sets are internally consistent with `FleetConfig`'s record shape. None of them touches `MemberRegistry` — a second, independent consumer in a different package that `ConfigRef` has no way to learn about. The checkers verify the classification against the *config record*, never against the *consumers*, so a consumer that disagrees with the classification is invisible to all three. That is the gap worth fixing beyond this one instance: a key's class is a claim about **every** site that reads it, and nothing tests it that way. ## What is wanted State the goal, not the mechanism — pick the implementation yourself. **The architect slot check must see an edit to `fleet.architects` without a restart, or the reload must stop claiming it applied.** Those are the only two honest end states. Either is acceptable, and the choice is a real design call: - Making it hot is the better outcome, and matches what the docs already promise. The care needed: `MemberRegistry` also owns **terminal bindings**, which are live mutable state it created, not config. A rebuild must not drop or invalidate an existing architect's binding, and a slot that is currently bound and then removed from config needs a defined answer. Say what you chose. - Making the report honest is the smaller change: `fleet.architects` moves into the frozen half of the `fleet:` split key, the "already applied" sentence is corrected, and a branch is added so that editing *only* the architect pool is reported at all rather than silently. Do **not** do half of the first option. A rebuild that fixes `requireSlotFor` while leaving `reserve` reading a stale map would be a one-way gate, and both are on the spawn path. ## Acceptance - A test that performs a real `ConfigRef.reload()` removing an architect slot, then asserts the spawn-side check refuses that profile. Assert `applied()` on the reload, so the test proves the reload really happened rather than passing because nothing changed. - The mirror test: a slot **added** by reload becomes usable, or the reload says a restart is needed. Whichever end state you chose, both directions get a test — one direction alone would pass on a registry that refuses everything. - If you chose "make it hot": a test that an already-bound architect survives the rebuild. - If you chose "correct the report": editing only `fleet.architects` must produce a non-empty report. Today it produces nothing at all. - Mutation proof: break your fix on purpose and quote the failing test name and assertion for each direction. A mutation that leaves the suite green means that half is not pinned. ## Out of scope - `fleet.developers` / `fleet.reviewers` in this registry are **dead data**, and this ticket does not change them. `requireSlotFor` and `reserve` both return early for those roles — see the comment at `MemberRegistry.java:247-250`: those pools are placement candidates only, never a live identity binding. Do not "fix" them; do not delete them either without a separate ticket. - The frozen default-profile problem in the same sweep is filed separately. Same root key, different consumer, different fix. - Do not add a `ConfigRef` exclusion to make a coverage test green. Found by the #417 reverse-mismatch sweep. Verified here: `Fleetd.java:121/355`, `MemberRegistry.java:20-28/64-76/247-250/258-295`, `ConfigRef.java:29-37/119/532/543-551`, and the zero-hit live-read grep above.
ltms commented 2026-09-10 14:38:48 +02:00
Author
Owner
Copy Link
Copy Source

Closing: this was already fixed and merged, and the ticket was simply never closed.

Why it stayed open

The fix landed as PR #428, merged in 7667727. The ticket was not closed with it. I only found out by putting a worker on it, which then correctly reported there was no work left. That wasted turn is my fault, not the worker's — I built the unit list from Gitea's open tickets without first checking whether main already contained the fix.

What I verified myself, in my own clone, on origin/main = 1fb6176

Commits are ancestors of origin/main:

  • 7f672f0 fleetd #424: revoke the ARCHITECT privilege on reload, not just future spawns
  • ce74e16 fleetd #424: make architect-slot identity checks read fleet.architects live
  • 7667727 Merge #428: revoke the ARCHITECT privilege on reload, not just future spawns

Control for that ancestry check: of 348 remote branches, 43 are not ancestors of origin/main, so git merge-base --is-ancestor does discriminate here rather than answering yes to everything.

The chosen end state was "make it hot", the better of the two the ticket offered. MemberRegistry now has 0 frozen FleetConfig.Fleet fields and 4 Supplier<FleetConfig.Fleet> references, with slots() reading flatten(fleet.get()) on every call.

Acceptance criteria, checked against the test

MemberRegistryLiveTest (360 lines, 11 @Test, 12 real ConfigRef.reload() calls, 13 applied() assertions). Every criterion has a named test, both directions:

  • removal revokes: requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload, reserveRefusesAProfileWhoseSlotWasRemovedByReload
  • the mirror, addition becomes usable: requireSlotForAllowsAProfileWhoseSlotWasAddedByReload, reserveAllowsAProfileWhoseSlotWasAddedByReload
  • the one-way-gate worry the ticket called out: both requireSlotFor and reserve are covered, so the pair cannot drift apart
  • an already-bound architect: anArchitectAlreadyBoundToASlotIsDemotedByReload, theOriginalBindingStillOccupiesTheRemovedSlotSoASecondTerminalCannotClaimIt, unbindStillSucceedsForTheOriginalTerminalAfterItsSlotIsRemoved

My own mutation, because named tests are not proof the behaviour is pinned

Run on 1fb6176 in a throwaway worktree. I re-froze slots() — the supplier stays, but the first flatten is cached forever, which reproduces the exact defect this ticket describes:

private Map<String, Entry> frozenZ;
public Map<String, Entry> slots() {
    if (frozenZ == null) { frozenZ = flatten(fleet.get()); }
    return frozenZ;
}

Harness-proof cell first, selector alone, unmutated: rc=0, Tests run: 11, Failures: 0, and No tests matching pattern count 0 — so the cell below is not void.

Mutant: rc=1, Tests run: 11, Failures: 8, Errors: 1. Nine of eleven tests fail. The fix is genuinely pinned, not just committed.

(My failed-method name extraction printed empty in that run — a regex fault on my side — so I am quoting the summary lines, which are unambiguous, rather than method names I did not actually capture.)

Residue, not reopened here

The ticket's own closing point stands and is untouched: nothing tests a key's hot/cold classification against its consumers. ConfigRefTopLevelCoverageTest and friends check the classification against the config record only, so a second consumer in another package that disagrees stays invisible. The worker's grep-level survey found no new instance of the frozen-field shape — only MemberRegistry (now live) and CompositePeerLauncher (already live) take a bare FleetConfig.Fleet, and the other hot keys are read through config.get() or a per-call resolver. That was a grep, not a reflection-based coverage check, so it is a weak negative rather than a proof.

Verified on origin/main = 1fb6176.

Closing: this was already fixed and merged, and the ticket was simply never closed. ## Why it stayed open The fix landed as PR #428, merged in `7667727`. The ticket was not closed with it. I only found out by putting a worker on it, which then correctly reported there was no work left. That wasted turn is my fault, not the worker's — I built the unit list from Gitea's open tickets without first checking whether main already contained the fix. ## What I verified myself, in my own clone, on `origin/main` = `1fb6176` Commits are ancestors of `origin/main`: - `7f672f0` fleetd #424: revoke the ARCHITECT privilege on reload, not just future spawns - `ce74e16` fleetd #424: make architect-slot identity checks read fleet.architects live - `7667727` Merge #428: revoke the ARCHITECT privilege on reload, not just future spawns Control for that ancestry check: of 348 remote branches, 43 are **not** ancestors of `origin/main`, so `git merge-base --is-ancestor` does discriminate here rather than answering yes to everything. The chosen end state was "make it hot", the better of the two the ticket offered. `MemberRegistry` now has **0** frozen `FleetConfig.Fleet` fields and **4** `Supplier<FleetConfig.Fleet>` references, with `slots()` reading `flatten(fleet.get())` on every call. ## Acceptance criteria, checked against the test `MemberRegistryLiveTest` (360 lines, 11 `@Test`, 12 real `ConfigRef.reload()` calls, 13 `applied()` assertions). Every criterion has a named test, both directions: - removal revokes: `requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload`, `reserveRefusesAProfileWhoseSlotWasRemovedByReload` - the mirror, addition becomes usable: `requireSlotForAllowsAProfileWhoseSlotWasAddedByReload`, `reserveAllowsAProfileWhoseSlotWasAddedByReload` - the one-way-gate worry the ticket called out: **both** `requireSlotFor` and `reserve` are covered, so the pair cannot drift apart - an already-bound architect: `anArchitectAlreadyBoundToASlotIsDemotedByReload`, `theOriginalBindingStillOccupiesTheRemovedSlotSoASecondTerminalCannotClaimIt`, `unbindStillSucceedsForTheOriginalTerminalAfterItsSlotIsRemoved` ## My own mutation, because named tests are not proof the behaviour is pinned Run on `1fb6176` in a throwaway worktree. I re-froze `slots()` — the supplier stays, but the first `flatten` is cached forever, which reproduces the exact defect this ticket describes: ```java private Map<String, Entry> frozenZ; public Map<String, Entry> slots() { if (frozenZ == null) { frozenZ = flatten(fleet.get()); } return frozenZ; } ``` Harness-proof cell first, selector alone, unmutated: `rc=0`, `Tests run: 11, Failures: 0`, and `No tests matching pattern` count **0** — so the cell below is not void. Mutant: `rc=1`, `Tests run: 11, Failures: 8, Errors: 1`. Nine of eleven tests fail. The fix is genuinely pinned, not just committed. (My failed-method name extraction printed empty in that run — a regex fault on my side — so I am quoting the summary lines, which are unambiguous, rather than method names I did not actually capture.) ## Residue, not reopened here The ticket's own closing point stands and is untouched: nothing tests a key's hot/cold classification **against its consumers**. `ConfigRefTopLevelCoverageTest` and friends check the classification against the config record only, so a second consumer in another package that disagrees stays invisible. The worker's grep-level survey found no new instance of the frozen-field shape — only `MemberRegistry` (now live) and `CompositePeerLauncher` (already live) take a bare `FleetConfig.Fleet`, and the other hot keys are read through `config.get()` or a per-call resolver. That was a grep, not a reflection-based coverage check, so it is a weak negative rather than a proof. Verified on `origin/main` = `1fb6176`.
ltms closed this issue 2026-09-10 14:38:59 +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#424
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?