fix(world-postgres): remove implicit queue HTTP deadlines - #4114
Conversation
Signed-off-by: Dmitry Petrov <Komly@yandex.ru>
🦋 Changeset detectedLatest commit: 79e5166 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@komly is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
@vercel/workflow Ready for review. The patch changeset and signed DCO commit are included. Real Graphile/PostgreSQL/HTTP regressions fail on the baseline and pass with the transport change; package build, source tests, and focused cancellation checks pass locally. |
…eue-owned pool Review follow-up. The deadlines are read from WORKFLOW_POSTGRES_HEADERS_TIMEOUT_MS / WORKFLOW_POSTGRES_BODY_TIMEOUT_MS (default 0, unbounded) and documented, mirroring world-local's WORKFLOW_LOCAL_*_TIMEOUT_MS, so an operator can still bound a hung handler. Requests dispatch on keep-alive agents the queue creates and destroys in close() rather than Node's process-global agent. The integration test always uses Testcontainers, like the package's other integration tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
I pushed a follow-up commit (79e5166) to this branch addressing the notes below, so the diff now includes both.
| } as any); | ||
| // Queue shutdown aborts the delivery through Graphile's signal; the | ||
| // deadlines are the operator's (see `getDeliveryTimeouts`). | ||
| const response = await nodeHttpFetch( |
There was a problem hiding this comment.
AI Review: Note
The transport swap is the right fix, and I verified the one open question about it: nodeHttpFetch without agents dispatches on http.globalAgent, which since Node 19 carries timeout: 5000. A 7s header delay still completes on both the global and a dedicated agent (the socket timeout event only notifies; nothing destroys the request), so that timeout was never a correctness problem. The follow-up commit still moves deliveries onto a keep-alive pool the queue creates and destroys in close(), so its sockets do not sit in the process-global agent shared with the application.
| * than hold its worker slot until restart sets these to a value above the | ||
| * longest inline step they expect. | ||
| */ | ||
| export function getDeliveryTimeouts() { |
There was a problem hiding this comment.
AI Review: Note
The original commit hard-coded headersTimeoutMs: 0, bodyTimeoutMs: 0. Unbounded is the right default (the issue's evidence is exactly that any bound below the longest inline step causes double execution), but with no override an operator has no way to prefer redelivery of a hung handler over holding a worker slot until restart. world-local exposes this knob as WORKFLOW_LOCAL_HEADERS_TIMEOUT_MS / WORKFLOW_LOCAL_BODY_TIMEOUT_MS; the follow-up adds WORKFLOW_POSTGRES_HEADERS_TIMEOUT_MS / WORKFLOW_POSTGRES_BODY_TIMEOUT_MS with the same 0 = disabled semantics, documented in docs/content/worlds/v5/postgres.mdx, plus a unit test that a 50ms operator deadline fails a stalled delivery with ETIMEDOUT and schedules no replacement job.
| let attempts: string[] = []; | ||
|
|
||
| beforeAll(async () => { | ||
| container = await new PostgreSqlContainer('postgres:15-alpine').start(); |
There was a problem hiding this comment.
AI Review: Nit
The original test branched on WORKFLOW_POSTGRES_URL and threw if that URL was not loopback. Anyone with that variable set in their shell (pointing at a dev database) would get a hard failure instead of the container the package's other integration tests use. The follow-up makes it Testcontainers-only, which also removes the separate admin pool and CREATE DATABASE step, and drops the README paragraph documenting the loopback path. The global-dispatcher trick with a 10ms undici deadline is a good way to prove the delivery no longer goes through fetch without waiting five minutes; kept as is.
|
(AI) CI note on 79e5166: Biome, Build, and Unit Tests (ubuntu + windows) pass. The Vercel-backed E2E lanes fail because a fork PR's deployments need maintainer authorization, which is expected. Two of the 28 Local Postgres lanes (astro - stable quickjs, nuxt - stable node) failed on |
2 participants
Description
Fixes #3811.
Deliver Graphile queue messages with the existing
@workflow/world/node-http.jsclient and explicit zero headers/body deadlines. Inline work can now finish without global fetch's implicit deadline failing and redelivering a still-running call. The Graphile abort signal remains authoritative during shutdown.No runtime dependency is added. The catalog-pinned undici dev dependency configures a real short-deadline global dispatcher in the regression, reproducing the timeout without a five-minute test. Existing fetch-specific unit cases now use actual loopback HTTP requests and retain their serialization, namespace, health-check, base-path, and cancellation assertions.
How did you test your changes?
git diff --checkpassed.The integration tests used PostgreSQL 18.2 on loopback, with an isolated database created and removed by the test. They use Testcontainers by default or an explicit loopback
WORKFLOW_POSTGRES_URL. No mocked HTTP server, PostgreSQL pool, or Graphile worker is added.PR Checklist - Required to merge
pnpm changeset status --since=mainpasses.@vercel/workflowin a comment once the PR is ready, and the above checklist is complete.Diff size
Docs — 2 files ·
+14 / -0A release note and package guide explain the HTTP delivery lifetime and real PostgreSQL test setup.
Implementation — 3 files ·
+18 / -7Uses the existing Node HTTP client with explicit deadlines disabled and adds the catalog-pinned test dependency and lockfile entry.
Tests — 2 files ·
+201 / -41Covers delayed headers and bodies with real Graphile, PostgreSQL, and HTTP, and replaces fetch-specific unit stubs with loopback requests.