perf(pebble): unlink spill chunks during merge, sort resources on import - #1107
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
General PR Review: perf(pebble): unlink spill chunks during merge, sort resources on importBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit ( All four prior findings were verified against the current tree as addressed, with one residual:
Risk triage (per Security IssuesNone found. The only new external input is the operator-supplied Correctness IssuesNone found. SuggestionsNone. |
The two k-way merge helpers held every sorted chunk file open until the staging directory was torn down, so for the length of each merge the staging directory carried the same entries twice: once in the chunks and once in the SST being built from them. Chunks are now closed and unlinked as soon as the merge reads their last entry, bounding the overlap to the chunks still in flight. Unlinking is confined to the exhausted-chunk path, where the file is provably fully consumed; a merge that fails partway closes its descriptors and leaves the rest to the staging-dir teardown that already owned them. Both helpers back the bulk import, the deferred grant index, the id-index migration, the segment layer, and the digest build, and the index builds run at EndSync on every pebble sync, so this bounds peak staging for all of them. AddResources also no longer requires rows pre-sorted by (resource_type_id, resource_id); it routes them through the same spill sorter entitlements already use. A converter scanning SQLite with a matching ORDER BY satisfied the old precondition for free, but a producer that rewrites resource ids cannot — the c1z sanitizer HMACs them, so its output order is unrelated to the order it read. Sorting inside the importer keeps every producer on one path instead of making sortedness a precondition each one has to re-establish, and it holds when a single resource type is too large to sort in memory.
The digest build's mergeGrantHashChunksToSST and the migration's mergeGrantPrimaryMigrationChunksToSST were hand-rolled copies of the merge prologue and still held every chunk open until teardown, so the previous commit's claim that the unlink bounded staging for them was wrong. Both now read through openSpillChunks, which puts all four of the engine's k-way merges on one cursor and leaves readSpillEntry with a single caller. Only the bulk import spills 8MiB chunks; these two use the 128MiB deferredIndexSpillChunkBytes, so they are where the doubling cost most. Two consequences of that consolidation: os.Open on a spill chunk now happens in exactly one place, collapsing four os-IO allowlist entries into one, and ingestSynthLayerSegment's post-merge os.Remove loop is dead work the merge already did. Also corrects the BulkSyncImport type doc, which still told callers resources must arrive in strictly increasing key order and fail with ErrBulkImportOutOfOrder, and guards advance against being called on an exhausted chunk — releasing a chunk nils its reader, where the inline readSpillEntry calls this replaced returned false idempotently. Adds spill_merge_test.go, the first coverage of any of this. A sorter only cuts a second chunk past 8MiB/128MiB and no test drives that much, so every merge under test was single-chunk; these build sorted runs directly with writeSortedSpillChunk and cover an empty chunk, a single-entry chunk, staggered exhaustion, and a duplicate key spanning two runs through both the strict and resolving merges. Confirmed non-vacuous by mutation: removing the unlink fails three of the four, and dropping one entry per advance fails all four. Co-authored-by: Cursor <cursoragent@cursor.com>
mergeGrantPrimaryMigrationChunksToSST was the only one of the four merges sharing spillChunkCursors with no test driving more than one chunk. TestIDIndexMigrationSemantics runs it end to end through Open, but with few enough rows to fit a single chunk, so the path that matters was never reached: advanceMigrationChunk fires once for a duplicate group's leader and again for every same-key follower, so one group can drain several chunks. It is also the merge where a mistake is worst, since it runs on the open-time id-index migration and a row it drops or double-counts stays sorted -- bulkSSTWriter.add does not notice -- and is written into the c1z. The gap predates this branch. What makes the cross-chunk fold safe is that heap items are owned copies, which the cursor refactor did not change, so this is coverage for a path the refactor touched rather than a fix for one it broke. The rows are laid out globally sorted and dealt round-robin so every chunk stays internally sorted while the duplicate group spans three of them; the test asserts that layout instead of assuming it, since nothing in the assertions would reveal a deal that stopped splitting the group. Beyond the folded primary rows it also checks the derived index sorters, because a fold bug can emit a correct primary row alongside one index entry per duplicate, leaving by_principal rows dangling against a primary that no longer has them. Confirmed non-vacuous by mutation: dropping followers from the fold, emitting the index per row, skipping a primary row, and removing the unlink each fail it. Also narrows the spill_merge_test.go header, which claimed to be the only coverage of multi-chunk exhaustion and release-on-exhaustion. TestGrantDigestSpillMerge forces a 512-byte chunk size through a real digest build and reaches both, so the header now names it to keep the two comments from drifting apart again. Co-authored-by: Cursor <cursoragent@cursor.com>
TestMergeGrantPrimaryMigrationChunksFoldsAcrossChunks built the two derived-index sorters and left them open, so nothing waited out their background chunk sorts before t.TempDir removed the directory holding them. teardown() documents that ordering as a requirement -- a chunk sort racing the removal can re-create a file mid-walk and strand the directory -- and production honors it by waiting on every sorter before removeStagingDir. The test cannot actually flake as written, since six keys of ~50 bytes never reach the 8MiB threshold that makes add() cut a chunk, but it would arm itself the moment the fixture grows. Closes them with finalize rather than abort. abort waits on a WaitGroup that, for these sorters, nothing has ever added to, so it would satisfy the ordering on paper while leaving the test with no dispatched sort to wait for. finalize cuts the pending arena, so the background sort really runs and the wait is load-bearing; asserting it flushed a chunk pins that, since only that goroutine writes one. Co-authored-by: Cursor <cursoragent@cursor.com>
Finish's teardown comment still described two ordered SST writers and the failing-one-leaves-the-other scenario; with resource types the only ordered writer left, finish() runs first and closes its handle even on error, so that clause is gone and the sorter-race rationale stands alone. advance's doc claimed merges depend on EOF idempotency in one breath and that no caller reaches it in the next; it now says what the test pins. The resources field comment led with an unconditional key-order mismatch that its own second paragraph walks back for ORDER BY converters, so the mismatch is now scoped to entitlements with resources presented as producer-dependent. The spillChunkCursors type doc drops its change-justification half — the subsystem enumeration and cost analysis live in the commit history — keeping the invariant, the ownership rules, and the only-caller fact that keeps the remaining claim verifiable. ingestSynthLayerSegment's cleanup note said the final dir cleanup "keeps" the SST path when it removes it. Also has the migration merge test capture the duplicate group's key while building the fixture rather than marshaling a throwaway record to recover it. Co-authored-by: Cursor <cursoragent@cursor.com>
…fast path A pebble-destination sanitize run now writes its four record families through BulkSyncImport (sorted SST construction + ingest) instead of per-batch Put upserts, the same split the SQLite->pebble converter uses: assets, EndSync, and supports_diff stay on the writer path. The bulk contract holds by construction — resumable+pebble and multi-sync +pebble are already rejected up front, so the destination sync is always fresh and checkpoints are no-ops. Computed stats (plus the asset count, which rides outside the import) are stashed so EndSync persists the sidecar without re-scanning the ingested keyspaces. Adds Options.TmpDir and a --tmp-dir sanitize flag (matching to-pebble) for spill/unpack staging, and a stats-sidecar assertion to the pebble end-to-end test. Verified by mutation: dropped resource/grant rows and an omitted asset-count stash each fail the parity suites. Co-authored-by: Cursor <cursoragent@cursor.com>
…g spills The resources and entitlements sorters spilled full marshaled records at bulkSpillKeyChunkBytes (8MiB), the size meant for key-only index spills; grants.go records what that does at whale scale (a 3,400-way merge with ~3.4GB of read buffers). Both are single sorters filled in sequential phases, so they move to deferredIndexSpillChunkBytes (128MiB) like the other record-carrying sorters; grant shards and index sorters stay key-sized because their arenas multiply across shards x families. The field comment now states the fd/buffer curve and the ~2x transient staging spill-sorting costs over the ordered writer it replaced. Also from review: ErrBulkImportOutOfOrder's doc now names AddResourceTypes as the only caller-reachable source (resources/ entitlements/grants re-sort internally; their duplicates surface at Finish), the redundant finished flag in the sanitizer's bulk sink is gone (Abort is already a no-op once Finish marks itself done), and the cross-engine parity test pins full record identity for resource types, resources, and entitlements — normalizing the two fields pebble's v3 schema stores as identity-only refs — verified non-vacuous by a field-corruption mutation no prior assertion could see. Co-authored-by: Cursor <cursoragent@cursor.com>
The grant shards and per-shard index sorters were the last spill sorters cutting 8MiB chunks. Chunk size sets Finish's merge fan-in — every chunk stays open behind a 1MiB reader for the whole merge — so a whale-sized grant family (~17GB) meant a ~2,100-way merge with ~2GB of read buffers and as many open files. A 3.7M-grant sanitize run merged 135 grant chunks; the same run now merges 9. All of the import's sorters now share one bounded arena freelist (the same pairing every other 128MiB spill user already has), which also closes the gap where resources and entitlements adopted the big chunk size without arena reuse. Verified compute-neutral on the 3.7M-grant run (+1.7% instructions retired) with identical output counts. Co-authored-by: Cursor <cursoragent@cursor.com>
…cing docs The duplicate-key sentinel moved into the public contract when resources stopped surfacing duplicates through ErrBulkImportOutOfOrder (they now fail at Finish), so export it for errors.Is. Also: the TmpDir doc now states the ~2x merge-peak staging cost instead of ~1x, the WithConvertParallelism doc notes the ~384MiB of spill arenas each lane pins, and the cross-engine parity oracle fails loudly if a fixture ever outgrows its single-page reads instead of silently narrowing. Co-authored-by: Cursor <cursoragent@cursor.com>
ErrBulkImportDuplicateKey surfaces from every spill merge (deferred index and digest builds, synth layer, id-index migration), not just BulkSyncImport.Finish; and the WithConvertParallelism memory budget missed the three lane-independent sorters that stay live through the scan, so the honest figure is ~384MiB x (lanes + 1). Co-authored-by: Cursor <cursoragent@cursor.com>
The bulk import cut every spill sorter at a fixed 128MiB, but its sorter count scales with the caller's scan fan-out (three lane-independent sorters plus, per grant shard, one primary and one per index family), so the constant set peak RSS by accident: ~19 live arenas at the converter's default four lanes, more with every index family added, and the doc comment that tried to state the figure was already short two terms (the in-flight sort arenas) and would go stale again on the next family. Chunk size is now derived: bulkImportSortBudgetBytes (1GiB) divided by the arenas that can be committed at once — one per sorter plus one per background sort slot — clamped to [16MiB, 128MiB]. StartBulkSyncImport takes the caller's expected shard count as a sizing hint (the converter computes its lane count once up front via grantScanLanes; the sanitizer opens one shard). Adding an index family or raising WithConvertParallelism now shrinks the chunks instead of silently growing the ceiling; the floor keeps a whale grant family to a few hundred chunks rather than the thousands 8MiB produced. spillSorter.add also cuts BEFORE an entry would overflow the arena's capacity. Appending past cap made Go reallocate at ~1.25x with a full copy — a 128MiB memcpy per fresh arena — and the freelist then kept the oversized copy for the rest of the build, so the budget arithmetic was never quite true. Pinned by a test that plants the old behavior (arena capacity grows) and fails; the derivation has a property test that the live set fits the budget for the shard counts callers use and that more shards never means bigger chunks. Measured on a synthetic 3M-grant / 60K-user / 1.5K-group SQLite c1z (1.7GB uncompressed) converted with ToPebble on an 8-CPU host (4 lanes), fresh process per run, three runs each, ru_maxrss: 8MiB fixed (pre-branch): peak RSS 0.8-1.15 GiB 128MiB fixed (branch as was): peak RSS 1.7-1.9 GiB budget-derived (~56MiB here): peak RSS 1.36-1.40 GiB Wall-clock was 30-43s in every configuration with no consistent ordering across runs, i.e. indistinguishable at this scale — consistent with the compute-neutral instruction count already recorded for the 128MiB move. The memory delta is the signal, and the budget lands where the derivation says it should. Bounding the merge side independently (read-buffer budget and a cascade above a fan-in cap, so chunk size stops being a safety knob at all) is filed as CXE-1363. Co-authored-by: Cursor <cursoragent@cursor.com>
kans
force-pushed
the
kans/pebble-sanitize
branch
from
September 4, 2026 18:53
27a3042 to
4aab87b
Compare
Comment on lines
+525
to
+527
| requireRecordSetEqual(t, base.records.rts, snaps[i].records.rts, arms[i].name, "resource type") | ||
| requireRecordSetEqual(t, base.records.ress, snaps[i].records.ress, arms[i].name, "resource") | ||
| requireRecordSetEqual(t, base.records.ents, snaps[i].records.ents, arms[i].name, "entitlement") |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the new full-record oracle covers the three families that moved to the bulk path but not the fourth. Grants also moved onto the non-upserting bulk path in this PR (single BulkGrantShard, identity fold at Finish), and the grant oracles above key by parityGrantRef — (entitlement id, principal) — so they never compare the grant's own transformed external id or its non-expansion annotations. A fold that keeps the wrong external id, or a translator that drops an annotation, passes all four grant assertions.
No fixture here has two grants sharing (entitlement, principal) with distinct external ids either, so the earliest-discovered_at-wins fold rule that now applies on the sanitize path has no coverage from this arm. Adding grants to fullRecords (keyed by parityGrantRef, with the same identity-only slimming the entitlement normalizer uses) plus one duplicate-identity pair would close both gaps.
Contributor
Author
There was a problem hiding this comment.
Factual, but decline, largely: Floor crossover at 15 shards for every sortSlots. sortSlots = min(4, max(2, GOMAXPROCS/2)) ∈ {2,3,4}; 3 + 4·14 + 4 = 63 < 64 and 3 + 4·15 + 2 = 65 ≥ 64. Correct.
~1,100 vs ~2,200 runs. Correct.
Fold rule (earliest discovered_at, ties to smallest id, needs_expansion OR'd). Matches recordIdentityInfoWins/mergeGrantRecordInto.
PrincipalRef carries the parent, so keeping ParentResourceId in the normalizer is right. Correct; principalRefToStubResource sets it.
"Budget test exempts the floor-bound regime." Accurate: bulk_import_budget_test.go:25 only asserts liveArenas × chunk ≤ budget when chunk > floor. That's deliberate (the floor trades budget for fan-in, and the test comment says so), but the bot is right that it's a gap in what the test pins.
Residual: no sanitize-path fixture for the duplicate-identity fold. Accurate — the cross-engine parity matrix can't host it because a sqlite destination keeps both rows and a pebble destination folds to one, so the arms diverge by design.
… parity Doc corrections, each verified against the code: - WithConvertParallelism: "sort memory does not scale with fan-out" only holds while the derived chunk size stays above the 16MiB floor, which binds at ~15 lanes (liveArenas = 3 + 4*shards + 4). State the bound and the ~4x16MiB-per-lane growth past it. - ToPebble overview and convertGrants: both still described the pre-spill model (ordered scans streaming straight into per-bucket SSTs, grant shards producing non-overlapping SSTs). Only resource types take the strictly-increasing path now; resources, entitlements, and grants are spill-sorted and k-way merged at Finish, with every grant shard's runs merged into one grants SST. - bulkImportSortBudgetBytes: a 17GB grant family is ~1,100 runs at the floor, not "a few hundred"; restate the floor's benefit as half the runs 8MiB chunks produce. - bulk_sink.go: "only surfaces on corrupt input" is true for resource types/resources/entitlements but not grants, which key by structural identity that UNIQUE(external_id, sync_id) does not cover. Legacy distinct-external-id/identical-ref grants are valid input; they now fold (earliest discovered_at wins, needs_expansion OR'd) where PutGrants was last-write-wins. Test: the cross-engine parity oracle compared resource types, resources, and entitlements field-by-field but only compared grants by ref, sources, and expansion blob, so a grant's own transformed external id was never checked across the sqlite/pebble write paths. Add grants to fullRecords, normalized to pebble's identity-only stubs and with GrantExpandable dropped (sqlite's plain ListGrants does not re-attach the side column; pebble's does; the blob is already compared via PendingExpansion). Co-authored-by: Cursor <cursoragent@cursor.com>
ggreer
approved these changes
Sep 4, 2026
| func (c *spillChunkCursors) key(i int) []byte { return c.keyBufs[i] } | ||
| func (c *spillChunkCursors) val(i int) []byte { return c.valBufs[i] } | ||
|
|
||
| func (c *spillChunkCursors) closeChunk(i int) { |
Contributor
There was a problem hiding this comment.
Not really a problem, but it'd be nice to return an error if file close fails. Same for any os.Remove() error in advance().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
perf(pebble): unlink spill chunks during merge, sort resources on import
The two k-way merge helpers held every sorted chunk file open until the
staging directory was torn down, so for the length of each merge the
staging directory carried the same entries twice: once in the chunks and
once in the SST being built from them. Chunks are now closed and unlinked
as soon as the merge reads their last entry, bounding the overlap to the
chunks still in flight. Unlinking is confined to the exhausted-chunk path,
where the file is provably fully consumed; a merge that fails partway
closes its descriptors and leaves the rest to the staging-dir teardown
that already owned them. Both helpers back the bulk import, the deferred
grant index, the id-index migration, the segment layer, and the digest
build, and the index builds run at EndSync on every pebble sync, so this
bounds peak staging for all of them.
AddResources also no longer requires rows pre-sorted by
(resource_type_id, resource_id); it routes them through the same spill
sorter entitlements already use. A converter scanning SQLite with a
matching ORDER BY satisfied the old precondition for free, but a producer
that rewrites resource ids cannot — the c1z sanitizer HMACs them, so its
output order is unrelated to the order it read. Sorting inside the
importer keeps every producer on one path instead of making sortedness a
precondition each one has to re-establish, and it holds when a single
resource type is too large to sort in memory.