Skip to content

fix(server): preserve main worker responses after request cancellation - #751

Open
luizfelmach wants to merge 1 commit into
supabase:mainfrom
luizfelmach:fix/preserve-main-worker-response
Open

luizfelmach wants to merge 1 commit into
supabase:mainfrom
luizfelmach:fix/preserve-main-worker-response

Conversation

@luizfelmach

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix.

What is the current behavior?

When a user worker terminates during a request, its teardown can cancel the request token shared with the main worker. If cancellation happens before the server processes the main worker's response, the server discards that response and returns an empty 503 with x-served-by: base/server.

For example, a CPU-limit failure can produce an empty 503 instead of the main worker's JSON error response. The result depends on the ordering of teardown and response delivery. Existing CPU-limit integration tests accept both outcomes.

What is the new behavior?

The server preserves a response successfully returned by the main worker, including its status, headers, and body, even when the request token has already been cancelled. The cancellation-based 503 fallback now applies only when obtaining the main worker's response also failed.

Regression coverage includes:

  • A deterministic test that cancels the token before delivering the main worker's response and verifies the complete streamed body.
  • Tests for request cleanup and cancellation on client disconnect.
  • Integration coverage for CPU limits, module-scope exceptions, unhandled rejections, and requests using a previously failed worker handle, with and without connection reuse.
  • Stricter existing CPU-limit assertions that reject the empty 503 outcome.

Additional context

The cancellation path was traced from DenoRuntime::drop through the task in op_net_accept to the shared request token. The fix changes the server's response selection; worker teardown remains active.

Validation on the original v1.76.2-based branch, before cherry-picking onto main:

  • 36 integration tests and 3 server unit tests passed; one existing integration test remained ignored.
  • The deterministic regression test failed with the original condition, passed with the fix, and failed again when only that condition was reverted.
  • Direct HTTP tests using examples/main recorded 208 empty 503 responses in 2,200 requests before the fix and none in 4,200 requests after it. These covered CPU, memory, wall-clock and exception failures, connection reuse, and runtime configuration variants.
  • The fixed matrix included two 15-second client timeouts during concurrent builds. A 200-request repeat of the affected profiles with a 60-second client deadline returned the expected responses throughout.
  • The repository Dockerfile built successfully, and 40 additional requests against the release image returned the expected mapped errors.

Temporary diagnostic instrumentation was removed. The cherry-pick onto main applied without conflicts; the test suite has not been rerun on that base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant