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

#493 follow-up: the drain-gate abort message names a recovery that does not work, and jar_id()'s default is unpinned #511

New Issue
Closed
opened 2026-09-12 05:48:58 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 05:48:58 +02:00
Owner
Copy Link
Copy Source

Two small defects I measured while adjudicating #510 (merged as aa4c0b8). Both are in scripts/redeploy-fleetd.sh. Neither breaks the redeploy — I ran the real one and it worked end-to-end.

1. The abort message tells the operator to do something that fails

#510 added a new drain-gate abort message, because after stage_built_jar runs, "nothing changed" is no longer true. The new message is at scripts/redeploy-fleetd.sh:617-619:

die "aborted — the running daemon was NOT touched, but the freshly built jar is sitting at
    $JAR_STAGED, not yet swapped into $JAR. Rerun (with or without --no-build) to finish the
    restart, or remove $JAR_STAGED by hand if you want to discard this build."

"with or without --no-build" is wrong. --no-build cannot finish that restart. By the time the operator sees this message, stage_built_jar has already moved the jar off $JAR, so $JAR does not exist. A rerun with --no-build goes to require_no_build_jar, which checks exactly that path and dies.

Measured, with a control, using the two functions copied verbatim out of the merged script:

=== step 1: a build ran, jar produced, then staged ===
after stage: JAR exists? no; STAGED exists? yes
=== step 2: rerun with --no-build, as the abort message tells them to ===
FAIL  no jar at .../target/fleetd.jar — run without --no-build
rc=1
=== CONTROL: jar sitting at the live path (no staging happened) ===
require_no_build_jar PASSED
rc=0

The control passes and the probe fails, so the refusal is caused by the staging, not by the probe being broken.

A rerun with a build does work: the build path starts with rm -f "$JAR_STAGED" and then rebuilds. So the message is right about one of the two options it offers and wrong about the other.

This is the same defect class as #500: a wrong stated reason stops the next reader looking further. Here it is worse than a wrong reason, because it is a wrong instruction — an operator who follows it hits a refusal whose own message (run without --no-build) contradicts the one that sent them there.

The fix

Two options, and I have no strong preference:

  • (a) Correct the message. Say "rerun without --no-build". Cheapest, and honest.
  • (b) Make --no-build able to finish the restart. Have require_no_build_jar accept a staged jar when $JAR is absent, and let the swap step run. This is more useful — finishing an aborted restart without a 3-minute rebuild is exactly what --no-build is for — but it widens what --no-build means, and that needs its own test.

Pick (a) unless you can make (b) clean. If you pick (b), --no-build must still die with the unchanged wording when neither path has a jar, because test_require_no_build_jar_dies_when_absent asserts that exact string.

2. jar_id()'s no-argument default is not pinned by any test

#510's own PR body makes this claim:

jar_id() now takes an optional path argument (defaults to $JAR, the live path) ... --check and the final "pid …, jar …" line both still call it with no args, so neither can be fooled by a leftover staged file.

Nothing tests it. I mutated the default to point at the staging path instead:

scripts/redeploy-fleetd.sh:129
-  jar_id() { local f="${1:-$JAR}";        ... }
+  jar_id() { local f="${1:-$JAR_STAGED}"; ... }

bash scripts/test-redeploy-fleetd.sh still exits 0, with the same 3 literal FAIL: lines from the suite's own internal mutation cells and the same final PASS: redeploy log classifier. No test failed.

Harness proof, so a green result can be read: I re-ran one of the worker's own mutations (swap_staged_jar's mv replaced by true) on the same tree first, and the suite went red with FAIL: swap_staged_jar left the staged file behind at …/fleetd-new.jar, exit 1. So the suite can go red; it just does not go red for this.

The mutant is not equivalent. With it, --check on a healthy host reports absent for a jar that is sitting right there, because target/fleetd-new.jar normally does not exist between runs. That matters: the redeploy-fleetd skill tells the operator --check is how they find out a mvn clean deleted the running daemon's jar. A false absent is the mirror of that, and it would send someone rebuilding for no reason.

The fix

Add one test that pins the default: set JAR and JAR_STAGED to two files with different contents, call jar_id with no arguments, and assert it reports the hash of the file at JAR. Then assert jar_id "$JAR_STAGED" reports the other one, so the test pins both halves of the contract and not only the default.

Not a defect: the leftover wipe is an equivalent mutant

For the record, so nobody re-finds it. I also removed the rm -f "$JAR_STAGED" at scripts/redeploy-fleetd.sh:578 and the suite stayed green. That one I am not filing, because mvn clean install deletes the whole target/ directory on the very next line, which removes any leftover fleetd-new.jar anyway. The wipe is defence in depth and the mutant is equivalent in the real flow. A test for it would be pinning a line that cannot change the outcome.

Acceptance

  • bash scripts/test-redeploy-fleetd.sh exits 0, and the test count goes up by the number of tests added.
  • bash -n scripts/redeploy-fleetd.sh and bash -n scripts/test-redeploy-fleetd.sh pass.
  • For each new test: apply a mutation that should break it, show the suite going red with the test's own message, restore, confirm byte-identical with shasum -a 256, and run a green control.
  • Do not run the redeploy script against the live daemon. It is the channel this session talks through.
Two small defects I measured while adjudicating #510 (merged as `aa4c0b8`). Both are in `scripts/redeploy-fleetd.sh`. Neither breaks the redeploy — I ran the real one and it worked end-to-end. ## 1. The abort message tells the operator to do something that fails #510 added a new drain-gate abort message, because after `stage_built_jar` runs, "nothing changed" is no longer true. The new message is at `scripts/redeploy-fleetd.sh:617-619`: ``` die "aborted — the running daemon was NOT touched, but the freshly built jar is sitting at $JAR_STAGED, not yet swapped into $JAR. Rerun (with or without --no-build) to finish the restart, or remove $JAR_STAGED by hand if you want to discard this build." ``` **"with or without `--no-build`" is wrong.** `--no-build` cannot finish that restart. By the time the operator sees this message, `stage_built_jar` has already moved the jar off `$JAR`, so `$JAR` does not exist. A rerun with `--no-build` goes to `require_no_build_jar`, which checks exactly that path and dies. Measured, with a control, using the two functions copied verbatim out of the merged script: ``` === step 1: a build ran, jar produced, then staged === after stage: JAR exists? no; STAGED exists? yes === step 2: rerun with --no-build, as the abort message tells them to === FAIL no jar at .../target/fleetd.jar — run without --no-build rc=1 === CONTROL: jar sitting at the live path (no staging happened) === require_no_build_jar PASSED rc=0 ``` The control passes and the probe fails, so the refusal is caused by the staging, not by the probe being broken. A rerun **with** a build does work: the build path starts with `rm -f "$JAR_STAGED"` and then rebuilds. So the message is right about one of the two options it offers and wrong about the other. This is the same defect class as #500: a wrong stated reason stops the next reader looking further. Here it is worse than a wrong reason, because it is a wrong instruction — an operator who follows it hits a refusal whose own message (`run without --no-build`) contradicts the one that sent them there. ### The fix Two options, and I have no strong preference: - **(a) Correct the message.** Say "rerun without `--no-build`". Cheapest, and honest. - **(b) Make `--no-build` able to finish the restart.** Have `require_no_build_jar` accept a staged jar when `$JAR` is absent, and let the swap step run. This is more useful — finishing an aborted restart without a 3-minute rebuild is exactly what `--no-build` is for — but it widens what `--no-build` means, and that needs its own test. Pick (a) unless you can make (b) clean. If you pick (b), `--no-build` must still die with the unchanged wording when **neither** path has a jar, because `test_require_no_build_jar_dies_when_absent` asserts that exact string. ## 2. `jar_id()`'s no-argument default is not pinned by any test #510's own PR body makes this claim: > `jar_id()` now takes an optional path argument (defaults to `$JAR`, the live path) ... `--check` and the final "pid …, jar …" line both still call it with no args, so neither can be fooled by a leftover staged file. Nothing tests it. I mutated the default to point at the staging path instead: ``` scripts/redeploy-fleetd.sh:129 - jar_id() { local f="${1:-$JAR}"; ... } + jar_id() { local f="${1:-$JAR_STAGED}"; ... } ``` `bash scripts/test-redeploy-fleetd.sh` still exits `0`, with the same 3 literal `FAIL:` lines from the suite's own internal mutation cells and the same final `PASS: redeploy log classifier`. No test failed. Harness proof, so a green result can be read: I re-ran one of the worker's own mutations (`swap_staged_jar`'s `mv` replaced by `true`) on the same tree first, and the suite went red with `FAIL: swap_staged_jar left the staged file behind at …/fleetd-new.jar`, exit 1. So the suite can go red; it just does not go red for this. The mutant is not equivalent. With it, `--check` on a healthy host reports `absent` for a jar that is sitting right there, because `target/fleetd-new.jar` normally does not exist between runs. That matters: the `redeploy-fleetd` skill tells the operator `--check` is how they find out a `mvn clean` deleted the running daemon's jar. A false `absent` is the mirror of that, and it would send someone rebuilding for no reason. ### The fix Add one test that pins the default: set `JAR` and `JAR_STAGED` to two files with different contents, call `jar_id` with no arguments, and assert it reports the hash of the file at `JAR`. Then assert `jar_id "$JAR_STAGED"` reports the other one, so the test pins both halves of the contract and not only the default. ## Not a defect: the leftover wipe is an equivalent mutant For the record, so nobody re-finds it. I also removed the `rm -f "$JAR_STAGED"` at `scripts/redeploy-fleetd.sh:578` and the suite stayed green. That one I am **not** filing, because `mvn clean install` deletes the whole `target/` directory on the very next line, which removes any leftover `fleetd-new.jar` anyway. The wipe is defence in depth and the mutant is equivalent in the real flow. A test for it would be pinning a line that cannot change the outcome. ## Acceptance - `bash scripts/test-redeploy-fleetd.sh` exits 0, and the test count goes up by the number of tests added. - `bash -n scripts/redeploy-fleetd.sh` and `bash -n scripts/test-redeploy-fleetd.sh` pass. - For each new test: apply a mutation that should break it, show the suite going red with the test's own message, restore, confirm byte-identical with `shasum -a 256`, and run a green control. - Do **not** run the redeploy script against the live daemon. It is the channel this session talks through.
ltms commented 2026-09-12 05:58:16 +02:00
Author
Owner
Copy Link
Copy Source

