Skip to content

feat(server): spill oversized TUIndex results to a store tmp file - #593

Closed
orionsutton wants to merge 2 commits into
clice-io:mainfrom
orionsutton:feat/index-spill
Closed

orionsutton wants to merge 2 commits into
clice-io:mainfrom
orionsutton:feat/index-spill

Conversation

@orionsutton

Copy link
Copy Markdown

Problem

A stateless worker returns the serialized TUIndex inline in the IPC response (BuildResult::tu_index_data). On large TUs this frame is enormous — on a 4795-TU field workspace the largest observed result is 283.5MB — and an inline frame that size costs several transient copies along the pipe (worker serialize → frame → 64KB ring-buffer chunks → master reassembly → codec decode) and head-of-line blocks every other message on that worker link while it streams. It is also what made the transport's frame cap reachable in the first place: results past the cap could never be delivered at all.

Change

Results above a threshold travel out-of-line:

  • The master allocates a per-task tmp path from its CacheStore (begin_store) and passes it in the existing BuildParams::index_output_path (the same pattern BuildPCH already uses for its PreambleState blob). The PendingEntry is held across the request, so the file is removed on every master-side exit path — completion, error, and coroutine cancellation; tmp files orphaned by a crashed master are swept by the next instance's open().
  • The worker writes the serialized TUIndex to that path and reports {tu_index_file_size, tu_index_hash} (xxh3_64) instead of the bytes; results at or under the threshold stay inline, so the common case has zero extra filesystem traffic. A failed spill write falls back to inline delivery.
  • The master mmaps the file (llvm::MemoryBuffer::getFile), verifies size and hash before parsing — the worker may have died mid-write, and a torn blob must not poison the merge — and on verification failure requeues the file like a crash (note_dispatch_failure) instead of serving the stale shard as fresh. The [perf:index] line gains transfer=file|inline.
  • The threshold is project.index_inline_limit (default 8MB, undocumented like test_hooks): its default only matters for results too large for CI to produce, and the knob is what makes the spill path testable at all.
  • Drive-by hardening: fs::write now surfaces mid-write failures (disk full, EIO) as ordinary errors — previously an unchecked stream error flag turned into a fatal error in the raw_fd_ostream destructor, which for a worker meant dying instead of reporting.

Master and workers are the same binary on the same machine, so a shared filesystem path is a given for this transport.

Tests

  • Unit: IndexSpillToFile pins the wire contract (empty inline bytes, on-disk size and xxh3 match the reply); SpillFailureFallsInline pins the fallback; the existing IndexRequest now also asserts a small index stays inline when a spill path is offered.
  • Integration: index_spill.test.ts reproduces the manual validation deterministically — index_inline_limit: 1 makes every background-index result spill; a cross-file reference proves the master verified and merged spilled bytes; transfer=file in the master log proves the file path actually ran; the tmp dir is empty after settling (PendingEntry cleanup end-to-end).
  • Manual field-shaped run: with the threshold at 64KB, a real serve session over a 400-TU workspace produced 366 file transfers and 18 inline ones — zero spill failures, zero verification rejections, clean shutdown.
  • All four suites pass locally (unit / integration / smoke / snap) plus npm run check.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fa72d182-4e3b-4359-9273-61994ac811cf

📥 Commits

Reviewing files that changed from the base of the PR and between bdf3aa1 and ed18493.

📒 Files selected for processing (1)
  • tests/integration/server/index_spill.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/integration/server/index_spill.test.ts

📝 Walkthrough

Walkthrough

The change adds configurable TUIndex spilling for oversized worker results. Workers write results to temporary files and return size and hash metadata. The indexer verifies and loads spilled data, handles failures, logs transfer mode, and removes temporary files. Unit and integration tests cover these paths.

Changes

TUIndex spill transfer

Layer / File(s) Summary
Transfer contract and configuration
src/server/protocol/worker.h, src/server/state/config.h
BuildParams carries the spill path and inline-size limit. BuildResult reports spilled file size and XXH3 hash. ProjectConfig defines an 8 MiB default limit.
Worker spill and fallback behavior
src/server/worker/stateless_worker.cpp, src/support/filesystem.h, tests/unit/server/stateless_worker_tests.cpp
The worker spills oversized indexes, returns metadata, and falls back to inline data when writing fails. Filesystem writes report stream failures. Tests validate inline results, file contents, hashes, and fallback behavior.
Indexer verification and round trip
src/server/compiler/indexer.cpp, tests/integration/server/index_spill.test.ts
The indexer loads and verifies spilled data before merging, logs the transfer mode, requeues invalid transfers, and cleans up temporary files. The integration test validates the complete round trip.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • clice-io/clice#402: Extends the same TUIndex transfer path used for serialized PCH, PCM, and open-file indexes.
  • clice-io/clice#432: Modifies the background indexer index_one flow and worker build-result handling.
  • clice-io/clice#502: Modifies Indexer::index_one and worker protocol handling for dispatch-failure recovery.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: spilling oversized TUIndex results to temporary store files.
Description check ✅ Passed The description explains the problem, implementation, fallback behavior, cleanup, validation, and tests; it is complete despite using Problem and Change headings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@16bit-ykiko

16bit-ykiko commented Aug 12, 2026 •

Copy link
Copy Markdown
Member

Hi — thanks for spending time on this.

Our workflow asks contributors to open an issue first so we can discuss the problem and agree on a direction before any code is written. Jumping straight to a PR skips that conversation, and we're unlikely to merge unsolicited PRs that haven't gone through that process — how a fix should look is ultimately a maintainer decision, and the approach may differ from what's proposed here.

If you've hit a real problem, please file an issue describing the scenario and we'll figure out the right fix together.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants