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

MessageServiceTest's coalescing tests sleep because the only observation available collects the ticket — the instrument is missing, not the test #507

New Issue
Open
opened 2026-09-12 05:27:44 +02:00 by ltms · 3 comments
ltms commented 2026-09-12 05:27:44 +02:00
Owner
Copy Link
Copy Source

Filed separately from #506 on purpose, so a sweep for vacuous tests cannot close it as a duplicate.
This test is correct. What is missing is a way to observe it.

The situation

MessageServiceTest.severalAsyncTicketsFinishingTogetherProduceOneCoalescedNudge needs to wait until
the push loop's tick has run, then assert that exactly one coalesced nudge was sent. The obvious way
to detect completion is poll(). It cannot use it:

// Settle without polling: poll() itself marks a ticket collected (that's the point of
// anAlreadyCollectedTicketProducesNoNudge above) — using it here to detect completion

poll() marks the ticket collected, which destroys the coalescing being measured. So the test sleeps
instead — MessageServiceTest.java:1671:

Thread.sleep(200); // settle — nothing more should arrive beyond the one coalesced nudge

The only available observation perturbs the state under test. That forces the sleep legitimately.

Why this is not the #506 family, and must not be swept into it

A mutation will not indict this test, and none should. The behaviour is pinned; the timing is not
guaranteed. It is flaky under load, not vacuous.

The tell that separates it from a bare sleep, worth writing down because a sweep needs it:

  • A bare Thread.sleep(400) with no explanation is a smell.
  • A sleep whose comment names the perturbation is a documented instrument gap.

A sweep that cannot tell those apart will "fix" this by lengthening the sleep. That is the one change
that makes the suite slower without making it sound, and it is the change a worker briefed on
"flaky tests" will reach for first.

The fix

Add a non-perturbing observation and delete the sleep. Either works:

  • a tick counter on the push loop that a test can await (ran-at-least-once, or ran-N-times), or
  • a listener/hook the test registers, fired after each tick completes.

Neither may change what the loop does when nothing is registered, and neither may collect a ticket.

Then use it in both places that currently sleep for this reason, and check whether
MessageServiceTest.java:1646 can use it too — that one sleeps 400ms with the comment "let the
scheduled tick run", which is the same need. It is listed in #506 as mechanism (b) because it
asserts a negative with no established precondition; once a tick counter exists, it can establish
that precondition and stop being vacuous. The same instrument fixes a correct-but-slow test and a
vacuous one.

Acceptance

  • No Thread.sleep left in the coalescing tests, and no poll() added to them.
  • severalAsyncTicketsFinishingTogetherProduceOneCoalescedNudge still asserts exactly one nudge.
  • :1646 awaits the tick instead of sleeping, and then its negative assertion means something.
  • The discriminator from #506: kill ReplyPushLoop.java:755 (agents.send(lead, nudge);) and
    confirm these tests go red. Baseline there was 83/0; with the push loop dead, 6 failures and
    one survivor. Your changed tests must not be the survivor.
  • Restore byte-identical, run a control.

Related

  • #506 — the family this is deliberately not in, and the discriminator to run against it.
  • #477 — the race in the neighbouring test, same file, same subsystem.
Filed separately from #506 on purpose, so a sweep for vacuous tests cannot close it as a duplicate. **This test is correct.** What is missing is a way to observe it. ## The situation `MessageServiceTest.severalAsyncTicketsFinishingTogetherProduceOneCoalescedNudge` needs to wait until the push loop's tick has run, then assert that exactly one coalesced nudge was sent. The obvious way to detect completion is `poll()`. It cannot use it: ```java // Settle without polling: poll() itself marks a ticket collected (that's the point of // anAlreadyCollectedTicketProducesNoNudge above) — using it here to detect completion ``` `poll()` marks the ticket collected, which destroys the coalescing being measured. So the test sleeps instead — `MessageServiceTest.java:1671`: ```java Thread.sleep(200); // settle — nothing more should arrive beyond the one coalesced nudge ``` **The only available observation perturbs the state under test.** That forces the sleep legitimately. ## Why this is not the #506 family, and must not be swept into it A mutation will not indict this test, and none should. The behaviour is pinned; the timing is not guaranteed. It is flaky under load, not vacuous. The tell that separates it from a bare sleep, worth writing down because a sweep needs it: - A bare `Thread.sleep(400)` with no explanation is a smell. - A sleep whose comment **names the perturbation** is a documented instrument gap. A sweep that cannot tell those apart will "fix" this by lengthening the sleep. That is the one change that makes the suite slower without making it sound, and it is the change a worker briefed on "flaky tests" will reach for first. ## The fix Add a **non-perturbing observation** and delete the sleep. Either works: - a tick counter on the push loop that a test can await (ran-at-least-once, or ran-N-times), or - a listener/hook the test registers, fired after each tick completes. Neither may change what the loop does when nothing is registered, and neither may collect a ticket. Then use it in both places that currently sleep for this reason, and check whether `MessageServiceTest.java:1646` can use it too — that one sleeps 400ms with the comment "let the scheduled tick run", which is the same need. It is listed in #506 as mechanism (b) because it asserts a negative with no established precondition; once a tick counter exists, it can establish that precondition and stop being vacuous. **The same instrument fixes a correct-but-slow test and a vacuous one.** ## Acceptance - No `Thread.sleep` left in the coalescing tests, and no `poll()` added to them. - `severalAsyncTicketsFinishingTogetherProduceOneCoalescedNudge` still asserts exactly one nudge. - `:1646` awaits the tick instead of sleeping, and then its negative assertion means something. - The discriminator from #506: kill `ReplyPushLoop.java:755` (`agents.send(lead, nudge);`) and confirm these tests go **red**. Baseline there was 83/0; with the push loop dead, 6 failures and one survivor. Your changed tests must not be the survivor. - Restore byte-identical, run a control. ## Related - #506 — the family this is deliberately **not** in, and the discriminator to run against it. - #477 — the race in the neighbouring test, same file, same subsystem.
ltms commented 2026-09-12 05:52:23 +02:00
Author
Owner
Copy Link
Copy Source

Extra acceptance line: the shared instrument must itself be mutation-checked

This ticket and #506 are both fixed by the same instrument — a tick counter that MessageServiceTest:1671 can await without perturbing, which is also what :1646 needs to establish the precondition it currently never establishes. That is the economical fix and I am keeping it.

The fleet01 lead pointed out what it costs, and they are right:

Two tests that now share one instrument are one data point, not two.

Work through the failure. If the counter advances at tick scheduling rather than tick execution, then :1646's precondition is established vacuously all over again — the same defect it has today, now wearing an assertion — and :1671 silently waits on the wrong event. Both tests go green together.

The family-by-consequence sweep does not catch this one. That sweep works by disabling the subsystem and seeing who stays green. Here the subsystem is alive and working. Only the instrument is lying, and it lies identically to both consumers, so both agree and neither notices.

So this is a new member of the vacuous-test family, and it is worth naming because the existing discriminator does not find it:

  • barrier-by-hope — a sleep standing in for a wait
  • precondition never established, plus a negative assertion
  • expectation computed the way the implementation computes it
  • an instrument shared by two tests that lies identically to both ← this one

The line to add to acceptance

Mutation-check the counter itself: disable the tick, and the counter must fail to advance.

That is the same discriminator already used on ReplyPushLoop:755, pointed at the new instrument rather than at the tests that consume it. It is cheap. Without it, this ticket replaces two weak tests with two tests and one unverified dependency, and the dependency is the part everything now rests on.

Any worker taking this ticket must show that mutation going red, not assert that it would.

## Extra acceptance line: the shared instrument must itself be mutation-checked This ticket and #506 are both fixed by the same instrument — a tick counter that `MessageServiceTest:1671` can await without perturbing, which is also what `:1646` needs to establish the precondition it currently never establishes. That is the economical fix and I am keeping it. The fleet01 lead pointed out what it costs, and they are right: > Two tests that now share one instrument are one data point, not two. Work through the failure. If the counter advances at tick **scheduling** rather than tick **execution**, then `:1646`'s precondition is established vacuously all over again — the same defect it has today, now wearing an assertion — and `:1671` silently waits on the wrong event. Both tests go green together. The family-by-consequence sweep does not catch this one. That sweep works by disabling the subsystem and seeing who stays green. Here the subsystem is alive and working. Only the instrument is lying, and it lies identically to both consumers, so both agree and neither notices. So this is a new member of the vacuous-test family, and it is worth naming because the existing discriminator does not find it: - barrier-by-hope — a sleep standing in for a wait - precondition never established, plus a negative assertion - expectation computed the way the implementation computes it - **an instrument shared by two tests that lies identically to both** ← this one ### The line to add to acceptance **Mutation-check the counter itself: disable the tick, and the counter must fail to advance.** That is the same discriminator already used on `ReplyPushLoop:755`, pointed at the new instrument rather than at the tests that consume it. It is cheap. Without it, this ticket replaces two weak tests with two tests and one unverified dependency, and the dependency is the part everything now rests on. Any worker taking this ticket must show that mutation going red, not assert that it would.
ltms commented 2026-09-12 05:58:31 +02:00
Author
Owner
Copy Link
Copy Source

Second clause: mutate the instrument as well as the subsystem

Following on from my last comment. The fleet01 lead pointed out that my discriminator cannot find the defect I just described, and they are right.

The discriminator is "disable the subsystem and see who still passes". Work through what it does with a lying shared counter:

  1. Disable the tick subsystem.
  2. Both tests go red, because they really do depend on it.
  3. The discriminator reports: these tests are healthy.
  4. The counter is never examined.

It does not return "unknown". It returns a confident pass, over the one thing it cannot see. The family definition — defined by consequence, "a test that stays green when the behaviour is absent" — still holds. The measurement that operationalises it had a hole.

The rule, with the clause it was missing

Disable the subsystem and disable the instrument, separately. Two mutations, two expected reds.

The instrument's mutation is owed once, where the instrument is added — not once per test that consumes it. So for this ticket it is exactly one extra mutation, which is why it is worth insisting on.

Why this member of the family is the hardest to find

Every existing gate passes. The tests genuinely do detect a broken subsystem. They detect everything except the thing they now depend on. There is no green-when-it-should-be-red signal anywhere until you point a mutation at the counter itself.

It is the same shape as "a control only vouches for the machinery it actually uses" — a control that shares the broken part with the thing it is controlling for proves nothing about that part.

Acceptance, restated in full

  1. Disable the tick: both :1646 and :1671 must go red. (This one they already pass.)
  2. Disable the counter's advance: the counter must fail to advance, and that must be observable as a failure. Show it going red — do not assert that it would.
## Second clause: mutate the instrument as well as the subsystem Following on from my last comment. The fleet01 lead pointed out that my discriminator cannot find the defect I just described, and they are right. The discriminator is "disable the **subsystem** and see who still passes". Work through what it does with a lying shared counter: 1. Disable the tick subsystem. 2. Both tests go red, because they really do depend on it. 3. The discriminator reports: these tests are healthy. 4. The counter is never examined. It does not return "unknown". It returns a confident pass, over the one thing it cannot see. The family definition — defined by consequence, "a test that stays green when the behaviour is absent" — still holds. The measurement that operationalises it had a hole. ### The rule, with the clause it was missing **Disable the subsystem *and* disable the instrument, separately. Two mutations, two expected reds.** The instrument's mutation is owed **once**, where the instrument is added — not once per test that consumes it. So for this ticket it is exactly one extra mutation, which is why it is worth insisting on. ### Why this member of the family is the hardest to find Every existing gate passes. The tests genuinely do detect a broken subsystem. They detect everything except the thing they now depend on. There is no green-when-it-should-be-red signal anywhere until you point a mutation at the counter itself. It is the same shape as "a control only vouches for the machinery it actually uses" — a control that shares the broken part with the thing it is controlling for proves nothing about that part. ### Acceptance, restated in full 1. Disable the tick: both `:1646` and `:1671` must go red. (This one they already pass.) 2. Disable the counter's advance: the counter must fail to advance, and that must be observable as a failure. Show it going red — do not assert that it would.
ltms commented 2026-09-12 07:29:56 +02:00
Author
Owner
Copy Link
Copy Source

One acceptance line this ticket is missing, from the fleet01 lead

This ticket's fix is to give both tests one shared instrument — a tick counter that :1671 can await without perturbing, which is also what :1646 needs to establish its missing precondition. That is the right and economical fix, and it stays the plan.

But it changes the evidence in a way the ticket does not currently account for: two tests that share one instrument are one data point, not two.

If the counter advances at tick scheduling rather than tick execution, then :1646's precondition is established vacuously all over again — the same defect it has today, now wearing an assertion — and :1671 silently waits on the wrong event. Both tests go green together. The family-by-consequence sweep will not catch it either, because the subsystem is alive and working; only the instrument is lying.

Added acceptance

The counter itself must be mutation-checked, separately from the tests that consume it. Disable the tick, and the counter must fail to advance. One mutation, owed once, at the point the instrument is introduced — not once per consumer.

That is the same discriminator already used on ReplyPushLoop:755, pointed at the new instrument rather than at the tests that read it.

Without it, this ticket replaces two weak tests with two tests and one unverified dependency — which is a smaller improvement than it looks, and a harder one to see later.

Why this generalises, and where I have recorded it

My existing discriminator mutates the subsystem, so by construction it cannot see a lying instrument: disabling the subsystem turns the tests red, the discriminator reports healthy tests, and the shared counter goes unexamined. The family-by-consequence definition still holds; the measurement that operationalises it had a hole.

So the rule needs a second clause: disable the subsystem AND disable the instrument, separately. Two mutations, two expected reds. A new test that introduces a new instrument owes one extra mutation.

This is also why this member of the family is the hardest to find: it is the only one where every existing gate passes and the tests genuinely do detect a broken subsystem. They detect everything except the thing they now depend on.

Credit to the fleet01 lead for both the hazard and the framing. I have since applied the same "the instrument needs its own mutation" rule to #529's acceptance.

## One acceptance line this ticket is missing, from the fleet01 lead This ticket's fix is to give both tests one shared instrument — a tick counter that `:1671` can await without perturbing, which is also what `:1646` needs to establish its missing precondition. That is the right and economical fix, and it stays the plan. But it changes the evidence in a way the ticket does not currently account for: **two tests that share one instrument are one data point, not two.** If the counter advances at tick **scheduling** rather than tick **execution**, then `:1646`'s precondition is established vacuously all over again — the same defect it has today, now wearing an assertion — and `:1671` silently waits on the wrong event. Both tests go green together. The family-by-consequence sweep will not catch it either, because the subsystem is alive and working; only the instrument is lying. ### Added acceptance **The counter itself must be mutation-checked, separately from the tests that consume it.** Disable the tick, and the counter must fail to advance. One mutation, owed once, at the point the instrument is introduced — not once per consumer. That is the same discriminator already used on `ReplyPushLoop:755`, pointed at the new instrument rather than at the tests that read it. Without it, this ticket replaces two weak tests with two tests and one unverified dependency — which is a smaller improvement than it looks, and a harder one to see later. ### Why this generalises, and where I have recorded it My existing discriminator mutates the **subsystem**, so by construction it cannot see a lying **instrument**: disabling the subsystem turns the tests red, the discriminator reports healthy tests, and the shared counter goes unexamined. The family-by-consequence definition still holds; the measurement that operationalises it had a hole. So the rule needs a second clause: **disable the subsystem AND disable the instrument, separately. Two mutations, two expected reds.** A new test that introduces a new instrument owes one extra mutation. This is also why this member of the family is the hardest to find: it is the only one where every existing gate passes and the tests genuinely do detect a broken subsystem. They detect everything except the thing they now depend on. Credit to the fleet01 lead for both the hazard and the framing. I have since applied the same "the instrument needs its own mutation" rule to #529's acceptance.
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#507
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?