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

A malformed exhaustedPattern crashes startup unnamed — only errorPattern is validated at load #273

New Issue
Closed
opened 2026-09-04 04:55:51 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 04:55:51 +02:00
Owner
Copy Link
Copy Source

Found by a bug-hunt fan-out; I verified this one in the code myself.

What is wrong

FleetConfig.rejectMalformedErrorPattern compiles every profile's errorPattern at config load and refuses to start with a message naming the profile and key. Its own javadoc says why:

an uncaught PatternSyntaxException there crashes startup without naming which profile or key is at fault. Validate eagerly here instead, at config load.

But exhaustedPattern is the sibling key, and it is compiled unguarded at Fleetd.java:347:

cfg.profiles().forEach((name, profile) -> {
    if (profile.hasExhaustedPattern()) {
        exhaustedPatternsByProfile.put(name, Pattern.compile(profile.exhaustedPattern()));
    }
});

So profiles.<name>.exhaustedPattern: "[" passes load(), then takes the whole daemon down at boot with a raw PatternSyntaxException that names neither the profile nor the key — precisely the failure the existing validator was written to prevent.

Why it matters

It is a fail-stop that blocks the entire fleet, and the operator gets a stack trace with no pointer to the offending line. fleetd.yaml is gitignored and lead-only, so the bad value is in a file no test ever sees.

Shape

This is the same one-way-gate shape as #258 and #272: a guard written while looking at one key, never extended to its sibling. The validator is correct — its coverage is what is wrong.

Fix

Extend the validator to compile every non-blank exhaustedPattern too, naming profiles.<name>.exhaustedPattern in the refusal, and rename it so the name no longer implies one key. Keep the existing sorted, joined message format.

Ask the wider question while you are there: which other config values are compiled or parsed after load, outside a validator? Report what you find; do not fix it in this PR.

Found by a bug-hunt fan-out; **I verified this one in the code myself.** ## What is wrong `FleetConfig.rejectMalformedErrorPattern` compiles every profile's `errorPattern` at config load and refuses to start with a message naming the profile and key. Its own javadoc says why: > an uncaught `PatternSyntaxException` there crashes startup without naming which profile or key is at fault. Validate eagerly here instead, at config load. But `exhaustedPattern` is the sibling key, and it is compiled unguarded at `Fleetd.java:347`: ```java cfg.profiles().forEach((name, profile) -> { if (profile.hasExhaustedPattern()) { exhaustedPatternsByProfile.put(name, Pattern.compile(profile.exhaustedPattern())); } }); ``` So `profiles.<name>.exhaustedPattern: "["` passes `load()`, then takes the whole daemon down at boot with a raw `PatternSyntaxException` that names neither the profile nor the key — precisely the failure the existing validator was written to prevent. ## Why it matters It is a fail-stop that blocks the entire fleet, and the operator gets a stack trace with no pointer to the offending line. `fleetd.yaml` is gitignored and lead-only, so the bad value is in a file no test ever sees. ## Shape This is the same one-way-gate shape as #258 and #272: a guard written while looking at one key, never extended to its sibling. The validator is correct — its **coverage** is what is wrong. ## Fix Extend the validator to compile every non-blank `exhaustedPattern` too, naming `profiles.<name>.exhaustedPattern` in the refusal, and rename it so the name no longer implies one key. Keep the existing sorted, joined message format. Ask the wider question while you are there: which *other* config values are compiled or parsed after load, outside a validator? Report what you find; do not fix it in this PR.
ltms commented 2026-09-04 05:11:21 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in f71ee49 (PR #278).

Verified by me rather than taken on the worker's word:

  • the actual merge of the branch into current main builds green — 1280 tests, BUILD SUCCESS. (The worker reported 1279 from its own branch; the difference is #276, which landed after it branched and adds one test. The two numbers agree.)
  • I reran the mutation independently: narrowing the loop back to errorPattern only turns exactly two tests red, one with Expected java.lang.IllegalStateException to be thrown, but nothing was thrown — the defect stated out loud.
  • the tests drive the real FleetConfig.load(f) entry point, not the validator directly, so they would have caught this as shipped.

The "same shape elsewhere" list — reported, and deliberately NOT filed

The worker was asked to list other config values parsed only after load() returns. It found four, and all four are really there (I read each):

Site Value Parsed as
Fleetd.java:154-155 herdrSocket Path.of(...)
Fleetd.java:159-160 memberHerdrSocket Path.of(...)
HerdrPeerLauncher.java:1447 worktreeRoot Path.of(...)
Fleetd.java:719 bind.host/bind.port straight into app.start(...)

I am not filing these, and the reason is the point. The structural shape matches, but the direction and likelihood of harm do not. Path.of on Unix rejects essentially only a NUL byte — nearly any other string is a valid path. A malformed regex like exhaustedPattern: "[" is a plausible typo a person actually makes; a NUL character inside a YAML scalar is not. A shape match is a search key, not a defect.

bind.port is the one with any real reachability (a typo like 99999 is possible), and even there the failure is a loud startup crash from the HTTP server rather than silent wrong behaviour. If it is ever worth doing, it belongs with a general "validate bind at load" change, not this ticket.

The worker's own ruled-out list was accurate too — broker/coordinator URIs already warn and fall back, SubscriptionGuard throws a profile-naming GuardException, and quarantineCooldownSeconds is typed so Jackson rejects it inside load().

Separate finding from this ticket, worth more than the ticket

While doing its revert/restore the worker discovered that git stash is one stack shared by every worktree of this repo — the primary's checkout and all worker worktrees. A concurrent git stash from the #274 worker landed in this worker's tree and overwrote its in-progress edit. It noticed, recovered, verified byte-equality, and put the other worker's change back untouched.

I measured it: git stash list from a worker worktree and from the primary's checkout return byte-identical output, and refs/stash is a single common ref, not a per-worktree one. Worktrees isolate the working tree, the index and HEAD — not repo-level refs.

The implementer skill now forbids git stash outright and points at a wip: commit or a patch file instead (7d54344).

Merged to `main` in `f71ee49` (PR #278). Verified by me rather than taken on the worker's word: * the **actual merge** of the branch into current `main` builds green — 1280 tests, `BUILD SUCCESS`. (The worker reported 1279 from its own branch; the difference is #276, which landed after it branched and adds one test. The two numbers agree.) * I reran the mutation independently: narrowing the loop back to `errorPattern` only turns exactly two tests red, one with `Expected java.lang.IllegalStateException to be thrown, but nothing was thrown` — the defect stated out loud. * the tests drive the real `FleetConfig.load(f)` entry point, not the validator directly, so they would have caught this as shipped. ## The "same shape elsewhere" list — reported, and deliberately NOT filed The worker was asked to list other config values parsed only after `load()` returns. It found four, and all four are really there (I read each): | Site | Value | Parsed as | |---|---|---| | `Fleetd.java:154-155` | `herdrSocket` | `Path.of(...)` | | `Fleetd.java:159-160` | `memberHerdrSocket` | `Path.of(...)` | | `HerdrPeerLauncher.java:1447` | `worktreeRoot` | `Path.of(...)` | | `Fleetd.java:719` | `bind.host`/`bind.port` | straight into `app.start(...)` | **I am not filing these, and the reason is the point.** The structural shape matches, but the *direction and likelihood of harm* do not. `Path.of` on Unix rejects essentially only a NUL byte — nearly any other string is a valid path. A malformed regex like `exhaustedPattern: "["` is a plausible typo a person actually makes; a NUL character inside a YAML scalar is not. A shape match is a search key, not a defect. `bind.port` is the one with any real reachability (a typo like `99999` is possible), and even there the failure is a loud startup crash from the HTTP server rather than silent wrong behaviour. If it is ever worth doing, it belongs with a general "validate `bind` at load" change, not this ticket. The worker's own ruled-out list was accurate too — broker/coordinator URIs already warn and fall back, `SubscriptionGuard` throws a profile-naming `GuardException`, and `quarantineCooldownSeconds` is typed so Jackson rejects it inside `load()`. ## Separate finding from this ticket, worth more than the ticket While doing its revert/restore the worker discovered that **`git stash` is one stack shared by every worktree of this repo** — the primary's checkout and all worker worktrees. A concurrent `git stash` from the #274 worker landed in this worker's tree and overwrote its in-progress edit. It noticed, recovered, verified byte-equality, and put the other worker's change back untouched. I measured it: `git stash list` from a worker worktree and from the primary's checkout return byte-identical output, and `refs/stash` is a single common ref, not a per-worktree one. Worktrees isolate the working tree, the index and `HEAD` — not repo-level refs. The `implementer` skill now forbids `git stash` outright and points at a `wip:` commit or a patch file instead (`7d54344`).
ltms closed this issue 2026-09-04 05:14:38 +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#273
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?