fix(sql-materialize): read each source once outside the scratch lock

Merged
AN
1d ago
+692-556

Issue for this PR

Closes #1485
Closes #1492

Stacked on feat/analyst-harness-dab-2026-10 (#1441)

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Materializing a remote source held the shared scratch lock for the whole remote read, and ran up to three full-timeout source scans (count, sample, extract), each resetting the deadline.

Root cause: sql-materialize entered withScratchConnection before preflight and extraction. The scratch callbacks are serialized per root session, so sibling sub-agents waited on the remote source. Preflight count/sample plus extraction each used the full timeout (plus a 5s grace), and a failed preflight could trigger another read.

Before: lock held across all source reads; up to 3 scans; deadline reset per scan. After: the source is read once and buffered before the destination lock (bounded by max_rows + 1, driver byte cap and a row-byte check, with type inference done pre-lock); one deadline with no grace, aborting pool admission and ignoring late results; only destination catalog checks, staging, transaction and rollback are serialized. Scratch sources use the managed connector for the single read. Create/replace/append and read-only protections are kept.

Deferred:

  • Separate queue_ms/exec_ms telemetry (needs queue instrumentation across scratch APIs and trace schema).
  • Independent concurrent scratch readers during a destination write (needs driver engine changes).
  • Review follow-up (P2): a session closed during a pending remote read can lazily recreate its scratch store when the result arrives.

Also addressed after review: scratch-to-scratch derivations read and write under one lock acquisition (no lost updates), and overlapping remote reads reserve their byte cap from a process-wide 512 MiB buffer budget.

How did you verify your code works?

  • bun test test/altimate/sql-materialize*.test.ts in packages/opencode: 88 pass, 0 fail (5 files), including new concurrency tests (sibling scratch query completes while a remote read is blocked; distinct reads run concurrently; concurrent appends keep all rows).
  • Pre-fix, the new regression tests failed (concurrency 1 pass/2 fail; single-read tests 0/3; Mongo 3/16).
  • bun run typecheck in packages/opencode: passes.
  • git diff --check clean; upstream marker check: no upstream-shared files modified.
  • Independent Codex review: no P0/P1; two P2 items listed above as follow-ups.

Screenshots / recordings

N/A

Checklist

  • I have tested my changes locally
  • I have added tests that prove my fix works
  • Existing tests pass

🤖 Generated with Claude Code

[!NOTE]
Medium Risk
Changes core materialization concurrency, memory budgeting, and timeout/abort behavior for remote Postgres reads; mistakes could cause lost updates, OOM pressure, or premature timeouts, though behavior is heavily covered by new concurrency tests.

Overview
sql_materialize no longer holds the scratch lock during remote source reads and drops the preflight count/sample path in favor of one bounded extraction (max_rows + 1, driver max_bytes, row-byte backstop) before any destination write.

Remote warehouses are read and buffered outside withScratchConnection, so sibling sub-agents can query scratch while a slow source is in flight; scratch-to-scratch derivations still read and write under a single lock so concurrent replaces stay consistent. Overlapping remote reads reserve each job's byte cap from a process-wide 512 MiB buffer budget until the scratch write completes.

PostgreSQL source reads use read-only + AbortSignal: the outer timeout matches the configured deadline (no +5s grace) and aborts the pool on expiry so late rows cannot be written. Preflight fallback warnings and the 200k-row clamp on failed counts are removed; caps are enforced on the single read (truncated/extra-row refusal unchanged in spirit).

Tests are updated for one driver call per materialization and a new sql-materialize-concurrency.test.ts covers scratch queries during blocked remotes, overlapping reads, append atomicity, buffer budgeting, and scratch derivation safety.

Reviewed by Cursor Bugbot for commit 307bf7a4d1a21f35ad9370d8c8d23582b0c10920. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by cubic

Materializes each remote source once and outside the scratch lock, so slow sources no longer block sibling sub-agents' scratch operations in the same root session, and removes the preflight count and sample queries. When the source is the scratch connection itself, the read and write share one scratch-lock acquisition so concurrent derivations from the same table cannot lose updates; remote sources still read outside the lock.

  • Runs a single bounded source read (max_rows + 1 rows, driver byte cap, and a defensive row-byte check) and refuses truncated results before touching the destination.
  • Reserves each remote read's byte cap from a process-wide 512 MiB buffer budget until the scratch write finishes, so overlapping large reads serialize instead of buffering concurrently.
  • Enforces one source deadline with no grace period; Postgres pool admission is aborted on expiry and late results never reach the write phase.
  • Performs type inference and bounds checks before taking the destination transaction lock, so only catalog checks, staging, transaction, and rollback are serialized; create/replace/append modes and read-only protections are unchanged.