also looking at this
fix(supervisor): stop reporting a shutdown as a readiness timeout
#2273
2 files
+47
-1
| @@ -323,6 +323,14 @@ export class SiteSupervisor { | ||
| 323 | 323 | await delay(this.pollIntervalMs) |
| 324 | 324 | } |
| 325 | 325 | |
| 326 | // The loop also exits when `stopAll()` flips `stopped`, which is a | |
| 327 | // deliberate shutdown rather than a timeout. Falling through to `fail()` | |
| 328 | // there logged "did not become ready within 120s" for every site that was | |
| 329 | // still booting, on every shutdown — a failure that never happened, and a | |
| 330 | // second teardown on top of the one `stop()` is already running. | |
| 331 | if (this.stopped) | |
| 332 | return | |
| 333 | ||
| 326 | 334 | if (state.status === 'starting') |
| 327 | 335 | this.fail(state, `did not become ready within ${Math.round(this.startupTimeoutMs / 1000)}s`) |
| 328 | 336 | } |
| @@ -1,10 +1,11 @@ | ||
| 1 | 1 | import type { ResolvedSite, SiteResolver } from '../src/site-resolver' |
| 2 | 2 | import type { SiteLauncher, SiteProcessHandle } from '../src/site-supervisor' |
| 3 | 3 | import type { RegistryEntry } from '../src/registry' |
| 4 | import { afterEach, describe, expect, it } from 'bun:test' | |
| 4 | import { afterEach, describe, expect, it, spyOn } from 'bun:test' | |
| 5 | 5 | import * as fsp from 'node:fs/promises' |
| 6 | 6 | import * as os from 'node:os' |
| 7 | 7 | import * as path from 'node:path' |
| 8 | import { log } from '../src/logger' | |
| 8 | 9 | import { SiteSupervisor } from '../src/site-supervisor' |
| 9 | 10 | |
| 10 | 11 | /** A controllable fake process: tests resolve `exit` to simulate it dying. */ |
| @@ -277,6 +278,43 @@ describe('SiteSupervisor', () => { | ||
| 277 | 278 | await sup.stopAll() |
| 278 | 279 | expect(proc.signals).toContain('SIGTERM') |
| 279 | 280 | }) |
| 281 | ||
| 282 | it('does not report a readiness timeout for a site still booting at stopAll', async () => { | |
| 283 | // The readiness loop exits on `stopped` as well as on the deadline, and used | |
| 284 | // to fall through to the timeout failure either way — so every shutdown | |
| 285 | // logged "did not become ready within 120s" for each still-booting site, a | |
| 286 | // failure that never happened. Worse for tests: the warn lands a poll after | |
| 287 | // the file that caused it has ended, so it is attributed to whatever file | |
| 288 | // runs next. | |
| 289 | await mkdir() | |
| 290 | const proc = fakeProc(321) | |
| 291 | const sup = new SiteSupervisor({ | |
| 292 | resolver: resolverFor(stacksSite()), | |
| 293 | rpxDir, | |
| 294 | registryDir: rpxDir, | |
| 295 | launcher: () => proc.handle, | |
| 296 | pickPort: async preferred => preferred, | |
| 297 | probePort: async () => false, // never becomes ready, so it is mid-boot at stopAll | |
| 298 | pollIntervalMs: 5, | |
| 299 | }) | |
| 300 | supervisors.push(sup) | |
| 301 | ||
| 302 | const warnSpy = spyOn(log, 'warn').mockImplementation(() => {}) | |
| 303 | try { | |
| 304 | await sup.onRequest('myapp.localhost') | |
| 305 | await sup.stopAll() | |
| 306 | // Let the readiness loop wake once more past the stop — the stray warn | |
| 307 | // arrived a poll interval later, which is exactly why it used to surface | |
| 308 | // under an unrelated test file's name. | |
| 309 | await new Promise(resolve => setTimeout(resolve, 60)) | |
| 310 | ||
| 311 | const warnings = warnSpy.mock.calls.map(call => String(call[0])) | |
| 312 | expect(warnings.filter(w => w.includes('did not become ready'))).toEqual([]) | |
| 313 | } | |
| 314 | finally { | |
| 315 | warnSpy.mockRestore() | |
| 316 | } | |
| 317 | }) | |
| 280 | 318 | }) |
| 281 | 319 | |
| 282 | 320 | async function waitFor(pred: () => boolean | Promise<boolean>, timeoutMs = 1000): Promise<void> { |