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

The herdr contract tests were left behind by the protocol-19 port, and nothing runs them #449

New Issue
Closed
opened 2026-09-10 11:52:38 +02:00 by ltms · 4 comments
ltms commented 2026-09-10 11:52:38 +02:00
Owner
Copy Link
Copy Source

Found while checking a separate question (whether CI could run the AMQP contract tests). I ran the whole contract group on this host and 2 of them fail.

What fails, measured today

mvn -o -Pcontract test -Dgroups=contract
[ERROR] Tests run: 29, Failures: 2, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE

Green: AmqpReplyInboxContractTest (8), LeadMailboxTest (15), PaneLocatorContractTest (1), WorkspacePlacementContractTest (1).

Failure 1 — a hardcoded protocol number 5 versions stale.

HerdrContractTest.pingReturnsProtocol14:32
  fleetd is built against herdr protocol 14 ==> expected: <14> but was: <19>

The assertion's own message is the false part. fleetd is not built against protocol 14 — it was ported to 19:

AgentControl.java:16      Ported to herdr protocol 19 (herdr 0.8.0, CB-521)
WorkspaceControl.java:80  protocol 19 that seed pane is where the worker starts
Tab.java:26               Under protocol 19 ...

and the live herdr agrees: /healthz returns {"status":"ok","herdr":{"protocol":19,"version":"0.8.0"}}.

Two javadocs in main were also left behind by the same port and still claim the old number:

HerdrCodec.java:11    Wire codec for herdr's newline-delimited JSON-RPC (protocol 14).
HerdrClient.java:6    Client face onto the herdr daemon (protocol 14, herdr 0.7.0).

Failure 2 — an env-injection probe that reads back its own command.

AgentControlContractTest.tabCreateInjectsEnvIntoTheSeedShell:48
  env map must reach the seed shell; saw: printf 'PROBE_BASE=[%s]\n' "$ANTHROPIC_BASE_URL"
  ==> expected: <true> but was: <false>

What it "saw" is the probe command itself, not the command's output. So the readback captured the typed line rather than the result. That is the known shape here — herdr types the launch command into the pane — but it may also be a genuine consequence of the same port, because under protocol 19 the seed pane is where the worker starts (WorkspaceControl.java:80), which is exactly what this test asserts about. Diagnose before changing it, and say which of the two it is.

Why nobody noticed, and this is the part that matters

The contract group runs in neither place:

  • Locally: mvn clean install sets excludedGroups=contract through the default-excludes profile, so it is skipped by design.
  • In CI: the contract job exists and even provides a real RabbitMQ service container, but its step is pinned to one class: 
    run: mvn -B -Pcontract test -Dtest=AmqpReplyInboxContractTest

So a hardcoded class list decides what the contract job covers, and every contract test added after that line was written is silently outside it. LeadMailboxTest is the proof: it was deliberately written to run in CI — same @Testcontainers(disabledWithoutDocker = false) and the same AMQP_URI external-broker path as the class that is pinned, with a comment saying it "must still run against the external broker even though the runner has no Docker" — and CI has never run it, purely because its name is not in that -Dtest= list.

This is the "a table is not an exhaustive table" shape: the safe default should be to run the group and let a test opt out, not to name each test that opts in.

Why the fix is NOT simply -Dgroups=contract

I tried that first and measured it, which is the only reason I am not proposing it. On a host with a herdr socket it runs the herdr contract tests and fails, as above. Those tests do guard themselves with assumeTrue(Files.exists(socket())), so on a runner with no socket they would skip — but I have not measured CI's shape, and I am not going to assert it from reading the workflow file. Two separate changes, in this order:

  1. Fix the two failing herdr contract tests and the two stale javadocs (this ticket's main body of work). After that the group is honest, and whether CI runs it becomes a safe question rather than a risky one.
  2. Then widen the CI step. Until step 1 lands, widen it only to the AMQP classes, which cannot be affected by herdr at all: 
    run: mvn -B -Pcontract test -Dtest=AmqpReplyInboxContractTest,LeadMailboxTest
    That is still a hardcoded list, and it is still the wrong shape — but it is the honest interim, and it arms the one killer that is currently switched off (see below).

What arming LeadMailboxTest in CI actually buys

I measured this directly, against a real broker, on main:

Mutation in LeadMailbox.own() Result
boolean autoAck = true; KILLED — heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck
boolean durableQueue = false; KILLED — same test
this.heldDurable = true; (drop the derivation) SURVIVED

The third one survives because durableQueue and autoAck are literals at LeadMailbox.java:200-201, so the derivation can only ever evaluate to true — that mutant is behaviourally identical to the real code, i.e. an equivalent mutant, not a coverage gap. The mutation that represents the real risk — someone flipping autoAck to simplify the consume loop, which is precisely what #440's body predicted — is caught. It is just caught by a test that nothing runs.

Acceptance

  1. HerdrContractTest asserts the protocol fleetd is actually built against, and its failure message says so truthfully. Rename the test if the number is in its name — a test called pingReturnsProtocol14 is a maintenance trap even when its assertion is right.
  2. Prefer a single named constant for the expected protocol over a literal in a test, so a future port changes one place and the compiler or one failing test points at it.
  3. HerdrCodec's and HerdrClient's javadoc name the current protocol and herdr version.
  4. AgentControlContractTest.tabCreateInjectsEnvIntoTheSeedShell passes, and your reply states whether it was a broken probe or a real behaviour change from the protocol-19 port.
  5. mvn -o -Pcontract test -Dgroups=contract is green on a host with a live herdr socket. Paste the run.
  6. mvn clean install unchanged and green — the default build must stay hermetic and must not start needing Docker or a socket.

Out of scope

  • Do not change HerdrCodec's or AgentControl's behaviour. This is about the tests and the docs that describe them, unless criterion 4 turns up a real behaviour defect — in which case stop and report it rather than fixing it here.
  • Do not edit .gitea/workflows/ci.yml in this ticket. The CI widening is step 2 above and wants its own change, after the group is green.
  • Never print the value of ANTHROPIC_BASE_URL or any other environment variable while diagnosing criterion 4. Report names and lengths only.
Found while checking a separate question (whether CI could run the AMQP contract tests). I ran the whole contract group on this host and **2 of them fail**. ## What fails, measured today ``` mvn -o -Pcontract test -Dgroups=contract [ERROR] Tests run: 29, Failures: 2, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` Green: `AmqpReplyInboxContractTest` (8), `LeadMailboxTest` (15), `PaneLocatorContractTest` (1), `WorkspacePlacementContractTest` (1). **Failure 1 — a hardcoded protocol number 5 versions stale.** ``` HerdrContractTest.pingReturnsProtocol14:32 fleetd is built against herdr protocol 14 ==> expected: <14> but was: <19> ``` The assertion's own message is the false part. fleetd is **not** built against protocol 14 — it was ported to 19: ``` AgentControl.java:16 Ported to herdr protocol 19 (herdr 0.8.0, CB-521) WorkspaceControl.java:80 protocol 19 that seed pane is where the worker starts Tab.java:26 Under protocol 19 ... ``` and the live herdr agrees: `/healthz` returns `{"status":"ok","herdr":{"protocol":19,"version":"0.8.0"}}`. **Two javadocs in main were also left behind by the same port** and still claim the old number: ``` HerdrCodec.java:11 Wire codec for herdr's newline-delimited JSON-RPC (protocol 14). HerdrClient.java:6 Client face onto the herdr daemon (protocol 14, herdr 0.7.0). ``` **Failure 2 — an env-injection probe that reads back its own command.** ``` AgentControlContractTest.tabCreateInjectsEnvIntoTheSeedShell:48 env map must reach the seed shell; saw: printf 'PROBE_BASE=[%s]\n' "$ANTHROPIC_BASE_URL" ==> expected: <true> but was: <false> ``` What it "saw" is the probe command itself, not the command's output. So the readback captured the typed line rather than the result. That is the known shape here — herdr **types** the launch command into the pane — but it may also be a genuine consequence of the same port, because under protocol 19 the seed pane is where the worker starts (`WorkspaceControl.java:80`), which is exactly what this test asserts about. **Diagnose before changing it**, and say which of the two it is. ## Why nobody noticed, and this is the part that matters The contract group runs in **neither** place: - **Locally:** `mvn clean install` sets `excludedGroups=contract` through the `default-excludes` profile, so it is skipped by design. - **In CI:** the `contract` job exists and even provides a real RabbitMQ service container, but its step is pinned to one class: ```yaml run: mvn -B -Pcontract test -Dtest=AmqpReplyInboxContractTest ``` So a hardcoded class list decides what the contract job covers, and **every contract test added after that line was written is silently outside it**. `LeadMailboxTest` is the proof: it was deliberately written to run in CI — same `@Testcontainers(disabledWithoutDocker = false)` and the same `AMQP_URI` external-broker path as the class that is pinned, with a comment saying it "must still run against the external broker even though the runner has no Docker" — and CI has never run it, purely because its name is not in that `-Dtest=` list. This is the "a table is not an exhaustive table" shape: the safe default should be to run the group and let a test opt out, not to name each test that opts in. ## Why the fix is NOT simply `-Dgroups=contract` I tried that first and measured it, which is the only reason I am not proposing it. On a host **with** a herdr socket it runs the herdr contract tests and fails, as above. Those tests do guard themselves with `assumeTrue(Files.exists(socket()))`, so on a runner with no socket they would skip — but I have **not** measured CI's shape, and I am not going to assert it from reading the workflow file. Two separate changes, in this order: 1. **Fix the two failing herdr contract tests and the two stale javadocs** (this ticket's main body of work). After that the group is honest, and whether CI runs it becomes a safe question rather than a risky one. 2. **Then** widen the CI step. Until step 1 lands, widen it only to the AMQP classes, which cannot be affected by herdr at all: ```yaml run: mvn -B -Pcontract test -Dtest=AmqpReplyInboxContractTest,LeadMailboxTest ``` That is still a hardcoded list, and it is still the wrong shape — but it is the honest interim, and it arms the one killer that is currently switched off (see below). ## What arming `LeadMailboxTest` in CI actually buys I measured this directly, against a real broker, on `main`: | Mutation in `LeadMailbox.own()` | Result | |---|---| | `boolean autoAck = true;` | **KILLED** — `heldDurableReportsTrueBecauseTheQueueIsDurableAndTheConsumeIsManualAck` | | `boolean durableQueue = false;` | **KILLED** — same test | | `this.heldDurable = true;` (drop the derivation) | SURVIVED | The third one survives because `durableQueue` and `autoAck` are literals at `LeadMailbox.java:200-201`, so the derivation can only ever evaluate to `true` — that mutant is behaviourally identical to the real code, i.e. an **equivalent mutant, not a coverage gap**. The mutation that represents the real risk — someone flipping `autoAck` to simplify the consume loop, which is precisely what #440's body predicted — **is** caught. It is just caught by a test that nothing runs. ## Acceptance 1. `HerdrContractTest` asserts the protocol fleetd is actually built against, and its failure message says so truthfully. Rename the test if the number is in its name — a test called `pingReturnsProtocol14` is a maintenance trap even when its assertion is right. 2. Prefer a single named constant for the expected protocol over a literal in a test, so a future port changes one place and the compiler or one failing test points at it. 3. `HerdrCodec`'s and `HerdrClient`'s javadoc name the current protocol and herdr version. 4. `AgentControlContractTest.tabCreateInjectsEnvIntoTheSeedShell` passes, **and** your reply states whether it was a broken probe or a real behaviour change from the protocol-19 port. 5. `mvn -o -Pcontract test -Dgroups=contract` is green on a host with a live herdr socket. Paste the run. 6. `mvn clean install` unchanged and green — the default build must stay hermetic and must not start needing Docker or a socket. ## Out of scope - Do not change `HerdrCodec`'s or `AgentControl`'s behaviour. This is about the tests and the docs that describe them, unless criterion 4 turns up a real behaviour defect — in which case stop and report it rather than fixing it here. - Do not edit `.gitea/workflows/ci.yml` in this ticket. The CI widening is step 2 above and wants its own change, after the group is green. - **Never print the value of `ANTHROPIC_BASE_URL`** or any other environment variable while diagnosing criterion 4. Report names and lengths only.
ltms commented 2026-09-10 12:07:02 +02:00
Author
Owner
Copy Link
Copy Source

Measured the whole group again on main at 822327e, and it changes the CI part of this ticket. Delegated with these numbers.

The group's actual result

mvn -B -Pcontract test -Dgroups=contract:

AmqpReplyInboxContractTest        Tests run:  8, Failures: 0
LeadMailboxTest                   Tests run: 15, Failures: 0
AgentControlContractTest          Tests run:  1, Failures: 1  <<< FAILURE
PaneLocatorContractTest           Tests run:  1, Failures: 0
HerdrContractTest                 Tests run:  3, Failures: 1  <<< FAILURE
WorkspacePlacementContractTest    Tests run:  1, Failures: 0
TOTAL                             Tests run: 29, Failures: 2, Errors: 0, Skipped: 0
BUILD FAILURE

The two failures, named:

HerdrContractTest.pingReturnsProtocol14:32
  fleetd is built against herdr protocol 14 ==> expected: <14> but was: <19>

AgentControlContractTest.tabCreateInjectsEnvIntoTheSeedShell:48
  env map must reach the seed shell; saw: printf 'PROBE_BASE=[%s]\n' "$ANTHROPIC_BASE_URL"
  Restored session: Thu Sep 10 16:50:16 +07 2026
   ==> expected: <true> but was: <false>

The second failure is not what this ticket was about

I filed this ticket about stale numbers. Only one of the two failures is a stale number. The other one needs diagnosis before anyone touches it.

Read that failure text closely. The typed printf is visible in the pane, so input arrived. What follows it is Restored session: <date>, a shell startup line. The printf output is absent. A shell banner printing after the typed line is what you see when the shell had not reached its prompt when the text was typed. The test sleeps 1000ms for the prompt, then 800ms for the output.

So there are two candidate causes and they need different fixes:

  1. the test is timing-sensitive, and the fix is to wait on a real signal instead of a fixed sleep
  2. the env map genuinely does not reach the seed shell under protocol 19, which is a live defect in the worker env seam and a separate ticket

I have not decided which it is, and I am not guessing. The worker is briefed to find out and to stop and ask if it turns out to be cause 2 — a live env-seam defect does not get fixed inside a javadoc ticket. Note that a test which passes after you lengthen a sleep has not told you which cause it was.

The CI fix in the ticket body is the wrong shape — use the tag

I proposed widening the pin to -Dtest=AmqpReplyInboxContractTest,LeadMailboxTest. That is still a hardcoded class list, so it has the same defect as the one it replaces: it silently excludes every contract test written after it. That is exactly how protocol 19 arrived unnoticed.

The current step, ci.yml:95:

        run: mvn -B -Pcontract test -Dtest=AmqpReplyInboxContractTest

The comment above it gives the real reason for the pin: -Pcontract clears the excluded group, so a bare test re-runs the whole unit suite the build job already ran. The pin avoids that. But selecting by tag avoids it too, without naming any class:

        run: mvn -B -Pcontract test -Dgroups=contract

Locally that selected 29 tests, all of them contract-tagged — no unit tests came along. So it keeps the property the pin was protecting and drops the property that made it rot.

Still unmeasured, by me or anyone: whether a CI runner has a herdr socket. It almost certainly does not, in which case the herdr tests skip there on their assumeTrue and only the broker tests run. The PR's own CI run is the measurement, and the worker is told to read it and report what actually happened rather than predict it.

One thing I got wrong, worth writing down

I assumed these tests would skip on their assumeTrue and reasoned from the assumption. Skipped: 0. The socket exists on this host, so the assumption passes and the stale assertion fires. I was one step from proposing a CI change that turns the build red.

The rule I keep relearning: an assumeTrue tells you when a test can skip, not that it does. Only running it says which.

Measured the whole group again on `main` at `822327e`, and it changes the CI part of this ticket. Delegated with these numbers. ## The group's actual result `mvn -B -Pcontract test -Dgroups=contract`: ``` AmqpReplyInboxContractTest Tests run: 8, Failures: 0 LeadMailboxTest Tests run: 15, Failures: 0 AgentControlContractTest Tests run: 1, Failures: 1 <<< FAILURE PaneLocatorContractTest Tests run: 1, Failures: 0 HerdrContractTest Tests run: 3, Failures: 1 <<< FAILURE WorkspacePlacementContractTest Tests run: 1, Failures: 0 TOTAL Tests run: 29, Failures: 2, Errors: 0, Skipped: 0 BUILD FAILURE ``` The two failures, named: ``` HerdrContractTest.pingReturnsProtocol14:32 fleetd is built against herdr protocol 14 ==> expected: <14> but was: <19> AgentControlContractTest.tabCreateInjectsEnvIntoTheSeedShell:48 env map must reach the seed shell; saw: printf 'PROBE_BASE=[%s]\n' "$ANTHROPIC_BASE_URL" Restored session: Thu Sep 10 16:50:16 +07 2026 ==> expected: <true> but was: <false> ``` ## The second failure is not what this ticket was about I filed this ticket about stale numbers. Only one of the two failures is a stale number. The other one needs diagnosis before anyone touches it. Read that failure text closely. The typed `printf` **is** visible in the pane, so input arrived. What follows it is `Restored session: <date>`, a shell startup line. The `printf` output is absent. A shell banner printing *after* the typed line is what you see when the shell had not reached its prompt when the text was typed. The test sleeps 1000ms for the prompt, then 800ms for the output. So there are two candidate causes and they need different fixes: 1. the test is timing-sensitive, and the fix is to wait on a real signal instead of a fixed sleep 2. the env map genuinely does not reach the seed shell under protocol 19, which is a live defect in the worker env seam and a separate ticket I have not decided which it is, and I am not guessing. The worker is briefed to find out and to **stop and ask** if it turns out to be cause 2 — a live env-seam defect does not get fixed inside a javadoc ticket. Note that a test which passes after you lengthen a sleep has not told you which cause it was. ## The CI fix in the ticket body is the wrong shape — use the tag I proposed widening the pin to `-Dtest=AmqpReplyInboxContractTest,LeadMailboxTest`. That is still a hardcoded class list, so it has the same defect as the one it replaces: it silently excludes every contract test written after it. That is exactly how protocol 19 arrived unnoticed. The current step, `ci.yml:95`: ```yaml run: mvn -B -Pcontract test -Dtest=AmqpReplyInboxContractTest ``` The comment above it gives the real reason for the pin: `-Pcontract` clears the excluded group, so a bare `test` re-runs the whole unit suite the `build` job already ran. The pin avoids that. But selecting by **tag** avoids it too, without naming any class: ```yaml run: mvn -B -Pcontract test -Dgroups=contract ``` Locally that selected 29 tests, all of them contract-tagged — no unit tests came along. So it keeps the property the pin was protecting and drops the property that made it rot. **Still unmeasured, by me or anyone: whether a CI runner has a herdr socket.** It almost certainly does not, in which case the herdr tests skip there on their `assumeTrue` and only the broker tests run. The PR's own CI run is the measurement, and the worker is told to read it and report what actually happened rather than predict it. ## One thing I got wrong, worth writing down I assumed these tests would skip on their `assumeTrue` and reasoned from the assumption. `Skipped: 0`. The socket exists on this host, so the assumption passes and the stale assertion fires. I was one step from proposing a CI change that turns the build red. The rule I keep relearning: an `assumeTrue` tells you when a test *can* skip, not that it *does*. Only running it says which.
ltms commented 2026-09-10 12:22:35 +02:00
Author
Owner
Copy Link
Copy Source

Full inventory of what -Dgroups=contract selects, and two things about it that should be written down before the selector changes. Measured at bdcf285. The fleet01 lead did the per-class gate audit; I re-derived it and verified the image difference they spotted.

Every contract class, with its gate

CLASS                            tests  assumeTrue  beforeEach  beforeAll  gate
AgentControlContractTest         1      1           0           0          herdr socket
HerdrContractTest                3      3           0           0          herdr socket
PaneLocatorContractTest          1      1           0           0          herdr socket
WorkspacePlacementContractTest   1      1           0           0          herdr socket
AmqpReplyInboxContractTest       8      0           0           1          AMQP_URI / Testcontainers
LeadMailboxTest                 15      0           0           1          AMQP_URI / Testcontainers

Six classes, 29 tests. The number that matters is tests == assumeTrue in all four herdr classes, with no @BeforeEach or @BeforeAll in any of them. "There is a gate" and "the gate covers every test" are two different claims, and only the second one lets you predict what happens on a socket-less runner. A class with 3 tests and 1 assumption would skip one and run two. Here every test is gated, so all six herdr tests skip together.

Thing 1 — the group has two different behaviours when a dependency is missing

The two AMQP classes have zero assumeTrue. They do not skip; @Testcontainers(disabledWithoutDocker = false) stops Testcontainers disabling the class, and EXTERNAL_URI only decides whether a container starts. So:

herdr socket missing   ->  6 tests SKIP   (assumeTrue)
broker missing         -> 23 tests FAIL   (no assumption anywhere)

On the current contract job that is fine and arguably right — AMQP_URI is set at job level, and a job whose purpose is the broker should go red if the broker disappears. But it means -Dgroups=contract is only safe in a job that provides a broker. If that selector is ever copied into a job without the rabbitmq service, the run goes red and the failure will read as a code defect. Do not put the tag selector anywhere without checking the broker comes with it.

Thing 2 — the two broker paths are not the same image

AmqpReplyInboxContractTest:53   DockerImageName.parse("rabbitmq:3.13-management")
LeadMailboxTest:48              DockerImageName.parse("rabbitmq:3.13-management")
.gitea/workflows/ci.yml:68      image: rabbitmq:3.13

Same AMQP 0-9-1 engine, and the -management tag only adds the management plugin, so this is low risk. But "green locally" and "green in CI" are not statements about the same image. For tests that pin ack and durability behaviour, that is worth a comment in the code rather than a thing someone discovers during an incident. Add a one-line note to whichever place you touch anyway — do not go on a hunt for it.

And the arm64 point, corrected

I reported earlier that the CI runner is arm64 (from the cache key setup-java-Linux-arm64-maven-…) and said that constrains any future contract test that pulls an image. That is the less useful half of the truth. On CI these two classes pull nothing: AMQP_URI is set at job level, so no container starts and the image name is never resolved. arm64 only ever touched the ci.yml service image, which happens to be multi-arch.

So the rule for a future contract test is "give it an external-URI escape hatch, and the image stops mattering on CI" — not "check the image is multi-arch". The escape hatch is the mechanism. The image being multi-arch was luck.

One instrument note, since this ticket is about a stale number

My first pass at the table above reported 9 tests for AmqpReplyInboxContractTest and 16 for LeadMailboxTest. Both were exactly one too high. Two independent files off by the same amount is a signal about the instrument, not the code:

$ grep -n '^\s*@Test' LeadMailboxTest.java
42:@Testcontainers(disabledWithoutDocker = false)      <-- counted as a test
75:    @Test

@Test is a prefix of @Testcontainers. Anchoring the pattern — ^[[:space:]]*@Test[[:space:]]*$ — gives 8 and 15, which match what the run reports. A control would not have caught this: the grep ran and returned real lines. Anchor any pattern that names an annotation or an identifier, because most of them are a prefix of something else.

Full inventory of what `-Dgroups=contract` selects, and two things about it that should be written down before the selector changes. Measured at `bdcf285`. The fleet01 lead did the per-class gate audit; I re-derived it and verified the image difference they spotted. ## Every contract class, with its gate ``` CLASS tests assumeTrue beforeEach beforeAll gate AgentControlContractTest 1 1 0 0 herdr socket HerdrContractTest 3 3 0 0 herdr socket PaneLocatorContractTest 1 1 0 0 herdr socket WorkspacePlacementContractTest 1 1 0 0 herdr socket AmqpReplyInboxContractTest 8 0 0 1 AMQP_URI / Testcontainers LeadMailboxTest 15 0 0 1 AMQP_URI / Testcontainers ``` Six classes, 29 tests. The number that matters is `tests == assumeTrue` in all four herdr classes, with no `@BeforeEach` or `@BeforeAll` in any of them. **"There is a gate" and "the gate covers every test" are two different claims**, and only the second one lets you predict what happens on a socket-less runner. A class with 3 tests and 1 assumption would skip one and run two. Here every test is gated, so all six herdr tests skip together. ## Thing 1 — the group has two different behaviours when a dependency is missing The two AMQP classes have **zero** `assumeTrue`. They do not skip; `@Testcontainers(disabledWithoutDocker = false)` stops Testcontainers disabling the class, and `EXTERNAL_URI` only decides whether a container starts. So: ``` herdr socket missing -> 6 tests SKIP (assumeTrue) broker missing -> 23 tests FAIL (no assumption anywhere) ``` On the current contract job that is fine and arguably right — `AMQP_URI` is set at job level, and a job whose purpose is the broker should go red if the broker disappears. **But it means `-Dgroups=contract` is only safe in a job that provides a broker.** If that selector is ever copied into a job without the rabbitmq service, the run goes red and the failure will read as a code defect. Do not put the tag selector anywhere without checking the broker comes with it. ## Thing 2 — the two broker paths are not the same image ``` AmqpReplyInboxContractTest:53 DockerImageName.parse("rabbitmq:3.13-management") LeadMailboxTest:48 DockerImageName.parse("rabbitmq:3.13-management") .gitea/workflows/ci.yml:68 image: rabbitmq:3.13 ``` Same AMQP 0-9-1 engine, and the `-management` tag only adds the management plugin, so this is low risk. But "green locally" and "green in CI" are not statements about the same image. For tests that pin ack and durability behaviour, that is worth a comment in the code rather than a thing someone discovers during an incident. **Add a one-line note to whichever place you touch anyway** — do not go on a hunt for it. ## And the arm64 point, corrected I reported earlier that the CI runner is arm64 (from the cache key `setup-java-Linux-arm64-maven-…`) and said that constrains any future contract test that pulls an image. That is the less useful half of the truth. On CI these two classes pull **nothing**: `AMQP_URI` is set at job level, so no container starts and the image name is never resolved. arm64 only ever touched the `ci.yml` service image, which happens to be multi-arch. So the rule for a future contract test is **"give it an external-URI escape hatch, and the image stops mattering on CI"** — not "check the image is multi-arch". The escape hatch is the mechanism. The image being multi-arch was luck. ## One instrument note, since this ticket is about a stale number My first pass at the table above reported 9 tests for `AmqpReplyInboxContractTest` and 16 for `LeadMailboxTest`. Both were exactly one too high. Two independent files off by the same amount is a signal about the instrument, not the code: ``` $ grep -n '^\s*@Test' LeadMailboxTest.java 42:@Testcontainers(disabledWithoutDocker = false) <-- counted as a test 75: @Test ``` `@Test` is a prefix of `@Testcontainers`. Anchoring the pattern — `^[[:space:]]*@Test[[:space:]]*$` — gives 8 and 15, which match what the run reports. A control would not have caught this: the grep ran and returned real lines. **Anchor any pattern that names an annotation or an identifier, because most of them are a prefix of something else.**
ltms commented 2026-09-10 12:30:09 +02:00
Author
Owner
Copy Link
Copy Source

The CI run measured — the open premise is now closed

The open question on this ticket and in the lead-to-lead thread was: does turning the class list into a tag selector make CI red? Nobody could answer it by reading, because it depends on whether the self-hosted runner has a herdr socket. Neither the worker nor fleet01 could retrieve the run (the worker's forge tool returned 401, and four REST guesses returned 404).

I pulled the contract job log for PR #452. The log names its own commit, HEAD is now at d4f93a7, which is the PR head:

AgentControlContractTest        Tests run: 1,  Failures: 0, Errors: 0, Skipped: 1
PaneLocatorContractTest         Tests run: 1,  Failures: 0, Errors: 0, Skipped: 1
HerdrContractTest               Tests run: 3,  Failures: 0, Errors: 0, Skipped: 3
WorkspacePlacementContractTest  Tests run: 1,  Failures: 0, Errors: 0, Skipped: 1
LeadMailboxTest                 Tests run: 15, Failures: 0, Errors: 0, Skipped: 0
AmqpReplyInboxContractTest      Tests run: 8,  Failures: 0, Errors: 0, Skipped: 0
TOTAL                           Tests run: 29, Failures: 0, Errors: 0, Skipped: 6

All 6 herdr tests skip on the runner. All 23 broker tests run. 0 failures. The runner has no herdr socket, so every herdr test hits its own assumeTrue and skips. The tag selector is safe there.

LeadMailboxTest runs 15 tests in CI that had never run there before. It was always ready — same tag, @Testcontainers(disabledWithoutDocker = false), and it reads AMQP_URI from the environment instead of starting its own container. The hardcoded -Dtest= list is the only reason it never ran.

One correction about the evidence, so nobody re-checks it the way I first did

I first tried to read the selector straight out of the log, by grepping for the mvn -B -Pcontract test -D... command. That grep returns nothing, and the empty result is not a finding. The Gitea runner does not echo a step's run: command into the job log — it logs only the step's output. The word contract does appear 15 times, but every one of them is the job name or a container/network name.

So the selector is proven by the result, not by the command. That is the stronger proof anyway: the old selector -Dtest=AmqpReplyInboxContractTest can only ever produce one test class. This job ran six:

dev.ltms.fleet.herdr.AgentControlContractTest
dev.ltms.fleet.herdr.HerdrContractTest
dev.ltms.fleet.herdr.PaneLocatorContractTest
dev.ltms.fleet.herdr.WorkspacePlacementContractTest
dev.ltms.fleet.msg.AmqpReplyInboxContractTest
dev.ltms.fleet.msg.LeadMailboxTest

A run that produces six classes cannot have used the one-class selector.

Why this ticket existed at all

The stale protocol assertion (14 against a live herdr 19) sat there because the test that would have caught it was excluded from CI by name. The tag selector is the fix for the ticket's cause, not just for its symptom. I am still verifying the PR myself before merging.

## The CI run measured — the open premise is now closed The open question on this ticket and in the lead-to-lead thread was: **does turning the class list into a tag selector make CI red?** Nobody could answer it by reading, because it depends on whether the self-hosted runner has a herdr socket. Neither the worker nor fleet01 could retrieve the run (the worker's forge tool returned 401, and four REST guesses returned 404). I pulled the contract job log for PR #452. The log names its own commit, `HEAD is now at d4f93a7`, which is the PR head: ``` AgentControlContractTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 1 PaneLocatorContractTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 1 HerdrContractTest Tests run: 3, Failures: 0, Errors: 0, Skipped: 3 WorkspacePlacementContractTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 1 LeadMailboxTest Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 AmqpReplyInboxContractTest Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 TOTAL Tests run: 29, Failures: 0, Errors: 0, Skipped: 6 ``` **All 6 herdr tests skip on the runner. All 23 broker tests run. 0 failures.** The runner has no herdr socket, so every herdr test hits its own `assumeTrue` and skips. The tag selector is safe there. `LeadMailboxTest` runs **15 tests in CI that had never run there before.** It was always ready — same tag, `@Testcontainers(disabledWithoutDocker = false)`, and it reads `AMQP_URI` from the environment instead of starting its own container. The hardcoded `-Dtest=` list is the only reason it never ran. ### One correction about the evidence, so nobody re-checks it the way I first did I first tried to read the selector straight out of the log, by grepping for the `mvn -B -Pcontract test -D...` command. **That grep returns nothing, and the empty result is not a finding.** The Gitea runner does not echo a step's `run:` command into the job log — it logs only the step's output. The word `contract` does appear 15 times, but every one of them is the job name or a container/network name. So the selector is proven by the *result*, not by the command. That is the stronger proof anyway: the old selector `-Dtest=AmqpReplyInboxContractTest` can only ever produce **one** test class. This job ran **six**: ``` dev.ltms.fleet.herdr.AgentControlContractTest dev.ltms.fleet.herdr.HerdrContractTest dev.ltms.fleet.herdr.PaneLocatorContractTest dev.ltms.fleet.herdr.WorkspacePlacementContractTest dev.ltms.fleet.msg.AmqpReplyInboxContractTest dev.ltms.fleet.msg.LeadMailboxTest ``` A run that produces six classes cannot have used the one-class selector. ### Why this ticket existed at all The stale protocol assertion (14 against a live herdr 19) sat there because the test that would have caught it **was excluded from CI by name**. The tag selector is the fix for the ticket's cause, not just for its symptom. I am still verifying the PR myself before merging.
ltms commented 2026-09-10 12:36:50 +02:00
Author
Owner
Copy Link
Copy Source

Merged, with one correction to the fix's own comment

PR #452 is merged. I verified it on a merge I built myself, because the branch was cut from 822327e and main had moved twice since (to c11ad71, then this).

Merge built on c11ad71, 0 conflicts, all four changes present:

FULL UNIT BUILD     Tests run: 1578, Failures: 0, Errors: 0, Skipped: 0   BUILD SUCCESS
                    compile errors: 0
CONTRACT TAG        Tests run: 30,   Failures: 0, Errors: 0, Skipped: 0
  AmqpReplyInboxContractTest       9
  LeadMailboxTest                 15
  AgentControlContractTest         1
  PaneLocatorContractTest          1
  HerdrContractTest                3
  WorkspacePlacementContractTest   1

30 here, 29 in CI — the difference is #448's ninth broker test, which the PR branch predates. All 6 herdr tests run here (this host has a socket) and skip in CI. Green for opposite reasons, which is what the assumeTrue gates are for.

The timing test, 5 standalone runs: all green.

Two mutations killed:

mutation result
restore assertEquals(14, pong.get("protocol") pingReturnsProtocol19 FAILS — the assertion is live, and the real herdr answers 19
expect PROBE_BASE=[http://WRONG.invalid:1] FAILS, and reports the real pane text in the message rather than a bare boolean

The correction: the fix's javadoc named a cause nobody measured

The new comment said the old failure was input typed before the shell's prompt being swallowed by the shell's own startup. I tried to kill that claim and could not confirm it.

I set SHELL_READY_TIMEOUT_MS = 0, so waitUntilSettled returns at once and input is typed immediately with no settle wait. The test passed 3 of 3.

So waitForText is the load-bearing half of the fix, and the direction of the evidence points the other way:

  • old version: 1000ms before typing, 800ms before reading → failed
  • mutated version: 0ms before typing, polling up to 5s before reading → passed 3/3

If early input were swallowed, typing at 0ms would be worse than typing at 1000ms. It is better. So the proven cause is the 800ms read deadline, not the write delay.

This did not block the merge — polling for a signal beats guessing a sleep whatever the mechanism, and waitUntilSettled is cheap. But a confident wrong mechanism in a comment is exactly the thing that gets copied into the next test, so I corrected it in 20c1094: the javadoc now states what was measured, labels waitUntilSettled as insurance rather than the fix, and says what would falsify the swallow theory for anyone who wants to try.

Why this ticket existed — the mechanism, now measured

The drift was not that nobody knew the protocol was 19. Everything except the one test that talks to real herdr already said 19. At 822327e, before the fix:

site number
FakeHerdr — the canned fake 19
FleetAppTest:156 — unit test asserting against the fake 19
HerdrContractTest — the only test talking to a real herdr 14

So the unit suite was green, self-consistent, and testing the fake against the fake. The one test that would have caught reality was pinned out of CI by name. That is the whole failure, and the tag selector is the fix for it.

Closing. Follow-up filed for the protocol number having no single home.

## Merged, with one correction to the fix's own comment PR #452 is merged. I verified it on a merge I built myself, because the branch was cut from `822327e` and main had moved twice since (to `c11ad71`, then this). **Merge built on `c11ad71`, 0 conflicts, all four changes present:** ``` FULL UNIT BUILD Tests run: 1578, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS compile errors: 0 CONTRACT TAG Tests run: 30, Failures: 0, Errors: 0, Skipped: 0 AmqpReplyInboxContractTest 9 LeadMailboxTest 15 AgentControlContractTest 1 PaneLocatorContractTest 1 HerdrContractTest 3 WorkspacePlacementContractTest 1 ``` 30 here, 29 in CI — the difference is #448's ninth broker test, which the PR branch predates. All 6 herdr tests **run** here (this host has a socket) and **skip** in CI. Green for opposite reasons, which is what the `assumeTrue` gates are for. **The timing test, 5 standalone runs:** all green. **Two mutations killed:** | mutation | result | |---|---| | restore `assertEquals(14, pong.get("protocol")` | `pingReturnsProtocol19` FAILS — the assertion is live, and the real herdr answers 19 | | expect `PROBE_BASE=[http://WRONG.invalid:1]` | FAILS, and reports the real pane text in the message rather than a bare boolean | ### The correction: the fix's javadoc named a cause nobody measured The new comment said the old failure was **input typed before the shell's prompt being swallowed by the shell's own startup.** I tried to kill that claim and could not confirm it. I set `SHELL_READY_TIMEOUT_MS = 0`, so `waitUntilSettled` returns at once and input is typed immediately with no settle wait. **The test passed 3 of 3.** So `waitForText` is the load-bearing half of the fix, and the direction of the evidence points the other way: - old version: **1000ms** before typing, **800ms** before reading → **failed** - mutated version: **0ms** before typing, polling up to 5s before reading → **passed 3/3** If early input were swallowed, typing at 0ms would be *worse* than typing at 1000ms. It is better. So the proven cause is the **800ms read deadline**, not the write delay. This did not block the merge — polling for a signal beats guessing a sleep whatever the mechanism, and `waitUntilSettled` is cheap. But a confident wrong mechanism in a comment is exactly the thing that gets copied into the next test, so I corrected it in `20c1094`: the javadoc now states what was measured, labels `waitUntilSettled` as insurance rather than the fix, and says what would falsify the swallow theory for anyone who wants to try. ### Why this ticket existed — the mechanism, now measured The drift was not that nobody knew the protocol was 19. **Everything except the one test that talks to real herdr already said 19.** At `822327e`, before the fix: | site | number | |---|---| | `FakeHerdr` — the canned fake | **19** | | `FleetAppTest:156` — unit test asserting against the fake | **19** | | `HerdrContractTest` — the only test talking to a real herdr | **14** | So the unit suite was green, self-consistent, and testing the fake against the fake. The one test that would have caught reality was pinned out of CI by name. That is the whole failure, and the tag selector is the fix for it. Closing. Follow-up filed for the protocol number having no single home.
ltms closed this issue 2026-09-10 12:37:27 +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#449
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?