trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:uxo2i7ava1 is past its budget: trevato/forge: "a subscription's standing > (unnamed)" failed on 3 pulls in the last hour — the lane's red, not theirs;… #4562

Merged scoutcrew opened this

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

ConversationFiles
Merged

The "a subscription's standing" test setup no longer waits on a child process, so its verdict no longer depends on the check machine's load. I couldn't reproduce the original failure here, where the file passes in about 250 ms both before and after the change. I haven't seen the forge's check run on this push yet.

shipped production is serving it deploy 749c831

scoutcrew opened this

The "a subscription's standing" test setup no longer waits on a child process, so its verdict no longer depends on the check machine's load. I couldn't reproduce the original failure here, where the file passes in about 250 ms both before and after the change. I haven't seen the forge's check run on this push yet.

  • Cause: the beforeAll hook in test/standing.test.ts ran the full createRepo action. That spawns git init and installs hooks, and it also checks storage limits. Under a loaded check machine that can overrun the hook's 60 s timeout, and the failure is reported as "(unnamed)" because it is the hook, not a test. The earlier fix (#4549) only removed the argon2 password hashing from the hook.
  • Change: the hook now inserts a bare repo row directly. The tests only need a repo id to attach usage runs to. They never clone or sign in, so nothing about what they check has changed.
  • Not done: I didn't touch createRepo or the other test suites that use it. I haven't audited them for the same load-dependence.
  • Checks: bun test test/standing.test.ts passes (8 tests). bun x tsc --noEmit couldn't run: it failed with "Cannot find type definition file for 'bun'", which bun install (which ran) didn't fix.
  • To try it: bun test test/standing.test.ts.

Closes #4550

scoutcrew commented

The "a subscription's standing" test setup no longer waits on a child process, so its verdict no longer depends on the check machine's load. I couldn't reproduce the original failure here, where the file passes in about 250 ms both before and after the change. I haven't seen the forge's check run on this push yet.

  • Cause: the beforeAll hook in test/standing.test.ts ran the full createRepo action. That spawns git init and installs hooks, and it also checks storage limits. Under a loaded check machine that can overrun the hook's 60 s timeout, and the failure is reported as "(unnamed)" because it is the hook, not a test. The earlier fix (#4549) only removed the argon2 password hashing from the hook.
  • Change: the hook now inserts a bare repo row directly. The tests only need a repo id to attach usage runs to. They never clone or sign in, so nothing about what they check has changed.
  • Not done: I didn't touch createRepo or the other test suites that use it. I haven't audited them for the same load-dependence.
  • Checks: bun test test/standing.test.ts passes (8 tests). bun x tsc --noEmit couldn't run: it failed with "Cannot find type definition file for 'bun'", which bun install (which ran) didn't fix.
  • To try it: bun test test/standing.test.ts.
junipercrew Approved — before the latest push

Test-only change: the standing suite hook inserts a bare repo row instead of running createRepo (git init child process), so the hook no longer depends on machine load. Columns match the repo table; bun test test/standing.test.ts passes (8 tests). No product behaviour changes, no new dependency, nothing dead. Two next steps: (1) audit other beforeAll hooks that call createRepo for the same load dependence, ideally via a shared test helper for a bare repo row so the raw INSERT is not copied; (2) if the hook still times out on the check machine, the cause is elsewhere (migrations / createApp), so log hook timing.

trevatoowner commented

The full gate on land was red; the pull is open again. Red tests:

  • the background class > the child and what it starts run at nice 10, SCHED_BATCH where chrt is, same argv
  • the background class > the child and what it starts run at nice 10, SCHED_BATCH where chrt is, same argv [16.97ms] [shard 315]
scoutcrew commented

Two tests changed, both so their verdict no longer depends on where they run.

  • Standing suite: its beforeAll hook seeds a bare repo row directly instead of running createRepo. createRepo spawns git init, which overran the hook's timeout when the check machine was loaded.
  • Background class test (the red test from the full gate): test/serving-priority.test.ts ("the child and what it starts run at nice 10…") expected the child at exactly nice 10. backgroundArgv runs nice -n 10, which is relative to the parent's nice level. A runner that is already niced therefore lands higher; I reproduced it here at nice 19. The test now reads its own nice from /proc/self/stat and expects min(19, own + 10). Under an un-niced runner it still asserts exactly 10. Product code is unchanged.
  • Verification: bun test test/serving-priority.test.ts passes (10 tests, 0 fail) in this niced shell. bun test test/standing.test.ts passed (8 tests) before this push. I did not re-run it for this push.

I did not take juniper's two suggestions: a shared bare-repo-row helper, and an audit of other beforeAll hooks that call createRepo. Both are reasonable follow-ups, but each is wider than this one lane. If the standing hook still times out, createApp and migrations are the next place to look, as juniper said.

junipercrew Approved — before the latest push

Test-only change that takes two sources of machine-dependence out of the lane. Product code is untouched, so none of the security or door checks apply. The standing hook now seeds a bare repo row, and the columns match the schema in 0001-foundation. The nice test now expects min(19, own+10), which is correct because nice -n is relative, and it still asserts exactly 10 under an un-niced runner. The change does what the issue asked and nothing more. Next two things I would do: (1) pull the bare-repo-row insert into a shared test helper if a second suite needs it; (2) audit other beforeAll hooks that call createRepo for the same load-dependence.

scoutcrew commented

I merged main into the branch and pushed. The only conflict was in test/serving-priority.test.ts.

main had already made the same fix I made in that file: the background-class test now expects the child's nice level to be the runner's own plus 10, capped at 19. I took main's version, which differs from mine only in comments and a variable name, so nothing of mine was lost there.

The pull's remaining change is the standing suite's beforeAll hook, which inserts a bare repo row instead of calling createRepo. After the merge, bun test test/serving-priority.test.ts test/standing.test.ts passes (18 tests, 0 fail).

junipercrew Approved

Test-only change: the standing suite beforeAll now inserts a bare repo row instead of running createRepo (git init child process), so the hook no longer depends on machine load. No product code touched, no new dependency, no security surface. bun test test/standing.test.ts passes here (8 tests). Next, I would: (1) pull the bare-repo-row insert into a shared test helper so other suites can drop createRepo too; (2) audit other beforeAll hooks that spawn git for the same load sensitivity. Minor: the inline INSERT line is long; a helper would also tidy that.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs