fix(world-postgres): preserve queue error metadata - #4117
Conversation
Signed-off-by: Dmitry Petrov <Komly@yandex.ru>
🦋 Changeset detectedLatest commit: a332cb4 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. Includes a patch changeset and a signed DCO commit. The real PostgreSQL/Graphile/HTTP subprocess regression reproduces the lost metadata on main and passes with this fix; CLI JSON-mode suppression, all 27 source tests, and the package build pass locally. |
Review follow-up. The replacer recursed through `cause` with no cycle guard, so a cyclic cause chain overflowed the stack inside JSON.stringify, from inside the logger. Already-expanded errors are now replaced by a marker, and AggregateError.errors is carried too. The serializer is exported for direct unit tests; the integration test always uses Testcontainers. 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: blocking issues found
One blocking issue in the original replacer, fixed in a follow-up commit (a332cb4) pushed to this branch.
| export function serializeGraphileMeta(meta: unknown): string { | ||
| const seen = new WeakSet<object>(); | ||
| const expandError = (error: Error): Record<string, unknown> => { | ||
| if (seen.has(error)) { |
There was a problem hiding this comment.
AI Review: Blocking
The original replacer returned { ...value, name, message, stack, cause: value.cause }, and JSON.stringify recurses into the returned object, so a cyclic cause chain (a.cause = b; b.cause = a) overflows the stack (verified: RangeError: Maximum call stack size exceeded). That throw happens inside Graphile Worker's logger, which has no fallback, so a delivery failure with such an error would take the worker's error path down with it. Rare in practice, but the whole point of this change is the error path, and the guard is cheap. The follow-up tracks expanded errors in a WeakSet and replaces a repeat with { name, message, repeated: true }; it also carries AggregateError.errors, which the spread does not copy. serializeGraphileMeta is exported so the unit test in src/queue-logging.test.ts can cover the cycle, the cause recursion, and enumerable code retention directly, without a subprocess.
| return; | ||
| } | ||
|
|
||
| let container: Awaited<ReturnType<PostgreSqlContainer['start']>>; |
There was a problem hiding this comment.
AI Review: Nit
Same as on #4114: the WORKFLOW_POSTGRES_URL branch threw on a non-loopback URL, so a dev database in the shell environment would fail the suite instead of falling back to the container the package's other integration tests use. The follow-up makes this Testcontainers-only and drops the README section. The subprocess approach itself is worth keeping: it is the only test here that proves Graphile actually passes the Error in meta rather than a string, which the unit test cannot show.
2 participants
Description
Fixes #4116.
Preserve
Errorfields in Graphile Worker log metadata. A real failed HTTP delivery currently produces"error": {}in the JSON metadata, even though Graphile's preceding text contains the top-level message and stack. The JSON replacer now includes the name, message, stack, and nested cause while retaining enumerable properties such as transport error codes.The existing stderr/stdout routing,
DEBUGfiltering, andWORKFLOW_JSON_MODE=1suppression are preserved. The README documents the logging behavior. No runtime dependency or public API is added.How did you test your changes?
{}and passes the JSON-mode case.git diff --checkpassed.The integration tests used PostgreSQL 18.2 on loopback, creating and removing an isolated database. They use Testcontainers by default or an explicit loopback
WORKFLOW_POSTGRES_URL. No PostgreSQL, worker, HTTP, logger, or stderr mock is added. Build the package before running the integration test because the subprocess imports its compiled queue, matching the existing package conformance tests.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 ·
+9 / -0A release note and package guide describe error metadata and existing logging controls.
Implementation — 1 file ·
+14 / -2Serializes standard Error fields and nested causes while retaining enumerable diagnostic properties.
Tests — 2 files ·
+143 / -0Uses real PostgreSQL, Graphile, HTTP, and subprocess output to verify failed-delivery metadata and CLI JSON-mode suppression.