ReviewOS

also looking at this

stacks/rpx

fix(supervisor): stop reporting a shutdown as a readiness timeout

#2273
Merged glennmichael123 wants to merge fix/2272-shutdown-readiness-timeout into main
2 files +47 -1
10 days ago

Closes #2272.

The bug

driveReadiness polls with:

while (this.now() < deadline && !this.stopped) { … }

if (state.status === 'starting')
  this.fail(state, `did not become ready within ${…}s`)

The loop ends for two different reasons — a genuine deadline, or stopAll() flipping stopped — and both fell through to the same fail(...).

So every shutdown logged a readiness failure for each site still booting, for something that never happened. fail() also calls killProcess, so it duplicated the teardown stop() was already running.

Why it mattered beyond the log line

The warn is emitted a poll interval after the test file that triggered it has ended, so it gets attributed to whatever file runs next. On main today it prints three times, once under packages/rpx/test/proxy-handler.test.ts, which has nothing to do with site supervision. That was part of the misleading signal while diagnosing #2267.

Before, on main:

packages/rpx/test/proxy-handler.test.ts:
[warn] rpx: site myapp.localhost failed — did not become ready within 120s

After: gone entirely from the suite output.

The fix

Return early when stopped is set, so only a real deadline expiry reports a timeout.

Test

The regression test drives a site that never becomes ready (probePort: async () => false), calls stopAll() while it is mid-boot, waits one poll interval past the stop, and asserts no readiness-timeout warn was emitted.

Verified it is a real guard — with the guard removed it fails, with it restored it passes:

=== WITH BUG REINTRODUCED ===
(fail) SiteSupervisor > does not report a readiness timeout for a site still booting at stopAll
 8 pass, 1 fail
=== WITH FIX RESTORED ===
 9 pass, 0 fail

Full suite: 519 pass / 0 fail. pickier and tsc --noEmit clean.

The spy is restored in a finally, and the supervisor is registered with supervisors so the afterEach reaches it — deliberately avoiding the two patterns filed as #2270 and #2271.

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.