Skip to content

🤖 test: check the promote deadlines without the wall clock - #361

Merged
ThomasK33 merged 1 commit into
mainfrom
test/promote-timing-deterministic
Oct 14, 2026
Merged

ThomasK33 merged 1 commit into
mainfrom
test/promote-timing-deterministic

Conversation

@ThomasK33

Copy link
Copy Markdown
Member

The promote timing tests in internal/aggregated/storage no longer fail when the test process stalls. Each time check now reads only the deadlines that the storage sets on its requests, never the test's wall clock. This PR changes only tests. It adds no production change and no production seam.

Fixes #354

Cause

The container's CPU cgroup has a quota of 32 CPUs per 100 ms period (cpu.max 3200000 100000). On this host it was throttled in 31 % of all periods (cpu.stat: 15,942 of 51,410). Memory was at 64.0 of 64 GiB, so reclaim stalls also happen. A throttled cgroup stops the whole test process while wall time goes on. Two checks could not survive that:

  • TestTemplatePromoteConfirmation/timeout_{before,after}_apply failed when the promotion took more than 1 s of wall time ("promotion waited 1.05s for a slow activation; the PATCH budget is 50ms").
  • TestTemplatePromoteDeadline/slow_re-read_ends_before_the_deadline gave the storage 200 ms of wall time between the re-read deadline and the request deadline ("the storage answered after the request deadline"). It also needed the lookups to finish within 400 ms, and it slept 3 s in the fake.

Change

  • holdPatchTransport now gets the PATCH budget. Before it sends the PATCH, it checks that the PATCH context has a deadline at most that budget away. Time only shortens the remaining budget, so a stall cannot fail this check. A PATCH without the budget still fails it, because only the client's 30 s timeout bounds it.
    • It no longer has a 2 s time.After fallback. It waits for the PATCH deadline, which the check above proved is set.
    • The storage takes any PATCH error as an uncertain result. So the transport also sends each failed check to a problems channel, and the test reads that channel.
    • The elapsed > 1s check is gone.
  • New rereadDeadlineTransport records the deadline of the confirming re-read (GET /api/v2/templates/{id}) and fails the re-read at once with context.DeadlineExceeded.
    • The test checks that the re-read deadline is exactly a fifth of the reserve before the request deadline.
    • The request deadline is the two default budgets plus 5 s (20 s in all). That is enough time for the lookups, and it ends before the client's own 30 s request timeout, so the storage sets the re-read deadline.
  • promoteFault.rereadDelay and its real 3 s sleep are removed. Nothing else used them.

Other tests in the file keep their real waits. TestTemplatePromoteEmitsNoTemplateWatchEvent waits 200 ms for an event that must not come, and a stall cannot make it fail.

Reproduction and results

Each row is one run of TestTemplatePromote(Confirmation|Deadline) with -count=50: 100 test runs, at host load 30 to 157.

Profile Before (78c29687) After (e86206bf)
The requested command, with 2 loops of go test ./internal/controller/... 0 / 100 failed 0 / 100 failed
The same, plus 2 loops of the full storage package 0 / 100 0 / 100
Test and 16 busy loops pinned to one CPU (taskset) 0 / 100 0 / 100
Process stalls: the test process gets SIGSTOP for 1.2 s, then runs 0.5 s, in a loop 49 / 100 (all "the storage answered after the request deadline") 0 / 100
Process stalls: stopped 1.2 s, runs 0.03 s 92 / 100 (95 "promotion waited … PATCH budget", 44 "answered after the request deadline") 0 / 100

Plain host load did not reproduce the flake, because Linux's fair scheduler gives these few-millisecond tests their slices. Stalls of the whole process reproduce both failure messages from the #352 run. The stall script stops only the test PID that it started, and it always sends SIGCONT on exit.

The fixed tests also run faster: 3 s instead of 2.5 minutes for -count=50 under load. They no longer wait for real deadlines.

Validation

  • Mutations of the production code, each made alone and reverted:
    1. A PATCH without the budget (WithCancel instead of WithTimeout): both timeout cases fail on "the PATCH deadline (set: true) is not within the PATCH budget 50ms".
    2. A re-read deadline at the request deadline: the re-read subtest fails.
    3. A re-read without its own deadline: the re-read subtest fails.
    4. No time-left check before the PATCH: "too little time left" fails.
  • make verify-vendor, make build, make lint, make test pass.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $89.72

Change-Id: Iaf2f1f696f5fc2a8be4a8f239c242638a48ce9f5
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-14T12:27:43.716958Z e86206b PR opened
🔒 Security Review ✅ Completed 2026-10-14T12:30:10.508981Z e86206b Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: e86206bf15

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2026
@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2026
@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 14, 2026
Merged via the queue into main with commit 785e14f Oct 14, 2026
10 checks passed
@ThomasK33
ThomasK33 deleted the test/promote-timing-deterministic branch October 14, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Promote timing tests in internal/aggregated/storage still flake under host load

1 participant