fleetd #603: share HEALTH_WAIT between the pid poll and the health check #607

Merged

ltms merged 2 commits from worker/redeploy-slowstart-ead0e5-5 into main 2026-09-20 11:30:10 +02:00

Conversation 2 Commits 2 Files Changed 2

+218 -22

agent commented 2026-09-20 11:22:17 +02:00

Member

Copy Link

Copy Source

Fixes #603.

The bug: the start step's pid-wait loop gave the new process its own short, fixed 10s budget before a hard die, while the health check right after it waits a full HEALTH_WAIT (60s) for the same daemon to answer. Under launchd, launchctl load returns before the java process exists, and on a slow host that took longer than 10s -- so the script reported "no process appeared" on a deploy that had fully succeeded (confirmed by hand: pid, healthz 200, and a fresh log line all present).

The fix: wait_for_new_pid (the dual of the existing wait_for_daemon_exit) now shares HEALTH_WAIT instead of holding its own shorter budget. await_daemon_started folds the pid-appeared check and the healthz check into one decision (same shape as swap_if_built/refuse_drain_gate/report_shutdown_drain): a pid miss is no longer fatal by itself -- it falls through to the health check, which is direct proof the daemon is up, rather than a proxy for it. A genuine failure still dies, and report_health's own die() still prints the log tail. The main flow calls await_daemon_started unconditionally (no bare if left to invert), so test_no_untested_main_flow_conditionals needed no allowlist changes.

Tests added (scripts/test-redeploy-fleetd.sh): unit tests for wait_for_new_pid (mirroring the existing wait_for_daemon_exit tests), and two behavioural tests on await_daemon_started covering both of the ticket's acceptance criteria in the same suite run: a slow-but-real start (pid appears well after the old 10s budget, health answers -> succeeds) and a genuine failure (pid never appears, health never answers -> dies, log tail included). Also updated test_report_health_call_site_present's grep target since that call site moved inside the new function, and added test_await_daemon_started_call_site_present.

Same-shape sweep (not fixed here): I looked for other fixed, short timeouts that decide a hard die while a longer, more truthful check is never reached. The only other timing-budget die in the script is wait_for_daemon_exit/STOP_WAIT (30s) at the stop step. I don't think it shares this defect: there is no healthz-like truer signal available there -- pid-still-alive really is the ground truth for "did the old daemon actually stop", so there's nothing more truthful being skipped. Flagging this reasoning for review rather than asserting it's clean.

Build: scripts/test-redeploy-fleetd.sh exits 0, ends with PASS: redeploy log classifier (129 test invocations, including the 5 new ones). mvn -o clean install in fleetd/: BUILD SUCCESS, Tests run: 1819, Failures: 0, Errors: 0, Skipped: 0.

Fixes #603. **The bug:** the start step's pid-wait loop gave the new process its own short, fixed 10s budget before a hard `die`, while the health check right after it waits a full `HEALTH_WAIT` (60s) for the same daemon to answer. Under launchd, `launchctl load` returns before the java process exists, and on a slow host that took longer than 10s -- so the script reported "no process appeared" on a deploy that had fully succeeded (confirmed by hand: pid, healthz 200, and a fresh log line all present). **The fix:** `wait_for_new_pid` (the dual of the existing `wait_for_daemon_exit`) now shares `HEALTH_WAIT` instead of holding its own shorter budget. `await_daemon_started` folds the pid-appeared check and the healthz check into one decision (same shape as `swap_if_built`/`refuse_drain_gate`/`report_shutdown_drain`): a pid miss is no longer fatal by itself -- it falls through to the health check, which is direct proof the daemon is up, rather than a proxy for it. A genuine failure still dies, and `report_health`'s own die() still prints the log tail. The main flow calls `await_daemon_started` unconditionally (no bare `if` left to invert), so `test_no_untested_main_flow_conditionals` needed no allowlist changes. **Tests added** (scripts/test-redeploy-fleetd.sh): unit tests for `wait_for_new_pid` (mirroring the existing `wait_for_daemon_exit` tests), and two behavioural tests on `await_daemon_started` covering both of the ticket's acceptance criteria in the same suite run: a slow-but-real start (pid appears well after the old 10s budget, health answers -> succeeds) and a genuine failure (pid never appears, health never answers -> dies, log tail included). Also updated `test_report_health_call_site_present`'s grep target since that call site moved inside the new function, and added `test_await_daemon_started_call_site_present`. **Same-shape sweep (not fixed here):** I looked for other fixed, short timeouts that decide a hard `die` while a longer, more truthful check is never reached. The only other timing-budget `die` in the script is `wait_for_daemon_exit`/`STOP_WAIT` (30s) at the stop step. I don't think it shares this defect: there is no healthz-like truer signal available there -- pid-still-alive really is the ground truth for "did the old daemon actually stop", so there's nothing more truthful being skipped. Flagging this reasoning for review rather than asserting it's clean. **Build:** `scripts/test-redeploy-fleetd.sh` exits 0, ends with `PASS: redeploy log classifier` (129 test invocations, including the 5 new ones). `mvn -o clean install` in fleetd/: BUILD SUCCESS, Tests run: 1819, Failures: 0, Errors: 0, Skipped: 0.

agent added 1 commit 2026-09-20 11:22:17 +02:00

fleetd #603: share HEALTH_WAIT between the pid poll and the health check ... 81c1d8e91c

The start step gave the new process its own short, fixed 10s budget before a
hard die, while the health check right after it waits a full HEALTH_WAIT
(60s) for the same daemon. Under launchd, launchctl load returns before the
java process exists, and on a slow host that took longer than 10s -- so the
script reported "no process appeared" on a deploy that had fully succeeded.

wait_for_new_pid/await_daemon_started fold the pid poll and the health check
into one decision: the pid poll now shares HEALTH_WAIT instead of its own
shorter budget, and a miss there falls through to the health check (direct
proof the daemon is up) instead of killing the run. A genuine failure still
dies, and still prints the log tail.

Adds behavioural tests for both acceptance criteria (slow start succeeds,
genuine failure still fails and prints the log tail) in one suite run, plus
unit tests for wait_for_new_pid and a call-site test for the new function.

ltms commented 2026-09-20 11:26:00 +02:00

Owner

Copy Link

Copy Source

Lead review. Strong work. The design is better than what I asked for, and the core fix is genuinely pinned — I proved that with my own mutation rather than taking your word. One untested path left. Not merging yet; it is one test.

Verified by me

bash scripts/test-redeploy-fleetd.sh     exit: 0   (unpiped)
final line: PASS: redeploy log classifier
no real FAIL lines (the "mutation: FAIL" lines are the suite's own deliberate stub output)

My mutation 1 — KILLED. The fix is real.

I put the hardcoded budget back:

-  if wait_for_new_pid "$health_wait"; then
+  if wait_for_new_pid 10; then

exit: 1
FAIL: await_daemon_started did not report the pid once it finally appeared

That is the evidence that matters. test_await_daemon_started_slow_pid_then_healthy_succeeds is a real behavioural test, not a source-text check: it drives running_pid to stay empty for 11 calls and then return, stubs sleep to a no-op, and asserts DIED_CALLED = 0. Revert the fix and it goes red. Good.

My mutation 2 — SURVIVED. Here is the gap.

I made a pid miss fatal again — the else branch straight back to the old behaviour:

-    warn "no process matched $PATTERN within ${health_wait}s of starting — falling through..."
+    die "no process appeared"

exit: 0    — whole suite still green

A survivor has more than one explanation, so I checked before reporting it. This is not an equivalent mutant and not a weak test. It is a different behaviour that nothing exercises:

• test_await_daemon_started_slow_pid_then_healthy_succeeds — the pid does appear, so wait_for_new_pid returns 0 and the else branch never runs.
• test_await_daemon_started_never_appears_dies_with_log_tail — pid never appears and health fails, so it dies either way. The mutation changes which die fires, not whether one does.

Neither test covers the case the fall-through exists for: pid never appears, but /healthz answers — must NOT die.

That is not a hypothetical. running_pid()'s own comment says it under-counts, because its allowlist only recognises a plain java -jar. The day the launch method changes, the pid never matches and the daemon is perfectly healthy. Your fall-through handles that correctly, and right now nothing would notice if someone removed it — which is how #603 came back through a different door the first time.

What to add

One test: running_pid never returns anything, poll_health_body succeeds. Assert DIED_CALLED = 0, and assert the warn line is emitted so the operator is told the pid could not be identified.

Acceptance as a property, both halves observed: it passes as written, and it fails when that warn is replaced by die "no process appeared". Run that mutation yourself and paste the green and the red. I have just shown the current suite stays green under it, so a new test that does not go red has not closed anything.

While you are there, decide what NEW_PID should be on that path. Your code does [ -n "$NEW_PID" ] || NEW_PID="$(running_pid)" afterwards, which on this path is still empty, so the result line prints pid unknown. I think that is right — it is the honest answer, and it matches the three-state thinking in the gauge work: never print a number you could not establish. Assert it rather than leave it incidental.

On your two flagged items — both judged correct

The doubled worst-case time-to-fail (~120s). Accepted, and thank you for surfacing it rather than burying it. It only costs time on a genuine failure, and trading a slower true failure for the removal of a false one is the right direction. The false FAIL was pushing people toward a hand-rolled stop/start, which is banned.

wait_for_daemon_exit / STOP_WAIT (30s). Your reasoning holds and I checked it: there is no truer signal to fall through to, because pid-still-alive is ground truth for "did the old daemon stop". A die there is correct. Good sweep, and right call to flag it as a conclusion for review rather than assert it clean.

test_report_health_call_site_present. Not weakened — the call genuinely moved into the function and the new grep target is equally specific. Correctly declared.

The call-site presence checks. I told you to prefer behavioural tests, and you did; these sit alongside real ones and match this file's existing convention, so they are fine. Your comment explaining why a grep cannot distinguish "falls through" from "still dies" is exactly right, and it is the reason mutation 2 matters.

Re-run the full suite after the change and report the real output.

Lead review. Strong work. The design is better than what I asked for, and the core fix is genuinely pinned — I proved that with my own mutation rather than taking your word. **One untested path left.** Not merging yet; it is one test. ## Verified by me ``` bash scripts/test-redeploy-fleetd.sh exit: 0 (unpiped) final line: PASS: redeploy log classifier no real FAIL lines (the "mutation: FAIL" lines are the suite's own deliberate stub output) ``` ## My mutation 1 — KILLED. The fix is real. I put the hardcoded budget back: ```bash - if wait_for_new_pid "$health_wait"; then + if wait_for_new_pid 10; then ``` ``` exit: 1 FAIL: await_daemon_started did not report the pid once it finally appeared ``` That is the evidence that matters. `test_await_daemon_started_slow_pid_then_healthy_succeeds` is a real behavioural test, not a source-text check: it drives `running_pid` to stay empty for 11 calls and then return, stubs `sleep` to a no-op, and asserts `DIED_CALLED = 0`. Revert the fix and it goes red. Good. ## My mutation 2 — SURVIVED. Here is the gap. I made a pid miss fatal again — the `else` branch straight back to the old behaviour: ```bash - warn "no process matched $PATTERN within ${health_wait}s of starting — falling through..." + die "no process appeared" ``` ``` exit: 0 — whole suite still green ``` A survivor has more than one explanation, so I checked before reporting it. This is not an equivalent mutant and not a weak test. It is a **different behaviour that nothing exercises**: - `test_await_daemon_started_slow_pid_then_healthy_succeeds` — the pid *does* appear, so `wait_for_new_pid` returns 0 and the `else` branch never runs. - `test_await_daemon_started_never_appears_dies_with_log_tail` — pid never appears **and** health fails, so it dies either way. The mutation changes which `die` fires, not whether one does. Neither test covers the case the fall-through exists for: **pid never appears, but `/healthz` answers — must NOT die.** That is not a hypothetical. `running_pid()`'s own comment says it under-counts, because its allowlist only recognises a plain `java -jar`. The day the launch method changes, the pid never matches and the daemon is perfectly healthy. Your fall-through handles that correctly, and right now nothing would notice if someone removed it — which is how #603 came back through a different door the first time. ## What to add One test: `running_pid` never returns anything, `poll_health_body` succeeds. Assert `DIED_CALLED = 0`, and assert the warn line is emitted so the operator is told the pid could not be identified. Acceptance as a property, both halves observed: it passes as written, and it **fails** when that `warn` is replaced by `die "no process appeared"`. Run that mutation yourself and paste the green and the red. I have just shown the current suite stays green under it, so a new test that does not go red has not closed anything. While you are there, decide what `NEW_PID` should be on that path. Your code does `[ -n "$NEW_PID" ] || NEW_PID="$(running_pid)"` afterwards, which on this path is still empty, so the result line prints `pid unknown`. I think that is right — it is the honest answer, and it matches the three-state thinking in the gauge work: never print a number you could not establish. Assert it rather than leave it incidental. ## On your two flagged items — both judged correct **The doubled worst-case time-to-fail (~120s).** Accepted, and thank you for surfacing it rather than burying it. It only costs time on a genuine failure, and trading a slower true failure for the removal of a false one is the right direction. The false FAIL was pushing people toward a hand-rolled stop/start, which is banned. **`wait_for_daemon_exit` / `STOP_WAIT` (30s).** Your reasoning holds and I checked it: there is no truer signal to fall through to, because pid-still-alive *is* ground truth for "did the old daemon stop". A `die` there is correct. Good sweep, and right call to flag it as a conclusion for review rather than assert it clean. **`test_report_health_call_site_present`.** Not weakened — the call genuinely moved into the function and the new grep target is equally specific. Correctly declared. **The call-site presence checks.** I told you to prefer behavioural tests, and you did; these sit alongside real ones and match this file's existing convention, so they are fine. Your comment explaining why a grep cannot distinguish "falls through" from "still dies" is exactly right, and it is the reason mutation 2 matters. Re-run the full suite after the change and report the real output.

agent added 1 commit 2026-09-20 11:29:00 +02:00

fleetd #603 review: close the untested fall-through path ... 856dfc6318

PR review (comment 17358) found a real gap via mutation testing: replacing
the "pid never found" warn with die "no process appeared" left the whole
suite green, because neither existing test drove the case the fall-through
exists for -- running_pid() never finds anything (as its own doc comment
says it eventually will) while /healthz answers anyway.

Adds test_await_daemon_started_pid_never_found_but_healthy_warns_and_survives:
running_pid always empty, poll_health_body succeeds. Asserts DIED_CALLED=0,
the warn line is emitted, and NEW_PID stays empty (the honest "could not
establish this" answer, never a guessed pid).

Verified both halves myself: reverting the warn to die "no process appeared"
turns this one test red (FAIL: await_daemon_started must not die...);
restoring it returns the suite to green.

agent commented 2026-09-20 11:29:13 +02:00

Author

Member

Copy Link

Copy Source

Added the missing test and pushed (856dfc6): test_await_daemon_started_pid_never_found_but_healthy_warns_and_survives -- running_pid never returns anything, poll_health_body succeeds. Asserts DIED_CALLED = 0, the warn line is emitted, and NEW_PID stays empty (pinned, not left incidental).

Ran your mutation myself before reporting, both halves:

# mutation applied (warn -> die "no process appeared")
bash scripts/test-redeploy-fleetd.sh
exit: 1
FAIL: await_daemon_started must not die when the pid is never found but healthz answers: no process appeared

# reverted to the real fix
bash scripts/test-redeploy-fleetd.sh
exit: 0
final line: PASS: redeploy log classifier

git diff --stat scripts/redeploy-fleetd.sh was empty after reverting, confirming the mutation only ever touched a scratch copy of the running state, not what's committed.

Full suite after the new test, unpiped: exit 0, final line PASS: redeploy log classifier (130 test invocations now). mvn -o clean install in fleetd/: BUILD SUCCESS, Tests run: 1819, Failures: 0, Errors: 0, Skipped: 0.

Added the missing test and pushed (856dfc6): `test_await_daemon_started_pid_never_found_but_healthy_warns_and_survives` -- `running_pid` never returns anything, `poll_health_body` succeeds. Asserts `DIED_CALLED = 0`, the warn line is emitted, and `NEW_PID` stays empty (pinned, not left incidental). Ran your mutation myself before reporting, both halves: ``` # mutation applied (warn -> die "no process appeared") bash scripts/test-redeploy-fleetd.sh exit: 1 FAIL: await_daemon_started must not die when the pid is never found but healthz answers: no process appeared # reverted to the real fix bash scripts/test-redeploy-fleetd.sh exit: 0 final line: PASS: redeploy log classifier ``` `git diff --stat scripts/redeploy-fleetd.sh` was empty after reverting, confirming the mutation only ever touched a scratch copy of the running state, not what's committed. Full suite after the new test, unpiped: exit 0, final line `PASS: redeploy log classifier` (130 test invocations now). `mvn -o clean install` in fleetd/: BUILD SUCCESS, Tests run: 1819, Failures: 0, Errors: 0, Skipped: 0.

ltms merged commit b85d9b0e46 into main 2026-09-20 11:30:10 +02:00

ltms referenced this pull request 2026-09-20 11:41:48 +02:00

Flaky: MessageServiceTest.anAlreadyCollectedTicketProducesNoNudge orders an async scheduler with Thread.sleep #608

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

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#607