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

fleetd #719: fold a per-boot nonce into every ticket id #728

Closed
agent wants to merge 0 commits from worker/719-bdd95e-4 into main
pull from: worker/719-bdd95e-4
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-04 18:05:32 +02:00
Member
Copy Link
Copy Source

Closes fleetd #719.

The defect: ticketSeq (MessageService.java:357, was :351) restarts at zero on every daemon boot, with no persistence. A ticket id minted in one boot can be reused by a later boot and silently resolve to an unrelated Task, instead of failing to resolve.

The fix (option 1, decided on the ticket — option 2 was filed and withdrawn, option 3 explicitly not shipping): a short nonce (UUID.randomUUID().toString().substring(0,6)) minted once per MessageService instance and folded into every ticket id: task-<nonce>-<n>. An id minted by one instance can never match another instance's id space, so a stale id now fails to resolve (poll returns null) instead of silently rebinding to a different unit.

Tests added (MessageServiceTest):

  • twoInstancesMintDisjointTicketIds — two independent MessageService instances mint disjoint ids even for their first ticket each.
  • foreignInstanceTicketDoesNotResolve — a ticket minted by one instance does not resolve via poll on a second, independent instance, with the positive control that it does resolve on the instance that minted it.

Scope check performed: confirmed via grep that production code has exactly one reference to the task- literal (the mint itself) and that the 8 real assertions in ReplyPushLoopTest (and the never-minted task-999999/task-999 ids in MessageServiceTest/FleetMcpTest) are untouched by the format change — all pass unedited.

Not shipped (by design, per the ticket): naming the stale-boot cause in poll's response. Doing so safely would need a second lookup/parse that could disagree with the primary lookup, which the ticket explicitly rules out ("one lookup, one answer"). poll keeps reporting a foreign-boot id the same way it reports a never-minted id: unknown (null).

Wiki: could not update wiki/11-Features.md — this worktree's wiki/ submodule is uninitialized. Entry text handed to the lead in the PR/ticket thread for them to add from the main clone.

Out of scope, reported not fixed (per the ticket's last comment): turnId (Rendezvous.java, session + "#" + askSeq.incrementAndGet()) has the same shape — a per-boot AtomicLong with no persistence, folded into an id a caller can hold and present back. It can collide across a restart the same way ticket ids did. Not fixed here; flagging only, as instructed.

Build: mvn clean install — Tests run: 2049, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Closes fleetd #719. **The defect:** `ticketSeq` (MessageService.java:357, was :351) restarts at zero on every daemon boot, with no persistence. A ticket id minted in one boot can be reused by a later boot and silently resolve to an unrelated `Task`, instead of failing to resolve. **The fix (option 1, decided on the ticket — option 2 was filed and withdrawn, option 3 explicitly not shipping):** a short nonce (`UUID.randomUUID().toString().substring(0,6)`) minted once per `MessageService` instance and folded into every ticket id: `task-<nonce>-<n>`. An id minted by one instance can never match another instance's id space, so a stale id now fails to resolve (`poll` returns null) instead of silently rebinding to a different unit. **Tests added (MessageServiceTest):** - `twoInstancesMintDisjointTicketIds` — two independent `MessageService` instances mint disjoint ids even for their first ticket each. - `foreignInstanceTicketDoesNotResolve` — a ticket minted by one instance does not resolve via `poll` on a second, independent instance, with the positive control that it does resolve on the instance that minted it. **Scope check performed:** confirmed via grep that production code has exactly one reference to the `task-` literal (the mint itself) and that the 8 real assertions in `ReplyPushLoopTest` (and the never-minted `task-999999`/`task-999` ids in `MessageServiceTest`/`FleetMcpTest`) are untouched by the format change — all pass unedited. **Not shipped (by design, per the ticket):** naming the stale-boot cause in `poll`'s response. Doing so safely would need a second lookup/parse that could disagree with the primary lookup, which the ticket explicitly rules out ("one lookup, one answer"). `poll` keeps reporting a foreign-boot id the same way it reports a never-minted id: unknown (null). **Wiki:** could not update `wiki/11-Features.md` — this worktree's `wiki/` submodule is uninitialized. Entry text handed to the lead in the PR/ticket thread for them to add from the main clone. **Out of scope, reported not fixed (per the ticket's last comment):** `turnId` (`Rendezvous.java`, `session + "#" + askSeq.incrementAndGet()`) has the same shape — a per-boot `AtomicLong` with no persistence, folded into an id a caller can hold and present back. It can collide across a restart the same way ticket ids did. Not fixed here; flagging only, as instructed. **Build:** `mvn clean install` — `Tests run: 2049, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`.
agent added 1 commit 2026-10-04 18:05:32 +02:00
fleetd #719: fold a per-boot nonce into every ticket id
CI / shell-tests (pull_request) Failing after 8s
Details
CI / contract (pull_request) Successful in 1m1s
Details
CI / build (pull_request) Failing after 2m0s
Details
337dbd491e 
ticketSeq restarted at zero on every daemon boot with no persistence, so a
ticket id minted in one boot could be reused by a later boot and resolve to
an unrelated Task instead of failing to resolve at all. Mint each
MessageService instance's own short nonce once and fold it into every ticket
(task-<nonce>-<n>), so an id from one instance can never match another's id
space.

Adds a disjoint-id-space test and a foreign-instance-ticket test (with the
positive control) in MessageServiceTest.
ltms commented 2026-10-04 18:11:57 +02:00
Owner
Copy Link
Copy Source

Lead review. The production change is right. One of the two tests does not test the nonce — it passes without it. One three-line fix needed, and I have already proven the fix works.

What I verified myself

I merged your branch into origin/main in a throwaway worktree and built it. My own numbers, not yours:

tests=2049 failures=0 errors=0 skipped=0   (176 surefire report files)
fleetd.jar  28800728 bytes

That matches your 2049. The production diff is minimal and correct: one field, one changed line, and
java.util.UUID was already imported on main, so no new import. I also checked that nothing parses
or validates a ticket id — the only ticket string handling in production is
SessionManager.java:832, which slugifies the fleet_spawn{ticket} label into a branch name and has
nothing to do with the task id. And ReplyPushLoop has no length cap, so a longer id cannot overflow a
nudge.

The finding: foreignInstanceTicketDoesNotResolve is vacuous

I mutated the mint back to its old form to check your tests catch the defect:

String ticket = "task-" + ticketSeq.incrementAndGet();   // nonce removed

Only one test died:

twoInstancesMintDisjointTicketIds   FAILED  expected: not equal but was: <task-1>
foreignInstanceTicketDoesNotResolve PASSED  <-- survived

Why it survived: other has minted nothing, so its tasks map is empty and poll returns null
for any id. The test proves an empty map returns null. It never reaches a colliding id, so it passes
whether the nonce exists or not.

Your positive control is good and I am not asking you to remove it. The control proves the plumbing
works; it does not make the subject reachable. Those are two different jobs.

This matters because that test is the one covering the actual defect in #719 — an id resolving to a
different unit, not an id resolving to nothing.

The fix, already proven

other must reach the same sequence number, so the ids collide without the nonce:

    MessageService other = newIndependentInstance();
    String ticket = messages.sendAsync(T, "task on first instance", null, null);
    // `other` must reach the same sequence number, or this test passes against an empty map
    // instead of against a colliding id.
    other.sendAsync(T, "task on second instance", null, null);

    // control: ...

With that added and the mutation still in place, both tests die, and the second one now names the
real failure:

foreignInstanceTicketDoesNotResolve  expected: <null>
  but was: <TaskView[ticket=task-1, phase=PENDING, reply=null, detail=worker idle, turnId=null]>

That is the silent wrong answer. Then I restored the nonce and re-ran: Tests run: 99, Failures: 0.

So the fix is verified in both directions — red with the defect, green with it fixed.

What I want from you

Add those three lines to foreignInstanceTicketDoesNotResolve, run mvn clean install, and push to the
same branch. Change nothing else. Do not touch the production file; it is correct as it stands.

Two things I agree with you on

  • Not naming the stale-boot cause was the right call. Naming it needs a second lookup that can
    disagree with the first, and I said one lookup, one answer. Your reasoning matches the instruction.
  • turnId has the same shape — session + "#" + askSeq.incrementAndGet() with a per-Rendezvous
    AtomicLong. Correctly flagged and correctly left alone. I will file it separately.

On closing the issue

Leave "Closes fleetd #719" in place. I will add the wiki/11-Features.md entry from the main clone
myself — your entry text is good and I will use it. The wiki being unreachable in your worktree is
expected, and reporting it rather than inventing the file was the right move.

Lead review. **The production change is right. One of the two tests does not test the nonce — it passes without it.** One three-line fix needed, and I have already proven the fix works. ## What I verified myself I merged your branch into `origin/main` in a throwaway worktree and built it. My own numbers, not yours: ``` tests=2049 failures=0 errors=0 skipped=0 (176 surefire report files) fleetd.jar 28800728 bytes ``` That matches your 2049. The production diff is minimal and correct: one field, one changed line, and `java.util.UUID` was already imported on `main`, so no new import. I also checked that **nothing parses or validates a ticket id** — the only `ticket` string handling in production is `SessionManager.java:832`, which slugifies the `fleet_spawn{ticket}` *label* into a branch name and has nothing to do with the task id. And `ReplyPushLoop` has no length cap, so a longer id cannot overflow a nudge. ## The finding: `foreignInstanceTicketDoesNotResolve` is vacuous I mutated the mint back to its old form to check your tests catch the defect: ```java String ticket = "task-" + ticketSeq.incrementAndGet(); // nonce removed ``` Only one test died: ``` twoInstancesMintDisjointTicketIds FAILED expected: not equal but was: <task-1> foreignInstanceTicketDoesNotResolve PASSED <-- survived ``` **Why it survived:** `other` has minted nothing, so its `tasks` map is empty and `poll` returns `null` for *any* id. The test proves an empty map returns null. It never reaches a colliding id, so it passes whether the nonce exists or not. Your positive control is good and I am not asking you to remove it. The control proves the plumbing works; it does not make the subject reachable. Those are two different jobs. This matters because that test is the one covering the actual defect in #719 — an id resolving to a **different unit**, not an id resolving to nothing. ## The fix, already proven `other` must reach the same sequence number, so the ids collide without the nonce: ```java MessageService other = newIndependentInstance(); String ticket = messages.sendAsync(T, "task on first instance", null, null); // `other` must reach the same sequence number, or this test passes against an empty map // instead of against a colliding id. other.sendAsync(T, "task on second instance", null, null); // control: ... ``` With that added and **the mutation still in place**, both tests die, and the second one now names the real failure: ``` foreignInstanceTicketDoesNotResolve expected: <null> but was: <TaskView[ticket=task-1, phase=PENDING, reply=null, detail=worker idle, turnId=null]> ``` That is the silent wrong answer. Then I restored the nonce and re-ran: `Tests run: 99, Failures: 0`. So the fix is verified in both directions — red with the defect, green with it fixed. ## What I want from you Add those three lines to `foreignInstanceTicketDoesNotResolve`, run `mvn clean install`, and push to the same branch. Change nothing else. Do **not** touch the production file; it is correct as it stands. ## Two things I agree with you on - **Not naming the stale-boot cause was the right call.** Naming it needs a second lookup that can disagree with the first, and I said one lookup, one answer. Your reasoning matches the instruction. - **`turnId` has the same shape** — `session + "#" + askSeq.incrementAndGet()` with a per-`Rendezvous` `AtomicLong`. Correctly flagged and correctly left alone. I will file it separately. ## On closing the issue Leave "Closes fleetd #719" in place. I will add the `wiki/11-Features.md` entry from the main clone myself — your entry text is good and I will use it. The wiki being unreachable in your worktree is expected, and reporting it rather than inventing the file was the right move.
agent added 1 commit 2026-10-04 18:14:07 +02:00
fleetd #719: make the foreign-id test reach a colliding sequence number
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 52s
Details
CI / build (pull_request) Failing after 1m49s
Details
cf0c9b9316 
other.sendAsync had never been called, so other's tasks map was empty and
poll(ticket) returned null regardless of whether the nonce existed — the
test passed against an empty map, not against a colliding id. Mint once on
other so it reaches the same sequence number as the first instance, making
the test exercise the actual collision the nonce guards against.
ltms commented 2026-10-04 18:20:42 +02:00
Owner
Copy Link
Copy Source

Merged locally into main as 8cf7215 and pushed. Closing this PR by hand, because we merge locally and Gitea does not close it by itself.

What I verified before merging, with my own commands

The tree I pushed is the tree I tested. I merged into a throwaway worktree, built it, then compared
tree hashes against the merge in the main clone:

main clone merge tree : db137f84cda6b5cc877fa44eb58077d3f20db51c
verified build tree   : db137f84cda6b5cc877fa44eb58077d3f20db51c   IDENTICAL
first parent          : 7cf6075  (== origin/main at the time)

Full build on that tree:

maven exit code: 0
[INFO] BUILD SUCCESS
surefire totals: tests=2049 failures=0 errors=0 skipped=0  files=176
failing classes: none

Mutation test on the committed code, not on my own patch. I put the mint back to
"task-" + ticketSeq.incrementAndGet() and both tests died:

foreignInstanceTicketDoesNotResolve:921  expected: <null>
  but was: <TaskView[ticket=task-1, phase=PENDING, reply=null, detail=worker idle, turnId=null]>
twoInstancesMintDisjointTicketIds:906    expected: not equal but was: <task-1>

So the stronger test now fails for the right reason, and the failure message names the exact wrong
answer #719 exists to stop.

Push confirmed by ref, not by the push output: local main 8cf7215, origin/main 8cf7215, 0 unpushed.

One mistake I made during this review, recorded because it nearly shipped

My first attempt to build the final merge ran against the wrong tree. My earlier mutation patch was
still uncommitted in the throwaway worktree, so git checkout and git merge both aborted — and the
build that followed still printed maven exit: 0, BUILD SUCCESS, 2049 tests, 0 failures. Every
number was true of my own patched tree, not of your commit.

I caught it only because I read the git output above the Maven output. A green build is a fact about
whatever is on disk, and it never tells you which revision that was. I redid it, proved the tree hash
matched, and only then merged. Worth knowing for anyone verifying a merge here.

Follow-ups, both filed, neither bundled here

  • #729 — turnId has the same <fixed>#<resetting counter> shape. I filed it with the premise it
    turns on written down and unproven: a member terminal id must be able to recur in a later daemon boot.
    If it cannot, there is no defect and the ticket should be closed instead of implemented.
  • The wiki/11-Features.md entry is mine to add from the main clone, using your text.

Good work. The production change was right first time, and the test fix was exactly the three lines with
nothing else touched.

Merged locally into `main` as `8cf7215` and pushed. Closing this PR by hand, because we merge locally and Gitea does not close it by itself. ## What I verified before merging, with my own commands **The tree I pushed is the tree I tested.** I merged into a throwaway worktree, built it, then compared tree hashes against the merge in the main clone: ``` main clone merge tree : db137f84cda6b5cc877fa44eb58077d3f20db51c verified build tree : db137f84cda6b5cc877fa44eb58077d3f20db51c IDENTICAL first parent : 7cf6075 (== origin/main at the time) ``` **Full build on that tree:** ``` maven exit code: 0 [INFO] BUILD SUCCESS surefire totals: tests=2049 failures=0 errors=0 skipped=0 files=176 failing classes: none ``` **Mutation test on the committed code, not on my own patch.** I put the mint back to `"task-" + ticketSeq.incrementAndGet()` and both tests died: ``` foreignInstanceTicketDoesNotResolve:921 expected: <null> but was: <TaskView[ticket=task-1, phase=PENDING, reply=null, detail=worker idle, turnId=null]> twoInstancesMintDisjointTicketIds:906 expected: not equal but was: <task-1> ``` So the stronger test now fails for the right reason, and the failure message names the exact wrong answer #719 exists to stop. **Push confirmed by ref, not by the push output:** `local main 8cf7215`, `origin/main 8cf7215`, 0 unpushed. ## One mistake I made during this review, recorded because it nearly shipped My first attempt to build the final merge **ran against the wrong tree.** My earlier mutation patch was still uncommitted in the throwaway worktree, so `git checkout` and `git merge` both aborted — and the build that followed still printed `maven exit: 0`, `BUILD SUCCESS`, `2049 tests, 0 failures`. Every number was true of my own patched tree, not of your commit. I caught it only because I read the `git` output above the Maven output. A green build is a fact about whatever is on disk, and it never tells you which revision that was. I redid it, proved the tree hash matched, and only then merged. Worth knowing for anyone verifying a merge here. ## Follow-ups, both filed, neither bundled here - **#729** — `turnId` has the same `<fixed>#<resetting counter>` shape. I filed it with the premise it turns on written down and unproven: a member terminal id must be able to recur in a later daemon boot. If it cannot, there is no defect and the ticket should be closed instead of implemented. - The `wiki/11-Features.md` entry is mine to add from the main clone, using your text. Good work. The production change was right first time, and the test fix was exactly the three lines with nothing else touched.
ltms closed this pull request 2026-10-04 18:20:49 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 52s
Details
CI / build (pull_request) Failing after 1m49s
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#728
Delete Branch "worker/719-bdd95e-4"

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?