From a2285ecfea04f53b9c741485e692237582095527 Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Tue, 14 Apr 2026 19:40:28 -0400 Subject: [PATCH 1/9] Fix intermittent 500 errors from worker pool concurrency issues Two related bugs caused intermittent HTTP 500 errors under normal concurrent request loads (e.g. a browser page load firing parallel requests for HTML, CSS, JS, and REST API). 1. Pool proxy released workers before PHP finished: handleRequest called playgroundPool.requestStreamed() through the pool proxy. The proxy released the worker when the promise resolved (streaming started), but the worker's SinglePHPInstanceManager held the PHP instance until the stream finished. A subsequent request routed to that 'free' worker hit isAcquired === true and returned HTTP 500. Fix: Switch to buffered request() which holds the worker until PHP completes. Change start-server.ts to a callback pattern so the full request-to-response lifecycle stays within the pool proxy scope. 2. Crashed workers were never removed from the pool: When a worker thread exited unexpectedly (e.g. EADDRINUSE), its API proxy remained in the pool. Requests routed to the dead proxy would fail and be released back, creating an infinite failure cycle. Fix: Add __removeInstance() to createObjectPoolProxy so callers can evict dead instances. Wire up the onExit handler in run-cli.ts to remove dead workers from the pool. Fixes #3492 ## AI disclosure Per WordPress AI Guidelines (https://make.wordpress.org/ai/handbook/ai-guidelines/): AI assistance: Yes Tool: Claude Code (Claude Opus 4.6) Used for: Root cause analysis of the pool proxy lifecycle mismatch, implementing the fix (buffered request approach, callback pattern, __removeInstance for dead worker eviction), updating tests, and drafting the commit message. All code reviewed, tested, and validated by the author against a running Studio site with concurrent request benchmarks. --- .../universal/src/lib/object-pool-proxy.ts | 47 ++++++- packages/playground/cli/src/run-cli.ts | 61 +++++++-- packages/playground/cli/src/start-server.ts | 16 ++- .../playground/cli/tests/start-server.spec.ts | 120 +++++++++++------- 4 files changed, 175 insertions(+), 69 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts index deccb14a7bf..072dac8277d 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts @@ -22,7 +22,19 @@ type Promisified = { export type Pooled = Omit< Promisified, typeof Symbol.dispose | typeof Symbol.asyncDispose ->; +> & { + /** + * Remove an instance from the pool. If the instance is currently + * free, it is removed immediately. If it is busy (acquired by a + * pending call), it will be discarded when that call releases it + * instead of being returned to the free list. + * + * This is useful when a worker thread backing a pool instance + * crashes — removing it prevents the pool from routing new + * requests to a dead worker. + */ + __removeInstance(instance: unknown): void; +}; /** * Creates a proxy that distributes method calls and property accesses @@ -42,6 +54,26 @@ export function createObjectPoolProxy( const freeInstances: T[] = [...instances]; const waitQueue: Array<(instance: T) => void> = []; + const removedInstances = new Set(); + + function removeInstance(instance: unknown): void { + const inst = instance as T; + + // Remove from the canonical list + const idx = instances.indexOf(inst); + if (idx !== -1) { + instances.splice(idx, 1); + } + + // Remove from the free list if it's currently free + const freeIdx = freeInstances.indexOf(inst); + if (freeIdx !== -1) { + freeInstances.splice(freeIdx, 1); + } else { + // Instance is currently busy — mark it for removal on release + removedInstances.add(inst); + } + } function acquire(): Promise { const free = freeInstances.shift(); @@ -54,6 +86,12 @@ export function createObjectPoolProxy( } function release(instance: T): void { + // If this instance was marked for removal while busy, discard it + if (removedInstances.has(instance)) { + removedInstances.delete(instance); + return; + } + const waiter = waitQueue.shift(); if (waiter) { waiter(instance); @@ -88,7 +126,12 @@ export function createObjectPoolProxy( }); } - return new Proxy({} as Pooled, { + const proxyTarget = {} as Pooled; + // Expose __removeInstance directly on the target so it's found + // by the `prop in _target` check before hitting the proxy trap. + (proxyTarget as any).__removeInstance = removeInstance; + + return new Proxy(proxyTarget, { get(_target, prop: string | symbol) { // Support returning assigned target properties. // The main reason for this is to allow us to override methods diff --git a/packages/playground/cli/src/run-cli.ts b/packages/playground/cli/src/run-cli.ts index 1c29604a4e8..bf6f3224673 100644 --- a/packages/playground/cli/src/run-cli.ts +++ b/packages/playground/cli/src/run-cli.ts @@ -1516,7 +1516,24 @@ export async function runCLI(args: RunCLIArgs): Promise { logger.error( `Worker ${workerIndex} exited with code ${exitCode}\n` ); - // @TODO: Should we respawn the worker if it exited with an error and the CLI is not shutting down? + + // Remove the dead worker's API proxy from the pool + // so new requests are not routed to it. + if (playgroundPool) { + const deadWorker = spawnedWorkers[workerIndex]; + if (deadWorker) { + const deadApi = + workerToPlaygroundMap.get(deadWorker); + if (deadApi) { + playgroundPool.__removeInstance( + deadApi + ); + logger.error( + `Worker ${workerIndex} removed from pool\n` + ); + } + } + } }, }).then( async ( @@ -1718,12 +1735,18 @@ export async function runCLI(args: RunCLIArgs): Promise { throw new Error(phpLogs, { cause: error }); } }, - async handleRequest(request: PHPRequest): Promise { + async handleRequest( + request: PHPRequest, + streamResponse: (response: StreamedPHPResponse) => Promise + ): Promise { if (!wordPressReady) { - return StreamedPHPResponse.forHttpCode( - 502, - 'WordPress is not ready yet' + await streamResponse( + StreamedPHPResponse.forHttpCode( + 502, + 'WordPress is not ready yet' + ) ); + return; } // Clear the playground_auto_login_already_happened cookie on the first request. // Otherwise the first Playground CLI server started on the machine will set it, @@ -1745,9 +1768,12 @@ export async function runCLI(args: RunCLIArgs): Promise { 'playground_auto_login_already_happened=1; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; Path=/', ]; } - return StreamedPHPResponse.fromPHPResponse( - new PHPResponse(302, headers, new Uint8Array()) + await streamResponse( + StreamedPHPResponse.fromPHPResponse( + new PHPResponse(302, headers, new Uint8Array()) + ) ); + return; } if (cookieStore) { request = { @@ -1764,20 +1790,27 @@ export async function runCLI(args: RunCLIArgs): Promise { }; } - // TODO: Explore switching to a worker thread method to adopt an entire HTTP connection - // It might be more efficient to let the worker respond directly - const response = await playgroundPool.requestStreamed(request); + // Use the non-streaming request() method through the pool proxy. + // This ensures the pool holds the worker for the entire PHP + // execution lifecycle. The pool proxy releases the worker when + // the method's promise resolves — with requestStreamed(), that + // happens when the response *starts* streaming (before PHP + // finishes), causing "PHP instance already acquired" errors + // on concurrent requests. Using request() instead means the + // worker is only released after PHP has fully completed. + const response = await playgroundPool.request(request); if (cookieStore) { - const headers = await response.headers; - cookieStore.rememberCookiesFromResponseHeaders(headers); + cookieStore.rememberCookiesFromResponseHeaders( + response.headers + ); // While we have an internal cookie store, we filter out the // Set-Cookie headers from responses so the browser does not // attempt to manage cookies at the same time as the server. - delete headers['set-cookie']; + delete response.headers['set-cookie']; } - return response; + await streamResponse(StreamedPHPResponse.fromPHPResponse(response)); }, }).catch((error) => { cliOutput.printError(describeError(error)); diff --git a/packages/playground/cli/src/start-server.ts b/packages/playground/cli/src/start-server.ts index 6e627f0ff3c..80dbb43fe7d 100644 --- a/packages/playground/cli/src/start-server.ts +++ b/packages/playground/cli/src/start-server.ts @@ -16,9 +16,16 @@ export interface ServerOptions { port: number; onBind: (server: Server, port: number) => Promise; /** - * Handler for requests. Always returns StreamedPHPResponse. + * Handler for requests. Receives the PHP request and a callback that + * streams the response to the HTTP client. The handler must not resolve + * until the response is fully streamed — this is critical for pool-based + * concurrency control where the worker must be held for the entire + * request lifecycle. */ - handleRequest: (request: PHPRequest) => Promise; + handleRequest: ( + request: PHPRequest, + streamResponse: (response: StreamedPHPResponse) => Promise + ) => Promise; } export function isPortInUse(port: number): Promise { @@ -63,8 +70,9 @@ export async function startServer( body: await bufferRequestBody(req), }; - const response = await options.handleRequest(phpRequest); - await handleStreamedResponse(response, res); + await options.handleRequest(phpRequest, (response) => + handleStreamedResponse(response, res) + ); } catch (error) { logger.error(error); if (!res.headersSent) { diff --git a/packages/playground/cli/tests/start-server.spec.ts b/packages/playground/cli/tests/start-server.spec.ts index 2fffe96dfdf..7e57d365729 100644 --- a/packages/playground/cli/tests/start-server.spec.ts +++ b/packages/playground/cli/tests/start-server.spec.ts @@ -28,35 +28,45 @@ describe('startServer', () => { { code: 'ERR_STREAM_UNABLE_TO_PIPE' } ); - const repondersForHandleRequest = [ + const repondersForHandleRequest: Array< + (...args: any[]) => Promise + > = [ // Demonstrate logged error before the ignored error // to confirm the logger was working beforehand. async () => { throw expectedErrorBefore; }, - async () => - new StreamedPHPResponse( - new ReadableStream({ - start(controller) { - const json = JSON.stringify({ - status: 200, - headers: ['content-type: text/plain'], - }); - controller.enqueue(new TextEncoder().encode(json)); - controller.close(); - }, - }), - new ReadableStream({ - start(controller) { - controller.enqueue( - new TextEncoder().encode('hello') - ); - controller.close(); - }, - }), - new ReadableStream({ start: (c) => c.close() }), - Promise.resolve(0) - ), + async (...args: any[]) => { + const streamResponse = args[1] as ( + response: StreamedPHPResponse + ) => Promise; + await streamResponse( + new StreamedPHPResponse( + new ReadableStream({ + start(controller) { + const json = JSON.stringify({ + status: 200, + headers: ['content-type: text/plain'], + }); + controller.enqueue( + new TextEncoder().encode(json) + ); + controller.close(); + }, + }), + new ReadableStream({ + start(controller) { + controller.enqueue( + new TextEncoder().encode('hello') + ); + controller.close(); + }, + }), + new ReadableStream({ start: (c) => c.close() }), + Promise.resolve(0) + ) + ); + }, // Demonstrate logged error after the ignored error // to confirm the logger was working afterward. async () => { @@ -71,7 +81,8 @@ describe('startServer', () => { const cliServer = await startServer({ port: 0, - handleRequest: () => repondersForHandleRequest.shift()!(), + handleRequest: (...args: any[]) => + repondersForHandleRequest.shift()!(...args), async onBind(server, port) { return { server, port } as any; }, @@ -133,7 +144,9 @@ describe('startServer', () => { const expectedErrorBefore = new Error('handler failure before'); const expectedErrorAfter = new Error('handler failure after'); - const repondersForHandleRequest = [ + const repondersForHandleRequest: Array< + (...args: any[]) => Promise + > = [ // Demonstrate logged error before the ignored error // to confirm the logger was working beforehand. async () => { @@ -141,28 +154,36 @@ describe('startServer', () => { }, // Provide a real streamed response so we can test what happens // when the client disconnects mid-stream. - async () => - new StreamedPHPResponse( - new ReadableStream({ - start(controller) { - const json = JSON.stringify({ - status: 200, - headers: ['content-type: text/plain'], - }); - controller.enqueue(new TextEncoder().encode(json)); - controller.close(); - }, - }), - new ReadableStream({ - start(controller) { - controller.enqueue( - new TextEncoder().encode('hello') - ); - }, - }), - new ReadableStream({ start: (c) => c.close() }), - Promise.resolve(0) - ), + async (...args: any[]) => { + const streamResponse = args[1] as ( + response: StreamedPHPResponse + ) => Promise; + await streamResponse( + new StreamedPHPResponse( + new ReadableStream({ + start(controller) { + const json = JSON.stringify({ + status: 200, + headers: ['content-type: text/plain'], + }); + controller.enqueue( + new TextEncoder().encode(json) + ); + controller.close(); + }, + }), + new ReadableStream({ + start(controller) { + controller.enqueue( + new TextEncoder().encode('hello') + ); + }, + }), + new ReadableStream({ start: (c) => c.close() }), + Promise.resolve(0) + ) + ); + }, // Demonstrate logged error after the ignored error // to confirm the logger was working afterward. async () => { @@ -174,7 +195,8 @@ describe('startServer', () => { port: 0, // Each time handleRequest is called, // move on to the next responder in the list. - handleRequest: () => repondersForHandleRequest.shift()!(), + handleRequest: (...args: any[]) => + repondersForHandleRequest.shift()!(...args), async onBind(server, port) { return { server, port } as any; }, From 9022794b54e46b9cb8fc8b86024bd2785cd35a63 Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Wed, 15 Apr 2026 07:33:44 -0400 Subject: [PATCH 2/9] fix: update error handling test to stub .request() instead of .requestStreamed() The handleRequest code was changed to call playgroundPool.request() instead of playgroundPool.requestStreamed(). The test still stubbed .requestStreamed, so the error was never thrown and WordPress returned a 404 instead of 500. --- packages/playground/cli/tests/run-cli.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/playground/cli/tests/run-cli.spec.ts b/packages/playground/cli/tests/run-cli.spec.ts index c55e1d6ed9e..197f3575ce1 100644 --- a/packages/playground/cli/tests/run-cli.spec.ts +++ b/packages/playground/cli/tests/run-cli.spec.ts @@ -1424,7 +1424,7 @@ describe('other run-cli behaviors', () => { const throwAnError = (() => { throw new Error('test error'); }) as any; - cliServer.playground.requestStreamed = throwAnError; + cliServer.playground.request = throwAnError; const response = await fetch(new URL('/', cliServer.serverUrl)); expect(response.status).toBe(500); From af1ea3e559fa271daca9d8b9fc5c8e9fae13ade6 Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Thu, 23 Apr 2026 15:00:26 -0400 Subject: [PATCH 3/9] CI: retrigger Windows test From f6a55bd34fe57c0e3769b30d3f0d6d83e2e33780 Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Thu, 23 Apr 2026 15:29:40 -0400 Subject: [PATCH 4/9] [Pool] Reject queued waiters when pool is drained; stop mutating caller's array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes to createObjectPoolProxy: 1. Deadlock when pool drains to empty. If every instance is removed (e.g. all workers crashed) while a caller is awaiting acquire(), the promise would previously hang forever — no instance will ever be released to wake the queue. Now: removeInstance() fails all queued waiters with a clear error when capacity drops to 0, and acquire() fails fast against an already-empty pool instead of enqueueing. Waiter queue entries now carry both resolve and reject to support this. 2. Caller array mutation. removeInstance() previously called instances.splice() on the caller-provided array, producing side effects visible outside the pool. Now the pool copies the input into an internally-owned allInstances array at construction and only ever mutates that copy. Three new tests cover the new behaviors (mid-flight drain rejects queued waiters; post-drain acquire fails fast; caller's array is left intact after eviction). --- .../src/lib/object-pool-proxy.spec.ts | 51 ++++++++++++++++++ .../universal/src/lib/object-pool-proxy.ts | 52 ++++++++++++++++--- 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts index c2509e5619b..bd9f68ea038 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts @@ -210,4 +210,55 @@ describe('createPoolProxy', () => { expect([first, second].sort()).toEqual(['a', 'b']); }); + + it('does not mutate the caller-provided instances array', () => { + const instances = [{ id: 1 }, { id: 2 }]; + const snapshot = [...instances]; + const proxy = createObjectPoolProxy(instances); + + // Evict both instances through the pool's control API. + // The caller's array must remain intact — only the pool's + // internal copy should be mutated. + proxy.__removeInstance(instances[0]); + proxy.__removeInstance(instances[1]); + + expect(instances).toEqual(snapshot); + }); + + it('rejects queued waiters when the pool is drained to empty', async () => { + const instance = { + async work() { + await new Promise((r) => setTimeout(r, 50)); + return 'done'; + }, + }; + + const proxy = createObjectPoolProxy([instance]); + + // Acquire the only instance, then queue a second call. + const firstCall = proxy.work(); + const queuedCall = proxy.work(); + + // Remove the only instance while the queued call is waiting. + // The queued call must reject with a clear error, not hang. + proxy.__removeInstance(instance); + + await expect(queuedCall).rejects.toThrow(/Pool is empty/); + // The in-flight call still completes normally — removal only + // marks it for discard on release. + await expect(firstCall).resolves.toBe('done'); + }); + + it('rejects fresh acquisitions after the pool is drained', async () => { + const instance = { value: 1 }; + const proxy = createObjectPoolProxy([instance]); + + proxy.__removeInstance(instance); + + // Any subsequent access must fail fast rather than hang + // forever waiting on a pool with zero capacity. + await expect(Promise.resolve(proxy.value)).rejects.toThrow( + /Pool is empty/ + ); + }); }); diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts index 072dac8277d..e1da9693a7a 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts @@ -52,17 +52,28 @@ export function createObjectPoolProxy( throw new Error('At least one instance is required'); } + // Copy the caller's array so pool eviction (via __removeInstance) + // never mutates input the caller may still hold a reference to. + // `freeInstances` and `allInstances` are both owned by the pool. + const allInstances: T[] = [...instances]; const freeInstances: T[] = [...instances]; - const waitQueue: Array<(instance: T) => void> = []; + // Waiters hold both resolve and reject so capacity loss + // (see removeInstance below) can fail them with a clear error + // instead of leaving them hung on acquire() forever. + interface Waiter { + resolve: (instance: T) => void; + reject: (error: Error) => void; + } + const waitQueue: Waiter[] = []; const removedInstances = new Set(); function removeInstance(instance: unknown): void { const inst = instance as T; // Remove from the canonical list - const idx = instances.indexOf(inst); + const idx = allInstances.indexOf(inst); if (idx !== -1) { - instances.splice(idx, 1); + allInstances.splice(idx, 1); } // Remove from the free list if it's currently free @@ -73,6 +84,24 @@ export function createObjectPoolProxy( // Instance is currently busy — mark it for removal on release removedInstances.add(inst); } + + // If the pool has drained to zero capacity, fail any queued + // waiters with a clear error instead of letting them hang + // forever. Without this, a caller awaiting acquire() when the + // last worker dies would deadlock: no instance will ever be + // released back to wake them. + if (allInstances.length === 0 && waitQueue.length > 0) { + const waiters = waitQueue.splice(0, waitQueue.length); + for (const waiter of waiters) { + waiter.reject( + new Error( + 'Pool is empty: all instances have been removed ' + + '(likely because every worker crashed). ' + + 'Pending acquisitions cannot be satisfied.' + ) + ); + } + } } function acquire(): Promise { @@ -80,8 +109,19 @@ export function createObjectPoolProxy( if (free !== undefined) { return Promise.resolve(free); } - return new Promise((resolve) => { - waitQueue.push(resolve); + if (allInstances.length === 0) { + // Pool was drained before this acquire() was called. + // Fail fast instead of pushing onto a queue that will + // never drain. + return Promise.reject( + new Error( + 'Pool is empty: cannot acquire an instance from ' + + 'a pool with zero capacity.' + ) + ); + } + return new Promise((resolve, reject) => { + waitQueue.push({ resolve, reject }); }); } @@ -94,7 +134,7 @@ export function createObjectPoolProxy( const waiter = waitQueue.shift(); if (waiter) { - waiter(instance); + waiter.resolve(instance); } else { freeInstances.push(instance); } From 4dc64386a4a0365e0ee88a14f5889ed74dca87b7 Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Thu, 23 Apr 2026 15:30:09 -0400 Subject: [PATCH 5/9] [PHP] Fix grammar in SinglePHPInstanceManager double-acquire error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'The PHP instance already acquired' → 'The PHP instance is already acquired'. This message surfaces during concurrency failures so tightening it up makes logs easier to parse. --- .../php-wasm/universal/src/lib/single-php-instance-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/php-wasm/universal/src/lib/single-php-instance-manager.ts b/packages/php-wasm/universal/src/lib/single-php-instance-manager.ts index 1f6fe39915e..36548864b20 100644 --- a/packages/php-wasm/universal/src/lib/single-php-instance-manager.ts +++ b/packages/php-wasm/universal/src/lib/single-php-instance-manager.ts @@ -57,7 +57,7 @@ export class SinglePHPInstanceManager implements PHPInstanceManager { async acquirePHPInstance(): Promise { if (this.isAcquired) { throw new Error( - 'The PHP instance already acquired. SinglePHPInstanceManager cannot spawn another PHP instance since, by definition, it only manages a single PHP instance.' + 'The PHP instance is already acquired. SinglePHPInstanceManager cannot spawn another PHP instance since, by definition, it only manages a single PHP instance.' ); } const php = await this.getPrimaryPhp(); From edbfee1ffb2fd43fc5b754c9f6ce28575814c57d Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Thu, 23 Apr 2026 16:51:29 -0400 Subject: [PATCH 6/9] [CLI] Restore streaming responses with lifecycle-aware pool release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 500-under-concurrency fix (a2285ecfe) switched handleRequest from requestStreamed() to buffered request() because the pool proxy released workers when the outer promise resolved — which, for streaming, was when the response *started* streaming (headers available), not when it finished. Concurrent requests could then be routed to a worker still draining its previous body, producing 'PHP instance already acquired' errors. Buffering fixes concurrency but regresses streaming: large bodies are held entirely in memory and SSE flows become request/response. Expose the existing internal withInstance helper on the pool proxy as __withInstance, then scope acquire/release around the full stream drain in run-cli.ts: await playgroundPool.__withInstance(async (instance) => { const response = await instance.requestStreamed(request); ...cookie handling... await streamResponse(response); // Instance released only after streamResponse fully drains. }); This ties the worker lifecycle to the HTTP response lifecycle: the pool holds the instance for the entire duration of a request, exactly as the buffered fix did, while the body still streams directly to the client without intermediate buffering. Tests: 4 new spec cases cover callback-held release semantics (pre-release concurrent calls must queue), sync/async error release, and raw-instance identity. Existing 17 tests unchanged. The Pooled / Pooled> type divergence was previously papered over by the Promisified method-proxy shape; __withInstance's contravariant callback parameter makes it observable. Localized unknown casts keep public signatures unchanged — a broader refactor of the playgroundPool typing is out of scope for this PR. --- .../src/lib/object-pool-proxy.spec.ts | 110 ++++++++++++++++++ .../universal/src/lib/object-pool-proxy.ts | 23 +++- packages/playground/cli/src/run-cli.ts | 64 ++++++---- 3 files changed, 174 insertions(+), 23 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts index bd9f68ea038..fd5626787c9 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts @@ -261,4 +261,114 @@ describe('createPoolProxy', () => { /Pool is empty/ ); }); + + describe('__withInstance', () => { + it('releases the instance only after the callback promise resolves', async () => { + // One instance, one long-running __withInstance call. + // A concurrent proxy call must wait until the callback + // finishes — not until any intermediate promise inside + // the callback resolves. + const accessLog: string[] = []; + const instance = { + async step(label: string) { + accessLog.push(`step-${label}`); + await new Promise((r) => setTimeout(r, 10)); + return label; + }, + }; + + const proxy = createObjectPoolProxy([instance]); + + let callbackDone = false; + const withInstanceCall = proxy.__withInstance(async (inst) => { + // Multiple proxied calls inside a single held instance. + // Simulates streaming where the response starts before + // the body is drained. + await inst.step('first'); + accessLog.push('mid'); + await new Promise((r) => setTimeout(r, 30)); + await inst.step('second'); + callbackDone = true; + return 'withInstance-done'; + }); + + // Start a concurrent proxied call after a short delay. + // It must queue behind the held instance and only run + // after the callback has fully resolved. + const concurrentCall = (async () => { + await new Promise((r) => setTimeout(r, 5)); + accessLog.push('concurrent-enqueued'); + const result = await proxy.step('concurrent'); + // Assert the callback had already completed by the + // time we acquired the instance. + expect(callbackDone).toBe(true); + return result; + })(); + + const [withResult, concResult] = await Promise.all([ + withInstanceCall, + concurrentCall, + ]); + + expect(withResult).toBe('withInstance-done'); + expect(concResult).toBe('concurrent'); + + // The concurrent call was enqueued mid-callback but ran + // only after the callback fully finished. + expect(accessLog).toEqual([ + 'step-first', + 'concurrent-enqueued', + 'mid', + 'step-second', + 'step-concurrent', + ]); + }); + + it('releases the instance when the callback throws async', async () => { + const instance = { + async ok() { + return 'ok'; + }, + }; + + const proxy = createObjectPoolProxy([instance]); + + await expect( + proxy.__withInstance(async () => { + await new Promise((r) => setTimeout(r, 10)); + throw new Error('callback boom'); + }) + ).rejects.toThrow('callback boom'); + + // Instance must be back in the free pool — subsequent + // calls proceed immediately. + expect(await proxy.ok()).toBe('ok'); + }); + + it('releases the instance when the callback throws sync', async () => { + const instance = { + async ok() { + return 'ok'; + }, + }; + + const proxy = createObjectPoolProxy([instance]); + + await expect( + proxy.__withInstance(() => { + throw new Error('sync boom'); + }) + ).rejects.toThrow('sync boom'); + + expect(await proxy.ok()).toBe('ok'); + }); + + it('passes the raw instance, not a proxy', async () => { + const instance = { marker: Symbol('raw-instance'), value: 1 }; + const proxy = createObjectPoolProxy([instance]); + + const received = await proxy.__withInstance(async (inst) => inst); + expect(received).toBe(instance); + }); + }); }); diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts index e1da9693a7a..20ea663e175 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts @@ -34,6 +34,23 @@ export type Pooled = Omit< * requests to a dead worker. */ __removeInstance(instance: unknown): void; + + /** + * Acquire an instance, invoke `fn` with it, and release the + * instance only after `fn`'s returned promise resolves (or + * throws). Use this when a single logical unit of work spans + * multiple proxied calls or outlives a single method invocation + * — e.g. a streaming HTTP response where PHP execution must + * remain bound to one worker until the stream fully drains. + * + * The default per-call acquire/release semantics of the proxy + * release the instance as soon as the proxied method's promise + * resolves, which is too early for streamed responses: the + * response object is returned before the body finishes + * streaming, so the worker would be handed to a concurrent + * request mid-stream. + */ + __withInstance(fn: (instance: T) => R | Promise): Promise; }; /** @@ -167,9 +184,11 @@ export function createObjectPoolProxy( } const proxyTarget = {} as Pooled; - // Expose __removeInstance directly on the target so it's found - // by the `prop in _target` check before hitting the proxy trap. + // Expose __removeInstance and __withInstance directly on the + // target so they're found by the `prop in _target` check before + // hitting the proxy trap. (proxyTarget as any).__removeInstance = removeInstance; + (proxyTarget as any).__withInstance = withInstance; return new Proxy(proxyTarget, { get(_target, prop: string | symbol) { diff --git a/packages/playground/cli/src/run-cli.ts b/packages/playground/cli/src/run-cli.ts index bf6f3224673..8f7912317b5 100644 --- a/packages/playground/cli/src/run-cli.ts +++ b/packages/playground/cli/src/run-cli.ts @@ -1579,12 +1579,20 @@ export async function runCLI(args: RunCLIArgs): Promise { } await Promise.all(promisesToBoot); + // The pool is created over comlink `RemoteAPI` + // instances, but `playgroundPool` is typed as `Pooled` + // to keep the method-proxy ergonomics used by callers elsewhere + // (e.g. `playgroundPool.request(...)`, `playgroundPool.fileExists(...)`). + // The two `Pooled<>` shapes were structurally compatible before + // `__withInstance` landed; the callback's instance type now makes + // the T difference observable. An `unknown` cast localizes the + // known-safe widening without churning public signatures. playgroundPool = createObjectPoolProxy( spawnedWorkers.map( (spawnedWorker) => workerToPlaygroundMap.get(spawnedWorker)! ) - ); + ) as unknown as Pooled; // NOTE: Using a free-standing block to isolate initial boot vars // while keeping the logic inline. @@ -1790,27 +1798,41 @@ export async function runCLI(args: RunCLIArgs): Promise { }; } - // Use the non-streaming request() method through the pool proxy. - // This ensures the pool holds the worker for the entire PHP - // execution lifecycle. The pool proxy releases the worker when - // the method's promise resolves — with requestStreamed(), that - // happens when the response *starts* streaming (before PHP - // finishes), causing "PHP instance already acquired" errors - // on concurrent requests. Using request() instead means the - // worker is only released after PHP has fully completed. - const response = await playgroundPool.request(request); - - if (cookieStore) { - cookieStore.rememberCookiesFromResponseHeaders( - response.headers - ); - // While we have an internal cookie store, we filter out the - // Set-Cookie headers from responses so the browser does not - // attempt to manage cookies at the same time as the server. - delete response.headers['set-cookie']; - } + // Scope the acquire/release around the full stream drain via + // __withInstance. The pool proxy's default per-call semantics + // release the worker when requestStreamed()'s promise resolves + // — which is when the response *starts* streaming, before PHP + // finishes writing the body. That caused "PHP instance already + // acquired" errors under concurrency. Holding the pool instance + // until streamResponse() fully drains ties the worker lifecycle + // to the HTTP response lifecycle, which restores true streaming + // for large responses and SSE without regressing concurrency. + // + // The `instance` cast is needed because `playgroundPool` is + // typed as `Pooled` for the method-proxy + // ergonomics used elsewhere, but the underlying instances are + // comlink `RemoteAPI` proxies. Calling + // methods on the raw instance requires the RemoteAPI shape. + await playgroundPool.__withInstance(async (rawInstance) => { + const instance = + rawInstance as unknown as RemoteAPI; + const response = await instance.requestStreamed(request); + + if (cookieStore) { + const headers = await response.headers; + cookieStore.rememberCookiesFromResponseHeaders(headers); + // While we have an internal cookie store, we filter out + // the Set-Cookie headers from responses so the browser + // does not attempt to manage cookies at the same time + // as the server. + delete headers['set-cookie']; + } - await streamResponse(StreamedPHPResponse.fromPHPResponse(response)); + // The callback must not resolve until streamResponse has + // fully drained — that is what keeps the worker bound to + // this request for the entire response lifecycle. + await streamResponse(response); + }); }, }).catch((error) => { cliOutput.printError(describeError(error)); From 18bda2d3d359280a493ccc6651dfc6bc780ba6eb Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Thu, 23 Apr 2026 17:08:47 -0400 Subject: [PATCH 7/9] [CLI] Retype playgroundPool to reflect remote-proxy instances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pool is constructed from RemoteAPI instances (comlink proxies over MessagePort), but was declared as Pooled. Before __withInstance, the two shapes were structurally compatible because Promisified only exposed method signatures — and RemoteAPI mirrors T's method surface with promisified returns. __withInstance's callback parameter puts T in a contravariant position, which makes the structural difference observable. The previous commit resolved this with two localized casts at the assignment site and inside the __withInstance callback. This commit replaces both with honest local typing: playgroundPool, the RunCLIServer.playground public shape, zipSite(), and both BlueprintsV{1,2}Handler.bootWordPress() signatures all move to Pooled>. The hot path (handleRequest + __withInstance callback) is now cast-free, and future lifecycle-hook additions to Pooled will type-check without per-site widening. The casts don't disappear entirely — they migrate outward to the cross-package boundary with @wp-playground/blueprints and xdebug-bridge, which expect UniversalPHP (= LimitedPHPApi | Remote<...> | Pooled<...>). UniversalPHP's Pooled arm shares the same underlying lie (the pool never holds raw LimitedPHPApi, only remote proxies to it), but widening it is a public-type change across ~100 call sites in blueprints, wordpress, and other packages — worth a dedicated PR rather than folding into a streaming-fix. A follow-up issue will track the UniversalPHP cleanup. Net: same number of casts, but they now live at the honest cross-package boundary instead of inside this PR's own additions. --- .../blueprints-v1/blueprints-v1-handler.ts | 3 +- .../blueprints-v2/blueprints-v2-handler.ts | 2 +- packages/playground/cli/src/run-cli.ts | 50 ++++++++++--------- 3 files changed, 29 insertions(+), 26 deletions(-) diff --git a/packages/playground/cli/src/blueprints-v1/blueprints-v1-handler.ts b/packages/playground/cli/src/blueprints-v1/blueprints-v1-handler.ts index a30ca4e01b0..e933fca895d 100644 --- a/packages/playground/cli/src/blueprints-v1/blueprints-v1-handler.ts +++ b/packages/playground/cli/src/blueprints-v1/blueprints-v1-handler.ts @@ -4,6 +4,7 @@ import { consumeAPI, isLegacyPHPVersion, type Pooled, + type RemoteAPI, type UniversalPHP, } from '@php-wasm/universal'; import type { BlueprintV1Declaration } from '@wp-playground/blueprints'; @@ -61,7 +62,7 @@ export class BlueprintsV1Handler { } async bootWordPress( - playground: Pooled, + playground: Pooled>, workerPostInstallMountsPort: NodeMessagePort ) { let wpDetails: any = undefined; diff --git a/packages/playground/cli/src/blueprints-v2/blueprints-v2-handler.ts b/packages/playground/cli/src/blueprints-v2/blueprints-v2-handler.ts index 31a257dba15..4bff9739c7a 100644 --- a/packages/playground/cli/src/blueprints-v2/blueprints-v2-handler.ts +++ b/packages/playground/cli/src/blueprints-v2/blueprints-v2-handler.ts @@ -45,7 +45,7 @@ export class BlueprintsV2Handler { } async bootWordPress( - playground: Pooled, + playground: Pooled>, workerPostInstallMountsPort: NodeMessagePort ) { const workerBootArgs = { diff --git a/packages/playground/cli/src/run-cli.ts b/packages/playground/cli/src/run-cli.ts index 8f7912317b5..2450c47bd78 100644 --- a/packages/playground/cli/src/run-cli.ts +++ b/packages/playground/cli/src/run-cli.ts @@ -7,6 +7,7 @@ import { type PathAlias, type RemoteAPI, type AllPHPVersion, + type UniversalPHP, } from '@php-wasm/universal'; import { PHPResponse, @@ -944,7 +945,7 @@ export type PlaygroundCliWorker = export const internalsKeyForTesting = Symbol('playground-cli-testing'); export interface RunCLIServer extends AsyncDisposable { - playground: Pooled; + playground: Pooled>; server: Server; serverUrl: string; @@ -989,7 +990,7 @@ export async function runCLI( ): Promise; export async function runCLI(args: RunCLIArgs): Promise; export async function runCLI(args: RunCLIArgs): Promise { - let playgroundPool: Pooled; + let playgroundPool: Pooled>; const cookieStore = args.internalCookieStore ? new HttpCookieStore() : undefined; @@ -1579,20 +1580,12 @@ export async function runCLI(args: RunCLIArgs): Promise { } await Promise.all(promisesToBoot); - // The pool is created over comlink `RemoteAPI` - // instances, but `playgroundPool` is typed as `Pooled` - // to keep the method-proxy ergonomics used by callers elsewhere - // (e.g. `playgroundPool.request(...)`, `playgroundPool.fileExists(...)`). - // The two `Pooled<>` shapes were structurally compatible before - // `__withInstance` landed; the callback's instance type now makes - // the T difference observable. An `unknown` cast localizes the - // known-safe widening without churning public signatures. playgroundPool = createObjectPoolProxy( spawnedWorkers.map( (spawnedWorker) => workerToPlaygroundMap.get(spawnedWorker)! ) - ) as unknown as Pooled; + ); // NOTE: Using a free-standing block to isolate initial boot vars // while keeping the logic inline. @@ -1631,6 +1624,19 @@ export async function runCLI(args: RunCLIArgs): Promise { wordPressReady = true; + // Casts to UniversalPHP at the blueprint/bridge handoff + // points: UniversalPHP is defined as + // `Pooled` (among other arms), but the + // runtime pool holds `RemoteAPI` + // instances. The two shapes expose the same methods + // through the proxy (both produce promises), but + // `Pooled` now reveals T via `__withInstance`'s + // callback — making the structural difference + // observable. Widening `UniversalPHP` to allow + // `Pooled>` is a cross-package + // public-type change worth a dedicated PR; these + // localized casts preserve the honest local typing + // without churning the blueprints API. if (!args['experimental-blueprints-v2-runner']) { const compiledBlueprint = await ( handler as BlueprintsV1Handler @@ -1641,7 +1647,7 @@ export async function runCLI(args: RunCLIArgs): Promise { if (compiledBlueprint) { await runBlueprintV1Steps( compiledBlueprint, - playgroundPool + playgroundPool as unknown as UniversalPHP ); } } @@ -1655,7 +1661,10 @@ export async function runCLI(args: RunCLIArgs): Promise { ) { const steps = await getPhpMyAdminInstallSteps(); const compiled = await compileBlueprintV1({ steps }); - await runBlueprintV1Steps(compiled, playgroundPool); + await runBlueprintV1Steps( + compiled, + playgroundPool as unknown as UniversalPHP + ); } if (args.command === 'build-snapshot') { @@ -1715,7 +1724,8 @@ export async function runCLI(args: RunCLIArgs): Promise { if (args.xdebug && args.experimentalDevtools) { const bridge = await startBridge({ - phpInstance: playgroundPool, + // See the UniversalPHP cast rationale above. + phpInstance: playgroundPool as unknown as UniversalPHP, phpRoot: '/wordpress', }); @@ -1807,15 +1817,7 @@ export async function runCLI(args: RunCLIArgs): Promise { // until streamResponse() fully drains ties the worker lifecycle // to the HTTP response lifecycle, which restores true streaming // for large responses and SSE without regressing concurrency. - // - // The `instance` cast is needed because `playgroundPool` is - // typed as `Pooled` for the method-proxy - // ergonomics used elsewhere, but the underlying instances are - // comlink `RemoteAPI` proxies. Calling - // methods on the raw instance requires the RemoteAPI shape. - await playgroundPool.__withInstance(async (rawInstance) => { - const instance = - rawInstance as unknown as RemoteAPI; + await playgroundPool.__withInstance(async (instance) => { const response = await instance.requestStreamed(request); if (cookieStore) { @@ -2093,7 +2095,7 @@ function openInBrowser(url: string): void { } async function zipSite( - playground: Pooled, + playground: Pooled>, outfile: string ) { await playground.run({ From a9acd75f07ee40c88cc3f383ba70366db626607b Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Thu, 23 Apr 2026 17:27:58 -0400 Subject: [PATCH 8/9] fix: update error-handling test stub to target __withInstance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error-handling test (tests/run-cli.spec.ts:'should return 500 when the request handler throws an error') stubs the entry point that handleRequest calls into, relying on the stub to throw and the Express error path to surface a 500. The test has now tracked three call sites: - Original streaming path → stubbed .requestStreamed() - Buffered-fix (a2285ecfe) → stubbed .request() (commit 9022794b5) - Lifecycle-scoped streaming revival (this PR) → now stubs .__withInstance() handleRequest no longer calls .request() or .requestStreamed() on the pool proxy directly; both are invoked on the raw instance inside a .__withInstance() callback. Stubbing .__withInstance() is the equivalent seam — it intercepts the entire acquire/invoke/release cycle and fails fast, which is what the test wants. CI signal for this change: test-playground-cli (macos, ubuntu) reported 'expected 404 to be 500' on the previous commit because the stale .request() stub was being ignored. --- packages/playground/cli/tests/run-cli.spec.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/playground/cli/tests/run-cli.spec.ts b/packages/playground/cli/tests/run-cli.spec.ts index 197f3575ce1..847b8805df6 100644 --- a/packages/playground/cli/tests/run-cli.spec.ts +++ b/packages/playground/cli/tests/run-cli.spec.ts @@ -1424,7 +1424,13 @@ describe('other run-cli behaviors', () => { const throwAnError = (() => { throw new Error('test error'); }) as any; - cliServer.playground.request = throwAnError; + // handleRequest now scopes acquire/release through + // __withInstance to bind the worker lifecycle to the full + // response stream. Stub that entry point so the request + // handler sees an error and surfaces a 500. (Previously we + // stubbed .request() for the buffered path, and before that + // .requestStreamed() for the original streaming path.) + (cliServer.playground as any).__withInstance = throwAnError; const response = await fetch(new URL('/', cliServer.serverUrl)); expect(response.status).toBe(500); From 64a8f0275ccb719cad314fee6f94797c660ba725 Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Fri, 24 Apr 2026 08:20:14 -0400 Subject: [PATCH 9/9] [Pool] Remove speculative dead-worker eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strip __removeInstance and the onExit→eviction wiring. The eviction code was defensive against a hypothetical case where a CLI worker thread exits unexpectedly post-startup — but that was never observed in practice, and the EADDRINUSE example in the original PR body was pattern-matched from an unrelated @php-wasm/node networking-proxy leak that doesn't touch the CLI worker pool. Per maintainer feedback (brandonpayton on #3494): worker crash recovery is a separate concern worth its own PR backed by real crash evidence, and 'fail startup altogether' may be the better answer for the startup case anyway. This PR now only addresses the observed pool-release-timing bug via __withInstance. Also removes the pool-drain/capacity-zero bookkeeping that only existed as a consequence of eviction: the removedInstances Set, the release() early-discard path, the acquire() fast-fail on empty pool, and the {resolve, reject} waiter shape (no caller rejects anymore). The internal allInstances copy is also gone — nothing mutates the caller's array now that eviction is out, so the defensive copy is unnecessary. Removes 3 eviction-related tests from the spec. The 4 __withInstance tests (added in edbfee1f) stay — they cover the streaming lifecycle fix directly. --- .../src/lib/object-pool-proxy.spec.ts | 51 ---------- .../universal/src/lib/object-pool-proxy.ts | 92 ++----------------- packages/playground/cli/src/run-cli.ts | 19 +--- 3 files changed, 7 insertions(+), 155 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts index fd5626787c9..473271e59cb 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.spec.ts @@ -211,57 +211,6 @@ describe('createPoolProxy', () => { expect([first, second].sort()).toEqual(['a', 'b']); }); - it('does not mutate the caller-provided instances array', () => { - const instances = [{ id: 1 }, { id: 2 }]; - const snapshot = [...instances]; - const proxy = createObjectPoolProxy(instances); - - // Evict both instances through the pool's control API. - // The caller's array must remain intact — only the pool's - // internal copy should be mutated. - proxy.__removeInstance(instances[0]); - proxy.__removeInstance(instances[1]); - - expect(instances).toEqual(snapshot); - }); - - it('rejects queued waiters when the pool is drained to empty', async () => { - const instance = { - async work() { - await new Promise((r) => setTimeout(r, 50)); - return 'done'; - }, - }; - - const proxy = createObjectPoolProxy([instance]); - - // Acquire the only instance, then queue a second call. - const firstCall = proxy.work(); - const queuedCall = proxy.work(); - - // Remove the only instance while the queued call is waiting. - // The queued call must reject with a clear error, not hang. - proxy.__removeInstance(instance); - - await expect(queuedCall).rejects.toThrow(/Pool is empty/); - // The in-flight call still completes normally — removal only - // marks it for discard on release. - await expect(firstCall).resolves.toBe('done'); - }); - - it('rejects fresh acquisitions after the pool is drained', async () => { - const instance = { value: 1 }; - const proxy = createObjectPoolProxy([instance]); - - proxy.__removeInstance(instance); - - // Any subsequent access must fail fast rather than hang - // forever waiting on a pool with zero capacity. - await expect(Promise.resolve(proxy.value)).rejects.toThrow( - /Pool is empty/ - ); - }); - describe('__withInstance', () => { it('releases the instance only after the callback promise resolves', async () => { // One instance, one long-running __withInstance call. diff --git a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts index 20ea663e175..18a73db1d7d 100644 --- a/packages/php-wasm/universal/src/lib/object-pool-proxy.ts +++ b/packages/php-wasm/universal/src/lib/object-pool-proxy.ts @@ -23,18 +23,6 @@ export type Pooled = Omit< Promisified, typeof Symbol.dispose | typeof Symbol.asyncDispose > & { - /** - * Remove an instance from the pool. If the instance is currently - * free, it is removed immediately. If it is busy (acquired by a - * pending call), it will be discarded when that call releases it - * instead of being returned to the free list. - * - * This is useful when a worker thread backing a pool instance - * crashes — removing it prevents the pool from routing new - * requests to a dead worker. - */ - __removeInstance(instance: unknown): void; - /** * Acquire an instance, invoke `fn` with it, and release the * instance only after `fn`'s returned promise resolves (or @@ -69,89 +57,23 @@ export function createObjectPoolProxy( throw new Error('At least one instance is required'); } - // Copy the caller's array so pool eviction (via __removeInstance) - // never mutates input the caller may still hold a reference to. - // `freeInstances` and `allInstances` are both owned by the pool. - const allInstances: T[] = [...instances]; const freeInstances: T[] = [...instances]; - // Waiters hold both resolve and reject so capacity loss - // (see removeInstance below) can fail them with a clear error - // instead of leaving them hung on acquire() forever. - interface Waiter { - resolve: (instance: T) => void; - reject: (error: Error) => void; - } - const waitQueue: Waiter[] = []; - const removedInstances = new Set(); - - function removeInstance(instance: unknown): void { - const inst = instance as T; - - // Remove from the canonical list - const idx = allInstances.indexOf(inst); - if (idx !== -1) { - allInstances.splice(idx, 1); - } - - // Remove from the free list if it's currently free - const freeIdx = freeInstances.indexOf(inst); - if (freeIdx !== -1) { - freeInstances.splice(freeIdx, 1); - } else { - // Instance is currently busy — mark it for removal on release - removedInstances.add(inst); - } - - // If the pool has drained to zero capacity, fail any queued - // waiters with a clear error instead of letting them hang - // forever. Without this, a caller awaiting acquire() when the - // last worker dies would deadlock: no instance will ever be - // released back to wake them. - if (allInstances.length === 0 && waitQueue.length > 0) { - const waiters = waitQueue.splice(0, waitQueue.length); - for (const waiter of waiters) { - waiter.reject( - new Error( - 'Pool is empty: all instances have been removed ' + - '(likely because every worker crashed). ' + - 'Pending acquisitions cannot be satisfied.' - ) - ); - } - } - } + const waitQueue: Array<(instance: T) => void> = []; function acquire(): Promise { const free = freeInstances.shift(); if (free !== undefined) { return Promise.resolve(free); } - if (allInstances.length === 0) { - // Pool was drained before this acquire() was called. - // Fail fast instead of pushing onto a queue that will - // never drain. - return Promise.reject( - new Error( - 'Pool is empty: cannot acquire an instance from ' + - 'a pool with zero capacity.' - ) - ); - } - return new Promise((resolve, reject) => { - waitQueue.push({ resolve, reject }); + return new Promise((resolve) => { + waitQueue.push(resolve); }); } function release(instance: T): void { - // If this instance was marked for removal while busy, discard it - if (removedInstances.has(instance)) { - removedInstances.delete(instance); - return; - } - const waiter = waitQueue.shift(); if (waiter) { - waiter.resolve(instance); + waiter(instance); } else { freeInstances.push(instance); } @@ -184,10 +106,8 @@ export function createObjectPoolProxy( } const proxyTarget = {} as Pooled; - // Expose __removeInstance and __withInstance directly on the - // target so they're found by the `prop in _target` check before - // hitting the proxy trap. - (proxyTarget as any).__removeInstance = removeInstance; + // Expose __withInstance directly on the target so it's found by + // the `prop in _target` check before hitting the proxy trap. (proxyTarget as any).__withInstance = withInstance; return new Proxy(proxyTarget, { diff --git a/packages/playground/cli/src/run-cli.ts b/packages/playground/cli/src/run-cli.ts index 2450c47bd78..842cbed83ce 100644 --- a/packages/playground/cli/src/run-cli.ts +++ b/packages/playground/cli/src/run-cli.ts @@ -1517,24 +1517,7 @@ export async function runCLI(args: RunCLIArgs): Promise { logger.error( `Worker ${workerIndex} exited with code ${exitCode}\n` ); - - // Remove the dead worker's API proxy from the pool - // so new requests are not routed to it. - if (playgroundPool) { - const deadWorker = spawnedWorkers[workerIndex]; - if (deadWorker) { - const deadApi = - workerToPlaygroundMap.get(deadWorker); - if (deadApi) { - playgroundPool.__removeInstance( - deadApi - ); - logger.error( - `Worker ${workerIndex} removed from pool\n` - ); - } - } - } + // @TODO: Should we respawn the worker if it exited with an error and the CLI is not shutting down? }, }).then( async (