fleetd #512 follow-up: record why the drain-complete line must not move into a finally #547

2026-09-12T09:22:47+02:00

ltms commented

2026-09-12 09:22:47 +02:00

Comment only. No behaviour change. SessionManager.drainAll's javadoc.

Why

drainAll's existing javadoc says the log.info "is a positive assertion that the drain actually
finished, on the normal path, every time". That wording reads as an invitation to the one edit
that destroys what it is describing: moving the line into a finally, or wrapping the method in
one.

In a finally the line prints after a drain that threw, carrying whatever partial tally it
had reached. That loses the absence signal and gains a confident wrong number in the same change.
Both halves of the value go at once.

The code is already correct — drainAll has no try/finally and the log.info is the last
statement of the successful path. Nothing here changes that. What was missing is the sentence that
stops a reviewer undoing it.

Where it came from

Raised by the fleet01 lead on 2026-09-12, from their 2026-09-10 incident on their host:

We only know that drain died because it threw. A NoClassDefFoundError reached the JVM's
uncaught handler and left a stack trace. Had the same drain hung on one session, or exited early
on a condition rather than an exception, there would be no trace, no ERROR token, no priority,
and no missing line — because there is no line.

The loud variant is the one we have seen. The quiet variants are what this line exists to catch, and
they are only detectable by the line's absence.

Their second, smaller point is also recorded: released and abandoned are counted incrementally
inside drainSnapshot's loop and folded with DrainTally.plus, not derived from a collection read
at the end. If a partial report is ever wanted it must be a different line with a different verb.
One line must not serve both, or the wording cannot tell a finished drain from an interrupted one.

I verified both properties in the code before writing the comment: no try/finally in drainAll,
and released++ / abandoned++ inside drainSnapshot's per-session loop.

Verification

Built in a scratch worktree, not in the main clone — a mvn install there would overwrite the jar
the live daemon is running on, which is #413's failure mode.

mvn -B clean install: exit 0, Tests run: 1716, Failures: 0, Errors: 0, Skipped: 0,
BUILD SUCCESS. Same count as main, as expected for a comment-only change.
#459's javadoc reference gate (mvn -B -DskipTests javadoc:javadoc -Ddoclint=reference): exit
0, 0 reference errors. The new {@link DrainTally#plus} resolves.

Reporting the exit code next to every count deliberately: a build that dies before running anything
prints no Tests run line at all, and grepping for failures then finds none.

Comment only. No behaviour change. `SessionManager.drainAll`'s javadoc. ## Why `drainAll`'s existing javadoc says the `log.info` "is a positive assertion that the drain actually finished, on the normal path, **every time**". That wording reads as an invitation to the one edit that destroys what it is describing: moving the line into a `finally`, or wrapping the method in one. In a `finally` the line prints after a drain that **threw**, carrying whatever partial `tally` it had reached. That loses the absence signal and gains a confident wrong number in the same change. Both halves of the value go at once. The code is already correct — `drainAll` has no `try`/`finally` and the `log.info` is the last statement of the successful path. Nothing here changes that. What was missing is the sentence that stops a reviewer undoing it. ## Where it came from Raised by the fleet01 lead on 2026-09-12, from their 2026-09-10 incident on their host: > We only know that drain died because it **threw**. A `NoClassDefFoundError` reached the JVM's > uncaught handler and left a stack trace. Had the same drain hung on one session, or exited early > on a condition rather than an exception, there would be no trace, no `ERROR` token, no priority, > and no missing line — because there is no line. The loud variant is the one we have seen. The quiet variants are what this line exists to catch, and they are only detectable by the line's absence. Their second, smaller point is also recorded: `released` and `abandoned` are counted incrementally inside `drainSnapshot`'s loop and folded with `DrainTally.plus`, not derived from a collection read at the end. If a partial report is ever wanted it must be a **different line with a different verb**. One line must not serve both, or the wording cannot tell a finished drain from an interrupted one. I verified both properties in the code before writing the comment: no `try`/`finally` in `drainAll`, and `released++` / `abandoned++` inside `drainSnapshot`'s per-session loop. ## Verification Built in a scratch worktree, not in the main clone — a `mvn install` there would overwrite the jar the live daemon is running on, which is #413's failure mode. - `mvn -B clean install`: exit **0**, `Tests run: 1716, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`. Same count as `main`, as expected for a comment-only change. - #459's javadoc reference gate (`mvn -B -DskipTests javadoc:javadoc -Ddoclint=reference`): exit **0**, **0** reference errors. The new `{@link DrainTally#plus}` resolves. Reporting the exit code next to every count deliberately: a build that dies before running anything prints no `Tests run` line at all, and grepping for failures then finds none.

ltms added 1 commit 2026-09-12 09:22:47 +02:00

fleetd #512 follow-up: record why the drain-complete line must not move into a finally

CI / contract (pull_request) Successful in 1m3s

Details

CI / build (pull_request) Successful in 2m8s

Details

5eb4267a4a

The javadoc said the log.info fires "every time", which reads as an invitation
to the exact edit that destroys it. The absence of the line is the signal that
the drain died, so a finally would remove the signal and print partial counts in
the same change.

Raised by the fleet01 lead from their 2026-09-10 incident: that drain is known to
have died only because it threw and left a stack trace. A drain that hung, or
returned early on a condition, leaves no trace, no ERROR token and no priority --
only a missing line.

Comment only. No behaviour change.

ltms merged commit fad99c4c5e into main

2026-09-12 09:29:47 +02:00

ltms deleted branch fix/512-drain-line-non-goal

2026-09-12 09:29:47 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#547