fix(PG-940): the retro table reads 25 PRs a page and retries a failed one by nrjdalal · Pull Request #959 · nrjdalal/playground

nrjdalal · 2026-11-05T06:32:52Z

TL;DR

The retro's table of pull requests no longer loses its rows when GitHub answers one page badly: it asks for smaller pages and asks a failed page again, up to three tries, before it gives up.
When a page still fails, the error names where it stopped, and a retro can now read a wide window in smaller time chunks.

Summary

 packages/scripts/src/retro/table.ts
-  search(first: 100)                      # 100 PRs, each with up to 100 full comment bodies
+  search(first: 25)
-  searchOf(repo, since)                   # updated:>=since
+  searchOf(repo, since, until?)           # updated:since..until with --until
+  pageOf(ask, after, wait = Bun.sleep)
+    try up to TRIES (3), RETRY_MS (5 s) apart
+      body not empty -> parse it, whatever gh's exit code (gh exits 1 on errors beside data)
+        does not parse (cut off)            -> try again
+        data.search present                 -> print any errors, keep the data
+      no data.search (a 502, data: null)    -> try again
+    throw "the page after cursor <c> failed 3 times: <why>"
 main
-  JSON.parse(stdout); exit 1 on the first bad page
+  pageOf(...).catch -> print the message, exit 1
+  readAll and rowsOf skip a PR or a comment GraphQL nulled beside its error
+  --since or --until with no time after it -> usage, exit 2
 scripts/briefs/optimizer.md               # step 4: on an exit 1 naming a cursor, read the window again in --until chunks

Evidence

Tests, red-first: tests/packages/scripts/src/retro/table.test.ts with a fake gh on PATH, one numbered answer per call:
- a 502 once, then a page: exit 0, the PR's row prints, nothing on stderr;
- first page, then a cut-off body, a 502 and a 502: exit 1, stderr names cursor Y3Vyc29yOjI1 and "3 times", and the third call asked after=Y3Vyc29yOjI1;
- QUERY asks first: 25; searchOf with an until reads updated:A..B.
- a page gh exits 1 on, with data beside its errors (gh's real behaviour, measured in review): exit 0 and its row from one read, the errors on stderr; and pageOf on that answer returns the data and prints the errors;
- a page whose errors come with data: null (a rate limit): three reads, then exit 1 naming the first page, with nothing on stdout;
- a comment and a PR that GraphQL nulled beside their errors (GraphQL nulls the item that failed, measured in review): exit 0, the other PR's row, the errors on stderr, and no "read 1 of 2 PRs" line, since a nulled PR counts as read;
- --until with no time after it: exit 2 with the usage line, and no gh call;
- each case that waits out the retries has the timeout (TRIES - 1) * RETRY_MS + 20_000.
- Before (canary's table.ts swapped in): 3 fail: the 502-once test, the three-failures test, and the QUERY test on first: 25.
- After (head): 11 pass, 0 fail at 3006e0a.
Canary merge (836e1f7): clean, 11 commits; none touches the retro table or what scripts/briefs/optimizer.md's step 4 describes (grep of the incoming diff for the retro, ops-tick, the roster, session-state and --log-failed).
Checks at 3006e0a: the packages rebuilt (9 of 9 turbo tasks), then the touched tests (117 files) with SKIP_ENV_VALIDATION=true: 7466 pass, 2 fail, both timing-bound under load and outside this diff (tests/api/hono/src/lib/onboard/fix.test.ts's docker-never-starts case read running at its deadline; tests/packages/scripts/src/verify/prebuild.test.ts's main-checkout case hit its 30 s timeout), each green in the run at 8a605a9 (116 files, 7454 pass, 0 fail); run again on their own at 3006e0a, the two files read 50 pass, 0 fail. That flag is needed because a fresh worktree's .env from .env.example leaves POSTGRES_URL empty and so turns off the sandbox's own skip (packages/scripts/src/test-sandbox.ts:35); without it, 8 api files that import none of this PR's code throw on import. Also bun run check-types:tests exit 0 (it failed once on a packages/*/dist older than the canary merge, green after the rebuild); bun run test:skills all 13 passed; docs:wrap --check exit 0; scripts/check-em-dashes.sh no em-dashes; precommit:light all passed; the pre-commit hooks green.
Round 1 (4/5 at 836e1f7), applied in fb00c03:
- should-fix 1: pageOf reads gh's body whatever its exit code, since gh exits 1 on an answer whose errors sit beside its data; the errors test answers with exit 1 and gh's stderr line.
- should-fix 2: the reviewer's optimizer.md patch.
- Nits: a bare --until takes the usage exit; the waiting cases' timeouts follow TRIES and RETRY_MS; fakeGh makes its gh runnable and returns its folder alone; test:skills below.
- A chunk's --until and the next chunk's --since share a second, and A..B is inclusive at both ends, so a PR updated in that exact second lists in both chunks' tables; the retro reads a row twice at worst, so the brief keeps the plain hand-off.
- Canary merged at 8a605a9, clean (feat(PG-943): run-slot --after <pid> holds a run until that run of its session lands #947 and feat(PG-883): the owner reads this Mac's dashboard from the hosted Yard, through a relay #934); neither touches the retro table or step 4 (grep of the incoming diff for the retro, --since and --until).
Round 2 (4/5 at 8a605a9), applied in c279c1a:
- should-fix: Page.nodes and Pr.comments.nodes allow null, so the type check makes each reader handle it; readAll skips a null PR and rowsOf a null comment.
- Nits: pageOf parses any non-empty body, so an HTML body keeps "a body that does not parse"; Answer.exitCode, which nothing read, is gone; the reviewer's wording patch, so the header comment and step 4 copy neither TRIES nor the page size.
- The short-search line counted only the PRs kept, so a nulled PR read as "read 1 of 2 PRs: give a later --since"; it now counts every node a page returned (3006e0a).
- Canary merged at af575b7, clean (feat(PG-939): a nightly clean takes what no merge's cleanup reaches, and asks before work goes #952, scripts/ops/optimizer/auto_end.ts), with no line about the retro table or step 4.
CI at 3006e0a: the first run's Suite (5) failed at tests/scripts/ops/run-slot-gauges-lock.test.ts:124, outside this diff: PG-956, where the CI runner's mkdir (uutils 0.10.0) is not atomic, so two looks can both take the lock. The failed jobs were run again (gh run rerun 42008110585 --failed).
Docs and skills:
- scripts/briefs/optimizer.md: step 4 now says that when the table exits 1 naming a page's cursor (a page that failed all its tries), the Optimizer reads the same window again in chunks, each run adding --until <iso> and the next starting its --since there, until every chunk exits 0; the reviewer's checked patch, word for word, so the page size and retry counts live in the code alone; writing-for-agents read.
- git grep -n "retro/table": scripts/briefs/optimizer.md:10 (updated) and tests/scripts/briefs/optimizer.test.ts:17, which checks the path alone; no other doc describes the fetch.
- Changed-term grep for first: 100 over the docs and skills: one hit, extensions/core/skills/playbook/GITHUB-BULK-READS.md:76, a PR's files page in another query, so not this fetch. The docs that name the retro with a table (GLOSSARY.md, docs/playground/playbook.md, extensions/core/skills/playbook/LIFECYCLE.md, web/next/content/docs/cli.mdx) say nothing about its pages or retries.
- No enumerated kind gains a member, so no list grep applies.

Merge Danger

Door: two-way

A revert brings back the 100-a-page read; no data or state changes.

Blast Radius: retros

Only the Optimizer's retro runs this script; a page that still fails three times exits 1 as before, about 10 s later.

Links

Closes #940

🤖 Generated with Claude Code

… one

…ide its data

nrjdalal · 2026-11-05T07:16:43Z

ReviewBot · round 1: 4/5 at `836e1f7` · 0 blocking, 2 should-fix, 4 nits

The retry, the 25-PR page, the cursor in the error and --until all do what the issue asked, and the fake-gh tests go red on canary. One path the PR claims never runs with the real gh, and the brief's new sentence leaves its trigger open.

Spec (the right thing)

S1 first: 25: delivered (packages/scripts/src/retro/table.ts:7, pinned by the QUERY test).
S2 retry up to 3 tries, 5 s apart, on a non-zero exit, a 5xx or an unparseable body: delivered (pageOf, table.ts:51-74). A 5xx reaches it as gh's non-zero exit.
S3 the final error names the cursor: delivered (table.ts:71-73).
S4 optional --until: delivered (searchOf, table.ts:42-43, and the flag at :132). GitHub's search takes the range with Z times: updated:2026-11-04T00:00:00Z..2026-11-04T23:59:59Z read 384 PRs, the same day with +00:00 offsets 384, and its evening 378 (measured, REST search).
D1 a 502 once, then a page, yields the rows: delivered (tests/packages/scripts/src/retro/table.test.ts:138-147).
D2 three failures exit 1 naming the cursor: delivered (table.test.ts:149-164). It also checks that the last retry asked after=<cursor>.
D3 red on canary: delivered. With canary's table.ts swapped in, the head's file fails at import (Export named 'pageOf' not found). A scratch copy without that import has the first: 25, 502-once and three-failures cases red (0 pass, measured).
D4 docs match by git grep -n "retro/table": delivered. Its only doc hit is scripts/briefs/optimizer.md:10, which is updated; tests/scripts/briefs/optimizer.test.ts:17 checks the path alone (measured).
B1 the fake gh carries the tests: delivered.
B2 writing-for-agents on the doc edit: partial, see the second should-fix.

No spec finding.

Standards (built well)

[should-fix] packages/scripts/src/retro/table.ts:62: the PR body's "data beside errors: print the errors, keep the data", and commit 46adcf0's test for it, describe a path the real gh never takes. gh api graphql exits 1 whenever its answer carries errors, even with data beside them: a query for viewer plus a missing repository printed {"data":{"viewer":...},"errors":[{"type":"NOT_FOUND",...}]} on stdout, the error on stderr, and exit 1 (measured). So gh.exitCode === 0 ? JSON.parse(...) : null drops such a page. A scratch probe giving the head's script that answer three times got all 3 tries (10.6 s), then exit 1 with the rows lost (measured). The test at table.test.ts:171 fakes exitCode: 0, which gh never gives, so it can't fail for the reason it exists (VERIFY.md, "Ask whether each test can fail"). Fix: read gh's stdout whatever the exit code whenever it holds a JSON body. When it parses and holds data.search, print the errors and return the data. Otherwise try again. Have the test answer with exit 1 and gh's stderr line, as gh does. Tried in the clone: answer = gh.exitCode === 0 || gh.stdout.trimStart().startsWith("{") ? JSON.parse(gh.stdout) : null. The same probe then exits 0 with the row and the errors on stderr, and the head's own table.test.ts stays green (7 pass, measured). This adds behaviour, so it is yours to write.
[should-fix] scripts/briefs/optimizer.md:10: "It reads 25 PRs a page and asks a failed page again twice, 5 s apart" restates QUERY, TRIES and RETRY_MS, a cache that goes stale when they move. "so read a wide window in chunks" names no trigger the Optimizer can check, since nothing says when a window counts as wide. writing-for-agents: one meaning in one place, and a step the agent runs without guessing. Fix: the checked patch below keys the chunked read to the exit the Optimizer can see. If you meant chunking up front for every long window instead, say how long, in your own words. (reasoned)
[nit] packages/scripts/src/retro/table.ts:132: --since X --until with no value runs updated:>=X and exits 0, so the missing value goes unnoticed (measured with the fake gh). A missing value could take the usage exit 2, as a bad date does.
[nit] tests/packages/scripts/src/retro/table.test.ts:147 and :164: both cases wait out the real RETRY_MS (once, then twice) under a literal 30_000. .agents/testing.md:61 ties a timer case's timeout to the constant it guards, for example (TRIES - 1) * RETRY_MS + 20_000. (reasoned)
[nit] tests/packages/scripts/src/retro/table.test.ts:120: fakeGh returns gh, which neither caller reads, and the chmodSync that makes it runnable sits in runTable (:124), so setting up the fake gh is split across two helpers. Possible Speculative Generality, a judgement call. (reasoned)
[nit] PR body: Evidence gives no test:skills result. GitHub's test is green at this head.

Also reasoned and left to you: A..B is inclusive at both ends, so a PR updated in the exact second where one chunk ends and the next starts lists in both chunks' tables.

Checked patch

Path on this machine (sent to the worker): /private/tmp/claude-501/-Users-nrjdalal-playground/0c5511ee-5a82-4661-b96c-fc943ed3ec1d/scratchpad/r959probe/fix.patch. It touches one file, scripts/briefs/optimizer.md:10, and swaps the added sentence for:

When it exits 1 naming a page's cursor, a page that failed all 3 of its tries, read the same window again in chunks, each run adding --until <iso> and the next starting its --since at that time, until every chunk exits 0.

Its checks, applied in the review clone:

bun run test:file tests/scripts/briefs/optimizer.test.ts tests/packages/scripts/src/trace-count/count.test.ts: 44 pass, 0 fail.
scripts/check-em-dashes.sh on the file: no em-dashes in prose.
bun run docs:wrap --check: exit 0.
After restoring the clone, git apply --check: exit 0.

Threads

None before this round. This round opens six inline threads for the five findings that sit on a diff line. The first should-fix takes two, one on table.ts:62 and one on its test at table.test.ts:171.

Checked and Not verified

Read in full: the whole diff (3 files), issue The retro table survives a bad GitHub page: smaller pages and a retry #940, the worker's brief, the PR body, .agents/testing.md, tests/scripts/briefs/optimizer.test.ts.
Ran:
- check-types in the clone: green.
- the red-first swap and the scratch fake-gh probes at canary and at the head: the real gh answer shape, a missing --until value, and the suggested fix above.
- the GitHub search range reads.
A fresh two-axis read of the whole diff found no spec gap, and its Standards notes are in the findings above.
GitHub's Audit, Lint, and Build and test are green at 836e1f7. The CI run merged the head into canary dde9780, which is still origin/canary.
Not verified: how often a real retro page comes back with data beside errors. The finding rests on gh's exit code for such an answer, not on a count.

ReviewBot · round 1. Push back on any finding you think is wrong.

nrjdalal

ReviewBot round 1 inline findings at 836e1f7; the verdict is the round's comment.

…a bare --until

nrjdalal · 2026-11-05T08:25:04Z

ReviewBot · round 2: 4/5 at `8a605a9` · 0 blocking, 1 should-fix, 3 nits

Every round 1 finding is fixed at fb00c03, and the optimizer.md patch went in word for word (the same blob, cee3aca5). The canary merge at 8a605a9 is clean. One problem is left, and round 1's fix direction opened the path to it: a page whose errors come with data keeps that data, but GraphQL nulls the item that failed, and the table code cannot handle a null item.

Spec (the right thing)

S1 first: 25: delivered.
S2 up to 3 tries, 5 s apart, on a non-zero exit, a 5xx or an unparseable body: delivered. A page that returns data beside errors is kept on its first read, as round 1 asked, since gh exits 1 on that answer.
S3 the final error names the cursor: delivered.
S4 --until: delivered. A bare --since or --until now takes the usage exit 2.
D1 to D4, B1 and B2: delivered. Swapping round 1's table.ts into the head's test makes the 3 new cases fail (7 pass, 3 fail, measured): data beside errors read through pageOf, the same answer through the fake gh, and the bare --until.

No spec finding. A fresh two-axis read of the whole diff found no spec gap.

Standards (built well)

[should-fix] packages/scripts/src/retro/table.ts:69 (it fails at :100, in rowsOf): when a page carries errors, its data is kept, but GraphQL replaces the item that failed with null. GitHub's schema (measured by introspection) has IssueComment.body non-null in a nullable nodes list, and PullRequest.comments, commits and timelineItems non-null in the search's nullable nodes list. So an error resolving one comment, the error the tests name, nulls that comment, and an error in one PR's connections nulls that PR. With a fake gh giving exit 1, a comments.nodes of [null, {...}] and an errors path into that comment, the head printed the errors, then TypeError: null is not an object (evaluating 'c.author') at table.ts:100, and exited 1 with no rows and no cursor (measured). A null PR node throws at a.number in the sort (measured on rowsOf). The tests' answers pair a comment error with data that holds no null, which is a shape GraphQL does not give, so they cannot catch this. Fix: allow null in Page.nodes and in Pr.comments.nodes, so tsc makes every reader handle it. Skip a null PR in readAll and a null comment in rowsOf. Add a fake-gh case where a nulled comment beside its error gives exit 0, the row and the error on stderr. I tried those four edits in the clone: the probe then exits 0 with row 12 and the errors on stderr. With them, table.test.ts gave 10 pass and check-types was green (measured). They add behaviour, so you write them. (measured)
[nit] packages/scripts/src/retro/table.ts:61: an answer that does not start with { now ends in failed 3 times: null when gh's stderr is empty (measured with pageOf on an HTML body at exit 0). At round 1's head, the same answer ended in "a body that does not parse: ...". It is still retried and still names its cursor. A guard on a non-empty stdout (gh.stdout.trim()) would keep the parse reason.
[nit] packages/scripts/src/retro/table.ts:48: pageOf no longer reads Answer.exitCode, so the field, the exitCode that ask returns at :159 and the one in the test at :187 serve nothing. Possible Speculative Generality, a judgement call.
[nit] scripts/briefs/optimizer.md:10 and packages/scripts/src/retro/table.ts:1: my round 1 patch wrote "all 3 of its tries", and the header comment says "25 PRs a page, each page asked up to 3 times". Both copy TRIES and QUERY's page size, which goes stale when those change (writing-for-agents: one meaning in one place). The checked patch below rewords both. Take it or leave it.

Reasoned and left as you chose: the inclusive boundary second between chunks, which your body states.

Checked patch (the last nit)

Path on this machine (sent to the worker): /private/tmp/claude-501/-Users-nrjdalal-playground/0c5511ee-5a82-4661-b96c-fc943ed3ec1d/scratchpad/r959probe/fix-r2.patch. Two edits:

optimizer.md:10: "a page that failed all 3 of its tries," becomes "a page that failed all its tries,".
table.ts:1: "read through GraphQL's search 25 PRs a page, each page asked up to 3 times," becomes "read through GraphQL's search a page at a time, each page asked again when it fails,".

Checks with the patch applied in the clone:

bun run test:file tests/scripts/briefs/optimizer.test.ts tests/packages/scripts/src/trace-count/count.test.ts tests/packages/scripts/src/retro/table.test.ts: 54 pass, 0 fail.
scripts/check-em-dashes.sh on both files: no em-dashes in prose.
bun run docs:wrap --check: exit 0.
After restoring the clone, git apply --check: exit 0.

Threads

All six round 1 threads are fixed at fb00c03. Each gets a reply and is resolved this round. This round opens 3 inline threads, for the should-fix and the first two nits. The patch nit is in this comment alone.

Checked and Not verified

Read in full: fb00c03's diff, the merge 8a605a9 (--remerge-diff is empty), the PR body, canary's table.ts, and what canary brought since dde9780 (40 files, none of the PR's).
Ran:
- the red-first swap above;
- the null-node probe through a fake gh at the head, and the tried fix;
- the HTML-body probe;
- GitHub's schema introspection for the nullability of those fields;
- check-types with the tried fix (green).
A fresh two-axis read (Standards and Spec sub-agents) of the whole diff. Its notes are in the findings above or set aside: the long one-line comments follow the repo's style, and mkdtemp folders are what .agents/testing.md:18 asks for.
GitHub's Audit, Lint, and Build and test are green at 8a605a9. The run merged the head into canary b83bde7, which is origin/canary now.
Not verified: how often GitHub returns a nulled item on a real retro page. The finding rests on the schema and on GraphQL's null rule, not on a count.

ReviewBot · round 2. Push back on any finding you think is wrong.

nrjdalal

ReviewBot round 2 inline findings at 8a605a9; the verdict is the round's comment.

…ide its error

…search line

nrjdalal · 2026-11-05T10:20:49Z

ReviewBot · round 3: 5/5 at `3006e0a` · 0 blocking, 0 should-fix, 1 nits

Round 2's should-fix and nits are fixed at c279c1a and 3006e0a, and the wording patch went in as written (the same blob, b86f07bf). The canary merge af575b7 is clean. A fresh two-axis read of the whole diff found no blocking or should-fix finding.

Spec (the right thing)

S1 first: 25: delivered.
S2 up to 3 tries, 5 s apart, on a non-zero exit, a 5xx or an unparseable body: delivered. Any non-empty body is parsed, so an HTML body keeps "a body that does not parse". A page with data beside errors is kept on its first read, and its nulled items are skipped.
S3 the final error names the cursor: delivered.
S4 --until: delivered.
D1 to D4, B1 and B2: delivered. Swapping 8a605a9's table.ts into the head's test makes the new nulled-comment-and-PR case fail (10 pass, 1 fail, measured). At the head: 11 pass.

No spec finding.

Standards (built well)

[nit] packages/scripts/src/retro/table.ts:86: a PR that GraphQL nulled leaves no row, and seen counts it as read, so no short-search line flags it. Only the raw errors JSON on stderr names it, by its index on the page, not by its number. A line under the table such as "1 PR on a page came back null beside its error" would tell the Optimizer that a row is missing. Take it or leave it. (reasoned)

Set aside from the fresh read, each with its reason:

The file's runtime: 31 s on the Mac (measured), under .agents/testing.md's 60 s for a subprocess file.
The brief's chunk width: its loop ends on a check the Optimizer can run, "until every chunk exits 0", and a chunk that fails is split again.
The found/seen/why names and the two data-beside-errors cases at two layers: judgement calls that I would not change.

Threads

All three round 2 threads are fixed at c279c1a. Each gets a reply and is resolved this round. None stays open, and this round opens none: the nit stays in this comment.

Checked and Not verified

Read in full: c279c1a and 3006e0a's diffs, the merge af575b7 (--remerge-diff is empty), and the list of files canary changed since 1310a29 (none of the PR's).
Ran:
- the red-first swap above;
- the head's table.test.ts (11 pass);
- tests/scripts/ops/run-slot-gauges-lock.test.ts at the head on the Mac (10 pass), after Suite (5) went red on it in CI's first attempt.
The fresh full read: code-review's Standards and Spec sub-agents on the whole diff from 1310a29. Neither found a blocking or should-fix item.
check-types: PR CI green at 3006e0a on canary 16bbb41.
touched tests: PR CI green at 3006e0a on canary 16bbb41: https://github.com/nrjdalal/playground/actions/runs/42008110585. Its one red shard on the first attempt passed on rerun, against the same canary. Canary has since moved to 92dc77f, which changes none of the PR's files, and merge-tree with it is clean.
Not verified: how often GitHub nulls a PR on a real retro page.

The fresh full read found nothing blocking or should-fix, and its one nit is listed above for you to take or leave.

ReviewBot · round 3. Push back on any finding you think is wrong.

nrjdalal · 2026-11-05T10:25:44Z

Merged by bun run pr:merge at 3006e0a.

open: open
author: by nrjdalal
held: not in held-prs.json
ready: ready for review
base: into canary
up to date: canary has 6 commits 3006e0a lacks; it merges clean with canary 92dc77f
keeps canary: no merge of canary into the PR dropped a line git's own merge keeps
ci: 18 checks passed
body: the PR template
review: ## ReviewBot · round 3: 5/5 at 3006e0a · 0 blocking, 0 should-fix, 1 nits
types: the merged tree 35967fe (fix(PG-940): the retro table reads 25 PRs a page and retries a failed one #959 at 3006e0a onto canary 92dc77f) type-checks: bundles 0s, check-types 2s, build 0s, metrics 0s, check-types:tests 14s

…buntu-26.04 (#1076) ## TL;DR - 🧪 The run-slot lock tests stop failing now and then on GitHub's Linux runner. One such failure held up a ship. - 🔍 The cause was the runner's own `mkdir` command, which can tell two programs that each made the same folder. The tests now use the runner's other `mkdir`, which tells only one, as the Mac's does. ## Summary ```diff tests/scripts/ops/run-slot-fake.ts, machine() slowChild("python3", bin) + if (GNU_MKDIR) // Bun.which("gnumkdir"): ubuntu-26.04 only + bin/mkdir = exec gnumkdir "$@" ``` - GitHub's `ubuntu-latest` is ubuntu-26.04, whose `/usr/bin/mkdir` is uutils coreutils 0.10.0 (`rust-coreutils` 0.10.0-1ubuntu2~26.04.1). Its `create_dir` checks `path.exists()`, then maps a failed `mkdir(2)` to success with `Err(_) if path.is_dir() => Ok(())`, even without `-p`. So when two processes race, both exit 0. - run-slot.sh takes every lock with a plain `mkdir`: the gauges refresh lock, the run slots and the model slots. On that runner, two looks can each hold the gauges lock and both run `top`: "eight looks with no reading yet run top once" got 6. - The image keeps GNU's mkdir as `gnumkdir`, and each fake machine's bin now runs that one wherever it exists. On the Mac and on GNU images nothing changes. - `.agents/testing.md`, Traps: one new trap, the racing `mkdir` and how a test avoids it. **Decisions:** - **The fix is in the test, not in run-slot.sh.** run-slot.sh runs for real only on the Mac, whose BSD `mkdir` is atomic: 60 rounds of the eight-look case there each ran one `top`. Only CI runs the script on Linux. - **The dead-lock takeover case stays out of this PR.** It is a separate race, not yet reproduced alone (Evidence, #959's row), now #1077, so this PR keeps to the two "run top once" cases #956 names. - **The reported lead is refuted.** The lead was that a reading stamped with the look's start time goes stale after a refresh slower than 10 s. The red run took 1.6 s, and six one-second tops in 1.6 s means they ran side by side. A stale stamp cannot do that, while two lock holders can. - **Nothing is posted upstream.** uutils fixed this in 0.11.0 (uutils/coreutils#13970, uutils/coreutils#13986), and Ubuntu's request to bring that fix to 26.04 (LP #2171333) already has a patch waiting for a sponsor. A comment would add no new facts. Once 26.04 takes that patch, the shim is harmless and can go. ## Evidence - **Cause, in an `ubuntu:26.04` container:** 8 racing `mkdir L`, 200 rounds each: | mkdir | rounds with more than one winner | | --- | --- | | `/usr/bin/mkdir` (uutils 0.10.0) | 33 of 200, and 38 of 200 on a second run | | `gnumkdir` (GNU 9.7) | 0 of 200 | - **No repro without uutils:** the eight-look case ran exactly one `top` in 60 of 60 rounds on the Mac (BSD mkdir) and 80 of 80 on `oven/bun:debian` (GNU mkdir, bash 5.2). - **Before / after in `ubuntu:26.04`** (`--cpus 2`, Linux bun 1.4.4): | run | canary's fake | this head's fake | | --- | --- | --- | | probe: 60 rounds of 8 looks with no reading, rounds that ran `top` more than once | 10 of 60 (2 or 3 tops) | 0 of 60 | | `run-slot-gauges-lock.test.ts`, 20 runs, the two "run top once" cases | failed in 6 of 20: the stale-reading case (line 46) in runs 1, 14 and 15, the no-reading case (line 52) in runs 5, 7 and 13 | failed in 0 of 20 | - **At this head on the Mac:** 20 runs in a row of `bun run test:file tests/scripts/ops/run-slot-gauges-lock.test.ts`: 20 of 20 passed, each 10 pass, 0 fail. `bun run check-types:tests`: no errors. `bun run test:skills`: all 13 passed. - **Sightings on CI:** - canary at d216f23, Suite (8): "eight looks with no reading yet run top once", expected 1, received 6, in 1631 ms. - #1063 at 1af2e05, Suite (7) (job 127580442051), ubuntu-26.04: "eight looks at a stale reading run top once", expected 1, received 2, in 1526 ms. The same case fails 3 of 20 on canary's fake above and 0 of 20 at this head. - #959 at 3006e0a, attempt 1, Suite (5) (job 127180040487): line 124 of the same file, "eight looks at a dead lock together each read the stale or the new reading", expected true, received false, in 1627 ms. **Not this race, and not fixed here.** That case still failed 1 of 20 runs at this head in `ubuntu:26.04` (run 12, line 125), with GNU's `mkdir` in the fake. A probe of the case alone found 0 bad rounds in 60 with GNU's `mkdir` and 0 in 60 with all 104 `gnu*` tools on PATH, so no tool swap is shown to fix it. It is #1077. - **doc-sync:** the change touches the test fake alone. `git grep -n -iE "mkdir"` over `.agents`, `scripts/README.md`, `scripts/ops/README.md`, `scripts/hooks/README.md`, `docs` and the core skills finds no line that calls `mkdir` atomic. The new trap in `.agents/testing.md` is the one place that says it is not on ubuntu-26.04. ## Merge Danger **Door:** two-way A revert brings back the uutils `mkdir` in the fake, and the flake with it. **Blast Radius:** small Tests only: every `run-slot*.test.ts` machine on a runner that has `gnumkdir`. run-slot.sh and every other script are unchanged. ## Links - Closes #956 - uutils/coreutils#13970 (the report) and uutils/coreutils#13986 (the fix, in 0.11.0) - #1077: the dead-lock takeover race, left out of this PR - Ubuntu: https://bugs.launchpad.net/ubuntu/+source/rust-coreutils/+bug/2171333 🤖 Generated with [Claude Code](https://claude.com/claude-code)

…ding (#1089) ## TL;DR - 🎛️ When run-slot counts how many runs the machine can take, it now uses one CPU reading for the whole count. Before, a new reading that arrived in the middle could give a count that matched neither the old reading nor the new one. - 🧪 This fixes a test that failed now and then on GitHub's Linux runner. When that test fails now, it shows what each check printed. ## Summary ```diff capacity() # slots, room and every take read it + local LANES=unread for s in run-slot run-slot-2 run-slot-3 run-slot-4 slot_open "$s" - ! lanes_open # slots 2, 3 and 4: each read the gauges again + ! lanes_now # the first check reads them, the rest share its verdict +lanes_now() + open|shut: the count's verdict; unread: lanes_open once and keep it; unset: lanes_open, as before ``` - `run-slot.sh slots` prints `capacity() / 2`. Each of slots 2 to 4 called `lanes_open`, and each `lanes_open` read `gauges()` again. In the dead-lock test the stale reading says 30% idle (shut) and the new one 80% (open). A look whose checks straddled the new reading counted slot 2 shut and slots 3 and 4 open, and printed `3`, or `2` when it landed between slots 3 and 4. - The fix is a `local` in `capacity()`, which bash's dynamic scope hands to `slot_open`'s checks. Outside a count, `LANES` is unset and every check runs `lanes_open` as before, so a waiter still reads the gauges on every poll. - `scripts/ops/README.md`, beside the takeover line: one line on the single verdict. **Decisions:** - **The fix is in run-slot.sh, not the test.** A real look on the Mac can count a mixed reading too: the bug isn't specific to Linux, only easier to hit on a 2-CPU runner. - **The take loop is unchanged.** It calls `slot_open` slot by slot to take the first open one, so a reading that lands in the middle only changes which open slot it takes. Its counts come from `capacity()`, which now reads once. - **The dead-lock test now fails on the list of bad looks, not on a boolean.** `expect(looks.filter(neither 1 nor 4)).toEqual([])` prints each bad look's slot line and stderr, so a CI failure says what a look printed. It stays whatever the cause turns out to be. ## Evidence - **The new test**, "a look counts slots 2 to 4 on one reading, even when a new one lands between their checks". It sets a stale 30% reading, a lock a live refresher holds, and a fake `stat`. `stat` runs on the lock only in `gauges_lock`, once its mkdir finds the lock held, so the fake writes a fresh 80% reading after the look's first lanes check has read the stale one, as a refresher's rename would. | | canary's `run-slot.sh` | this head | | --- | --- | --- | | Mac | 10 pass, 1 fail: expected `["1"]`, received `["3"]` | passes | | `ubuntu:26.04` container | 10 pass, 1 fail: received `["3"]` | passes | - **20 runs in a row** of `bun run test:file tests/scripts/ops/run-slot-gauges-lock.test.ts` on the Mac: 20 of 20, each 11 pass, 0 fail. Of the same file in `ubuntu:26.04` (`--cpus 2 --memory=1g`, Linux bun 1.4.4): 20 of 20, each 11 pass, 0 fail. - **Every run-slot test** (`tests/scripts/ops/run-slot*.test.ts`) on the Mac: 424 pass, 0 fail. `bun run check-types:tests`: no errors. `bun run test:skills`: all 13 passed. - **The first look, which caught nothing:** - the dead-lock case alone in `ubuntu:26.04` went 0 bad rounds in 60 with GNU's mkdir, and 0 in 60 with all 104 `gnu*` tools on PATH; - 60 runs of the whole file with a `bash -x` trace per look went 60 of 60 green. So it isn't a uutils tool, and a run can't be relied on to catch it. The deterministic test above is the evidence. - **Sightings on CI:** #959 at 3006e0a, Suite (5), job 127180040487, ubuntu-26.04: the dead-lock case's `slots.every(1 or 4)` received false. In `ubuntu:26.04` at #1076's head it failed 1 of 20 runs. - **doc-sync:** - `git grep -n -E "lanes_open|capacity|run-slot.sh slots|one reading"` over `.agents`, `docs`, the READMEs, `AGENTS.md` and the skills finds `scripts/ops/README.md:22, 26, 41`; - lines 22 and 41 name `lanes_open` for what it opens, which is unchanged; - the new line sits after line 28's takeover line. ## Merge Danger **Door:** two-way A revert brings back the per-slot readings and the flake. **Blast Radius:** small `run-slot.sh`'s slot count only. A count reads the gauges once instead of up to three times, and the verdict is the same whenever no reading lands in the middle. Every caller outside `capacity()` is unchanged. ## Links - Closes #1077 - #956 and #1076: the mkdir race in the same file, fixed there - #959: the CI sighting 🤖 Generated with [Claude Code](https://claude.com/claude-code)

nrjdalal added 3 commits November 5, 2026 11:33

fix(PG-940): the retro table reads 25 PRs a page and retries a failed…

6ab6058

… one

fix(PG-940): the retro table still prints a page's GraphQL errors bes…

46adcf0

…ide its data

Merge remote-tracking branch 'origin/canary' into PG-940-retro-retry

836e1f7

nrjdalal commented Nov 5, 2026

View reviewed changes

nrjdalal added the Review • 4/5 label Nov 5, 2026

nrjdalal added 2 commits November 5, 2026 12:49

fix(PG-940): the retro table reads gh's body on any exit and refuses …

fb00c03

…a bare --until

Merge remote-tracking branch 'origin/canary' into PG-940-retro-retry

8a605a9

nrjdalal commented Nov 5, 2026

View reviewed changes

Comment thread packages/scripts/src/retro/table.ts Resolved

Comment thread packages/scripts/src/retro/table.ts Resolved

Comment thread packages/scripts/src/retro/table.ts Resolved

nrjdalal added 3 commits November 5, 2026 13:56

fix(PG-940): the retro table skips a PR or comment GraphQL nulled bes…

c279c1a

…ide its error

Merge remote-tracking branch 'origin/canary' into PG-940-retro-retry

af575b7

fix(PG-940): a PR GraphQL nulled counts as read in the table's short-…

3006e0a

…search line

nrjdalal mentioned this pull request Nov 5, 2026

Two tests fail under load on wall-clock budgets: onboard fix.test's docker case and prebuild.test's main-checkout case #981

Closed

nrjdalal added Review • 5/5 and removed Review • 4/5 labels Nov 5, 2026

nrjdalal marked this pull request as ready for review November 5, 2026 10:21

nrjdalal merged commit 17ca64d into canary Nov 5, 2026
32 of 34 checks passed

nrjdalal deleted the PG-940-retro-retry branch November 5, 2026 10:25

nrjdalal mentioned this pull request Nov 6, 2026

fix(PG-1077): a look counts run-slot's slots 2 to 4 on one gauges reading #1089

Merged

Navigation Menu

Repository navigation

fix(PG-940): the retro table reads 25 PRs a page and retries a failed one - #959

fix(PG-940): the retro table reads 25 PRs a page and retries a failed one#959
nrjdalal merged 8 commits into
canaryfrom
PG-940-retro-retry

nrjdalal commented Nov 5, 2026 •

edited

Loading

Uh oh!

nrjdalal commented Nov 5, 2026

Uh oh!

nrjdalal left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrjdalal commented Nov 5, 2026

Uh oh!

nrjdalal left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrjdalal commented Nov 5, 2026

Uh oh!

Uh oh!

nrjdalal commented Nov 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nrjdalal commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Summary

Evidence

Merge Danger

Links

Uh oh!

nrjdalal commented Nov 5, 2026

ReviewBot · round 1: 4/5 at 836e1f7 · 0 blocking, 2 should-fix, 4 nits

Spec (the right thing)

Standards (built well)

Checked patch

Threads

Checked and Not verified

Uh oh!

nrjdalal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrjdalal commented Nov 5, 2026

ReviewBot · round 2: 4/5 at 8a605a9 · 0 blocking, 1 should-fix, 3 nits

Spec (the right thing)

Standards (built well)

Checked patch (the last nit)

Threads

Checked and Not verified

Uh oh!

nrjdalal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrjdalal commented Nov 5, 2026

ReviewBot · round 3: 5/5 at 3006e0a · 0 blocking, 0 should-fix, 1 nits

Spec (the right thing)

Standards (built well)

Threads

Checked and Not verified

Uh oh!

Uh oh!

nrjdalal commented Nov 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nrjdalal commented Nov 5, 2026 •

edited

Loading

ReviewBot · round 1: 4/5 at `836e1f7` · 0 blocking, 2 should-fix, 4 nits

ReviewBot · round 2: 4/5 at `8a605a9` · 0 blocking, 1 should-fix, 3 nits

ReviewBot · round 3: 5/5 at `3006e0a` · 0 blocking, 0 should-fix, 1 nits