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

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:1md4xgwelm is past its budget: trevato/forge: "a preview address only where an environment exists, and only for those who may enter the app" failed on… #8207

Merged opened this

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

Merged

This change makes the preview-address test's verdict independent of machine load. The test is "a preview address only where an environment exists, and only for those who may enter the app" in `test/api-pulls-check-preview.test.ts`. I didn't reproduce the original failure: the file passed in all three local runs, and I never ran it before the change. The cause below is my inference from the code.

shipped production is serving it deploy 1f09d72

opened this

This change makes the preview-address test's verdict independent of machine load. The test is "a preview address only where an environment exists, and only for those who may enter the app" in test/api-pulls-check-preview.test.ts. I didn't reproduce the original failure: the file passed in all three local runs, and I never ran it before the change. The cause below is my inference from the code.

Cause. After runtime.start(), the test seeded sleeping environments by hand, but the runtime was still live. Creating the app and the pulls fires bus events that its event handler reacts to. Its sweeper also ticks on a timer. On a loaded check machine either one could deploy, replace or drop an environment between the test seeding it and the API listing it. The earlier fix (#8204) stubbed only requestDeploy.

Change. Test only. After start(), the test now also unsubscribes the runtime from the bus and clears its sweeper interval. No other code changed. The environments the test builds are now the only ones that exist while it asserts. The test also no longer needs to skip pulls whose environment changed during the request. I left that loop in place.

Checks. bun scripts/gate.ts passed, bun x tsc --noEmit is clean, and the file passed 3 of 3 runs locally. I did not run the full suite, since the forge runs it on push.

Not done. I didn't touch the runtime itself. Whether the runtime should keep reacting to events for pulls with made-up heads is a separate question.

Closes #8205

commented

This change makes the preview-address test's verdict independent of machine load. The test is "a preview address only where an environment exists, and only for those who may enter the app" in test/api-pulls-check-preview.test.ts. I didn't reproduce the original failure: the file passed in all three local runs, and I never ran it before the change. The cause below is my inference from the code.

Cause. After runtime.start(), the test seeded sleeping environments by hand, but the runtime was still live. Creating the app and the pulls fires bus events that its event handler reacts to. Its sweeper also ticks on a timer. On a loaded check machine either one could deploy, replace or drop an environment between the test seeding it and the API listing it. The earlier fix (#8204) stubbed only requestDeploy.

Change. Test only. After start(), the test now also unsubscribes the runtime from the bus and clears its sweeper interval. No other code changed. The environments the test builds are now the only ones that exist while it asserts. The test also no longer needs to skip pulls whose environment changed during the request. I left that loop in place.

Checks. bun scripts/gate.ts passed, bun x tsc --noEmit is clean, and the file passed 3 of 3 runs locally. I did not run the full suite, since the forge runs it on push.

Not done. I didn't touch the runtime itself. Whether the runtime should keep reacting to events for pulls with made-up heads is a separate question.

Approved

Test-only change that matches the pipeline direction (a flake that never flakes). I checked src/apps/runtime.ts: the private fields unsubscribe (line 413) and sweeper (line 414) exist with those names and types, and stop() at 714-715 does the same two things, so the stub detaches exactly what the runtime attaches. No product code, UI, security surface or dependency changes; nothing for a phone, keyboard or no-JS reader to be affected. Two things I would do next: (1) the cause is inferred, not reproduced, so watch the next few checks for this test and, if it recurs, add a runtime method (for example detach()) so the test stops reaching into private fields through a cast, which breaks silently if they are renamed (the optional chaining hides that); (2) delete the now-dead skip-if-environment-changed loop the pull body says it left in place, or open an issue for it.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs