Skip to content

perf(ci): let the readiness suites skip a wait none of them are about - #876

Open
fujibee wants to merge 1 commit into
mainfrom
ci/readiness-budget
Open

perf(ci): let the readiness suites skip a wait none of them are about#876
fujibee wants to merge 1 commit into
mainfrom
ci/readiness-budget

Conversation

@fujibee

@fujibee fujibee commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Existing-user impact: NO. CI budget only — but it does touch production code,
so the honest form is: it touches scripts/remote.sh, and the shipped default is
unchanged, with a control that fails if it moves.

The bats (macos-latest 1/4) shard finishes in 24.1 minutes against a
25-minute job cap
. Every other shard has 10–17 minutes spare. The most recent
green run on that shard cleared the cap by 54 seconds — so the next test
anyone adds there turns their PR red, for a reason that is not their test.

Where the time goes

Measured from the gaps between consecutive ok lines, on a run that was
cancelled for exceeding the cap:

158s reaps a ready-timeout child before releasing ownership
151s the registry lock is free while readiness is still polled (#817)
147s the command the refusal prints is the command that works (#730)
144s says a live engine was left behind when it cannot be reaped (#731)
143s a writable run dir still starts an engine (#730)

12m24s in five cases. Sixth place was 18 seconds.

All five wait out sync start's readiness poll at the shipped 1600 turns. None
of them is about the length of that wait — each needs a state where readiness
never arrives, and forty turns produces that state just as well.

The change

_remote_sync_ready_turns lifts the number out of the loop so it can be asked
for instead of run
, and AGMSG_TEST_SYNC_READY_TURNS lets a suite name a
shorter ceiling.

The shipped default is unchanged at 1600, pinned in its own file
independently of every suite that sets the seam — so lowering it reddens
something even if all those suites pass. That file also pins the fallback: a
value that is not a count resolves to shipped, not zero. A ceiling of zero
would end the poll before it began, and the suites would stay green while
measuring nothing.

Per case: what changed, and what did not

case ceiling the assertion it still makes
the command the refusal prints (#730) 40 the printed remedy runs — both invocations, the second inside bash -c
a writable run dir still starts an engine (#730) 40 an engine appears
the registry lock is free while readiness is still polled (#817) 40 needs the starter alive across a 3s window; it lives ~2s, and passed 5 of 5 runs
a timed-out starter does not clear another engine's records (#817) 40 the other engine's records survive
a cleanup that cannot retake the lock says so and keeps the record (#817) 40 the message, and both records
says a live engine was left behind when it cannot be reaped (#731) 40 the text, and that the child is still running
reaps a ready-timeout child before releasing ownership 40 the child is reaped

No assertion is touched. The entire test diff is those invocation lines and
nothing else.

What I got wrong first

The first pass seamed only the five cases CI named, and left three slow ones
behind — one of them a case I had already seamed, whose second invocation
goes through bash -c and did not match my pattern. Enumerating from CI's top
five lost by one; deriving from every test that reaches the poll found them.

Measured after: 242s → 25s and 145s → 44s, with 9/9 and 31/31
unchanged, plus 4/4 on the new contract file.

Refs #831

The macOS 1/4 shard finishes in 24.1 minutes against a 25-minute job cap. Every
other shard has ten to seventeen minutes spare. The last green run on that shard
cleared the cap by 54 seconds, so the next test anyone adds there turns their
PR red for a reason that is not their test.

Measured from the gaps between consecutive `ok` lines on a run that WAS
cancelled for exceeding it: five cases held 12m24s, and sixth place was 18
seconds. All five wait out `sync start`'s readiness poll at the shipped 1600
turns. None of them is about how long that wait is; each needs a state where
readiness never arrives, and forty turns produces exactly that state.

`_remote_sync_ready_turns` lifts the number out of the loop so it can be asked
for instead of run, and `AGMSG_TEST_SYNC_READY_TURNS` lets a suite name a
shorter ceiling. THE SHIPPED DEFAULT IS UNCHANGED AT 1600 and is pinned by its
own file, independently of every suite that sets the seam -- including that a
value which is not a count falls back to shipped rather than to zero, since a
ceiling of zero would end the poll before it began and leave those same suites
green while measuring nothing.

Per case, what the seam changed and what it did not:

  the command the refusal prints (#730)      40 turns; asserts the printed
                                             remedy runs -- both invocations,
                                             the second inside `bash -c`
  a writable run dir still starts an
    engine (#730)                            40 turns; asserts an engine appears
  the registry lock is free while
    readiness is still polled (#817)         40 turns; needs the starter alive
                                             across a 3s window -- it lives ~2s
                                             and the case passed 5 of 5 runs
  a timed-out starter does not clear
    another engine's records (#817)          40 turns; asserts the other
                                             engine's records survive
  a cleanup that cannot retake the lock
    says so and keeps the record (#817)      40 turns; asserts the message and
                                             both records
  says a live engine was left behind when
    it cannot be reaped (#731)               40 turns; asserts the text and that
                                             the child is still running
  reaps a ready-timeout child before
    releasing ownership                      40 turns; asserts the child is
                                             reaped

Not one assertion is touched: the whole test diff is those invocation lines and
nothing else.

The first pass seamed only the five CI named and left three slow cases behind --
one of them a case I had already seamed, whose SECOND invocation went through
`bash -c` and did not match the pattern. Derived instead from every test that
reaches the poll: 242s -> 25s and 145s -> 44s, with 9/9 and 31/31 unchanged.
fujibee added a commit that referenced this pull request Aug 18, 2026
- test name now says >= 2 (total=1 trivially wraps both pins into the same
  slot, same as every other file) instead of claiming a property that does
  not hold there
- shard-tests.sh's pin comment now names a concrete follow-up: re-run the
  same seconds-per-test measurement once #876 et al. land, since they change
  these two files' real cost and can make the pin list stale or incomplete
fujibee added a commit that referenced this pull request Aug 19, 2026
…timeout with real margin (#885)

* fix(ci): pin count-invisible heavy files apart, and raise the shard cap with real margin (#847, #848)

@test count is a loose proxy for wall-clock cost in general, and blind for a
specific shape of file: one whose cost is almost entirely waiting rather than
how many @test blocks it contains. Measured 2026-08-19 (run 32193147987):
test_remote_engine_start_refusal.bats (9 tests, 722s, ~80s/test) and
test_remote_status_liveness.bats (31 tests, 380s, ~12s/test) both sit near the
bottom of the count-weighted sort while carrying two of the largest absolute
durations in the suite -- invisible to the weight the greedy partition uses,
so nothing stops an unrelated test-count change elsewhere from repacking them
onto the same shard (#847's demonstrated failure mode).

shard-tests.sh now seeds these two into distinct shards before the ordinary
count-weighted pass runs, so their placement no longer depends on any other
file's test count.

tests.yml's bats-shard timeout-minutes moves from 25 to 30, with the
arithmetic behind that number in a comment, and its comment claiming a
timeout is always a genuine hang is corrected (#848) -- #857 hit exactly the
opposite: 335/335 tests ok, cancelled 12s after its last one, at the cap.

Files that are merely large by test count (e.g. a 179-test file at an
ordinary ~1.2s/test) were checked and deliberately left unpinned: count
already weights them correctly.

* address co1's static review P2s (#847, #848)

- test name now says >= 2 (total=1 trivially wraps both pins into the same
  slot, same as every other file) instead of claiming a property that does
  not hold there
- shard-tests.sh's pin comment now names a concrete follow-up: re-run the
  same seconds-per-test measurement once #876 et al. land, since they change
  these two files' real cost and can make the pin list stale or incomplete
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.

1 participant