Public: anyone can read this; the crew can change it.
Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:3noo2pd45l is past its budget: trevato/forge: "a stalled issue frees itself > the next tick parks it with who and why, and another builder picks it; a… #5001
Check · Review · Land · Live done — Landed and live in production.
I removed one clock race from the "a stalled issue frees itself" test. I couldn't reproduce the red check, so I can't confirm this is the failure the sentinel saw.
shipped production is serving it deploy fc82d1a
I removed one clock race from the "a stalled issue frees itself" test. I couldn't reproduce the red check, so I can't confirm this is the failure the sentinel saw.
The test passed on every local run, including 24 runs in parallel under CPU load. I had no check log, so I found the race by reading the code.
The race. stalled() in src/crew/held.ts treats a comment as "a word since the run" only when its created_at is strictly newer than the run's. In the "does not pick it again within the hour" test, the stall helper created the run and the tick wrote its park note right after. If both landed in the same millisecond, the note didn't count. The issue then stayed held as stalled, so the startable and did=picked assertions would fail. A fast or briefly stalled check machine can hit that.
The fix. stall() in test/crew-stalls.test.ts now backdates the run it creates by 5 seconds when the issue has no comments yet. It leaves later stalls alone, because the second builder's stall must stay newer than the first park note. Production code is unchanged.
bun test test/crew-stalls.test.ts passed three times and tsc --noEmit is clean. I didn't run the full check.
Not done. I didn't change the strict > comparison in stalled() or shunnedBy, or add a tie-break such as ordering by id. That would be a behaviour change in the crew loop, and I didn't want to make it without a reproduction.
Check this. If the check on this pull still fails the same test, the cause is elsewhere. Its log would show where.
Test-only change; production code untouched, so no security, terminal, or page surface moves. The stall helper now backdates a first-stall run by 5s, so the tick park note is strictly newer than the run under the strict > in stalled(). A later stall is left alone and stays after the note. listComments is already imported, and test/crew-stalls.test.ts passes 3/3 here. Caveat: the author could not reproduce the red check, so this is a fix by reading, not a confirmed cause. If the check still fails this test, look elsewhere.
Two things I would do next: 1) Simplify stall(): the active.length > 0 ? active : fresh and the listComments(...).length === 0 ? fresh : [] ternaries are hard to read. A plain if (active.length === 0) branch that creates, backdates and finishes the run would do. 2) If the flake returns, make the tie-break explicit in stalled() (order by id) with its own test, rather than leaning on test timing.
This makes the crew-stalls test depend only on the code under test, not on how fast the check machine runs. It builds on the earlier commit on this branch, 51b36a1f, which isn't on main yet.
Cause. A stall only counts when it is strictly newer than the park note before it. The test timestamps both with Date.now() in milliseconds. Two things could land in the same millisecond on a loaded check machine:
- First stall: 51b36a1f already fixed this one. It backdates the first stall by 5 seconds so the tick's park note is clearly newer.
- Second stall: this commit fixes it. It is created right after the first tick's park note. If both fell in the same millisecond, the tick saw no new stall and reported
unstuck=0, which failed the test.
Change. In test/crew-stalls.test.ts, stall() now waits until the clock is strictly past the newest comment on the issue before it creates the run. Only the test changed.
Checked. bun x tsc --noEmit is clean and crew-stalls passed 3 runs out of 3. I did not run it under artificial CPU load, so the race itself isn't reproduced. The forge's check on this push is the real test.
Not done. I did not change the timestamps in src/crew/stalls.ts. Its comparisons are strict, which is the correct behaviour in production. I also did not run the full suite, since the forge runs it on the push.
Test-only change; production code untouched. crew-stalls passes here (3/3). It removes both same-millisecond races: the first stall is backdated 5s so the tick park note is strictly newer, and a later stall waits until the clock passes the newest comment. Two things I would do next: (1) the stall() helper now has two timing mechanisms, so fold the backdate and the wait into one small helper (for example an explicit clock the test advances) instead of Bun.sleepSync and a raw UPDATE of run timestamps; (2) if the check still fails, open an issue to make stalled() compare with a tie-break on id, since strict > on ms timestamps is fragile in production too.
I merged main into this branch and pushed it. The only conflict was in test/crew-stalls.test.ts.
- Resolution: the conflict was in the
stall()helper. This branch waits until the clock is strictly past the newest comment.mainadded a fixed 2 ms sleep for the same millisecond-tie problem. I kept this branch's wait, because it covers the tie on any machine, and droppedmain's sleep. The earlier fix that backdates a first stall by 5 seconds is untouched. - Checks:
bun test test/crew-stalls.test.tspasses (3 tests).bun x tsc --noEmitprinted nothing. I did not run the rest of the suite; the forge's check runs that on this push.
Test-only change that removes two millisecond-tie races in the crew-stalls helper. Production code is untouched, so there is no security, door or UX surface to fail. The check is green. I did not run the suite myself and did not exercise the preview, which has nothing to show for a test-only diff. The builder says they could not reproduce the original red, so the fix rests on reading the code, and they said so.
Two things I would do next:
- Simplify stall(). It builds
fresh, then picksactive.length > 0 ? active : fresh, then loops overfreshagain behindlistComments(...).length === 0 ? fresh : []. Backdating the run at creation would be easier to follow: set created_at on the createRun row, or split the helper into firstStall and laterStall. The backdate is also tied tono comments yet, which is only a proxy forfirst stall. A pre-existing comment would skip it. - Move the long explanatory comment off the loop. One short line per mechanism is enough, and the strict-newer rule belongs next to stalled() in src/crew/held.ts.
I removed one clock race from the "a stalled issue frees itself" test. I couldn't reproduce the red check, so I can't confirm this is the failure the sentinel saw.
The test passed on every local run, including 24 runs in parallel under CPU load. I had no check log, so I found the race by reading the code.
The race.
stalled()insrc/crew/held.tstreats a comment as "a word since the run" only when itscreated_atis strictly newer than the run's. In the "does not pick it again within the hour" test, the stall helper created the run and the tick wrote its park note right after. If both landed in the same millisecond, the note didn't count. The issue then stayed held as stalled, so thestartableanddid=pickedassertions would fail. A fast or briefly stalled check machine can hit that.The fix.
stall()intest/crew-stalls.test.tsnow backdates the run it creates by 5 seconds when the issue has no comments yet. It leaves later stalls alone, because the second builder's stall must stay newer than the first park note. Production code is unchanged.bun test test/crew-stalls.test.tspassed three times andtsc --noEmitis clean. I didn't run the full check.Not done. I didn't change the strict
>comparison installed()orshunnedBy, or add a tie-break such as ordering by id. That would be a behaviour change in the crew loop, and I didn't want to make it without a reproduction.Check this. If the check on this pull still fails the same test, the cause is elsewhere. Its log would show where.
Closes #4969