Skip to content

test(curl): widen the monitor wait slack to survive a loaded CI runner - #1024

Merged
nahime0 merged 2 commits into
mainfrom
test/890-curl-monitor-wait-slack
Sep 18, 2026
Merged

nahime0 merged 2 commits into
mainfrom
test/890-curl-monitor-wait-slack

Conversation

@Guikingone

Copy link
Copy Markdown
Collaborator

Follow-up to #890 / #1002.

curl_monitor_excludes_write_callback_cpu_from_network_wait failed on four unrelated pull requests in one afternoon#1011 (an ir_lower release-ordering change), #1012 (array_fill argument boxing), #1017 and #1019 (test-only additions). None of them touches curl, the monitor, or timing. Each cleared on a re-run.

The measurement is still doing its job; the slack is what does not survive a shared runner. The control stayed at ~0.42 ms every time while the instrumented run came back at:

5 560 376 ns
7 802 876 ns
9 111 124 ns

against WAIT_SLACK_NS = 5_000_000.

That is loopback scheduling jitter, not billed callback CPU — the burn is ~130 ms, so billed CPU shows up as ~130 ms, which is exactly what the defect reported (133.9 ms against a 1.2 ms control).

20 ms covers the observed values with better than 2x to spare and still leaves ~6.6x below the 132.7 ms gap the defect produced. The test keeps failing outright on the regression it guards — two orders of magnitude is not a threshold anyone has to tune — while no longer failing on the machine it runs on.

Nothing else changes. BURN_ROUNDS, the total > 20_000_000 floor that rejects a fixture too short to measure anything, and the after_wait > 0 control that rejects a build recording no network wait at all are all untouched, so neither of the two ways this test could pass vacuously is widened.

Opened as its own PR rather than folded into one of the four it was blocking: it is your fixture, and the margin is a judgement call you may want to make differently.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

@github-actions github-actions Bot added area:triage No primary component could be inferred from changed paths. size:xs Very small pull request. type:test Changes tests or test infrastructure only. labels Sep 15, 2026
@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes the curl monitoring regression test resilient to loaded CI runners while preserving its ability to detect callback CPU incorrectly billed as network wait.

  • Raises the permitted loopback scheduling jitter from 5 ms to 20 ms.
  • Adds a fixture guard requiring CPU nested inside curl_exec() to exceed twice the tolerance.
  • Leaves the existing total-duration and nonzero-network-wait checks intact.

Confidence Score: 5/5

The PR appears safe to merge; the previously identified risk of masking the regression has been addressed by the new nested callback CPU guard.

No actionable new issue remains. The previous finding was manually resolved after the author added a guard ensuring the measured nested CPU duration remains substantially larger than the widened tolerance.

Important Files Changed
Filename Overview
tests/codegen/curl/monitoring.rs Widens the network-wait tolerance and adds a callback-duration assertion that prevents the regression test from becoming vacuous.

Reviews (3): Last reviewed commit: "test(curl): assert the burn outweighs th..." | Re-trigger Greptile

Comment thread tests/codegen/curl/monitoring.rs
@Guikingone Guikingone self-assigned this Sep 15, 2026
`curl_monitor_excludes_write_callback_cpu_from_network_wait` failed on four
unrelated pull requests in one afternoon -- #1011 (an `ir_lower` release-ordering
change), #1012 (`array_fill` argument boxing), #1017 and #1019 (test-only
additions). None of them touches curl, the monitor, or timing.

The measurement is still doing its job; the slack is what does not survive a
shared runner. The control stayed at ~0.42 ms every time while the instrumented
run came back at:

    5 560 376 ns
    7 802 876 ns
    9 111 124 ns

against `WAIT_SLACK_NS = 5_000_000`. That is loopback scheduling jitter, not
billed callback CPU: the burn is ~130 ms, so billed CPU would show as ~130 ms,
which is exactly what the defect reported (133.9 ms against a 1.2 ms control).

20 ms covers the observed values with better than 2x to spare and still leaves
~6.6x below the 132.7 ms gap the defect produced. The test keeps failing
outright on the regression it guards -- two orders of magnitude is not a
threshold anyone has to tune -- while no longer failing on the machine it runs
on.

Nothing else changes: `BURN_ROUNDS`, the `total > 20_000_000` floor that rejects
a fixture too short to measure, and the `after_wait > 0` control that rejects a
build recording no wait at all are all untouched.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Review follow-up on the widened tolerance. Widening a bound can only move a test
toward vacuous, and the existing floor could not rule that out: `total >
20_000_000` measures the whole PROCESS, so a fixture whose nested burn shrank
below `WAIT_SLACK_NS` -- a faster target, or an edit to `BURN_ROUNDS` -- would
still clear it while a COMPLETELY misbilled callback moved reported wait by less
than the tolerance. The regression assertion would hold for exactly the build it
exists to catch.

The burn is now measured where it happens. `curl_exec`'s inclusive minus
exclusive time IS the nested callback -- on the in-callback shape the write
callback is the only frame called from the transfer -- and it must exceed the
slack by 2x. Read off the capture rather than found by symbol name, so a change
to closure mangling cannot quietly turn it into a check of nothing.

Measured: 123 ms in-callback locally, about 6x the slack. Verified against the
hole rather than asserted: at `BURN_ROUNDS = 6_000_000` the whole-run floor
passes and the new one fails with

    the burn nested inside `curl_exec()` measured 27844947 ns, which is not far
    enough above the 20000000 ns slack for the comparison below to mean anything

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@Guikingone

Copy link
Copy Markdown
Collaborator Author

Two more data points for this, both from #1066 — a PR that touches __rt_str_to_int and nothing on the curl monitoring path:

run 1:  control 372625 ns → in-callback 7815417 ns
run 2:  control 432417 ns → in-callback 7418999 ns   (a re-run of run 1's failed job)

The 7.80 ms in this PR's comment is run 1. What is new is run 2: the job was re-run and reproduced. So on the current macos-aarch64 runner this is not an occasional flake that a re-run clears — it is reliable, and it is blocking unrelated PRs from ever going green. The control stayed sub-millisecond both times, and both samples sit ~18x below the 133.9 ms the defect produced.

That makes this PR the unblock rather than a tidy-up.

@nahime0

nahime0 commented Sep 18, 2026

Copy link
Copy Markdown
Member

20 ms is fine

@nahime0
nahime0 merged commit c828579 into main Sep 18, 2026
149 checks passed
@nahime0
nahime0 deleted the test/890-curl-monitor-wait-slack branch September 18, 2026 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:triage No primary component could be inferred from changed paths. size:xs Very small pull request. type:test Changes tests or test infrastructure only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants