feat(server): Gracefully handle oversized batch items instead of aborting the stream#3137
Conversation
…over mulitple chunks instead of in 1 chunk
🦋 Changeset detectedLatest commit: 8117721 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
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 |
WalkthroughThe PR adds graceful handling for oversized NDJSON batch items by emitting an OversizedItemMarker instead of aborting the stream and by skipping the remainder of oversized lines that span chunks. It exports OversizedItemMarker and extractIndexAndTask, updates NDJSON parsing and stream processing to enqueue pre-failed batch items with PAYLOAD_TOO_LARGE, and records pre-failed runs via TriggerFailedTaskService. It also adds PAYLOAD_TOO_LARGE to error enums and non-retryable codes, normalizes null/undefined payload serialization, adjusts span/triggered-runs presentation, and updates related tests. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ericallam
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/test/engine/streamBatchItems.test.ts (1)
3-15: Consider documenting why mocks are necessary here.The mocks for
~/db.serverand~/services/platform.v3.serverappear to be required for module resolution to prevent indirect imports ofenv.server.ts. While the actual container tests correctly use real Prisma from testcontainers, a brief comment explaining this necessity would help future maintainers understand this pattern.📝 Suggested documentation
+// These mocks are required to prevent module loading errors from indirect +// imports of env.server.ts. The actual container tests use real Prisma +// and Redis passed via the containerTest callback. // Mock the db prisma client - needs to be before other imports vi.mock("~/db.server", () => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/engine/streamBatchItems.test.ts` around lines 3 - 15, Add a short comment above the two vi.mock calls in streamBatchItems.test.ts explaining that these mocks prevent indirect imports of env.server.ts during module resolution in unit tests (so the test runner doesn't load real Prisma or environment-only code), and note that integration/container tests still use real Prisma from testcontainers; reference the mocked modules '~/db.server' and '~/services/platform.v3.server' and the fact that getEntitlement is being stubbed to make the intent clear for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/test/engine/streamBatchItems.test.ts`:
- Around line 3-15: Add a short comment above the two vi.mock calls in
streamBatchItems.test.ts explaining that these mocks prevent indirect imports of
env.server.ts during module resolution in unit tests (so the test runner doesn't
load real Prisma or environment-only code), and note that integration/container
tests still use real Prisma from testcontainers; reference the mocked modules
'~/db.server' and '~/services/platform.v3.server' and the fact that
getEntitlement is being stubbed to make the intent clear for future maintainers.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/test/engine/streamBatchItems.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: typecheck / typecheck
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Test files should only import classes and functions from
app/**/*.tsfiles and should not importenv.server.tsdirectly or indirectly; pass configuration through options instead
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx,js}: Never mock anything in tests - use testcontainers instead for Redis and PostgreSQL
Test files should be placed next to source files (e.g.,MyService.ts→MyService.test.ts)
Use vitest exclusively for testing
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
apps/{webapp,supervisor}/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying only server components in
apps/webapp/,apps/supervisor/, etc. with no package changes, add a.server-changes/file instead of a changeset
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import from
@trigger.dev/coreusing subpaths only, never the root
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
apps/webapp/**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
For webapp development, never import
env.server.tsin test files - pass configuration as options instead for testable code
Files:
apps/webapp/test/engine/streamBatchItems.test.ts
🧬 Code graph analysis (1)
apps/webapp/test/engine/streamBatchItems.test.ts (1)
apps/webapp/app/runEngine/services/streamBatchItems.server.ts (3)
createNdjsonParserStream(484-688)OversizedItemMarker(17-23)extractIndexAndTask(359-468)
🔇 Additional comments (6)
apps/webapp/test/engine/streamBatchItems.test.ts (6)
27-29: LGTM!The new imports for
extractIndexAndTaskandOversizedItemMarkerare correctly added to support testing the new oversized item handling functionality.
646-663: LGTM!Good edge case coverage. This test correctly verifies that
JSON.stringifyescapes embedded newlines as\n(backslash-n), ensuring they don't break NDJSON line boundaries. The test name and comments clearly explain the expected SDK behavior.
729-746: LGTM!The test correctly verifies that the parser emits an
OversizedItemMarkerinstead of throwing for lines exceedingmaxItemBytes. The assertions properly check all marker fields including the extractedindexandtaskvalues.
769-799: LGTM! Excellent coverage for the cross-chunk oversized line handling.This test correctly validates Case 2 behavior where an oversized line spans multiple chunks:
- Normal item 1 is parsed before the oversized line starts
- Buffer exceeds limit mid-line, triggering marker emission and
skipUntilNewlinemode- Subsequent chunks are discarded until the newline is found
- Normal item 2 after the newline is parsed successfully
The chunk boundaries are well-designed to exercise the skip logic across multiple
transformcalls.
906-926: LGTM!Good integration test verifying that normal items and oversized markers coexist correctly in a single stream. The single-chunk delivery ensures that Case 1 (complete oversized line) is exercised rather than Case 2.
939-967: LGTM!The
extractIndexAndTasktests provide solid coverage:
- Basic extraction (lines 942-947)
- Empty/malformed input returns defaults (lines 949-953)
- Key order independence (lines 955-960)
- Depth-aware parsing ignores nested objects (lines 962-967)
The nested key test at line 963 correctly validates that the depth-1 constraint in the implementation prevents matching keys inside nested objects.
| if (b === LBRACE || b === LBRACKET) { | ||
| depth++; | ||
| i++; | ||
| continue; | ||
| } | ||
| if (b === RBRACE || b === RBRACKET) { | ||
| depth--; | ||
| i++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 extractIndexAndTask depth tracking corrupted by braces inside string values at nested depths
The extractIndexAndTask function tracks JSON nesting depth by counting {, }, [, ] bytes. However, it only skips over string contents when it encounters a " at depth 1 (via the key-reading loop). At depth > 1, quote characters are simply i++'d past, so the string body—including any brace characters—is processed by the depth-tracking code.
Root Cause and Reproduction
Consider the input: {"payload":{"description":"closing brace: }"},"index":0,"task":"my-task"}
When scanning at apps/webapp/app/runEngine/services/streamBatchItems.server.ts:383-391:
- First
{→ depth=1 "payload"is read as a key at depth 1 (not matching), i advances past its closing quote:→ i++{→ depth=2"description"at depth 2 → each byte is just i++ (not depth 1, so no key-reading)- The
:and opening"of the value are also just i++ - The
}byte inside the string"closing brace: }"hits the depth-tracking code at line 388-391:depth--→ depth becomes 1 prematurely - Then
"at depth 1 triggers key-reading, but it's in the middle of the string value, causing the parser to misidentify subsequent content - The real top-level
"index"and"task"keys are never correctly matched
The function returns the fallback { index: -1, task: "unknown" }. While the caller handles index: -1 with lastIndex + 1, the task: "unknown" propagates to TriggerFailedTaskService which creates a pre-failed run with taskIdentifier: "unknown" instead of the actual task name.
Impact: For oversized batch items whose first 512 bytes contain a nested object with string values containing } or { characters (before the index/task fields), the pre-failed run will have the wrong task identifier, making it harder to identify in the dashboard.
Prompt for agents
In apps/webapp/app/runEngine/services/streamBatchItems.server.ts, the extractIndexAndTask function needs to skip over string contents at ALL depths (not just depth 1) to avoid depth counter corruption from brace characters inside strings.
The fix should add string-skipping logic for quotes encountered at any depth other than 1. When a quote byte (0x22) is encountered at depth != 1, instead of just doing i++, the code should scan forward to find the matching closing quote (handling backslash escapes), similar to the key-reading loop used at depth 1. This ensures that braces inside string values at nested depths don't corrupt the depth counter.
Specifically, around lines 383-462, after the depth == 1 quote handling block (line 395), add an else branch for quotes at other depths:
if (b === QUOTE && depth !== 1) {
// Skip over string content at non-top-level depths
i++; // skip opening quote
while (i < limit && bytes[i] !== QUOTE) {
if (bytes[i] === BACKSLASH) i++; // skip escaped char
i++;
}
i++; // skip closing quote
continue;
}
Place this BEFORE the general i++ at line 464. Also add a test case for this scenario, e.g.:
it('should handle nested strings containing braces', () => {
const bytes = encoder.encode('{"payload":{"desc":"a}b"},"index":3,"task":"my-task"}');
const result = extractIndexAndTask(bytes);
expect(result.index).toBe(3);
expect(result.task).toBe('my-task');
});
Was this helpful? React with 👍 or 👎 to provide feedback.
matt-aitken
deleted the
feature/tri-7510-graceful-handling-of-large-batch-item-payloads
branch
2 participants
Gracefully handle oversized batch items instead of aborting the stream.
When an NDJSON batch item exceeds the maximum size, the parser now emits an error marker instead of throwing, allowing the batch to seal normally. The oversized item becomes a pre-failed run with
PAYLOAD_TOO_LARGEerror code, while other items in the batch process successfully. This preventsbatchTriggerAndWaitfrom seeing connection errors and retrying with exponential backoff.Also fixes the NDJSON parser not consuming the remainder of an oversized line split across multiple chunks, which caused "Invalid JSON" errors on subsequent lines.