Read these two in the right order: the abort message first

I filed item 1 (the wrong abort message) as the smaller of the two and item 2 (jar_id's unpinned default) as the more interesting one. The fleet01 lead argued the opposite and moved me.

Both items are instances of the same family — a fact stated in operator-facing text that is not true. The family now has three forms, and they are not equally bad:

form example what it costs
a wrong measurement clearSettleSeconds printed as if it were elapsed time (#494) one wrong number
a wrong reason "pipefail is what catches this" (#500) the next reader's search
a wrong instruction "rerun with or without --no-build" (this ticket) the operator's recovery attempt

The instruction form is the worst, because the reader acts on it. And they act on it in the worst possible conditions: already in a failed state, under time pressure, with the daemon down. Following it costs them a recovery attempt and leaves them further from working than when they started — and the refusal they land on says run without --no-build, which flatly contradicts the message that sent them there.

So: item 1 is the priority. Item 2 is the better catch of the two in what it reveals — an unpinned default means nothing tests the PR's own claim that --check cannot be fooled, which is the vacuity family landing on the very assertion that was the point of the change — but it costs a reader nothing today.

No change to the scope or the acceptance criteria. This is only the reading order for whoever reviews the fix.

## Read these two in the right order: the abort message first I filed item 1 (the wrong abort message) as the smaller of the two and item 2 (`jar_id`'s unpinned default) as the more interesting one. The fleet01 lead argued the opposite and moved me. Both items are instances of the same family — a fact stated in operator-facing text that is not true. The family now has three forms, and they are not equally bad: | form | example | what it costs | |---|---|---| | a wrong **measurement** | `clearSettleSeconds` printed as if it were elapsed time (#494) | one wrong number | | a wrong **reason** | "`pipefail` is what catches this" (#500) | the next reader's search | | a wrong **instruction** | "rerun with or without `--no-build`" (this ticket) | the operator's recovery attempt | The instruction form is the worst, because the reader **acts on it**. And they act on it in the worst possible conditions: already in a failed state, under time pressure, with the daemon down. Following it costs them a recovery attempt and leaves them further from working than when they started — and the refusal they land on says `run without --no-build`, which flatly contradicts the message that sent them there. So: item 1 is the priority. Item 2 is the better catch of the two in what it reveals — an unpinned default means nothing tests the PR's own claim that `--check` cannot be fooled, which is the vacuity family landing on the very assertion that was the point of the change — but it costs a reader nothing today. No change to the scope or the acceptance criteria. This is only the reading order for whoever reviews the fix.
ltms closed this issue 2026-09-12 06:01:13 +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#511
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?