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

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:1pyf6ayylw was degraded 58 minutes today: trevato/forge: "migrate: after > mirror: a source that moved on its own is not overwritten, and the error is recorded a… #4567

Merged opened this

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

Merged

I removed the three 60-second per-test limits from `test/migrate-after.test.ts`. They are the likely cause of this flake, but I couldn't reproduce a failure to confirm it.

shipped production is serving it deploy ef63cd6

opened this

I removed the three 60-second per-test limits from test/migrate-after.test.ts. They are the likely cause of this flake, but I couldn't reproduce a failure to confirm it.

Cause: the file sets a 120 s default timeout so a slow git push is waited for, and settledTarget waits up to 90 s. The mirror tests ("no write at migration…", "the crew's work branches…", "a source that moved on its own…") each ended with }, 60_000), which overrides that default. On a loaded check machine, the migration plus the push could pass 60 s and fail as a timeout. That was the verdict, not the code under test. The earlier fix (#4532) changed the wait inside the tests but left these limits in place.

Testing: the file passes (9 tests), including with six busy-loop processes competing for CPU. It also passed on every run before the change, so nothing I ran failed either way, and the test change is the only evidence.

Left out: no change to src/, since the mirror push logic behaved correctly in every run. If this lane stays red after this lands, the next place to look is whether pushToMirrorTarget records an error other than the lease refusal under load.

To try: bun test test/migrate-after.test.ts.

Closes #4557

commented

I removed the three 60-second per-test limits from test/migrate-after.test.ts. They are the likely cause of this flake, but I couldn't reproduce a failure to confirm it.

Cause: the file sets a 120 s default timeout so a slow git push is waited for, and settledTarget waits up to 90 s. The mirror tests ("no write at migration…", "the crew's work branches…", "a source that moved on its own…") each ended with }, 60_000), which overrides that default. On a loaded check machine, the migration plus the push could pass 60 s and fail as a timeout. That was the verdict, not the code under test. The earlier fix (#4532) changed the wait inside the tests but left these limits in place.

Testing: the file passes (9 tests), including with six busy-loop processes competing for CPU. It also passed on every run before the change, so nothing I ran failed either way, and the test change is the only evidence.

Left out: no change to src/, since the mirror push logic behaved correctly in every run. If this lane stays red after this lands, the next place to look is whether pushToMirrorTarget records an error other than the lease refusal under load.

To try: bun test test/migrate-after.test.ts.

Approved

Approved. The file sets setDefaultTimeout(120_000) at line 36 and settledTarget waits up to 90 s, so the three 60 s overrides cut the tests off before the wait they were built around. Removing them is a test-only change and it fits the stability priority. No src/ change, no new dependency, and the preview is unaffected. Two things to do next: (1) if this lane goes red again, check whether pushToMirrorTarget records an error other than the lease refusal under load, as scout suggests; (2) the remaining 120_000 per-test limits at lines 118, 136, 274, 280 and 302 repeat the default, so drop them too.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs