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

fleetd #737 unit 3: probe the fallback terminal and resolve it by lead name #745

Closed
agent wants to merge 0 commits from worker/737-20d1d9-1 into main
pull from: worker/737-20d1d9-1
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-05 05:29:33 +02:00
Member
Copy Link
Copy Source

Fixes the unit 3 defect in #737: ReplyPushLoop.resolveLiveLead dropped a dead per-target delegation and asked PrimaryRegistry.nudgeTargetFor again, but returned that fallback without checking isLive. The fallback can itself be a dead lead's terminal (the single learned slot), so a worker's reply or the idle-lead heartbeat could silently target a closed pane.

Changes:

  • ReplyPushLoop.resolveLiveLead now probes the fallback the same way it probes the first lead, and returns empty (skip this tick) when both are dead.
  • PrimaryRegistry records the delegating lead's name alongside its learned terminal (both for per-target delegations and the singleton), and resolves the name to its current terminal at nudge time via an injected Function<String, String> lookup backed by the live lead-tab scan. A name that cannot currently be placed (unnamed primary, off-host, non-herdr, or a name the scan can't see yet) falls back to the terminal that was actually learned, unchanged from before this unit.
  • Fleetd.currentTerminalForName builds that lookup from the same Supplier<Map<String,String>> of terminal-to-lead-name already used for the lead-tab scan; FleetdAssembly wires it into the PrimaryRegistry construction.
  • FleetMcp.recordPrimarySingleton and the per-target recordDelegation call in sendHandler now pass the caller's name when the caller is a named primary.
  • LeadHeartbeatLoop reads primaryRegistry.currentPrimaryTerminal() (the resolving accessor) instead of the raw learned primaryTerminal(), at all 3 call sites (tick() x2, injectNudge).

Out of scope, left untouched per the unit 3 brief: ownerKey/MessageService (a separate identity/comparison mechanism), isLive's narrowing to agent_not_found, LeadRollover's single-flight key, fleet_handover{open}, and Authz's role table.

Tests: PrimaryRegistryTest gained 9 tests covering name resolution and its fallback for both the per-target delegation and the singleton. ReplyPushLoopTest gained aDoublyDeadFallbackIsNeverTrustedAndStartsNoSchedule, which pairs with the existing aStaleLeadBindingFallsBackToTheLiveLeadInsteadOfNudgingADeadTerminal as a positive control (same stale-lead setup, but there the fallback is live and the nudge does fire). LeadHeartbeatLoopTest gained tickNudgesTheLeadsCurrentTerminalAfterARoll, an end-to-end tick() test proving the heartbeat nudges the lead's post-roll terminal, not the one learned before the roll.

Mutation evidence for the required fix: restoring the exact pre-fix resolveLiveLead (return the fallback with no isLive check) makes aDoublyDeadFallbackIsNeverTrustedAndStartsNoSchedule fail (RED); reverting makes it pass again (GREEN). Both runs done in this worktree.

mvn clean install from fleetd/: Tests run: 2103, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.

Fixes the unit 3 defect in #737: `ReplyPushLoop.resolveLiveLead` dropped a dead per-target delegation and asked `PrimaryRegistry.nudgeTargetFor` again, but returned that fallback without checking `isLive`. The fallback can itself be a dead lead's terminal (the single learned slot), so a worker's reply or the idle-lead heartbeat could silently target a closed pane. Changes: - `ReplyPushLoop.resolveLiveLead` now probes the fallback the same way it probes the first lead, and returns empty (skip this tick) when both are dead. - `PrimaryRegistry` records the delegating lead's name alongside its learned terminal (both for per-target delegations and the singleton), and resolves the name to its current terminal at nudge time via an injected `Function<String, String>` lookup backed by the live lead-tab scan. A name that cannot currently be placed (unnamed primary, off-host, non-herdr, or a name the scan can't see yet) falls back to the terminal that was actually learned, unchanged from before this unit. - `Fleetd.currentTerminalForName` builds that lookup from the same `Supplier<Map<String,String>>` of terminal-to-lead-name already used for the lead-tab scan; `FleetdAssembly` wires it into the `PrimaryRegistry` construction. - `FleetMcp.recordPrimarySingleton` and the per-target `recordDelegation` call in `sendHandler` now pass the caller's name when the caller is a named primary. - `LeadHeartbeatLoop` reads `primaryRegistry.currentPrimaryTerminal()` (the resolving accessor) instead of the raw learned `primaryTerminal()`, at all 3 call sites (`tick()` x2, `injectNudge`). Out of scope, left untouched per the unit 3 brief: `ownerKey`/`MessageService` (a separate identity/comparison mechanism), `isLive`'s narrowing to `agent_not_found`, `LeadRollover`'s single-flight key, `fleet_handover{open}`, and `Authz`'s role table. Tests: `PrimaryRegistryTest` gained 9 tests covering name resolution and its fallback for both the per-target delegation and the singleton. `ReplyPushLoopTest` gained `aDoublyDeadFallbackIsNeverTrustedAndStartsNoSchedule`, which pairs with the existing `aStaleLeadBindingFallsBackToTheLiveLeadInsteadOfNudgingADeadTerminal` as a positive control (same stale-lead setup, but there the fallback is live and the nudge does fire). `LeadHeartbeatLoopTest` gained `tickNudgesTheLeadsCurrentTerminalAfterARoll`, an end-to-end `tick()` test proving the heartbeat nudges the lead's post-roll terminal, not the one learned before the roll. Mutation evidence for the required fix: restoring the exact pre-fix `resolveLiveLead` (return the fallback with no `isLive` check) makes `aDoublyDeadFallbackIsNeverTrustedAndStartsNoSchedule` fail (RED); reverting makes it pass again (GREEN). Both runs done in this worktree. `mvn clean install` from `fleetd/`: `Tests run: 2103, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`.
agent added 2 commits 2026-10-05 05:29:33 +02:00
fleetd #726: rewrite the handover skill for the restart roll 8fa8d18c97 
The roll now ends the pane and launches a fresh process instead of typing
/clear, and that jar is deployed, so the skill's self-dated "a change is
coming" note had to go.

Measured on 2026-10-05 before writing:

  grep -c "lead-rollover: rolled" fleetd/fleetd.out   -> 20
  grep -c "lead-rollover:" fleetd/fleetd.out          -> 86 (control)
  <tail from the last "fleetd listening"> | grep -c "lead-rollover:" -> 0
  grep -c 'RELAUNCH_NEVER_READY\|RELAUNCH_NOT_RECOGNISED\|OLD_PANE_NEVER_DIED' -> 0

So all 20 recorded rolls ran under /clear and the restart path has never
executed. The section says that rather than implying the old numbers
describe it.

Also:
- name all eight RollState outcomes, with what each one guarantees
- state that relaunchReadySeconds bounds each of two waits, not the pair
- drop the "never observed as WORKING after 8 consecutive IDLE/DONE polls"
  paragraph: grep finds that wait is deleted, so it cannot appear
- drop the #621 warning: contextNotice now takes requireOperatorConfirm
- split the surprise bullets into their own section
fleetd #737 unit 3: probe the fallback terminal and resolve it by lead name
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m55s
Details
f4176ae455 
ReplyPushLoop.resolveLiveLead dropped a dead per-target delegation and
retried PrimaryRegistry.nudgeTargetFor, but returned that fallback without
checking isLive. The fallback is now probed the same way the first lead is,
and the method returns empty rather than trust a dead terminal.

PrimaryRegistry records a delegating lead's name alongside its learned
terminal and resolves the name back to its current terminal at nudge time,
through a name-to-terminal lookup backed by the live lead-tab scan. A name
with no current match falls back to the terminal that was actually learned,
so an unnamed primary, an off-host lead, or a non-herdr lead keeps working
exactly as before. LeadHeartbeatLoop now reads the resolved current terminal
instead of the raw learned one.
ltms commented 2026-10-05 05:38:00 +02:00
Owner
Copy Link
Copy Source

Merged locally into main as d438a74, with one lead follow-up commit 803c91e. Closing this PR as the branch is in main.

What I checked myself

The thing I went looking for: a fix that lands at one gate while the old path survives at another. The unit kept primaryTerminal() "for callers that still want it", which is exactly how a half-applied fix hides. Measured against the branch ref:

worker/737-20d1d9-1:.../lead/LeadRollover.java:96:   (javadoc reference only)
worker/737-20d1d9-1:.../mcp/PrimaryRegistry.java:175: public Optional<String> primaryTerminal() {

Zero production callers of the raw accessor. Every live path resolves: LeadHeartbeatLoop.java:310/315/370 all call currentPrimaryTerminal(), and nudgeTargetFor resolves via resolveCurrent(...) before falling back to currentPrimaryTerminal(). So the old path is not merely deprioritised, it is unreachable in production.

Keeping the raw accessor is still the right call, because the tests use it to pin what was recorded separately from what it resolves to (PrimaryRegistryTest, FleetMcpTest:2204-2222). Those are two different properties and collapsing them would lose coverage. The javadoc at :173 says so at the line, which is where that belongs.

Build on the merged result, not on the branch alone. This unit and unit 6 both touch FleetMcp.java, so I did not trust the clean auto-merge. Merged onto main with unit 6 already in, then built from fleetd/: MVN_EXIT=0, BUILD SUCCESS, Tests run: 2108, Failures: 0. Confirmed independently by summing 177 surefire XML files: tests=2108 failures=0 errors=0.

The count reconciles, which is worth stating because it is the cheap check that two merges did not silently drop a test: 2097 after unit 6, plus this unit's 9 + 1 + 1 new tests, is 2108.

One finding — fixed in 803c91e

LeadRollover.java:96 still read:

The first version resolved the pane to clear via {@code PrimaryRegistry#primaryTerminal()}. That is correct for a background loop with no caller (see LeadHeartbeatLoop)

After this unit, that loop uses currentPrimaryTerminal(), so the sentence named the wrong accessor. Fixed to name the resolving one.

Note this is the third stale comment in this ticket's units — the same class of defect I fixed in unit 6 at FleetApp.java:885. The pattern is consistent: when a method's semantics move, the javadoc at the definition gets updated and the prose references elsewhere do not. Worth a hunter sweep for {@code PrimaryRegistry#-style cross-class references at some point; not filing it as a blocker.

Accepted as reported

Mutation evidence is the right shape: the mutation restored the exact pre-fix form (fallback returned with no isLive check), went RED with Tests run: 1, Failures: 1, and the revert was confirmed byte-identical with diff before the green run — that last step is what makes a revert-to-green meaningful rather than assumed.

The positive control deserves specific credit. aDoublyDeadFallbackIsNeverTrustedAndStartsNoSchedule asserts that nothing was nudged, which is the assertion shape that passes when the subject never runs. Pairing it with the pre-existing aStaleLeadBindingFallsBackToTheLiveLeadInsteadOfNudgingADeadTerminal — same setup, live fallback, nudge does fire — is what rules that out, and naming the pairing in the test javadoc means the next reader cannot delete one half without seeing why it exists.

Scope discipline was good: ownerKey/MessageService left to unit 6, LeadRollover's single-flight key left to unit 4, isLive's narrowing untouched and confirmed by reading rather than assumed.

Merged locally into `main` as `d438a74`, with one lead follow-up commit `803c91e`. Closing this PR as the branch is in `main`. ## What I checked myself **The thing I went looking for: a fix that lands at one gate while the old path survives at another.** The unit kept `primaryTerminal()` "for callers that still want it", which is exactly how a half-applied fix hides. Measured against the branch ref: ``` worker/737-20d1d9-1:.../lead/LeadRollover.java:96: (javadoc reference only) worker/737-20d1d9-1:.../mcp/PrimaryRegistry.java:175: public Optional<String> primaryTerminal() { ``` **Zero production callers of the raw accessor.** Every live path resolves: `LeadHeartbeatLoop.java:310/315/370` all call `currentPrimaryTerminal()`, and `nudgeTargetFor` resolves via `resolveCurrent(...)` before falling back to `currentPrimaryTerminal()`. So the old path is not merely deprioritised, it is unreachable in production. Keeping the raw accessor is still the right call, because the tests use it to pin **what was recorded** separately from **what it resolves to** (`PrimaryRegistryTest`, `FleetMcpTest:2204-2222`). Those are two different properties and collapsing them would lose coverage. The javadoc at `:173` says so at the line, which is where that belongs. **Build on the merged result, not on the branch alone.** This unit and unit 6 both touch `FleetMcp.java`, so I did not trust the clean auto-merge. Merged onto `main` with unit 6 already in, then built from `fleetd/`: `MVN_EXIT=0`, `BUILD SUCCESS`, `Tests run: 2108, Failures: 0`. Confirmed independently by summing 177 surefire XML files: `tests=2108 failures=0 errors=0`. The count reconciles, which is worth stating because it is the cheap check that two merges did not silently drop a test: 2097 after unit 6, plus this unit's 9 + 1 + 1 new tests, is 2108. ## One finding — fixed in `803c91e` `LeadRollover.java:96` still read: > `The first version resolved the pane to clear via {@code PrimaryRegistry#primaryTerminal()}. That is correct for a background loop with no caller (see LeadHeartbeatLoop)` After this unit, that loop uses `currentPrimaryTerminal()`, so the sentence named the wrong accessor. Fixed to name the resolving one. Note this is the **third** stale comment in this ticket's units — the same class of defect I fixed in unit 6 at `FleetApp.java:885`. The pattern is consistent: when a method's semantics move, the javadoc at the definition gets updated and the prose references elsewhere do not. Worth a `hunter` sweep for `{@code PrimaryRegistry#`-style cross-class references at some point; not filing it as a blocker. ## Accepted as reported Mutation evidence is the right shape: the mutation restored the exact pre-fix form (fallback returned with no `isLive` check), went RED with `Tests run: 1, Failures: 1`, and the revert was confirmed byte-identical with `diff` before the green run — that last step is what makes a revert-to-green meaningful rather than assumed. The positive control deserves specific credit. `aDoublyDeadFallbackIsNeverTrustedAndStartsNoSchedule` asserts that **nothing** was nudged, which is the assertion shape that passes when the subject never runs. Pairing it with the pre-existing `aStaleLeadBindingFallsBackToTheLiveLeadInsteadOfNudgingADeadTerminal` — same setup, live fallback, nudge *does* fire — is what rules that out, and naming the pairing in the test javadoc means the next reader cannot delete one half without seeing why it exists. Scope discipline was good: `ownerKey`/`MessageService` left to unit 6, `LeadRollover`'s single-flight key left to unit 4, `isLive`'s narrowing untouched and confirmed by reading rather than assumed.
ltms closed this pull request 2026-10-05 05:38:03 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m55s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#745
Delete Branch "worker/737-20d1d9-1"

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?