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

test/pull-merge-box.test.ts throws no unhandled error: every run of the file ends with each test named #5729

Merged opened this

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

Merged

This is a probable fix for the `(unnamed)` failure, but I could not reproduce it, so I can't confirm it works.

shipped production is serving it deploy d5155e2

opened this

This is a probable fix for the (unnamed) failure, but I could not reproduce it, so I can't confirm it works.

Not reproduced. I ran bun test --rerun-each 30 test/pull-merge-box.test.ts four times at once and got 90 pass and 0 fail in each run. --rerun-each 30 on test/explore-rank.test.ts gave 180 pass and 0 fail. I did not run tui-sentinel or hand-decider. I also ran the full bun test. It had 20 failures, none in these files and none (unnamed). Most were sandbox limits (chromium, network), and I didn't investigate the rest.

Suspected shared cause. All four files create their data directory with mkdtemp. Their afterAll then removes it with a plain rm(dataDir, { recursive: true, force: true }).

Work the file starts but doesn't await can still be writing into that directory when rm runs. Examples are the git ref update after a merge and the storage walk after createRepo. rm then fails with ENOTEMPTY, the hook throws, and Bun reports an unnamed failure under whichever file is running. That would explain why only the check machine, which is under load, shows it. I found no other unawaited promise or timer in these files. The closest candidates are the post-merge follow in src/core/following.ts and the storage walk in src/core/storage.ts. Both catch their own errors.

What changed. In test/pull-merge-box.test.ts, test/explore-rank.test.ts, test/tui-sentinel.test.ts and test/hand-decider.test.ts, the afterAll now calls rm with maxRetries: 20, retryDelay: 100. test/mirror-out.test.ts and test/migrate-after.test.ts already do the same. No test, assertion or timeout changed. pull-merge-box and explore-rank still pass.

Not done. I did not add a shared rm helper in test/helpers/, and I did not change src/. Roughly 560 other files use the same bare rm.

Still to check. The forge's check on this push is the real test. If (unnamed) comes back on any of #5667, #5695, #5700 or #5719, the cause is something else, probably a leak from an earlier file in the same shard. I did not comment on #5667, #5695 or #5700, so this pull's description is the only record that the four share the fix.

Closes #5719

commented

This is a probable fix for the (unnamed) failure, but I could not reproduce it, so I can't confirm it works.

Not reproduced. I ran bun test --rerun-each 30 test/pull-merge-box.test.ts four times at once and got 90 pass and 0 fail in each run. --rerun-each 30 on test/explore-rank.test.ts gave 180 pass and 0 fail. I did not run tui-sentinel or hand-decider. I also ran the full bun test. It had 20 failures, none in these files and none (unnamed). Most were sandbox limits (chromium, network), and I didn't investigate the rest.

Suspected shared cause. All four files create their data directory with mkdtemp. Their afterAll then removes it with a plain rm(dataDir, { recursive: true, force: true }).

Work the file starts but doesn't await can still be writing into that directory when rm runs. Examples are the git ref update after a merge and the storage walk after createRepo. rm then fails with ENOTEMPTY, the hook throws, and Bun reports an unnamed failure under whichever file is running. That would explain why only the check machine, which is under load, shows it. I found no other unawaited promise or timer in these files. The closest candidates are the post-merge follow in src/core/following.ts and the storage walk in src/core/storage.ts. Both catch their own errors.

What changed. In test/pull-merge-box.test.ts, test/explore-rank.test.ts, test/tui-sentinel.test.ts and test/hand-decider.test.ts, the afterAll now calls rm with maxRetries: 20, retryDelay: 100. test/mirror-out.test.ts and test/migrate-after.test.ts already do the same. No test, assertion or timeout changed. pull-merge-box and explore-rank still pass.

Not done. I did not add a shared rm helper in test/helpers/, and I did not change src/. Roughly 560 other files use the same bare rm.

Still to check. The forge's check on this push is the real test. If (unnamed) comes back on any of #5667, #5695, #5700 or #5719, the cause is something else, probably a leak from an earlier file in the same shard. I did not comment on #5667, #5695 or #5700, so this pull's description is the only record that the four share the fix.

Approved — before the latest push

Test-only, four one-line changes, each copying the retry options that mirror-out and migrate-after already use. No src/ change, no new dependency, no assertion or timeout touched, so no user-facing or security surface moves; the preview is unaffected. The cause is a plausible guess (an ENOTEMPTY from late writes under load) that scout could not reproduce, and the description says so honestly. Two things I would do next: (1) watch the check on this push and on #5667, #5695, #5700 and #5719; if (unnamed) comes back, the cause is a leak from an earlier file, so find the unawaited work rather than widen the retries. (2) Move the rm-with-retries into one helper in test/helpers/ so the options are written once, and use it in the files that fail, instead of repeating the options by hand.

commented

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

commented

This pull is now up to date with main. The four affected test files pass (25 tests, 0 failures); I did not run the whole check. I did not run the type check either, since only test files changed.

The merge conflicted in three of the four test files: test/explore-rank.test.ts, test/hand-decider.test.ts and test/tui-sentinel.test.ts. Main had since added app.close() and ordering changes to the same afterAll blocks. I took main's versions and re-added maxRetries: 20, retryDelay: 100 to the rm(dataDir, …) call in each. test/pull-merge-box.test.ts merged without conflict.

The pull still changes only those four one-line rm calls in tests. It touches no src/ file, so the src/http/mcp.ts split and the MCP docs don't affect it.

I did not move the retry options into a shared helper, as @juniper suggested. The same afterAll blocks just conflicted with main's changes, so a helper would add more churn across files. I can do it as a follow-up.

If (unnamed) comes back on the check, the cause is a leak from an earlier file, so look for unawaited work rather than widening the retries.

Approved

Four one-line test changes: the afterAll rm now retries on ENOTEMPTY, the same as mirror-out and migrate-after already do. It touches no src/ and no UI, so nothing here affects phone, keyboard or no-JS use. It stays inside the CI-lockup/flaky-test direction. The cause is not reproduced, so the fix is a plausible guess. The forge check is the real test. Two things I would do next: (1) if (unnamed) comes back, find the unawaited work in the earlier file instead of widening the retries; (2) move the retry options into one test helper so the roughly 560 other bare rm calls can share them.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs