Skip to content

perf(restore): drop unused page index map on restore decode paths - #1497

Open
corylanou wants to merge 4 commits into
perf/1477-ltx-spill-dirfrom
perf/1486-restore-skip-page-index
Open

perf(restore): drop unused page index map on restore decode paths#1497
corylanou wants to merge 4 commits into
perf/1477-ltx-spill-dirfrom
perf/1486-restore-skip-page-index

Conversation

@corylanou

@corylanou corylanou commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Description

Adds two unexported helpers and wires them into the restore paths:

  • newRestoreDecoder calls ltx.Decoder.SetRetainPageIndex(false) so Close() validates but does not materialize the per-page index map.
  • newRestoreCompactor sets the restore header flags and enables output page-index spilling via ltx.Compactor.SetSpillDir, used by Replica.Restore (spills to the restore output directory) and the VFS Hydrator (spills to the hydration file's directory).

newRestoreDecoder is used on the three decode paths that materialize a database file and never read the decoder's page index:

The other six ltx.NewDecoder call sites were audited and left unchanged: db.go:1704, db.go:2795, vfs.go readPageSizeFromInfo, and Hydrator.ApplyLTX decode only the header (or never call Close()), so no index is ever built there; db.go Pos() uses Verify() on a single small L0 file and is out of scope for this change.

Motivation and Context

Fixes #1486 — with #1483, which this PR is based on and depends on.

Full credit to @darkgnotic for the diagnosis: the issue includes the exact call path, both allocations, and an RSS graph showing a ~124 GB restore flat at ~8 GB until the final instant, then OOM-killed past 20 GB.

Decoder.Close() in ltx had two per-database costs on the restore path, both wasted because DecodeDatabaseTo never reads dec.PageIndex():

  1. io.ReadAll(dec.r) slurping the whole trailing page-index block into one []byte.
  2. DecodePageIndex(...) building a map[uint32]PageIndexElem with one entry per page (~2.5 GB at ~30M pages).

The split between the two PRs: the ltx bump in #1483 (v0.5.3-0.20260828134549) removes cost 1 by streaming the index instead of slurping it, and adds the SetRetainPageIndex opt-out — but nothing in litestream called it, so the map (cost 2) was still built. This PR wires up the opt-out. Neither PR alone fully closes #1486; this one completes it.

Correctness note: with retention off, Decoder.Close() in the pinned ltx version still streams, checksums, and validates the page index (entry ordering, overlap, entry count vs. decoded pages, page-sequence hash, file checksum, post-apply checksum) — it only skips materializing the map, and PageIndex() returns nil.

Adversarial review also flagged the last per-database allocation on the restore pipe: the ltx.Compactor's encoder kept its own output page index in memory (compact 24-byte entries, ~687 MiB at 30M pages) because the restore paths never called SetSpillDir, unlike store compaction (compactor.go:173) and snapshots (db.go:2890). This PR fixes that too via a newRestoreCompactor helper: Replica.Restore spills into the restore output directory and the VFS Hydrator into the hydration file's directory — both writable by construction, consistent with the spill-dir rationale from #1477 (hardened images may lack /tmp). The compactor's input decoders already disable retention upstream, so restore memory no longer scales with database page count anywhere in the pipe.

How Has This Been Tested?

  • New TestNewRestoreDecoder (written first, watched fail): asserts PageIndex() is nil after DecodeDatabaseTo with the helper while decoded bytes are intact, that the stock decoder still retains the index (guards against silently relying on an upstream default change), and that a corrupted page index still fails the decode with retention off (validation is not skipped).
  • Hardened after adversarial review: an out-of-order index entry is rejected by structural validation (which runs before any checksum comparison) with identical errors in both retention modes, and a checksum-tracked snapshot with a wrong post-apply checksum fails with a post-apply checksum error — the NoChecksum fixture alone could not exercise that path.
  • New TestNewRestoreCompactor: compacts a snapshot plus an incremental through the spill-configured helper into DecodeDatabaseTo, verifying the merged output and that the spill directory is left empty after cleanup. (Honest limitation: the fixture is far below the 1M-entry spill threshold, so actual spill mechanics are covered by ltx's own encoder_spill_test.go, not here — the compactor API does not expose the threshold.)
  • New TestNewRestoreCompactor_ConsumerAbort: closes the pipe reader mid-stream and asserts Compact unblocks with an error and cleanup leaves the spill directory empty — the contract the error paths below rely on.

A second adversarial review pass on the spill commit found that neither restore path closed the pipe reader when DecodeDatabaseTo failed, leaving the compactor goroutine blocked in a pipe write with its deferred cleanup (and any active spill file) stranded. Fixed by closing the reader with the decode error, mirroring store compaction's Compactor.compact; the hydrator additionally waits for the compactor goroutine to exit so its caller-side Cleanup cannot run concurrently with Compact. That pass also confirmed the spill temp file (.ltx-page-index-*.tmp via os.CreateTemp) cannot collide with the restore temp output and is rejected by litestream's directory discovery, and flagged one pre-existing, unrelated data race (h.compactor written in Restore vs. read in Status) that predates this PR and is left untouched.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project (go fmt, go vet)
  • I have tested my changes (go test ./...)
  • I have updated the documentation accordingly (if needed)

https://claude.ai/code/session_01JAbGJy6uwUcj87ZRJkVXnF

The ltx decoder builds a map[uint32]PageIndexElem with one entry per
page during Close. On restore paths the index is never read, so for
large databases the map is built and immediately discarded at peak
memory (issue #1486, ~30M pages).

Add newRestoreDecoder, which disables page index retention, and use it
on the three decode paths that never read the index: Replica.Restore,
Replica.applyLTXFile, and Hydrator hydration. Validation of the index
in ltx Decoder.Close is unaffected by the flag.

Claude-Session: https://claude.ai/code/session_01JAbGJy6uwUcj87ZRJkVXnF
Address adversarial review findings: prove structural index validation
still runs with retention off (out-of-order entry rejected identically
in both retention modes, before any checksum comparison) and exercise
post-apply checksum validation with a checksum-tracked snapshot fixture,
which the NoChecksum fixture could not reach.

Claude-Session: https://claude.ai/code/session_01JAbGJy6uwUcj87ZRJkVXnF
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

PR Build Metrics

⚠️ Attention needed — vulnerabilities found

Check Status Summary
Binary size 37.19 MB (0.0 KB / 0.00%)
Dependencies No changes
Vulnerabilities ⚠️ Issues found — expand details below
Go toolchain 1.25.14 (latest)
Module graph 1230 edges (0)

Binary Size

Size Change
Base (73b7d0b) 37.19 MB
PR (f1b673e) 37.19 MB 0.0 KB (0.00%)

Dependency Changes

No dependency changes.

govulncheck Output

=== Symbol Results ===

Vulnerability #1: GO-2026-6303
    Source-address critical option not enforced for non-public-key auth
    callbacks in golang.org/x/crypto/ssh
  More info: https://pkg.go.dev/vuln/GO-2026-6303
  Module: golang.org/x/crypto
    Found in: golang.org/x/crypto@v0.52.0
    Fixed in: golang.org/x/crypto@v0.55.0
    Example traces found:
      #1: internal/testingutil/testingutil.go:485:45: testingutil.MockSFTPServer calls ssh.NewServerConn

Your code is affected by 1 vulnerability from 1 module.
This scan also found 1 vulnerability in packages you import and 3
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.

Build Info

Metric Value
Build time 41s
Go version go1.25.14
Commit f1b673e

History (3 previous)

Commit Updated Status Summary
974ab8e 2026-08-31 22:31 UTC 37.19 MB (0.0 KB / 0.00%)
81c8198 2026-08-31 21:51 UTC 37.19 MB (0.0 KB / 0.00%)
512388f 2026-08-31 21:32 UTC 37.19 MB (0.0 KB / 0.00%)

🤖 Updated on each push.

@github-actions github-actions Bot added the metrics: vulns-found govulncheck found vulnerabilities label Aug 31, 2026
The restore pipe's compactor never enabled index spilling, so its
encoder held a 24-byte entry per output page in memory (~687 MiB at 30M
pages) for the duration of the restore. Spill into the restore output
directory for Replica.Restore and the hydration file's directory for the
VFS Hydrator, matching store compaction and snapshot encoding.

Claude-Session: https://claude.ai/code/session_01JAbGJy6uwUcj87ZRJkVXnF
If DecodeDatabaseTo returns early, the compactor goroutine on the other
side of the pipe stays blocked in a pipe write, so its deferred Cleanup
never runs and an active spill file is left behind. Close the pipe
reader with the decode error, mirroring the store compaction path. The
hydrator additionally waits for the compactor goroutine to exit so its
caller-side Cleanup cannot run concurrently with Compact.

Claude-Session: https://claude.ai/code/session_01JAbGJy6uwUcj87ZRJkVXnF
@corylanou
corylanou force-pushed the perf/1486-restore-skip-page-index branch from cdb854c to ca66a47 Compare August 31, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

metrics: vulns-found govulncheck found vulnerabilities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant