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

fleetd #677: guard exact lead tab collisions #686

Closed
agent wants to merge 0 commits from worker/677-8cdaaf-5 into main
pull from: worker/677-8cdaaf-5
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-03 22:10:49 +02:00
Member
Copy Link
Copy Source

Adds exact lead-tab collision validation with wildcard template matching, keeps the existing tabPrefix guard, and updates the validator test documentation for fleetd #676. Tests: mvn -o clean install passed.

Adds exact lead-tab collision validation with wildcard template matching, keeps the existing tabPrefix guard, and updates the validator test documentation for fleetd #676. Tests: mvn -o clean install passed.
agent added 1 commit 2026-10-03 22:10:49 +02:00
fleetd #677: guard exact lead tab collisions
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 46s
Details
CI / build (pull_request) Failing after 1m56s
Details
cb4a6869b9
ltms commented 2026-10-03 22:30:51 +02:00
Owner
Copy Link
Copy Source

Do not merge. One confirmed defect, high. Details and the fix are on ticket #677, comment 18261.

Short version: validateLeadTabPrefixes() compares each lead's tab only against
fleet.tabLabel() and the profile overrides. It never compares one lead's tab against another
lead's tab, so two leads sharing one exact tab label load cleanly. I ran that config through the
validator on this branch and it was accepted, with a distinct-tabs control passing in the same run.

That is the case this unit exists for, since the tab label is the only test that decides whether a
session in a pane is read back as a lead.

The rest of the PR is good. Extending the existing validator instead of adding one is the right
call, and I checked the 76 deleted test lines myself: 58 comments, 3 code, no assertion and no
@Test removed, 7 test methods before and after. A replacement PR builds on this commit rather
than redoing it. I will close this one as superseded when that lands.

**Do not merge.** One confirmed defect, high. Details and the fix are on ticket #677, comment 18261. Short version: `validateLeadTabPrefixes()` compares each lead's `tab` only against `fleet.tabLabel()` and the profile overrides. It never compares one lead's `tab` against another lead's `tab`, so two leads sharing one exact tab label load cleanly. I ran that config through the validator on this branch and it was accepted, with a distinct-tabs control passing in the same run. That is the case this unit exists for, since the tab label is the only test that decides whether a session in a pane is read back as a lead. The rest of the PR is good. Extending the existing validator instead of adding one is the right call, and I checked the 76 deleted test lines myself: 58 comments, 3 code, no assertion and no `@Test` removed, 7 test methods before and after. A replacement PR builds on this commit rather than redoing it. I will close this one as superseded when that lands.
ltms commented 2026-10-03 22:45:56 +02:00
Owner
Copy Link
Copy Source

Superseded by #691, which is merged. Closing this unmerged.

PR #691 carries this PR's commit (cb4a686) plus the two-leads-sharing-one-tab guard on top. Merged to main as bfee23a, pushed (7dec74f..bfee23a). Closed by hand, because we merge locally and Gitea never closes a PR by itself.

What I verified myself

Build, in a throwaway worktree, not the main clone. Trial-merged refs/pull/691/head onto origin/main, rm -rf target/surefire-reports, then mvn -o clean install:

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

exit=0, and 173 target/surefire-reports/*.xml files as the stale-XML control. The arithmetic checks: 1942 baseline + 1 (this PR's anExactFleetTabLabelCollisionRefusesToStart) + 2 (#691's new pair) = 1945.

That build covers exactly what landed. After merging, git rev-parse HEAD^{tree} on main is 30c0fc51d8a3a6411c913acef97e681ff53c93bb, equal to the tree I built. Identical trees, so I did not rebuild main.

The worker's file claim was accurate. It said it staged only FleetConfig.java and FleetConfigTest.java. The PR diff against main shows three files, which looked like a discrepancy, so I split the commits: 482598e (the worker's own) touches exactly those two, and the third file comes from this PR's cb4a686. No overreach.

The guard logic. The pair-walk sorts lead names, walks unordered pairs, skips a null or blank tab, and throws its own IllegalStateException separate from the member-template one so each message stays true to its own relation. I read it rather than inferring it from the test.

One mutation I ran that the worker did not, and it survived

The worker mutated the new condition to false && … and saw its collision test go RED — a sound control for that line. I mutated a different thing: a.tab().equalsIgnoreCase(b.tab()) → a.tab().equals(b.tab()), then ran mvn -o -Dtest=FleetConfigTest test:

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

The mutation survived. So the case-insensitivity this method's own javadoc advertises is not pinned by any test.

I then checked whether equalsIgnoreCase is even the right call, and it is. LeadTabScanner.java:145 builds the tab→name map keyed on tab.strip().toLowerCase(Locale.ROOT), and :254 looks up through the same lowercasing. So two lead tabs differing only in case really do collide at runtime — the map put overwrites one — which is precisely the "silently unreachable" harm this guard's message describes. The behaviour is correct and load-bearing, and it is untested. Filed separately; it does not block this merge.

What I did not do

I did not drive a live daemon with two colliding lead tabs. The guard is a startup config validator and the test exercises it directly, so a live run would add nothing I could not already see — but that means "the daemon really refuses to boot" is a code reading plus a unit test, not an observed boot failure.

## Superseded by #691, which is merged. Closing this unmerged. PR #691 carries this PR's commit (`cb4a686`) plus the two-leads-sharing-one-tab guard on top. Merged to `main` as `bfee23a`, pushed (`7dec74f..bfee23a`). Closed by hand, because we merge locally and Gitea never closes a PR by itself. ## What I verified myself **Build, in a throwaway worktree, not the main clone.** Trial-merged `refs/pull/691/head` onto `origin/main`, `rm -rf target/surefire-reports`, then `mvn -o clean install`: ``` Tests run: 1945, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` `exit=0`, and 173 `target/surefire-reports/*.xml` files as the stale-XML control. The arithmetic checks: 1942 baseline + 1 (this PR's `anExactFleetTabLabelCollisionRefusesToStart`) + 2 (#691's new pair) = 1945. **That build covers exactly what landed.** After merging, `git rev-parse HEAD^{tree}` on `main` is `30c0fc51d8a3a6411c913acef97e681ff53c93bb`, equal to the tree I built. Identical trees, so I did not rebuild `main`. **The worker's file claim was accurate.** It said it staged only `FleetConfig.java` and `FleetConfigTest.java`. The PR diff against `main` shows three files, which looked like a discrepancy, so I split the commits: `482598e` (the worker's own) touches exactly those two, and the third file comes from this PR's `cb4a686`. No overreach. **The guard logic.** The pair-walk sorts lead names, walks unordered pairs, skips a null or blank `tab`, and throws its own `IllegalStateException` separate from the member-template one so each message stays true to its own relation. I read it rather than inferring it from the test. ## One mutation I ran that the worker did not, and it survived The worker mutated the new condition to `false && …` and saw its collision test go RED — a sound control for *that* line. I mutated a different thing: `a.tab().equalsIgnoreCase(b.tab())` → `a.tab().equals(b.tab())`, then ran `mvn -o -Dtest=FleetConfigTest test`: ``` Tests run: 155, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **The mutation survived.** So the case-insensitivity this method's own javadoc advertises is not pinned by any test. I then checked whether `equalsIgnoreCase` is even the right call, and it is. `LeadTabScanner.java:145` builds the tab→name map keyed on `tab.strip().toLowerCase(Locale.ROOT)`, and `:254` looks up through the same lowercasing. So two lead tabs differing only in case really do collide at runtime — the map `put` overwrites one — which is precisely the "silently unreachable" harm this guard's message describes. The behaviour is correct and load-bearing, and it is untested. Filed separately; it does not block this merge. ## What I did not do I did not drive a live daemon with two colliding lead tabs. The guard is a startup config validator and the test exercises it directly, so a live run would add nothing I could not already see — but that means "the daemon really refuses to boot" is a code reading plus a unit test, not an observed boot failure.
ltms closed this pull request 2026-10-03 22:46:00 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 46s
Details
CI / build (pull_request) Failing after 1m56s
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#686
Delete Branch "worker/677-8cdaaf-5"

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?