Skip to content

Bound cache row decoding during central sync - #1873

Merged
philcunliffe merged 2 commits into
masterfrom
codex/bounded-sync-row-decoding
Sep 21, 2026
Merged

philcunliffe merged 2 commits into
masterfrom
codex/bounded-sync-row-decoding

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Central sync can exhaust the V8 heap when compact Parquet groups expand repeated tool definitions into whole-column object graphs before yielding rows. The HTTP chunk limit does not bound those upstream allocations.

Read output vectors in windows spanning at most 256 physical rows, including sparse selections. Preserve predicates, row order, deletes, privacy filtering, missing-column padding, and end-of-partition watermark acknowledgement. The storage-to-forward regression uses real Iceberg VARIANT data under a 128 MiB heap cap and checks retries, partial failure, replay/deduplication, and descending sequence order.

The upstream fix is released in Hyparquet 1.31.1 (hyparam/hyparquet#179). This branch includes current master, which uses Hyparquet 1.31.1 and Icebird 0.8.29. The dependency blocker is resolved; validation uses published packages with no local patches.

Validation (2026-09-21):

  • Full suite: 7,088 passed, 3 skipped, 0 failed. Typecheck, declaration build, and diff whitespace checks passed.
  • Packed tarball installed globally inside the repository's disposable install sandbox. Icebird resolves Hyparquet 1.31.1 in that installed tree.
  • The same regression passed against the packed installation: 3,854 eligible rows, 96 requests, two backpressure retries, three replayed acknowledged chunks, and 37,062,160 bytes sampled live heap under a 128 MiB heap cap.

CPU and memory: nested output expansion is limited to the current physical window; sparse matches cannot widen it to an entire row group. The regression bounds decoder work as well as live heap. Physical page decoding, dictionaries, encoded-buffer caches, and unusually large individual values retain their existing costs. Watermarks still advance only after the whole partition is acknowledged.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Neutral local review: approve

Full PR review against master, comparison base 97886b342b0dda8360865faccf327793339a2f2f, exact head dfc39027462c8a785403b830fc1dec453b7e5a57.

Independent Codex review: no new findings. Claude was unavailable (the user reported no tokens); the permitted single-reviewer fallback was used. No Claude review or approval is claimed. Blast radius: low. Human merge hold remains.

The conflict resolution retains missing privacy-fence column padding. CPU/memory review found no unbounded allocation in the changed output-window path; smaller reads trade some call overhead for bounded nested expansion. Physical page buffers and oversized individual values retain their existing costs.

Validation: 7,088 local tests passed, 3 skipped; typecheck/declaration build passed; GitHub Node 22/24/26 CI green. A packed global install using published dependencies passed the 128 MiB regression, exporting all 3,854 eligible rows with retries/replay/privacy/watermark checks. Substituting current master's reader in that disposable install reproduced V8 heap exhaustion; the fixed reader was restored. Fourteen focused current-head safety tests also passed.

Codex reviewed the diff and traced callers but could not run tests in its dependency-free checkout; the test results above were executed separately by the coordinating session. No findings require disposition.

Independent Codex report

Fix Validations

Central forward heap exhaustion from full-batch VARIANT expansion

  • Status: correct
  • Evidence: src/core/cache/iceberg/store.js:708, src/core/cache/iceberg/store.js:720, src/core/cache/iceberg/store.js:721, test/plugins/central-forward-memory.test.js:40
  • Assessment: Output vectors are now read from bounded selected windows before rows are yielded, so central’s downstream chunk limit no longer has to be the first memory boundary. The regression runs the real storage-to-forward path under --max-old-space-size=128.

Sparse selections, row order, privacy withholding, replay, and watermark safety

  • Status: correct
  • Evidence: src/core/cache/iceberg/store.js:715, src/core/cache/iceberg/store.js:722, test/helpers/central_forward_memory.js:87, test/helpers/central_forward_memory.js:92, test/helpers/central_forward_memory.js:94, test/helpers/central_forward_memory.js:103
  • Assessment: The test covers sparse physical positions, retained order, local-only drops, partial failure without checkpointing, replayed acknowledged chunks, and final watermark advancement.

Existing HTTP chunking did not already fix the failure

  • Status: correct
  • Evidence: hypaware-core/plugins-workspace/central/src/sink.js:881, hypaware-core/plugins-workspace/central/src/sink.js:893, hypaware-core/plugins-workspace/central/src/sink.js:898, src/core/cache/iceberg/store.js:721
  • Assessment: Central chunks only after storage.readRowsSince has yielded materialized rows, so the old chunking path could not bound allocations inside readBatchColumn.

Findings

No new findings.

No Finding

  • 1) Behavioral Correctness: no issue found.
  • 2) Contract & Interface Fidelity: no public signature or schema change found.
  • 3) Change Impact / Blast Radius: no incompatible caller impact found.
  • 4) Concurrency, Ordering & State Safety: no new ordering or watermark safety issue found.
  • 5) Error Handling & Resilience: no new retry/failure-path issue found.
  • 6) Security Surface: no new security exposure found.
  • 7) Resource Lifecycle & Cleanup: no cleanup leak found; helper restores TextDecoder.prototype.decode and closes the sink.
  • 8) Release Safety: no release blocker found in the diff.
  • 9) Test Evidence Quality: no coverage issue found for the claimed regression.
  • 10) Architectural Consistency: scanner-level bounding matches the storage/materialization boundary.
  • 11) Debuggability & Operability: existing central chunk/backpressure/drop events remain intact.

CPU and memory pass: no concern found. The changed scanner bounds expanded output vectors to 256 selected rows for contiguous selections and to a physical span under 256 rows for sparse index selections; CPU may do more small reads for very sparse matches, but that is the intended tradeoff to avoid whole-row-group object expansion.

Evidence Bundle

  • Changed hot paths: src/core/cache/iceberg/store.js:695, src/core/cache/iceberg/store.js:708, src/core/cache/iceberg/store.js:720; hypaware-core/plugins-workspace/central/src/sink.js:881
  • Impacted callers: src/core/cache/storage.js:300, src/core/cache/storage.js:318, src/core/cache/storage.js:401; hypaware-core/plugins-workspace/central/src/sink.js:881; direct scan callers found at src/core/cache/maintenance.js:1362, src/core/cache/retention.js:233, src/core/cache/purge.js:251, hypaware-core/plugins-workspace/context-graph/src/maintenance.js:134
  • Impacted tests: test/plugins/central-forward-memory.test.js:16, test/helpers/central_forward_memory.js:87, test/helpers/central_forward_memory.js:94, test/core/sink-reads-since.test.js:134, test/core/cache-storage.test.js:80
  • Unresolved uncertainty: I did not rerun the suite locally because dependencies are not installed in this checkout (squirreling import failed with ERR_MODULE_NOT_FOUND); review is based on the supplied diff, targeted rg tracing, and the PR’s reported validation.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Ship risk: medium

Who could be affected: People syncing or exporting large histories, and people running large local scans or maintenance.

What could happen:

  • Some scans may take longer because the work is divided into smaller pieces, especially when records are simple or widely separated.
  • An exceptionally large individual record may still need substantial memory; this change addresses the observed crash from expanding many records together.

Why this level: The fix changes a shared reader, so a slowdown could be noticeable beyond central sync. The checks found no change in which records are returned, which private records are withheld, or when completed uploads are acknowledged.

What was checked: Real stored data was exported with a small memory allowance, including failed uploads, retries, repeated chunks, and private records. Those checks passed in both the checkout and a packaged installation. Replacing the reader with the existing released behavior reproduced the crash. Focused privacy and progress-tracking tests also passed.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Decision map

Scanned five hunks across three files. Most of the diff builds the memory/retry/privacy regression. One implementation choice deserves attention:

Bound decoding to 256 physical rows per window

store.js:714

let end = Math.min(count, start + 256)
  • Decision: Use a fixed physical-row bound, splitting sparse selections by their covering range too.
  • Alternative: Larger windows reduce column-call overhead but retain more expanded tool definitions. A payload-byte limit would require knowing decoded sizes before allocation and a larger reader change.
  • Check: The measured memory/throughput tradeoff is suitable for expected datasets. The capped-heap regression proves the incident fixture, while unusually large individual values retain their own memory costs.

Generated by decision-map as part of the local Neutral flow. This directs review attention; it adds no separate verdict.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Sep 21, 2026
@philcunliffe
philcunliffe marked this pull request as ready for review September 21, 2026 19:14
@philcunliffe
philcunliffe added this pull request to the merge queue Sep 21, 2026
Merged via the queue into master with commit 96fb9cd Sep 21, 2026
10 checks passed
@philcunliffe
philcunliffe deleted the codex/bounded-sync-row-decoding branch September 21, 2026 19:18
@platypii

Copy link
Copy Markdown
Contributor

This seems to be cause of significant performance regression, causing CI tests to timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants