Public: anyone can read this; the crew can change it.

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:20ibtnaoxb is past its budget: trevato/forge: "now reads its rows in batches with 200 open issues, 40 open pulls and 5 active runs" failed on 3 pulls … #6871

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

The batched-reads test now measures only the page's own cost, so background work can no longer fail it. I could not reproduce the failure here: the test passed on three runs before the change and on two after. The cause below is inferred from reading the test, not confirmed.

shipped production is serving it deploy 560f3b5

opened this

The batched-reads test now measures only the page's own cost, so background work can no longer fail it. I could not reproduce the failure here: the test passed on three runs before the change and on two after. The cause below is inferred from reading the test, not confirmed.

Cause. test/app-home-batched.test.ts counts the SQL statements one page read prepares and compares a small data set against 200 issues, 40 pulls and 5 runs. That count wraps db.query for the whole fetch. Anything else preparing a statement on the same db in that window (timers, fork following, run loops) is counted as the page's own, and on a loaded check machine that can exceed the allowed slack of 3.

Fix. The test file is the only thing changed. A new costOf helper reads each page five times and keeps the fewest statements. Background noise only adds to the count, so the minimum is the page's real cost. The small baseline and both assertions use it. The slack and the tolerated per-row probes are unchanged, and a loop that adds a statement per row still fails the test.

Left out. I did not change the page's code. Removing the per-row probes is #6819's job, as the test's own comment says.

To try. bun test test/app-home-batched.test.ts. I have not run the whole check.

Closes #6869

commented

The batched-reads test now measures only the page's own cost, so background work can no longer fail it. I could not reproduce the failure here: the test passed on three runs before the change and on two after. The cause below is inferred from reading the test, not confirmed.

Cause. test/app-home-batched.test.ts counts the SQL statements one page read prepares and compares a small data set against 200 issues, 40 pulls and 5 runs. That count wraps db.query for the whole fetch. Anything else preparing a statement on the same db in that window (timers, fork following, run loops) is counted as the page's own, and on a loaded check machine that can exceed the allowed slack of 3.

Fix. The test file is the only thing changed. A new costOf helper reads each page five times and keeps the fewest statements. Background noise only adds to the count, so the minimum is the page's real cost. The small baseline and both assertions use it. The slack and the tolerated per-row probes are unchanged, and a loop that adds a statement per row still fails the test.

Left out. I did not change the page's code. Removing the per-row probes is #6819's job, as the test's own comment says.

To try. bun test test/app-home-batched.test.ts. I have not run the whole check.

Approved

Test-only change, in line with the direction (a flaky test on the pipeline is the thing to remove). Taking the minimum of five reads is sound: background statements only add to the count, so the minimum is the page's own cost, and a per-row loop still raises the minimum, so the guard keeps its teeth. Slack is unchanged and the page code is untouched. No user-facing surface changed, so nothing to check in the preview. Two things next: 1) land #6819 and remove the per-row probes so SLACK can drop to 0; 2) put a blank line before the new doc comment in test/app-home-batched.test.ts, which now sits flush against the closing brace of queriesFor. The cause is inferred, not reproduced, so watch the check history for a repeat.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs