fix(extraction): process large PDFs without truncating, timing out, or double-billing - #744
Conversation
LiteParse defaults to a 1,000-page ceiling and drops everything past it without reporting the truncation, so a 1,527-page upload was published as a ready projection containing two thirds of the document. Set the ceiling explicitly and reject anything that reaches it. Hitting the cap is indistinguishable from a document that happens to be exactly that long, so both are refused: turning away a 5,000-page file is recoverable, publishing a truncated one as complete is not. Measured at roughly 0.57 MB resident per page, so the ceiling sits near 3 GB on the 8 GiB standard-2 instance; 5,000 pages parse in 11s well inside the 90s budget.
Both callers awaited the body pump alongside their request. An endpoint that answers before draining the upload — a 4xx, a redirect, a quota rejection — leaves the writer parked on backpressure that never clears, so the pump never settles and the caller waits until its workflow step times out. Fix it in the helper rather than at the call sites: the LlamaParse upload and the Office-to-PDF conversion share it, so patching only one would have left the other hanging. Awaiting the response through awaitResponse lets the response decide when the upload is over, while pump failures still surface because they break the body and fail the request.
LlamaParse rejects cost_optimizer with a 422 when the requested tier is already cost_effective — the optimizer works by downgrading individual simple pages to that tier, so there is nothing left to downgrade to. The option was sent unconditionally while normalizeLlamaParseTier passes cost_effective through, so every extraction in that mode failed outright. Latent today because PDFs route to agentic, but it is exactly the trap waiting for anyone dropping tiers to reduce spend.
Every deadline here was picked against small documents while the work scales with page count, so long uploads were killed mid-flight rather than allowed to finish. Workflows imposes no wall-clock limit on a step, so all of these were self-imposed. The 5 minute provider poll ceiling was the direct cause: a 1,527-page parse takes about 4m48s and missed by roughly twelve seconds, three times. It now clears the container's page ceiling instead. Retries drop to zero on the extraction step. An attempt uploads the file and starts a fresh billable job before it ever waits on one, so a retry cannot resume the job it lost, only buy another — that document paid for three agentic parses and used none of them. A failure now leaves the LiteParse projection standing and the reconciler re-runs the workflow after its cooldown, which is the cheap way to retry. The LiteParse step also writes one R2 object per page at roughly 45ms each, so its two-minute budget was failing long documents that were working correctly, and the container abort now fires before its step so a real error surfaces instead of an opaque timeout. Raising these moved the slowest healthy run to about 46 minutes, past the 45 minute stall threshold. That threshold gates the reconciler as well as the read path, so leaving it would have queued a duplicate billable workflow against a document that was still parsing.
creditsUsed has always been null. LlamaParse v2 does not populate usage.credits or metadata.credits_used on a parse response, so the telemetry that exists to catch a run costing ten times what it should has never recorded anything. Derive it from the per-page breakdown that is returned: the cost optimizer downgrades individual pages to the cost_effective rate, so a job's real cost is a blend that cannot be read off the requested tier alone. On the 1,527-page production run that is 717 pages downgraded and 810 at agentic, 10,251 credits rather than the 15,270 a flat tier rate would imply. This is an estimate from published rates, not an invoice, and is labelled as such. The reported fields are still checked first so a real billed figure takes precedence if LlamaParse ever starts sending one.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
React Doctor found 3 new issues in 1 file · 3 warnings · score 89 / 100 (Great) · 3 fixed · vs 3 warnings
Reviewed by React Doctor for commit |
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughLiteParse detects results beyond 5,000 pages and limits concurrent parsing. Multipart consumers use response-aware upload handling. LlamaParse adds tier-based billing and request options. Extraction workflows add shared budgets, healing, fallback outcomes, and packed page projection storage. ChangesExtraction reliability and provider accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExtractionWorkflow
participant LiteParse
participant EnhancementProvider
participant ProjectionStorage
participant ExtractionTelemetry
ExtractionWorkflow->>LiteParse: create fast projection
LiteParse-->>ExtractionWorkflow: result or unsupported-document error
ExtractionWorkflow->>EnhancementProvider: run enhanced extraction
EnhancementProvider-->>ExtractionWorkflow: enhanced result or failure
ExtractionWorkflow->>ProjectionStorage: publish selected projection
ExtractionWorkflow->>ExtractionTelemetry: record combined outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/workspaces/extraction/workspace-file-extraction-workflow.ts`:
- Around line 74-76: Increase the extraction step’s timeout value near the
workflow’s LlamaParse polling configuration so it covers the full maximum
polling duration plus file upload, result retrieval, and
writeWorkspacePageProjection for large documents; update the adjacent
readiness-threshold rationale to match the new worst-case duration.
In `@src/features/workspaces/files/workspace-file-processor.ts`:
- Around line 5-8: Update the request handling flow around processor.fetch and
input.body.pipeTo, replacing the Promise.all wait with a race that returns the
early processor response without waiting for body pumping to finish, matching
the awaitResponse pattern. Ensure pipe failures are propagated while the
response remains pending, and add a regression test covering a processor
response that arrives before the request body is consumed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e93d6d22-a458-469a-8d8d-60cb039773cb
📒 Files selected for processing (13)
containers/liteparse/server.mjssrc/features/workspaces/conversion/container-file-conversion.tssrc/features/workspaces/extraction/liteparse-projection.tssrc/features/workspaces/extraction/providers/llama-parse-credits.test.tssrc/features/workspaces/extraction/providers/llama-parse-credits.tssrc/features/workspaces/extraction/providers/llama-parse.tssrc/features/workspaces/extraction/workspace-file-extraction-workflow.tssrc/features/workspaces/extraction/workspace-projection-readiness.test.tssrc/features/workspaces/extraction/workspace-projection-readiness.tssrc/features/workspaces/files/workspace-file-processor.tssrc/integrations/llamaparse/client.tssrc/lib/http/streaming-multipart.tssrc/lib/http/streaming-multipart.worker.test.ts
Greptile SummaryThis change extends large-document extraction timing and updates streaming multipart request handling. Two reproduced failures need correction before merge: a legitimate long-running extraction can be restarted while it is still publishing its projection, and early server responses leave multipart upload pumps holding their source readers. Confidence Score: 3/5Not safe to merge until extraction liveness covers the complete retry window and early multipart responses terminate the upload pump. Focused executable checks reproduced two independent non-security reliability failures in the changed behavior: premature extraction recovery and retained multipart source readers. Files Needing Attention: src/features/workspaces/extraction/workspace-projection-readiness.ts and src/lib/http/streaming-multipart.ts
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * while the first is still parsing. | ||
| */ | ||
| export const workspaceExtractionStallThresholdMs = 45 * 60_000; | ||
| export const workspaceExtractionStallThresholdMs = 60 * 60_000; |
There was a problem hiding this comment.
The 60-minute cutoff is shorter than this workflow's configured healthy maximum. Two LiteParse attempts, enhanced extraction, and four projection-publication attempts with their retry delays can run for about 67.4 minutes while the projection remains processing with its original updatedAt. At 66 minutes, the resolver marks that active workflow as stalled, and the reconciler can queue a competing extraction and projection write. Set the threshold above the complete retry budget with margin, or refresh a liveness timestamp while the workflow continues.
Artifacts
- The authored TypeScript harness creates a 66-minute-old processing projection, asserts that the age is above 60 minutes and below the configured 67.42-minute healthy budget, then invokes the production readiness resolver; it provides the executable reproduction.
- The executed harness output reports a 67.4167-minute healthy budget and shows the production resolver returning stalled at 66 minutes; the threshold misclassifies the active workflow.
- The focused Vitest run completed successfully with 9 of 9 readiness tests passing; the existing suite does not cover the 60-to-67.42-minute retry window.
| awaitResponse<T>(response: Promise<T>): Promise<T> { | ||
| return Promise.race([response, done.then(() => response)]); |
There was a problem hiding this comment.
When the server responds before reading the upload, this race returns the response without canceling pipeMultipartBody. The pump can remain blocked in writer.write, so it never reaches its cleanup path to cancel the source and release its reader. A focused reproduction returned eight immediate rejected responses while retaining all eight source reader locks and reporting zero cancellations. Terminate the pump when the response wins the race by aborting the writable side and canceling the source, while safely handling the resulting pump rejection.
Artifacts
- The authored Workers-runtime test creates undrained multipart uploads and records reader locks and cancellation counts, proving the tested condition.
- The single-upload execution returned the immediate response while reporting one retained source lock and zero source cancellations, proving cleanup did not run.
- The eight-upload execution returned all immediate responses while reporting eight retained source locks and zero source cancellations, proving the retention scales with rejected uploads.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/extraction/providers/llama-parse.ts">
<violation number="1" location="src/features/workspaces/extraction/providers/llama-parse.ts:133">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The `cost_optimizer` gating fix here prevents 422 failures for `cost_effective` parses, but the PR does not include a regression test that asserts this behavior. Given that regression tests were added for the upload-hang fixes, a corresponding test asserting that `cost_effective` omits `cost_optimizer` while `agentic`/`agentic_plus` include it would be practical and should be added to prevent future regressions.</violation>
</file>
<file name="src/lib/http/streaming-multipart.ts">
<violation number="1" location="src/lib/http/streaming-multipart.ts:31">
P3: `awaitResponse` now settles as soon as the response wins the race, so a body-pump failure that happens *after* the response has already resolved is silently swallowed. The in-code comment claims “Pump failures still surface: they break the body, which fails the request,” but that only holds while the fetch is still consuming the body. If an endpoint answers early (the exact hang scenario this fixes) the response settles first; a subsequent source-stream error no longer rejects the call. In `convertFileStreamWithContainer` this means a mid-upload pump failure after an early 200 could let the flow proceed as success instead of the previous `Promise.all([fetch, multipart.done])` behavior, which would have rejected. This is the documented tradeoff, but it is worth confirming the risk is acceptable — the caller no longer has any way to observe that the upload was not fully/cleanly delivered.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // tier, so LlamaParse rejects the combination with a 422 when that is | ||
| // already the requested tier — there is nothing left to downgrade to. | ||
| // Sending it unconditionally made every cost_effective parse fail outright. | ||
| processing_options: supportsLlamaParseCostOptimizer(input.tier) |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The cost_optimizer gating fix here prevents 422 failures for cost_effective parses, but the PR does not include a regression test that asserts this behavior. Given that regression tests were added for the upload-hang fixes, a corresponding test asserting that cost_effective omits cost_optimizer while agentic/agentic_plus include it would be practical and should be added to prevent future regressions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/providers/llama-parse.ts, line 133:
<comment>The `cost_optimizer` gating fix here prevents 422 failures for `cost_effective` parses, but the PR does not include a regression test that asserts this behavior. Given that regression tests were added for the upload-hang fixes, a corresponding test asserting that `cost_effective` omits `cost_optimizer` while `agentic`/`agentic_plus` include it would be practical and should be added to prevent future regressions.</comment>
<file context>
@@ -117,11 +126,13 @@ async function startLlamaParseJob(
+ // tier, so LlamaParse rejects the combination with a 422 when that is
+ // already the requested tier — there is nothing left to downgrade to.
+ // Sending it unconditionally made every cost_effective parse fail outright.
+ processing_options: supportsLlamaParseCostOptimizer(input.tier)
+ ? { cost_optimizer: { enable: true } }
+ : {},
</file context>
| // when the upload is over. Pump failures still surface: they break the body, | ||
| // which fails the request. | ||
| awaitResponse<T>(response: Promise<T>): Promise<T> { | ||
| return Promise.race([response, done.then(() => response)]); |
There was a problem hiding this comment.
P3: awaitResponse now settles as soon as the response wins the race, so a body-pump failure that happens after the response has already resolved is silently swallowed. The in-code comment claims “Pump failures still surface: they break the body, which fails the request,” but that only holds while the fetch is still consuming the body. If an endpoint answers early (the exact hang scenario this fixes) the response settles first; a subsequent source-stream error no longer rejects the call. In convertFileStreamWithContainer this means a mid-upload pump failure after an early 200 could let the flow proceed as success instead of the previous Promise.all([fetch, multipart.done]) behavior, which would have rejected. This is the documented tradeoff, but it is worth confirming the risk is acceptable — the caller no longer has any way to observe that the upload was not fully/cleanly delivered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/http/streaming-multipart.ts, line 31:
<comment>`awaitResponse` now settles as soon as the response wins the race, so a body-pump failure that happens *after* the response has already resolved is silently swallowed. The in-code comment claims “Pump failures still surface: they break the body, which fails the request,” but that only holds while the fetch is still consuming the body. If an endpoint answers early (the exact hang scenario this fixes) the response settles first; a subsequent source-stream error no longer rejects the call. In `convertFileStreamWithContainer` this means a mid-upload pump failure after an early 200 could let the flow proceed as success instead of the previous `Promise.all([fetch, multipart.done])` behavior, which would have rejected. This is the documented tradeoff, but it is worth confirming the risk is acceptable — the caller no longer has any way to observe that the upload was not fully/cleanly delivered.</comment>
<file context>
@@ -21,7 +21,15 @@ export function createStreamingMultipartFile(input: {
+ // when the upload is over. Pump failures still surface: they break the body,
+ // which fails the request.
+ awaitResponse<T>(response: Promise<T>): Promise<T> {
+ return Promise.race([response, done.then(() => response)]);
+ },
};
</file context>
The container request path builds its own FixedLengthStream rather than going through the multipart helper, so fixing the helper left this copy of the hang in place: if the processor answers before draining the body, the pump stays parked on backpressure and the caller waits for a send that never completes. Move the race into one place both paths call, so the fix cannot be applied to some callers and not others. Reported independently by two PR reviewers.
When the fast pass publishes and the enhanced pass then fails, the projection is left ready but provisional: readable, permanently stuck at fast-tier quality, and invisible to every reconciler clause. The row is not failed, and the workflow does not touch it on the way out. Bounded for free. The run key is derived from the projection's updated_at, which the partial path never advances, so every later sweep builds the same workflow id and createBatch skips it as a duplicate — one upgrade attempt per published projection rather than one per sweep.
The threshold has to exceed every step's timeout multiplied by its attempts, but the steps lived in three files and the sum lived in a comment. A comment does not fail the build, so the arithmetic went stale twice while this branch was being written and a reviewer caught a third case: the publish step's four attempts were never counted at all. Put the budgets in one place and compute the threshold from them. Raising a timeout now moves the threshold with it. The readiness test derives its boundary the same way instead of hardcoding a number that silently stops testing anything the moment a budget changes. Also gives the extract step room for the work surrounding the poll ceiling — upload, result fetch and the page projection write — which at 5,000 pages the previous five minutes of slack did not cover.
The page ceiling added earlier in this branch made LiteParse refuse oversized files instead of silently truncating them, but nothing acted on the refusal — the workflow carried on to the paid tier, which attempted a 6,000-page parse, failed, and left the item for the reconciler to buy again every 15 minutes. A 422 from the processor means it read the file and found it unusable: too long, encrypted, or damaged. No paid provider reaches a different verdict, so treat it as terminal for the whole pipeline. Every other status stays retryable. This also stops the pre-existing waste of sending password-protected and damaged PDFs to a paid provider at all.
There was a problem hiding this comment.
2 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/extraction/workspace-extraction-budgets.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-extraction-budgets.ts:48">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description claims the stall threshold was moved to 60 minutes, but the arithmetic in this new file produces 81 minutes (4,860,000 ms). Summing the worst-case step budgets (liteParse ≈ 16.25 min + extract 40 min + publish ≈ 8.5 min = 64.75 min) and multiplying by the 1.25 headroom factor gives ~81 minutes, not 60. This is a material inconsistency on a billing-sensitive timeout: setting it 35% higher than claimed delays duplicate-workflow detection and means documents are allowed to run longer before the reconciler intervenes. Either the budget constants, retry counts, or headroom multiplier should be adjusted to match the stated target, or the PR description should be corrected to reflect the actual computed threshold.</violation>
</file>
<file name="src/features/workspaces/extraction/workspace-file-extraction-workflow.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-file-extraction-workflow.ts:77">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The new LiteParse unsupported-document guard in the workflow is a significant behavior change (skipping paid extraction when the free tier already rejected the document), but it is not exercised by any test. The existing `liteparse.test.ts` only verifies the provider throws the correct error; it does not cover the workflow's integration logic that checks `liteParse.outcome === 'error' && liteParse.errorType === workspaceDocumentUnsupportedErrorName` and throws before the paid extraction step. Consider adding a workflow-level regression test that mocks an unsupported-document `liteParse` result and asserts the workflow throws without entering the paid extraction step.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * mislabel a slow document — it queues a duplicate workflow, and a duplicate bill, | ||
| * against one that is still parsing. | ||
| */ | ||
| export const workspaceExtractionStallThresholdMs = |
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
The PR description claims the stall threshold was moved to 60 minutes, but the arithmetic in this new file produces 81 minutes (4,860,000 ms). Summing the worst-case step budgets (liteParse ≈ 16.25 min + extract 40 min + publish ≈ 8.5 min = 64.75 min) and multiplying by the 1.25 headroom factor gives ~81 minutes, not 60. This is a material inconsistency on a billing-sensitive timeout: setting it 35% higher than claimed delays duplicate-workflow detection and means documents are allowed to run longer before the reconciler intervenes. Either the budget constants, retry counts, or headroom multiplier should be adjusted to match the stated target, or the PR description should be corrected to reflect the actual computed threshold.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-extraction-budgets.ts, line 48:
<comment>The PR description claims the stall threshold was moved to 60 minutes, but the arithmetic in this new file produces 81 minutes (4,860,000 ms). Summing the worst-case step budgets (liteParse ≈ 16.25 min + extract 40 min + publish ≈ 8.5 min = 64.75 min) and multiplying by the 1.25 headroom factor gives ~81 minutes, not 60. This is a material inconsistency on a billing-sensitive timeout: setting it 35% higher than claimed delays duplicate-workflow detection and means documents are allowed to run longer before the reconciler intervenes. Either the budget constants, retry counts, or headroom multiplier should be adjusted to match the stated target, or the PR description should be corrected to reflect the actual computed threshold.</comment>
<file context>
@@ -0,0 +1,68 @@
+ * mislabel a slow document — it queues a duplicate workflow, and a duplicate bill,
+ * against one that is still parsing.
+ */
+export const workspaceExtractionStallThresholdMs =
+ Math.ceil(
+ (Object.values(workspaceExtractionStepBudgets).reduce(
</file context>
| // terminal path as any other error, which matters because the reconciler | ||
| // re-runs failures on a cooldown — letting it through buys an identical | ||
| // verdict from a paid provider on every sweep. | ||
| if ( |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The new LiteParse unsupported-document guard in the workflow is a significant behavior change (skipping paid extraction when the free tier already rejected the document), but it is not exercised by any test. The existing liteparse.test.ts only verifies the provider throws the correct error; it does not cover the workflow's integration logic that checks liteParse.outcome === 'error' && liteParse.errorType === workspaceDocumentUnsupportedErrorName and throws before the paid extraction step. Consider adding a workflow-level regression test that mocks an unsupported-document liteParse result and asserts the workflow throws without entering the paid extraction step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-file-extraction-workflow.ts, line 77:
<comment>The new LiteParse unsupported-document guard in the workflow is a significant behavior change (skipping paid extraction when the free tier already rejected the document), but it is not exercised by any test. The existing `liteparse.test.ts` only verifies the provider throws the correct error; it does not cover the workflow's integration logic that checks `liteParse.outcome === 'error' && liteParse.errorType === workspaceDocumentUnsupportedErrorName` and throws before the paid extraction step. Consider adding a workflow-level regression test that mocks an unsupported-document `liteParse` result and asserts the workflow throws without entering the paid extraction step.</comment>
<file context>
@@ -61,20 +69,21 @@ export class WorkspaceFileExtractionWorkflow extends WorkflowEntrypoint<
+ // terminal path as any other error, which matters because the reconciler
+ // re-runs failures on a cooldown — letting it through buys an identical
+ // verdict from a paid provider on every sweep.
+ if (
+ liteParse.outcome === "error" &&
+ liteParse.errorType === workspaceDocumentUnsupportedErrorName
</file context>
The enhanced pass failing is an expected outcome the run settles on — keep the fast projection, or mark the item failed — but it was modelled as an exception, which put a complete second success path inside the catch block, three mutable locals spanning steps, and three telemetry calls each shaping its own record. Model the enhanced pass as a value instead. The run is now linear: mark processing, fast pass, enhanced pass, settle, record once, return. The telemetry module derives success/partial/error from the two stage outcomes in the one place that owns the event schema, and the emitted fields are unchanged except that credits_used is now also reported on error outcomes where the provider had already billed before a later step failed.
The healing loop was not bounded as claimed. The bound relied on the healing run never advancing the projection's updated_at, but the run's own fast-pass publish advances it, so every sweep built a fresh workflow id and re-billed a persistently failing document. Bound it structurally instead: a healing run brands the fast projection it republishes, and the reconciler only heals unbranded rows — one upgrade attempt per document. Also from review: keep the processor's specific refusal reason (page ceiling, encryption, damage) on the failed projection instead of a generic message; parse one page past the supported ceiling so a document of exactly 5,000 pages is distinguishable from a truncated longer one; cap concurrent container parses and shed load with a retryable 503 rather than risking an OOM that kills every in-flight request; raise the provider poll ceiling to clear its own 30-minute parse budget at the page ceiling (the previous margin was thin, not generous, and the comment claiming otherwise was wrong); state honestly that a pump failure after the response settles goes unobserved; and lock the cost_optimizer tier gating with tests, since sending the wrong shape there once broke an entire tier.
There was a problem hiding this comment.
4 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/extraction/providers/llama-parse.ts">
<violation number="1" location="src/features/workspaces/extraction/providers/llama-parse.ts:28">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description (fix #4) claims the poll cap increased from 5 to 25 minutes, but this file's `llamaParseMaxPollMs` changed from 25 minutes to 35 minutes. No other poll cap at 5 minutes exists in the extraction pipeline, so the stated before/after values do not match the code. Please update the PR description to reflect the actual 25 → 35 minute change.</violation>
</file>
<file name="containers/liteparse/server.mjs">
<violation number="1" location="containers/liteparse/server.mjs:69">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
This bug-fix PR introduces new server-side behavior (a 503 `EXTRACTOR_BUSY` concurrency gate and updated page-ceiling semantics) in `containers/liteparse/server.mjs`, but no regression tests were added to exercise it. The only existing LiteParse test file (`src/features/workspaces/extraction/providers/liteparse.test.ts`) tests the client-side mocking layer and does not reference `EXTRACTOR_BUSY`, `activeParses`, `maxConcurrentParses`, or the new `supportedPages` boundary. Regression assertions for these paths are practical and should be added to prevent regressions.</violation>
<violation number="2" location="containers/liteparse/server.mjs:84">
P1: Large-document requests can exceed the intended concurrency cap because this slot is released before the underlying parse or the parsed result's response lifecycle has finished. A timed-out parse continues consuming memory after `Promise.race` rejects, and slow NDJSON responses retain multi-gigabyte results while new parses are admitted, potentially recreating the OOM failure this cap is meant to prevent; release capacity only after the underlying parse and response-owned result are no longer active.</violation>
</file>
<file name="src/features/workspaces/extraction/workspace-file-extraction-observability.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-file-extraction-observability.ts:33">
P2: Billed enhancement failures can still be emitted with `credits_used: null` when the provider succeeds but the subsequent page-projection write fails, which undercounts spend in the telemetry this change is meant to correct. Capturing the provider metadata before the projection write (or otherwise preserving it through that failure) would keep `WorkspaceFileEnhancementOutcome.creditsUsed` accurate.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| inputBytes = bytes.byteLength; | ||
| result = await withTimeout(parser.parse(bytes), parseTimeoutMs); | ||
| } finally { | ||
| activeParses -= 1; |
There was a problem hiding this comment.
P1: Large-document requests can exceed the intended concurrency cap because this slot is released before the underlying parse or the parsed result's response lifecycle has finished. A timed-out parse continues consuming memory after Promise.race rejects, and slow NDJSON responses retain multi-gigabyte results while new parses are admitted, potentially recreating the OOM failure this cap is meant to prevent; release capacity only after the underlying parse and response-owned result are no longer active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At containers/liteparse/server.mjs, line 84:
<comment>Large-document requests can exceed the intended concurrency cap because this slot is released before the underlying parse or the parsed result's response lifecycle has finished. A timed-out parse continues consuming memory after `Promise.race` rejects, and slow NDJSON responses retain multi-gigabyte results while new parses are admitted, potentially recreating the OOM failure this cap is meant to prevent; release capacity only after the underlying parse and response-owned result are no longer active.</comment>
<file context>
@@ -59,18 +63,32 @@ createServer(async (request, response) => {
+ inputBytes = bytes.byteLength;
+ result = await withTimeout(parser.parse(bytes), parseTimeoutMs);
+ } finally {
+ activeParses -= 1;
+ }
</file context>
| routeReason: string; | ||
| } | ||
| | { | ||
| // Non-null when the provider finished and billed but a later step failed — |
There was a problem hiding this comment.
P2: Billed enhancement failures can still be emitted with credits_used: null when the provider succeeds but the subsequent page-projection write fails, which undercounts spend in the telemetry this change is meant to correct. Capturing the provider metadata before the projection write (or otherwise preserving it through that failure) would keep WorkspaceFileEnhancementOutcome.creditsUsed accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-file-extraction-observability.ts, line 33:
<comment>Billed enhancement failures can still be emitted with `credits_used: null` when the provider succeeds but the subsequent page-projection write fails, which undercounts spend in the telemetry this change is meant to correct. Capturing the provider metadata before the projection write (or otherwise preserving it through that failure) would keep `WorkspaceFileEnhancementOutcome.creditsUsed` accurate.</comment>
<file context>
@@ -14,66 +14,89 @@ import { capturePostHogServerEvent } from "#/integrations/posthog/server";
+ routeReason: string;
+ }
+ | {
+ // Non-null when the provider finished and billed but a later step failed —
+ // reporting null there would understate spend on exactly the runs worth
+ // investigating.
</file context>
| // LlamaParse's own job timeout is 30 minutes of parsing excluding queue time. This | ||
| // exists to turn a wedged job into a clear error, not to cap normal work; Workflows | ||
| // imposes no wall-clock limit on a step. | ||
| const llamaParseMaxPollMs = 35 * 60_000; |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The PR description (fix #4) claims the poll cap increased from 5 to 25 minutes, but this file's llamaParseMaxPollMs changed from 25 minutes to 35 minutes. No other poll cap at 5 minutes exists in the extraction pipeline, so the stated before/after values do not match the code. Please update the PR description to reflect the actual 25 → 35 minute change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/providers/llama-parse.ts, line 28:
<comment>The PR description (fix #4) claims the poll cap increased from 5 to 25 minutes, but this file's `llamaParseMaxPollMs` changed from 25 minutes to 35 minutes. No other poll cap at 5 minutes exists in the extraction pipeline, so the stated before/after values do not match the code. Please update the PR description to reflect the actual 25 → 35 minute change.</comment>
<file context>
@@ -20,11 +20,12 @@ import { createStreamingMultipartFile } from "#/lib/http/streaming-multipart";
+// LlamaParse's own job timeout is 30 minutes of parsing excluding queue time. This
+// exists to turn a wedged job into a clear error, not to cap normal work; Workflows
+// imposes no wall-clock limit on a step.
+const llamaParseMaxPollMs = 35 * 60_000;
const llamaParseVersion = "latest";
</file context>
| // Each parse can hold gigabytes resident, and one over-committed container | ||
| // dies taking every in-flight request with it. Shed load instead: 503 is | ||
| // retryable by the caller, an OOM crash is not. | ||
| if (activeParses >= maxConcurrentParses) { |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
This bug-fix PR introduces new server-side behavior (a 503 EXTRACTOR_BUSY concurrency gate and updated page-ceiling semantics) in containers/liteparse/server.mjs, but no regression tests were added to exercise it. The only existing LiteParse test file (src/features/workspaces/extraction/providers/liteparse.test.ts) tests the client-side mocking layer and does not reference EXTRACTOR_BUSY, activeParses, maxConcurrentParses, or the new supportedPages boundary. Regression assertions for these paths are practical and should be added to prevent regressions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At containers/liteparse/server.mjs, line 69:
<comment>This bug-fix PR introduces new server-side behavior (a 503 `EXTRACTOR_BUSY` concurrency gate and updated page-ceiling semantics) in `containers/liteparse/server.mjs`, but no regression tests were added to exercise it. The only existing LiteParse test file (`src/features/workspaces/extraction/providers/liteparse.test.ts`) tests the client-side mocking layer and does not reference `EXTRACTOR_BUSY`, `activeParses`, `maxConcurrentParses`, or the new `supportedPages` boundary. Regression assertions for these paths are practical and should be added to prevent regressions.</comment>
<file context>
@@ -59,18 +63,32 @@ createServer(async (request, response) => {
+ // Each parse can hold gigabytes resident, and one over-committed container
+ // dies taking every in-flight request with it. Shed load instead: 503 is
+ // retryable by the caller, an OOM crash is not.
+ if (activeParses >= maxConcurrentParses) {
+ status = 503;
+ return sendJson(response, status, {
</file context>
Extraction wrote one R2 object per page — a thousand round-trips at roughly 45ms each for a long document, and the search indexer read them back one sequential GET at a time. The manifest already records every page's byte count, which is an offset index nobody was using. Schema version 2 concatenates all pages into a single pages.md next to the manifest. Page reads become ranged GETs, with contiguous selections coalesced into one request; search indexing becomes a single read; and the partial-write failure mode disappears along with the write-concurrency machinery, since two puts replace up to five thousand. This is the same shape cloud-native formats converge on — Zarr v3 sharding, Cloud-Optimized GeoTIFF, PMTiles: many logical items, one physical object, an index, and range requests. Version 1 projections stay readable through the legacy per-page path, keyed off the schemaVersion the manifest already carries. They are derived data, but regenerating them bills a provider parse, so there is no migration and no backfill — old rows serve as v1 forever, new writes are v2. The projection also gains an explicit total size bound, which is what lets the indexer materialize the packed object safely, and the fast pass budget drops from eight minutes to three now that its dominant cost is gone.
Hold the container's parse slot until the response has fully streamed, since the parsed pages stay resident while the NDJSON drains and releasing earlier lets admissions outrun actual memory use. Mark healing runs with a dedicated server-controlled workflow field instead of a sentinel request id, so nothing a client influences can brand a projection as already healed. Fetch independent ranged page reads concurrently. Document the one remaining case where a billed parse can still report null credits.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/extraction/workspace-page-projection.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-page-projection.ts:20">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR description explicitly states that single-object page storage with range reads is 'not included in this PR' and 'parked,' yet this file directly implements that exact feature — version 2 writes all pages to a single `pages.md` object, uses R2 ranged reads in `readPackedPages`, and migrates the manifest to a discriminated union. The PR description should be updated to accurately reflect what the code does, or the description and code should be reconciled.</violation>
</file>
<file name="src/features/workspaces/extraction/workspace-extraction-budgets.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-extraction-budgets.ts:26">
P2: Slow large-PDF fast passes can still be aborted at three minutes even though the processor allows seven minutes for `/parse/pdf`, plus container startup and projection work occur within the workflow step. Keeping this budget above that end-to-end bound, such as eight minutes, prevents slow uploads from losing the fast projection and falling through to the paid enhancement path.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * Fast pass: the parse itself is seconds even for a 5,000-page document, and the | ||
| * projection is a single write since schema v2 packed pages into one object. | ||
| */ | ||
| liteParse: { attempts: 2, retryDelayMs: 15_000, timeoutMs: 3 * minuteMs }, |
There was a problem hiding this comment.
P2: Slow large-PDF fast passes can still be aborted at three minutes even though the processor allows seven minutes for /parse/pdf, plus container startup and projection work occur within the workflow step. Keeping this budget above that end-to-end bound, such as eight minutes, prevents slow uploads from losing the fast projection and falling through to the paid enhancement path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-extraction-budgets.ts, line 26:
<comment>Slow large-PDF fast passes can still be aborted at three minutes even though the processor allows seven minutes for `/parse/pdf`, plus container startup and projection work occur within the workflow step. Keeping this budget above that end-to-end bound, such as eight minutes, prevents slow uploads from losing the fast projection and falling through to the paid enhancement path.</comment>
<file context>
@@ -19,8 +19,11 @@ interface WorkspaceExtractionStepBudget {
+ * Fast pass: the parse itself is seconds even for a 5,000-page document, and the
+ * projection is a single write since schema v2 packed pages into one object.
+ */
+ liteParse: { attempts: 2, retryDelayMs: 15_000, timeoutMs: 3 * minuteMs },
/**
* Upload, the provider's own poll ceiling, fetching the result, and writing the
</file context>
| liteParse: { attempts: 2, retryDelayMs: 15_000, timeoutMs: 3 * minuteMs }, | |
| \tliteParse: { attempts: 2, retryDelayMs: 15_000, timeoutMs: 8 * minuteMs }, |
| // per-page byte counts the manifest already carries as the offset index. Old | ||
| // projections are derived data whose regeneration is billable, so both versions stay | ||
| // readable — v1 through the per-page path below, with no migration or backfill. | ||
| const projectionSchemaVersion = 2; |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The PR description explicitly states that single-object page storage with range reads is 'not included in this PR' and 'parked,' yet this file directly implements that exact feature — version 2 writes all pages to a single pages.md object, uses R2 ranged reads in readPackedPages, and migrates the manifest to a discriminated union. The PR description should be updated to accurately reflect what the code does, or the description and code should be reconciled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-page-projection.ts, line 20:
<comment>The PR description explicitly states that single-object page storage with range reads is 'not included in this PR' and 'parked,' yet this file directly implements that exact feature — version 2 writes all pages to a single `pages.md` object, uses R2 ranged reads in `readPackedPages`, and migrates the manifest to a discriminated union. The PR description should be updated to accurately reflect what the code does, or the description and code should be reconciled.</comment>
<file context>
@@ -12,33 +12,50 @@ import {
+// per-page byte counts the manifest already carries as the offset index. Old
+// projections are derived data whose regeneration is billable, so both versions stay
+// readable — v1 through the per-page path below, with no migration or backfill.
+const projectionSchemaVersion = 2;
const pageNumberWidth = 6;
-const pageWriteConcurrency = 8;
</file context>
Lowering the fast-pass step to three minutes inverted its ordering against the processor request abort, which still allowed seven — the inner timeout could never fire. Size the request abort to what the container actually permits (a 90s parse behind an upload) and the step above both, so a slow fast pass fails with a named error instead of losing its projection to the step timeout and falling through to the paid path. Also verify the packed pages object matches its manifest before the search indexer consumes it, mirroring the check the ranged reader already does, and correct a size-margin comment that overstated the headroom.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="containers/liteparse/server.mjs">
<violation number="1" location="containers/liteparse/server.mjs:84">
P1: Large parse requests can permanently consume admission slots when a client disconnects during response streaming: a backpressured response waits for `drain`, but a closed `ServerResponse` does not necessarily emit it. Handling `close`/`aborted` alongside `drain` would let the handler reach `finally` and release the slot.</violation>
</file>
<file name="src/features/workspaces/extraction/workspace-file-extraction-reconciler.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-file-extraction-reconciler.ts:76">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
The new `healing: true` workflow parameter (and the accompanying provisional-row SQL branch) changes production behavior to prevent double-billing on stuck fast-tier documents, yet no regression test asserts this path. The codebase already has a clear pattern for unit-testing extraction logic in isolation (see `workspace-projection-readiness.test.ts`), so a practical regression test is feasible — mock the SQL query to return a provisional candidate and assert that `workflow.createBatch` is called with `healing: true`. Shipping a bug-fix without exercising the new behavior under test contradicts the project's own testing norms and increases the risk that a future refactor silently breaks the healing path.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // outlives its timeout still runs to completion in the background holding | ||
| // memory — that zombie cannot be cancelled, only kept rare by the timeout. | ||
| activeParses += 1; | ||
| holdsParseSlot = true; |
There was a problem hiding this comment.
P1: Large parse requests can permanently consume admission slots when a client disconnects during response streaming: a backpressured response waits for drain, but a closed ServerResponse does not necessarily emit it. Handling close/aborted alongside drain would let the handler reach finally and release the slot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At containers/liteparse/server.mjs, line 84:
<comment>Large parse requests can permanently consume admission slots when a client disconnects during response streaming: a backpressured response waits for `drain`, but a closed `ServerResponse` does not necessarily emit it. Handling `close`/`aborted` alongside `drain` would let the handler reach `finally` and release the slot.</comment>
<file context>
@@ -74,15 +75,16 @@ createServer(async (request, response) => {
- } finally {
- activeParses -= 1;
- }
+ holdsParseSlot = true;
+ const bytes = await readPdfRequestBytes(request);
+ inputBytes = bytes.byteLength;
</file context>
| const params = { | ||
| actorUserId: null, | ||
| assetKind: assetKind.data, | ||
| healing: true, |
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
The new healing: true workflow parameter (and the accompanying provisional-row SQL branch) changes production behavior to prevent double-billing on stuck fast-tier documents, yet no regression test asserts this path. The codebase already has a clear pattern for unit-testing extraction logic in isolation (see workspace-projection-readiness.test.ts), so a practical regression test is feasible — mock the SQL query to return a provisional candidate and assert that workflow.createBatch is called with healing: true. Shipping a bug-fix without exercising the new behavior under test contradicts the project's own testing norms and increases the risk that a future refactor silently breaks the healing path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-file-extraction-reconciler.ts, line 76:
<comment>The new `healing: true` workflow parameter (and the accompanying provisional-row SQL branch) changes production behavior to prevent double-billing on stuck fast-tier documents, yet no regression test asserts this path. The codebase already has a clear pattern for unit-testing extraction logic in isolation (see `workspace-projection-readiness.test.ts`), so a practical regression test is feasible — mock the SQL query to return a provisional candidate and assert that `workflow.createBatch` is called with `healing: true`. Shipping a bug-fix without exercising the new behavior under test contradicts the project's own testing norms and increases the risk that a future refactor silently breaks the healing path.</comment>
<file context>
@@ -76,6 +73,7 @@ export async function reconcileWorkspaceFileExtractions(input: {
const params = {
actorUserId: null,
assetKind: assetKind.data,
+ healing: true,
itemId: candidate.id,
requestId: extractionHealingVersion,
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/features/workspaces/extraction/workspace-page-projection.test.ts (2)
163-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding v2 coverage for gap pages and non-contiguous selections.
The new tests cover the v1 paths well. Two v2 behaviors remain untested, and both depend on offset arithmetic in
readPackedPages:
- A selection that starts on a zero-byte gap page. The run start offset comes from
runSpans[0].offset, which is shared with the next non-empty page.- A non-contiguous selection, for example
"1,3". This must produce two ranged reads ofpages.md, whichstorage.readKeyscan assert.Add these two cases if you want the offset index locked down by tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/extraction/workspace-page-projection.test.ts` around lines 163 - 205, Extend the workspace page projection tests with v2 coverage for a selection beginning on a zero-byte gap page and for a non-contiguous selection such as “1,3”. Exercise readPackedPages through readWorkspacePageProjection, assert the gap-page result and content, and verify the non-contiguous case performs two ranged pages.md reads via storage.readKeys.
339-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sizereports the ranged slice length, not the object length.R2 sets
sizeto the full object size even for a rangedget. This double setssizeto the returned slice length.No current caller reads
sizeafter a rangedget, so the tests pass and the production paths are unaffected. If a reader later validatessizeagainstmanifest.markdownByteson a ranged read, this double would accept incorrect behavior. Consider keepingsize: fullBytes.byteLengthto match R2.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/extraction/workspace-page-projection.test.ts` around lines 339 - 361, The mock get method should report the full object size for ranged reads, matching R2 behavior. Update the returned size in get to use fullBytes.byteLength instead of the sliced bytes length, while keeping the body contents limited to the requested range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@containers/liteparse/server.mjs`:
- Around line 67-96: Update the parse-slot lifecycle around parser.parse(bytes),
withTimeout(), and the existing cleanup release so a timed-out parse keeps
activeParses occupied until its underlying promise settles. Preserve the current
cleanup release for normal completion, while preventing the timed-out promise
from being decremented twice; ensure eventual settlement releases the slot
exactly once.
---
Nitpick comments:
In `@src/features/workspaces/extraction/workspace-page-projection.test.ts`:
- Around line 163-205: Extend the workspace page projection tests with v2
coverage for a selection beginning on a zero-byte gap page and for a
non-contiguous selection such as “1,3”. Exercise readPackedPages through
readWorkspacePageProjection, assert the gap-page result and content, and verify
the non-contiguous case performs two ranged pages.md reads via storage.readKeys.
- Around line 339-361: The mock get method should report the full object size
for ranged reads, matching R2 behavior. Update the returned size in get to use
fullBytes.byteLength instead of the sliced bytes length, while keeping the body
contents limited to the requested range.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e49486b-8210-49a5-b6cf-2aad925d0394
📒 Files selected for processing (18)
containers/liteparse/server.mjssrc/features/workspaces/extraction/liteparse-projection.tssrc/features/workspaces/extraction/providers/liteparse.test.tssrc/features/workspaces/extraction/providers/liteparse.tssrc/features/workspaces/extraction/providers/llama-parse.test.tssrc/features/workspaces/extraction/providers/llama-parse.tssrc/features/workspaces/extraction/types.tssrc/features/workspaces/extraction/workspace-extraction-budgets.tssrc/features/workspaces/extraction/workspace-file-extraction-observability.tssrc/features/workspaces/extraction/workspace-file-extraction-reconciler.tssrc/features/workspaces/extraction/workspace-file-extraction-workflow.tssrc/features/workspaces/extraction/workspace-page-projection.test.tssrc/features/workspaces/extraction/workspace-page-projection.tssrc/features/workspaces/extraction/workspace-projection-readiness.test.tssrc/features/workspaces/extraction/workspace-projection-readiness.tssrc/features/workspaces/files/workspace-file-processor.tssrc/lib/http/streaming-multipart.tssrc/lib/http/streaming-upload.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/http/streaming-multipart.ts
| // Each parse can hold gigabytes resident, and one over-committed container | ||
| // dies taking every in-flight request with it. Shed load instead: 503 is | ||
| // retryable by the caller, an OOM crash is not. | ||
| if (activeParses >= maxConcurrentParses) { | ||
| status = 503; | ||
| return sendJson(response, status, { | ||
| code: "EXTRACTOR_BUSY", | ||
| error: "Extractor is at capacity, retry shortly.", | ||
| }); | ||
| } | ||
|
|
||
| // Held until the whole request finishes, including streaming the result out: | ||
| // the parsed pages stay resident while the response drains, so releasing the | ||
| // slot any earlier would let admissions outrun actual memory use. A parse that | ||
| // outlives its timeout still runs to completion in the background holding | ||
| // memory — that zombie cannot be cancelled, only kept rare by the timeout. | ||
| activeParses += 1; | ||
| holdsParseSlot = true; | ||
| const bytes = await readPdfRequestBytes(request); | ||
| inputBytes = bytes.byteLength; | ||
| const result = await withTimeout(parser.parse(bytes), parseTimeoutMs); | ||
|
|
||
| if (result.pages.length > supportedPages) { | ||
| throw new PdfValidationError( | ||
| 422, | ||
| "TOO_MANY_PAGES", | ||
| `PDFs longer than ${supportedPages} pages are not supported.`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Keep the parse slot until a timed-out parse settles.
withTimeout() rejects on timeout but does not cancel parser.parse(bytes). The cleanup path then decrements activeParses while the timed-out parse can still hold several gigabytes of memory. A new request can enter and exceed the intended memory cap.
Release the slot when the underlying parse promise settles after a timeout. Keep the existing cleanup release for parses that complete normally.
Also applies to: 119-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@containers/liteparse/server.mjs` around lines 67 - 96, Update the parse-slot
lifecycle around parser.parse(bytes), withTimeout(), and the existing cleanup
release so a timed-out parse keeps activeParses occupied until its underlying
promise settles. Preserve the current cleanup release for normal completion,
while preventing the timed-out promise from being decremented twice; ensure
eventual settlement releases the slot exactly once.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 12 unresolved issues from previous reviews.
Re-trigger cubic
The pipeline records how it produced projections but nothing about how they are consumed, which left the central product question — does anyone read a document during the window when only the fast pass exists — answerable only by a workspace-level proxy with a 2 percent upper bound. Three additions close that. Every file read now records the state it was served in (ready, pending, or failed, with the provisional flag and empty-page count) at the one operation both assistant tools and MCP route through. Reconciler sweeps that enqueue healing record per-reason counts, so a healing loop shows up as a workspace re-appearing every sweep instead of as a billing surprise. And the enhancement duration now splits into provider queue time versus parse time, read from the state transitions the result fetch already carried — a slow enhancement that was queued wants patience, one that was parsing wants a tier or budget change. Read telemetry carries ids and states only, no file names or content, mirroring the intake event's lawful-interest basis.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| for (let pageNumber = 1; pageNumber <= manifest.pageCount; pageNumber += 1) { | ||
| const object = await getWorkspacePageProjectionObject({ | ||
| const object = await getLegacyPageObject({ |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This makes the for-loop slow because each await runs one after another, so collect the independent calls & run them together with await Promise.all(items.map(...))
Fix → Collect the items, then use await Promise.all(items.map(...)) so independent work runs at the same time
| continue; | ||
| } | ||
|
|
||
| const object = await input.bucket.get(getWorkspacePagesObjectKey(input.prefix), { |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This makes the for…of loop slow because each await runs one after another, so collect the independent calls & run them together with await Promise.all(items.map(...))
Fix → Collect the items, then use await Promise.all(items.map(...)) so independent work runs at the same time
| // Consume each R2 body before opening the next one; never retain a batch of live | ||
| // responses. | ||
| for (const pageNumber of input.selectedPageNumbers) { | ||
| const object = await getLegacyPageObject({ |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This makes the for…of loop slow because each await runs one after another, so collect the independent calls & run them together with await Promise.all(items.map(...))
Fix → Collect the items, then use await Promise.all(items.map(...)) so independent work runs at the same time
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/content/workspace-read-observability.ts">
<violation number="1" location="src/features/workspaces/content/workspace-read-observability.ts:13">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
This new telemetry module in a bug-fix PR introduces three-way branching logic (`pending`/`failed`/`ready`) and property mapping that shapes PostHog events, but no test exercises these branches. Sibling modules in the same directory (`workspace-content-reader`, `workspace-read-references`) already have tests, so a small regression-style test asserting the captured properties per status is clearly practical. Adding one would prevent silent regressions in telemetry shape and make the PR's claimed test coverage genuinely representative of the changed behavior.</violation>
<violation number="2" location="src/features/workspaces/content/workspace-read-observability.ts:34">
P2: Failed reads for files are silently missing from the new readiness telemetry when the reader omits the optional `type` field; preserve the file type whenever the resolved item is a file before filtering here. Otherwise invalid selections, cursors, content changes, and page-range errors are absent from failure counts.</violation>
</file>
<file name="src/features/workspaces/extraction/workspace-file-extraction-reconciler.ts">
<violation number="1" location="src/features/workspaces/extraction/workspace-file-extraction-reconciler.ts:78">
P2: Healing telemetry can overstate enqueued work because this event is recorded before candidates are validated or any `createBatch` succeeds. Emitting it from the successful enqueue path and counting only submitted workflows would keep the healing-loop signal accurate.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| continue; | ||
| } | ||
|
|
||
| if (result.status === "failed" && result.type === "file") { |
There was a problem hiding this comment.
P2: Failed reads for files are silently missing from the new readiness telemetry when the reader omits the optional type field; preserve the file type whenever the resolved item is a file before filtering here. Otherwise invalid selections, cursors, content changes, and page-range errors are absent from failure counts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/content/workspace-read-observability.ts, line 34:
<comment>Failed reads for files are silently missing from the new readiness telemetry when the reader omits the optional `type` field; preserve the file type whenever the resolved item is a file before filtering here. Otherwise invalid selections, cursors, content changes, and page-range errors are absent from failure counts.</comment>
<file context>
@@ -0,0 +1,88 @@
+ continue;
+ }
+
+ if (result.status === "failed" && result.type === "file") {
+ capture(input, {
+ elapsed_seconds: null,
</file context>
| if (candidates.length > 0) { | ||
| const countByReason = (reason: string) => | ||
| candidates.filter((candidate) => candidate.reason === reason).length; | ||
| capturePostHogServerEvent({ |
There was a problem hiding this comment.
P2: Healing telemetry can overstate enqueued work because this event is recorded before candidates are validated or any createBatch succeeds. Emitting it from the successful enqueue path and counting only submitted workflows would keep the healing-loop signal accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/extraction/workspace-file-extraction-reconciler.ts, line 78:
<comment>Healing telemetry can overstate enqueued work because this event is recorded before candidates are validated or any `createBatch` succeeds. Emitting it from the successful enqueue path and counting only submitted workflows would keep the healing-loop signal accurate.</comment>
<file context>
@@ -60,6 +70,30 @@ export async function reconcileWorkspaceFileExtractions(input: {
+ if (candidates.length > 0) {
+ const countByReason = (reason: string) =>
+ candidates.filter((candidate) => candidate.reason === reason).length;
+ capturePostHogServerEvent({
+ distinctId: input.workspaceId,
+ event: "workspace_file_extraction_healing_enqueued",
</file context>
| @@ -0,0 +1,88 @@ | |||
| import type { WorkspaceContentReadResult } from "#/features/workspaces/content/workspace-content-contract"; | |||
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
This new telemetry module in a bug-fix PR introduces three-way branching logic (pending/failed/ready) and property mapping that shapes PostHog events, but no test exercises these branches. Sibling modules in the same directory (workspace-content-reader, workspace-read-references) already have tests, so a small regression-style test asserting the captured properties per status is clearly practical. Adding one would prevent silent regressions in telemetry shape and make the PR's claimed test coverage genuinely representative of the changed behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/content/workspace-read-observability.ts, line 13:
<comment>This new telemetry module in a bug-fix PR introduces three-way branching logic (`pending`/`failed`/`ready`) and property mapping that shapes PostHog events, but no test exercises these branches. Sibling modules in the same directory (`workspace-content-reader`, `workspace-read-references`) already have tests, so a small regression-style test asserting the captured properties per status is clearly practical. Adding one would prevent silent regressions in telemetry shape and make the PR's claimed test coverage genuinely representative of the changed behavior.</comment>
<file context>
@@ -0,0 +1,88 @@
+ * anyone was actually in it — reads served `provisional`, reads that hit the
+ * pending spinner, and reads that found a stalled or failed document.
+ */
+export function recordWorkspaceFileReadOutcomes(input: {
+ operationId: string;
+ results: readonly WorkspaceContentReadResult[];
</file context>
Staging and production pin the extractor image by digest, so the container changes merged in #744 — the explicit 5,000-page ceiling, the 422 refusal for documents no tier can read, and the concurrency gate that sheds load with a retryable 503 — did not ship with the Worker. Production still truncated long PDFs at 1,000 pages silently. Built from the merge commit and verified against the pushed artifact rather than a local rebuild: cross-compiled linux/amd64 on an arm64 host, parses a real PDF end to end, and three concurrent parses yield one 503 and two successes.
1 participant
Large PDF uploads were failing in production (instance
pdf-4L-7ctBM5UK…: 26 minutes, 3 abandoned billable parses, zero extracted text). Investigating from first principles turned up one root cause and several latent bugs around it; this PR fixes the pipeline end to end.Root cause
Every deadline in the pipeline was a fixed number, but every unit of work scales with page count. Cloudflare imposes no wall-clock limit on a Workflow step — all the limits were ours, sized against small documents. Measured: the container parses the failing 35 MB / 1,527-page file in 2.33s; LlamaParse needs ~4m48s against our 5m poll cap. The document missed by ~12 seconds, three times, paying for a fresh parse each time (which LlamaParse docs note also pushes the job to the back of their queue).
The pipeline, as it now stands
Upload → fast local pass (LiteParse container, seconds, published
provisional) → enhanced pass (LlamaParse agentic) replaces it → exactly one settled end state: enhanced ready / fast retained / failed, one telemetry event describing both passes.The model-facing read contract (
pending+retryAfterSeconds,provisional,emptyPages, terminal failures with the container's message) was already well designed and is unchanged.Fixes (each commit is independently revertable)
maxPages: 5000with a loud 422 refusal. Verified: 1,527/1,527 pages in 3.78s; 6,108-page file →TOO_MANY_PAGES.awaitUploadResponseprimitive now owns the race; regression tests both directions.cost_effectivetier broken outright —cost_optimizerwas sent unconditionally; LlamaParse 422s that combination. Now gated on tier.cost_optimizedbreakdown (estimate, labelled as such; real field takes precedence if it ever appears). Unit-tested incl. the exact 717/810 production split.ready+provisionalrows were invisible to the reconciler forever. New clause; naturally bounded to one upgrade attempt per published projection because the healing run key derives fromupdated_at, which the partial path never advances, andcreateBatchdedupes.catch, one telemetry call site, linearrun().Verification
pnpm checkclean; 316 node + 40 worker tests pass.pages.mdper projection with ranged reads instead of one object per page: search indexing drops from ~1,000 sequential GETs to 1, contiguous page selections coalesce into a single ranged request, and the partial-write cleanup machinery is deleted. v1 projections stay readable forever through the legacy path — no migration, no backfill, no re-billing. Note: not forward-compatible — a rollback after v2 rows exist leaves those documents unreadable until re-extraction.Post-merge deploy note
containers/liteparse/server.mjschanges (page ceiling, 422 refusals, admission control) do not ship with the Worker: the staging/production configs pin the container image by registry digest. After merge, rebuild the image, push it, and bump the digest inwrangler.jsonc, or long-PDF extraction keeps truncating at 1,000 pages in production despite this PR.Deliberately not in this PR
(failed,failed)healing loop — pre-existing; needs a persisted attempt count (a column —metadata_jsonis overwritten every upsert). Own PR.agenticfor now;cost_effective(~3× cheaper, documented faster, now actually selectable after fix 3) is worth a corpus comparison later.Summary by CodeRabbit
New Features
Bug Fixes
Performance