test(e2e): fix flaky Extract pipeline E2E document-processing timeout (#1844)

[JSv4]

Summary

Fixes the intermittently-failing Extract pipeline (PDF upload → run → CSV) check (issue #1844). The check is not live-LLM gated — .github/workflows/frontend-e2e-extract.yml runs the backend with OC_LLM_VCR_MODE=replay so the extract LLM call is served from a recorded cassette; only Docling PDF parsing runs live. It is flaky, not broken (the workflow has recent success runs).

There are two distinct failure modes, both fixed here:

Mode A — document-processing timeout (rarer)

waitForDocumentReady waited at most 8 min for parse + embed. On a cold runner, Docling parsing of the large US Code fixture occasionally exceeds 8 min →

Error: Document "USC Title 1 …" still processing (data-processing="true")
  - Timeout 480000ms exceeded

Fix: ceiling 8 → 14 min (both PDFs upload before either wait, so Celery processes them concurrently — the constraint is the slowest single doc, not the sum); spec test.setTimeout 20 → 30 min.

Mode B — frontend grid-hydration race (dominant / consistent)

This was the real blocker. The backend extract succeeds — the failing run's Celery logs show:

Raw extraction result: TITLE 1-GENERAL PROVISIONS
✓ Successfully extracted and saved data for cell 6
Linked 33 retrieval citations to datacell 6
Extract 3 marked complete

…yet the spec reported row has no non-empty extracted cell. The "Extraction in progress" overlay (which runExtractAndWaitForFinish waits on) clears when the backend marks the extract complete — a beat before AG-Grid finishes its per-cell Apollo data fetch. The spec's one-shot cell read raced that hydration and saw an empty cell.
Fix: wrap the per-row cell check in expect(...).toPass({ timeout: 120s }) so it retries until the cell hydrates. A genuinely empty extract still fails when the poll times out, so the assertion is not weakened.

Also

Corrected the stale spec docstring/skip-reason that claimed "CI does not set the gate" — CI sets it and replays the LLM via VCR.

Verification

• Run 26687429414 (before the Mode-B fix) showed exactly the diagnostic signature: 3.5-min attempts, no processing-timeout, all failing on the cell read while the backend logged a successful extract.
• Re-running on this PR after the poll fix.

Closes #1844

[claude]

Code Review

PR: test(e2e): fix flaky Extract pipeline E2E document-processing timeout (#1844)

---

Overview

This is a focused, well-justified fix for a flaky E2E test. The two changes are:

1. waitForDocumentReady default timeout: 8 min → 14 min
2. test.setTimeout: 20 min → 30 min

The PR description is excellent — root cause analysis, concurrency reasoning (both PDFs upload before either wait, so Celery processes them in parallel), and the fact that the budget must cover 2×14 min + overhead are all laid out clearly. The stale comment correction is a welcome cleanup.

---

Code Quality

✅ Positives

• Minimal blast radius — only 2 files, 26 additions, 9 deletions
• New JSDoc on waitForDocumentReady is genuinely useful: explains why the ceiling is generous, the concurrency reasoning, and the caller contract (test.setTimeout >= 2x this value)
• The corrected skip-reason comment accurately reflects current CI behavior (VCR replay) rather than the old "no mock in CI" claim

⚠️ Minor issue — magic numbers

Per the project's CLAUDE.md:

No magic numbers — use constants files (frontend/src/assets/configurations/constants.ts for frontend hardcoded values)

Both timeout values are raw arithmetic expressions scattered across two files. If the Docling ceiling needs to change again, a reviewer has to update helpers.ts and remember to adjust test.setTimeout in the spec.

Suggested refactor:

// frontend/src/assets/configurations/constants.ts (or a test-specific constants file)
export const DOCLING_PARSE_TIMEOUT_MS = 14 * 60 * 1000;  // cold-runner ceiling per #1844
export const EXTRACT_E2E_SUITE_TIMEOUT_MS = 30 * 60 * 1000;  // 2×DOCLING_PARSE + extract/CSV/diff

// helpers.ts
import { DOCLING_PARSE_TIMEOUT_MS } from "../../src/assets/configurations/constants";
...
export async function waitForDocumentReady(
  page: Page,
  documentTitle: string,
  timeoutMs: number = DOCLING_PARSE_TIMEOUT_MS
)

// extract-pdf-workflow.spec.ts
import { EXTRACT_E2E_SUITE_TIMEOUT_MS } from "../../src/assets/configurations/constants";
...
test.setTimeout(EXTRACT_E2E_SUITE_TIMEOUT_MS);

This is a non-blocking nit — the existing inline comments mitigate the risk — but it would make future tuning safer and keeps the project's own conventions.

---

Correctness

• The concurrency reasoning is sound: both PDFs are uploaded before either waitForDocumentReady call, so Celery can process them in parallel. The binding constraint is the slower document, not the sum.
• The test.setTimeout(30 * 60 * 1000) ≈ 2 × 14 min + ~2 min headroom. That math works, though the headroom is thin if the extract/CSV/fork/diff steps themselves are slow on a cold runner. If flakiness persists, bumping to 35 min would be a low-risk follow-up.
• The PR correctly identifies the secondary failure mode ("row has no non-empty extracted cell") as a separate, rarer race and defers it to Flaky CI: "Extract pipeline (PDF upload → run → CSV)" E2E times out on document processing #1844 — that's the right call rather than over-engineering this fix.

---

Security / Performance

No security concerns. These are test-only changes with no production code paths affected.

---

Summary

✅ Approve with one non-blocking suggestion. The fix is correct, well-documented, and appropriately scoped. The only actionable suggestion is extracting the timeout values into named constants per project conventions. Happy to see this merged as-is if the team prefers to keep test-only constants inline.

[claude]

Code Review

Overview

This PR fixes two distinct flaky-test failure modes in the Extract pipeline (PDF upload → run → CSV) E2E check (issue #1844). The diagnosis is well-documented and the fixes are targeted and minimal.

---

Mode A — Document-Processing Timeout

Fix: Default timeout in waitForDocumentReady raised from 8 → 14 minutes.

The JSDoc update in helpers.ts is thorough and explains the rationale clearly (cold-runner Docling timing, concurrent Celery processing, the single-document bottleneck). The comment is long but the reasoning is genuinely non-obvious, so this is appropriate per project conventions.

Minor nit: 120_000 in the spec is expressed as a numeric literal, while every other timeout in the file uses the self-documenting N * 60 * 1000 form. Consider 2 * 60 * 1000 for consistency:

// current
}).toPass({ timeout: 120_000, intervals: [1_000, 2_000, 5_000] });

// suggested (matches the style of the surrounding timeouts)
}).toPass({ timeout: 2 * 60 * 1000, intervals: [1_000, 2_000, 5_000] });

---

Mode B — Frontend Grid-Hydration Race (the dominant failure)

Fix: Wrap the per-row cell read in expect(async () => { … }).toPass({ timeout: 120_000 }).

This is idiomatic Playwright for retrying flaky assertions and is the right tool here. A few observations:

1. Assertion integrity preserved. A genuinely empty extract still fails after the 2-minute poll exhausts, so the assertion strength is unchanged. ✓

2. Internal assertions throw correctly. Both expect(cellCount).toBeGreaterThan(0) and expect(nonEmptySeen, …).toBe(true) use expect() (not bare returns), so they propagate exceptions and trigger retries correctly inside toPass. ✓

3. 2-minute poll adds to failure-scenario runtime. When a cell is legitimately empty (genuine regression), this now takes 2 min to report instead of failing instantly. That's an acceptable trade-off given it only applies to failure paths, but worth noting for future maintainers.

4. Intervals backoff. [1_000, 2_000, 5_000] gives a sensible ramp-up. Playwright repeats the last interval indefinitely, so after the third attempt it retries every 5 s — good balance between responsiveness and noise. ✓

---

test.setTimeout Budget

30 min = 2 × 14 min (serialized worst-case doc waits) + ~2 min headroom for extract/CSV/fork/diff steps. The comment in the spec explains this clearly. ✓

One edge case to be aware of: waitForDocumentReady is called for each doc separately after both uploads, so they process concurrently — but if the first wait takes the full 14 min the second call still gets a fresh 14-min window. In the absolute worst case the budget is 14 + 14 + overhead ≈ 28+ min. The 30-min ceiling is tight but should hold; if cold-runner variance increases, bumping to 35 min would add safety margin.

---

Docstring / Skip-reason Corrections

The updated docstring and skip message correctly reflect that CI sets E2E_RUN_LLM_TESTS=true and uses VCR replay. ✓

---

Summary

	Assessment
Correctness	✅ Both fixes are sound
Idiomatic Playwright	✅ toPass is the right approach for grid-hydration races
Assertion strength	✅ Not weakened
Code style	⚠️ 120_000 literal vs 2 * 60 * 1000 expression (minor)
Test budget math	✅ Adequate, tight but justified
Comments	✅ Detailed where the WHY is non-obvious

The one style nit aside, this is a clean, well-reasoned fix. Approving with that suggestion to address at your discretion.