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

Three more identifiers built from a per-instance counter with no per-boot component — reachability unestablished for all three #734

New Issue
Closed
opened 2026-10-04 19:01:23 +02:00 by ltms · 1 comment
ltms commented 2026-10-04 19:01:23 +02:00
Owner
Copy Link
Copy Source

Why this ticket exists

#719 and #729 both fixed the same shape: an identifier minted from a per-instance AtomicLong that
restarts at zero on every daemon boot, so an id from one boot can be minted again in the next and
resolve to something unrelated with no error.

While implementing #729 a worker swept fleetd/src/main/java for the same shape. It found three more.
None of them is confirmed reachable, and the sweep deliberately changed nothing. This ticket exists
so the three are not lost in a merged PR's history.

Read this as a list of candidates, not a list of defects. The rule from #729 applies: a defect on paper
is not a reachable defect. Each item below needs its reachability settled before any code is written,
and "not reachable, close it" is a perfectly good outcome for any of them.

The three candidates

1. placement/BackendOutagePolicy.java:107 — incident id

"outage-" + credentialId + "-" + incidentSequence.incrementAndGet()

incidentSequence is a plain per-instance AtomicLong with no boot component.

Not established: whether this id is ever written down outside the daemon's own memory, which is what
makes reuse dangerous. The two consumers found were Fleetd.java:1266 and ReplyPushLoop.java, and both
keep it in per-boot in-memory state (pendingIncidents / deliveredIncidents) that is cleared on
restart. If nothing outside the process ever holds one of these ids, reuse cannot hurt and this should be
closed.

2. herdr/UnixSocketHerdrClient.java:69 — JSON-RPC request id

Long.toString(ids.getAndIncrement())

Plain per-instance AtomicLong, no boot component.

Not established: whether this can ever cross a boundary where reuse matters. The class's own javadoc
says herdr is one-shot per connection, and the id looks matched synchronously within a single
call/response pair. If the id never outlives its own request, there is nothing to fix. This is the most
likely of the three to be a non-issue.

3. member/HerdrPeerLauncher.java:558 — tab label number

nextLabelSeq / labelSeq, a per-(role, profile) AtomicLong with no boot component, used to mint a
human-facing tab name such as dev: sonnet #1.

Not established: whether anything ever resolves a session by this number rather than only
displaying it. If it is display-only, a repeat is cosmetic confusion for an operator reading panes, not a
correctness bug. Worth knowing which, because a lead's identity does come from a tab label
(FleetConfig.java:1112-1115) — though by the lead's configured tab:, not by this generated member
label.

Already checked and ruled out — do not re-sweep these

Four sites match the pattern textually but already carry a per-boot or per-call random component, so they
are not candidates:

Site Why it is fine
member/HerdrPeerLauncher.java:768 (nameSeq) mixed with a separate per-process nonce; see its own comment at :158
lead/LeadLauncher.java:367 (nameSeq) already folds in nameNonce
worktree/GitWorktrees.java:1405 (seq) prefixed with SecureRandom.nextInt(1 << 24), drawn fresh on every call, not once per boot
session/SessionManager.java:836 (nonceSeq) same shape as above, nonceRandom.nextInt(1 << 24) per call

The last two are stronger than the #719/#729 fix, not weaker: a per-call random beats a per-boot nonce.

How to settle each one

For each candidate, in this order:

  1. Enumerate who holds the id, and whether any holder outlives one daemon boot. A human, a ticket
    comment, a handover file and an on-disk record all count; in-memory state cleared at restart does not.
  2. If no holder outlives a boot, close that item. Say so explicitly rather than fixing it quietly.
  3. If one does, apply the #719/#729 remedy and nothing cleverer: fold a per-instance nonce into the
    mint so staleness is carried by the identifier the caller holds. Do not record a boot id
    server-side and compare — #719 tried that and withdrew it, because the caller presents only the
    string.
  4. Prove it by mutation, with a test that reaches a colliding id rather than an empty map. #719's
    first test had a positive control and still passed vacuously because the second instance had minted
    nothing. Drive both instances to the same sequence number.

Credit and scope

Found by the #729 implementer during a sweep I asked for in the brief ("find what else has this shape,
report it, do not fix it"). The reachability caveats above are that worker's own words, kept rather
than upgraded into claims. Nothing in the three candidates was changed.

## Why this ticket exists #719 and #729 both fixed the same shape: an identifier minted from a per-instance `AtomicLong` that restarts at zero on every daemon boot, so an id from one boot can be minted again in the next and resolve to something unrelated with no error. While implementing #729 a worker swept `fleetd/src/main/java` for the same shape. It found three more. **None of them is confirmed reachable**, and the sweep deliberately changed nothing. This ticket exists so the three are not lost in a merged PR's history. Read this as a list of candidates, not a list of defects. The rule from #729 applies: *a defect on paper is not a reachable defect.* Each item below needs its reachability settled before any code is written, and "not reachable, close it" is a perfectly good outcome for any of them. ## The three candidates ### 1. `placement/BackendOutagePolicy.java:107` — incident id ```java "outage-" + credentialId + "-" + incidentSequence.incrementAndGet() ``` `incidentSequence` is a plain per-instance `AtomicLong` with no boot component. **Not established:** whether this id is ever written down outside the daemon's own memory, which is what makes reuse dangerous. The two consumers found were `Fleetd.java:1266` and `ReplyPushLoop.java`, and both keep it in per-boot in-memory state (`pendingIncidents` / `deliveredIncidents`) that is cleared on restart. If nothing outside the process ever holds one of these ids, reuse cannot hurt and this should be closed. ### 2. `herdr/UnixSocketHerdrClient.java:69` — JSON-RPC request id ```java Long.toString(ids.getAndIncrement()) ``` Plain per-instance `AtomicLong`, no boot component. **Not established:** whether this can ever cross a boundary where reuse matters. The class's own javadoc says herdr is one-shot per connection, and the id looks matched synchronously within a single call/response pair. If the id never outlives its own request, there is nothing to fix. This is the most likely of the three to be a non-issue. ### 3. `member/HerdrPeerLauncher.java:558` — tab label number `nextLabelSeq` / `labelSeq`, a per-`(role, profile)` `AtomicLong` with no boot component, used to mint a human-facing tab name such as `dev: sonnet #1`. **Not established:** whether anything ever *resolves* a session by this number rather than only displaying it. If it is display-only, a repeat is cosmetic confusion for an operator reading panes, not a correctness bug. Worth knowing which, because a lead's identity does come from a tab label (`FleetConfig.java:1112-1115`) — though by the lead's configured `tab:`, not by this generated member label. ## Already checked and ruled out — do not re-sweep these Four sites match the pattern textually but already carry a per-boot or per-call random component, so they are **not** candidates: | Site | Why it is fine | |---|---| | `member/HerdrPeerLauncher.java:768` (`nameSeq`) | mixed with a separate per-process nonce; see its own comment at `:158` | | `lead/LeadLauncher.java:367` (`nameSeq`) | already folds in `nameNonce` | | `worktree/GitWorktrees.java:1405` (`seq`) | prefixed with `SecureRandom.nextInt(1 << 24)`, drawn fresh on **every call**, not once per boot | | `session/SessionManager.java:836` (`nonceSeq`) | same shape as above, `nonceRandom.nextInt(1 << 24)` per call | The last two are stronger than the #719/#729 fix, not weaker: a per-call random beats a per-boot nonce. ## How to settle each one For each candidate, in this order: 1. **Enumerate who holds the id**, and whether any holder outlives one daemon boot. A human, a ticket comment, a handover file and an on-disk record all count; in-memory state cleared at restart does not. 2. **If no holder outlives a boot, close that item.** Say so explicitly rather than fixing it quietly. 3. **If one does**, apply the #719/#729 remedy and nothing cleverer: fold a per-instance nonce into the mint so staleness is carried by the identifier the caller holds. Do **not** record a boot id server-side and compare — #719 tried that and withdrew it, because the caller presents only the string. 4. Prove it by mutation, with a test that reaches a **colliding** id rather than an empty map. #719's first test had a positive control and still passed vacuously because the second instance had minted nothing. Drive both instances to the same sequence number. ## Credit and scope Found by the #729 implementer during a sweep I asked for in the brief ("find what else has this shape, report it, do **not** fix it"). The reachability caveats above are that worker's own words, kept rather than upgraded into claims. Nothing in the three candidates was changed.
ltms commented 2026-10-04 20:26:45 +02:00
Author
Owner
Copy Link
Copy Source

Lead verification — I checked all three paths myself (2026-10-04)

I told the hunter I would check its paths rather than take its verdicts. I did. All three verdicts hold, and the report found one thing this ticket got wrong. Below is what I ran, not what I was told.

This ticket's count is wrong: three touch points, not two

The ticket says the incident id has two consumers. It has three.

$ grep -n 'incidentId' msg/ReplyPushLoop.java
209:    private record IncidentLead(String incidentId, String lead) {
579:    public void onBackendIncident(String incidentId, Collection<String> targets, String credential,
585:        log.warn("push: backend incident {} has no known lead for target {}", incidentId, target);
592:        IncidentLead key = new IncidentLead(incidentId, entry.getKey());

So the id is used at :592 as the dedup key, and at :585 it is written to the log. The third one is the log line, and the hunter found it — this ticket did not name it.

It is a one-way sink. Nothing reads the id back out of the log. I also checked that the id does not reach the operator-facing nudge text:

$ grep -A2 'formatBackendIncidentNudge' msg/ReplyPushLoop.java
879:    private static String formatBackendIncidentNudge(PendingIncident incident) {
880:        return BACKEND_INCIDENT_NUDGE_FORMAT.formatted(incident.credential(),
                   incident.remainingCoolOffSeconds(), ...

The nudge carries the credential and the remaining cool-off seconds. It never carries the id.

Candidate 2 — the herdr request id: not reachable, and the reason is structural

The hunter said HerdrCodec.decodeResult never reads the response id. That is true. The whole file is 70 lines, so I enumerated every field read instead of trusting a pattern:

46:    JsonNode decodeResult(String line) {
53:        JsonNode error = root.get("error");
59:        JsonNode result = root.get("result");

Two reads, error and result. No id. The pattern does work on this file — it found req.put("id", id) at :29 — so this zero is a real zero and not a broken grep.

But the stronger reason is in the client, and it is better than "the field is not read". UnixSocketHerdrClient.call opens a new socket for every call:

try (SocketChannel ch = SocketChannel.open(StandardProtocolFamily.UNIX)) {
    ch.connect(UnixDomainSocketAddress.of(socketPath));
    writeFully(ch, ByteBuffer.wrap(frame));
    return codec.decodeResult(readLine(ch));
}

One request per connection, one line read, then close. There is no multiplexing, so there is nothing for an id to correlate. The id exists only because herdr rejects a missing or integer id — the comment at HerdrCodec.java:29 says so.

This answer expires if the client ever keeps the connection open and sends more than one request on it. At that moment the id becomes a correlation key and a predictable or colliding id becomes a real defect. Whoever makes that change has to reopen this.

Candidate 3 — the lead tab label space: already guarded at boot

validateLeadTabPrefixes exists at config/FleetConfig.java:2722 and it runs at startup. The route is not obvious, and this matters:

$ grep -rn 'validateLeadTabPrefixes' --include='*.java' src/main
config/FleetConfig.java:2722:    public void validateLeadTabPrefixes() {
herdr/LeadTabScanner.java:62:     * ... by {@code FleetConfig.validateLeadTabPrefixes} rather than documented here.

There is no call site at all. Only the declaration and a javadoc mention. A plain search for callers returns nothing, so this reads as dead code. It is not. It runs through a reflective sweep:

Fleetd.java:201        cfg.validateAll();
FleetConfig.java:3150  public void validateAll() { invokeAllValidators(this); }
FleetConfig.java:3173  ... && !m.getName().equals("validateAll")) {

invokeAllValidators enumerates the declared methods and calls every one whose name starts with validate, except validateAll itself. So the method is wired by its name, not by a call.

That has a consequence worth recording, because it is a trap and not a nicety: renaming this method, or any validate* method, silently unwires it. There is no call site to break, so nothing fails to compile. The only thing standing in the way is FleetConfigValidateAllTest.java:195, which asserts the exact set of swept method names. That test is load-bearing. Anyone who "fixes" it by pasting in a new expected set has removed a boot check and will see a green build.

Verdicts

Candidate Verdict Checked by
incident id predictable not reachable — one-way log sink and a dedup key; never read back, never in the nudge enumerated all 4 uses
herdr request id predictable not reachable — one request per connection, nothing to correlate read all 70 lines of the codec, plus call
lead tab label collision not reachable — rejected at boot by validateLeadTabPrefixes traced Fleetd:201 → validateAll → the sweep

The hunter also reported that Rendezvous.askSeq and MessageService.ticketSeq already fold in a boot nonce. I have not checked those two myself, and I am not resting this closure on them — all three candidates above close on their own evidence.

Closing

Closing as not reachable. Nothing was changed, and the hunter's worktree was clean — correct for this ticket, whose accepted outcome included "not reachable, close it".

Two things to carry forward rather than lose with the ticket:

  1. The ticket's "two consumers" was wrong. Three.
  2. The validate* naming contract and the test that pins it are worth their own issue, because the hazard is live whether or not this ticket closes.
## Lead verification — I checked all three paths myself (2026-10-04) I told the hunter I would check its paths rather than take its verdicts. I did. **All three verdicts hold**, and the report found one thing this ticket got wrong. Below is what I ran, not what I was told. ### This ticket's count is wrong: three touch points, not two The ticket says the incident id has two consumers. It has three. ``` $ grep -n 'incidentId' msg/ReplyPushLoop.java 209: private record IncidentLead(String incidentId, String lead) { 579: public void onBackendIncident(String incidentId, Collection<String> targets, String credential, 585: log.warn("push: backend incident {} has no known lead for target {}", incidentId, target); 592: IncidentLead key = new IncidentLead(incidentId, entry.getKey()); ``` So the id is used at `:592` as the dedup key, and at `:585` it is written to the log. The third one is the log line, and the hunter found it — this ticket did not name it. It is a one-way sink. Nothing reads the id back out of the log. I also checked that the id does **not** reach the operator-facing nudge text: ``` $ grep -A2 'formatBackendIncidentNudge' msg/ReplyPushLoop.java 879: private static String formatBackendIncidentNudge(PendingIncident incident) { 880: return BACKEND_INCIDENT_NUDGE_FORMAT.formatted(incident.credential(), incident.remainingCoolOffSeconds(), ... ``` The nudge carries the credential and the remaining cool-off seconds. It never carries the id. ### Candidate 2 — the herdr request id: not reachable, and the reason is structural The hunter said `HerdrCodec.decodeResult` never reads the response `id`. That is true. The whole file is 70 lines, so I enumerated every field read instead of trusting a pattern: ``` 46: JsonNode decodeResult(String line) { 53: JsonNode error = root.get("error"); 59: JsonNode result = root.get("result"); ``` Two reads, `error` and `result`. No `id`. The pattern does work on this file — it found `req.put("id", id)` at `:29` — so this zero is a real zero and not a broken grep. But the stronger reason is in the client, and it is better than "the field is not read". `UnixSocketHerdrClient.call` opens a **new socket for every call**: ```java try (SocketChannel ch = SocketChannel.open(StandardProtocolFamily.UNIX)) { ch.connect(UnixDomainSocketAddress.of(socketPath)); writeFully(ch, ByteBuffer.wrap(frame)); return codec.decodeResult(readLine(ch)); } ``` One request per connection, one line read, then close. There is no multiplexing, so there is nothing for an id to correlate. The id exists only because herdr rejects a missing or integer id — the comment at `HerdrCodec.java:29` says so. **This answer expires if the client ever keeps the connection open and sends more than one request on it.** At that moment the id becomes a correlation key and a predictable or colliding id becomes a real defect. Whoever makes that change has to reopen this. ### Candidate 3 — the lead tab label space: already guarded at boot `validateLeadTabPrefixes` exists at `config/FleetConfig.java:2722` and it runs at startup. The route is not obvious, and this matters: ``` $ grep -rn 'validateLeadTabPrefixes' --include='*.java' src/main config/FleetConfig.java:2722: public void validateLeadTabPrefixes() { herdr/LeadTabScanner.java:62: * ... by {@code FleetConfig.validateLeadTabPrefixes} rather than documented here. ``` **There is no call site at all.** Only the declaration and a javadoc mention. A plain search for callers returns nothing, so this reads as dead code. It is not. It runs through a reflective sweep: ``` Fleetd.java:201 cfg.validateAll(); FleetConfig.java:3150 public void validateAll() { invokeAllValidators(this); } FleetConfig.java:3173 ... && !m.getName().equals("validateAll")) { ``` `invokeAllValidators` enumerates the declared methods and calls every one whose name starts with `validate`, except `validateAll` itself. So the method is wired by its **name**, not by a call. That has a consequence worth recording, because it is a trap and not a nicety: **renaming this method, or any `validate*` method, silently unwires it.** There is no call site to break, so nothing fails to compile. The only thing standing in the way is `FleetConfigValidateAllTest.java:195`, which asserts the exact set of swept method names. That test is load-bearing. Anyone who "fixes" it by pasting in a new expected set has removed a boot check and will see a green build. ### Verdicts | Candidate | Verdict | Checked by | |---|---|---| | incident id predictable | **not reachable** — one-way log sink and a dedup key; never read back, never in the nudge | enumerated all 4 uses | | herdr request id predictable | **not reachable** — one request per connection, nothing to correlate | read all 70 lines of the codec, plus `call` | | lead tab label collision | **not reachable** — rejected at boot by `validateLeadTabPrefixes` | traced `Fleetd:201` → `validateAll` → the sweep | The hunter also reported that `Rendezvous.askSeq` and `MessageService.ticketSeq` already fold in a boot nonce. I have **not** checked those two myself, and I am not resting this closure on them — all three candidates above close on their own evidence. ### Closing Closing as not reachable. Nothing was changed, and the hunter's worktree was clean — correct for this ticket, whose accepted outcome included "not reachable, close it". Two things to carry forward rather than lose with the ticket: 1. The ticket's "two consumers" was wrong. Three. 2. The `validate*` naming contract and the test that pins it are worth their own issue, because the hazard is live whether or not this ticket closes.
ltms closed this issue 2026-10-04 20:26:52 +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#734
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?