fix(engine): a status poll writes only over the status it read, so a cancelled run stays cancelled

[bmscomp]

Why

The reconciler, or a GET /api/tests/{id}, reads a RUNNING run, and refreshStatus polls each of its tasks. A cancel can land during that poll. cancelTest settles the run first: it drops the handles, stops the workers and frees the permit. Then it stores the run FAILED with saveIfStatus(cancelled, RUNNING). Next the poll wrote its own copy with saveIfPresent(run). That method checks only that the row exists, so the poll's copy replaced the cancel's FAILED:

• The stopped tasks still answer RUNNING. The row goes back to RUNNING with no handles and no permit, and nothing polls it again. kates test get and --wait show RUNNING until the timeout reaper ends the run at its deadline. By default that is up to 2 h 5 min after the run was created.
• They answer DONE (the native backend marks a stopped task DONE). The cancelled run is stored DONE and reads as passed. The poll's ending block also runs: it fires a DONE event after the cancel's FAILED, so webhooks hear the run end twice, and it calls recordTestCompleted a second time.

The window is one poll. On Trogdor each task poll is an HTTP call, so the window grows with the number of tasks. TestOrchestratorCancelTest had no case for this.

The code shows two more writes the poll could overwrite:

• the timeout reaper's FAILED, which it also writes with saveIfStatus;
• the submission's RUNNING, when the poll read the run while it was PENDING, for example a GET that arrives just before executeAsync's saveIfStatus(run, PENDING).

The finding's fix was incomplete in two ways:

• Ordering. Moving only the event and the completion metrics after the write still dropped the handles, ended the meters and freed the permit before the write. Take a poll that loses only because the run moved from PENDING to RUNNING. No other writer has settled that run, so it would stay RUNNING with nothing left to poll it until the reaper ends it. All three steps have to move after the write too.
• Callers. saveIfPresent has no caller left in main, but five test stubs and three TestRunRepositoryTest tests still call it, and three javadoc links name it.

Changes

TestOrchestrator.refreshStatus

• The poll writes with repository.saveIfStatus(run, priorStatus), so it writes only over the status it read. The write now comes before the ending block, not after it.

• When the write loses, the poll reads the run back with findById:

  • If the row is gone because of a delete, the poll settles the run and returns it, as before.
  • If the row is there, the poll returns it as stored and does not settle it. A cancel, the reaper or a delete has already settled the run, and a run that only moved from PENDING to RUNNING still needs its handles.

• The ending block now runs only after this poll's write lands, under the same allDone && !updatedResults.isEmpty() test as before. The block covers:

  • dropping the handles;
  • firing DONE or FAILED;
  • recordTestCompleted;
  • the duration, throughput, records and task-status metrics;
  • benchmarkMetrics.endRun;
  • releasePermit.

  Because of this, two polls that read the same RUNNING run can no longer both count its completion: the second one's write loses.

• The OptimisticLockException catch is unchanged.

• deleteTest's javadoc now links saveIfStatus instead of saveIfPresent. The line count is the same.

TestRunRepository

• saveIfPresent and its javadoc are removed.
• saveIfStatus's javadoc takes over the guarantee saveIfPresent documented: it only updates and never inserts, so a write that read the run before a delete cannot bring the run back. {@link #delete} takes the same row lock. Its @return now also says false when the row is gone. The method already behaved that way.
• delete()'s javadoc names only saveIfStatus's lock.

Test stubs

• The saveIfPresent stubs are gone from InMemoryEngine, TestOrchestratorCancelTest, TestOrchestratorLiveMetricsTest, TestOrchestratorTest and TestTimeoutReaperSubmissionTest. Their saveIfStatus stubs already compare statuses, so the polls in those tests still store their writes. In TestTimeoutReaperSubmissionTest, only the reaper's write arms the beforeConditionalWrite hooks, so poll writes do not trigger them.
• InMemoryEngine's class javadoc and the comment on TestOrchestratorLiveMetricsTest's saveIfStatus stub now say that polls write through saveIfStatus too.
• TestOrchestratorCancelTest.RecordingBackend gets three fields:
  • onPoll, which runs first in each poll;
  • stoppedAnswer, the status a stopped task answers with (FAILED by default, as before);
  • liveAnswer, the status a task that was not stopped answers with (RUNNING by default, as before).

kates mcp refs (no caveat text changes)

• refreshStatus is 16 lines longer, so every TestOrchestrator.java ref past it moves by +16:
  • mcp_caveats.go: :70, :92 (only its ranges past line 698), :115, :116 and :159;
  • mcp_tools_runs.go: the anchored refs at :2233 (second range only), :2270, :2303 and :2306, and the comment refs at :187, :662 and :788;
  • mcp_tools_scenario.go: :1247 and :1263 (:1263 stays identical to mcp_caveats.go:115);
  • mcp_tools_runs_test.go: the comment ref at :815.
• TestRunRepository.java is 19 lines shorter above findWithResults, so mcp_caveats.go:171 moves from 354-374 to 335-355.
• cancel-stored-as-failed is still true as written.

Tests

Added:

• TestOrchestratorCancelTest.aCancelDuringAPollStaysTheRunsEnding, run twice: with the stopped tasks answering RUNNING, and with them answering DONE.
  • Setup. executeAsync submits the run. The poll's first task poll calls cancelTest, and then refreshStatus returns.
  • Asserts. The row is the run as the cancel stored it, and the poll answers FAILED. The cancel's ending is the only one announced. recordTestCompleted("LOAD", "failed") runs once and ("LOAD", "done") never runs. A later reconcileActiveRuns leaves the row alone, and every slot is free.
• TestOrchestratorCancelTest.aPollThatLosesToTheSubmissionKeepsTheRunGoing. This test pins the ordering fix described under Why.
  • Setup. The poll reads the run while it is PENDING. The submission's RUNNING lands during the poll, and every task answers DONE.
  • Asserts. The poll's write loses and the row stays RUNNING. Nothing announces or counts an ending, and endRun is not called. The next reconcileActiveRuns ends the run DONE and announces it once.
• TestRunRepositoryTest, on a real database:
  • saveIfPresentUpdatesAStoredRun becomes saveIfStatusWritesOnlyOverTheStatusItExpects. A write from PENDING to RUNNING lands. A second write that expects PENDING does not, and RUNNING stays stored.
  • saveIfPresentNeverInsertsARun becomes saveIfStatusNeverInsertsARun. The write returns false after a delete, and also for a run that was never stored.
  • anIntegrityResultIsStoredWithItsTask now writes with saveIfStatus(..., RUNNING), then saveIfStatus(..., DONE).

Run locally, all passing:

• cd kates && ./mvnw -o test -Dquarkus.grpc.codegen.skip=true -Dquarkus.http.test-port=0: 1,469 tests, 0 failures, 0 errors, 0 skipped. The one ERROR line in the log is expected: OutboxCleanerTest exercises dead-letter handling on purpose.
• cd kates && JAVA_HOME=$HOME/.sdkman/candidates/java/25.0.2-graalce ./mvnw -o spotless:check -Dquarkus.grpc.codegen.skip=true: 399 files clean.
• cd cli && go test ./...: every package ok. This includes TestMCPCaveatRefAnchors in cmd, so the re-pointed refs are anchored.
• cd cli && go test -p 1 -race -count=1 ./cmd/: run again with CI's flags on the changed package; ok in 103 s.
• cd cli && go vet ./...: no output.
• cd cli && golangci-lint run --new-from-rev=origin/main ./...: 0 issues.
• cd cli && govulncheck ./...: no vulnerabilities.
• scripts/check-cli-style.sh and scripts/check-cli-compat.sh: OK.
• The book, chart, Makefile, actionlint, shellcheck and docs checks are not triggered, so they were not run. The diff touches only cli/cmd/ (4 files) and kates/src/{main,test} (8 files).

Run only in CI:

• ci.yml Java job, ./mvnw verify: the *IT failsafe tests need Docker/Testcontainers (EngineLifecycleIT runs on Postgres), and the jacoco:check coverage floor is bound to verify.
• ci.yml Go job: the full go test -p 1 -race -count=1 with coverage across every cli package. Locally only cli/cmd ran with -race.
• native.yml: the native image build and smoke test, triggered by kates/**.
• integration.yml: the Kind end-to-end tests.
• ci-docker.yml: the Docker image builds.
• ci.yml: the dependency CVE scan, which needs network.
• codeql.yml: CodeQL.
• The github-advanced-security AI scan, which fails on every PR on GitHub's side.

Decisions

• Keep the OptimisticLockException catch. EntityMapper copies no version and saveIfStatus holds the row lock, so the exception should not fire. Removing the catch would be churn.
• Re-read with a plain findById after a lost compare-and-set, as cancelTest does, not in QuarkusTransaction.requiringNew().
• No refresh-vs-refresh double-count test. The same compare-and-set covers that case, and the cancel test pins the order.
• Move activeHandles.remove, endRun and releasePermit after the write, together with the event and the metrics (the ordering fix under Why).

Deliberately not done

• Not verified: one stale GET answer. Outside a transaction, the findById after a lost write may return the request session's copy. A GET that lost to a cancel may then answer the pre-cancel status once. The stored row is right, and the next GET reads it fresh, the same as the existing re-reads in refreshStatus and cancelTest.
• Two polls that both read RUNNING can still both write. A status compare-and-set cannot see a move from RUNNING to RUNNING, so an older poll's numbers can land. The next tick corrects them.
• mcp_tools_runs.go:662 is still stale. On main, this comment ref already cited lines that do not hold the <type>-test topic default (it cited 1598 and 1925-1926; the default was at 1612 and 1939-1940). This PR moves it by +16, like every other ref, to 1614 and 1941-1942. Here the right lines are 1628 and 1955-1956. It is a comment ref, so no test catches it. It is left for a follow-up.
• TestRunRepository.delete()'s javadoc keeps its line count, so after the reflow its third line is short.
• Old line refs in docs/backend-enhancement-plan*.md. These files cite TestOrchestrator.java and TestRunRepository.java lines that were already stale on main. They are not touched.
• Shared ref with plan PR-H (run retention keeps the run a baseline names; not open yet). That PR also re-points mcp_caveats.go:171 (TestRunRepository.java, findWithResults). Whichever lands second must recompute the ref from the merged file: 335 here, plus the lines H adds.

Landing

Lands after #375 (plan PR 6) and #376 (plan PR 7), because both edit files this PR also edits.

• fix(chaos): compound runs and resilience faults go through the safety guard and the disruption lease #375, in cli/cmd/mcp_tools_runs.go and cli/cmd/mcp_tools_scenario.go. The hunks are disjoint:
  • mcp_tools_runs.go: fix(chaos): compound runs and resilience faults go through the safety guard and the disruption lease #375 edits :2395 and :2397; this PR edits :187, :662, :788, :2233, :2270, :2303 and :2306.
  • mcp_tools_scenario.go: fix(chaos): compound runs and resilience faults go through the safety guard and the disruption lease #375 edits :1289; this PR edits :1247 and :1263.
• fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376, in cli/cmd/mcp_caveats.go, cli/cmd/mcp_tools_runs.go, cli/cmd/mcp_tools_scenario.go, TestOrchestrator.java and TestOrchestratorTest.java.
  • One conflict, in the integrity-group ref block of mcp_caveats.go, which sits two lines lower after fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376's insert at :105. This PR changes the range on the TestOrchestrator.java:1619-1623,1714-1718 line, and fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376 changes the anchor on the next line plus the NativeKafkaBackend.java ref. To resolve it, take this PR's ranges (1635-1639,1730-1734) and fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376's anchor and NativeKafkaBackend line.
  • fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376 rewrites TestOrchestrator.java :1715-1718 in place without changing the line count, so the +16 still holds.
  • The other hunks are disjoint.

Recipe once both have merged:

1. Rebase onto main.
2. Build a diff line map of TestOrchestrator.java and TestRunRepository.java from this branch's base to the merged files.
3. Re-point every moved ref from that map, including the unanchored ones (mcp_caveats.go:92 and the comment refs).
4. Run cd cli && go test ./cmd -run TestMCPCaveat.

Main has also gained #370 since this branch's base. It touches only Makefile and docs/kates-chaos-chart.md, so it does not overlap this PR.