Skip to content

fix(storage): never collect blobs that retained history still references - #472

Merged
tnunamak merged 4 commits into
mainfrom
tnunamak/history-blob-gc-1024
Oct 24, 2026
Merged

tnunamak merged 4 commits into
mainfrom
tnunamak/history-blob-gc-1024

Conversation

@tnunamak

@tnunamak tnunamak commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Impact

A blob that only a retained history row references can be deleted automatically during migration or startup. The history row keeps its blob_ref, but the bytes are gone. Affected blobs are pre-v4 history blobs with no current record and no claim.

The regression tests show the loss directly: after a writable open, getBlobMeta and getBlobBytes return undefined for the history-referenced blob.

Cause

  • packages/server/src/storage/pdpp-records-sqlite-store.ts:228-232 (v4 migration): blob claims are seeded only from pdpp_records, not pdpp_record_changes. Historical blobs get no claim.
  • packages/server/src/storage/pdpp-records-sqlite-store.ts:443-462 (sweepStaleBlobClaims): on origin/main it ran the collector at startup. The collector deleted any blob with no claim and no live current record. It never consulted history.
  • packages/server/src/storage/pdpp-records-sqlite-store.ts:690: sweepStaleBlobClaims runs on every writable open, so the deletion happens at startup.

Fix

Three changes, in this order:

  1. History-aware collection. deleteUnclaimedBlobs also requires that no pdpp_record_changes row references the blob. The predicate is built by one helper, noRetainedHistoryReferenceSql, which both deletes use.
  2. Startup gate. collectStartupUnclaimedBlobs runs in the startup sweep only. If any history row holds invalid JSON, it skips blob collection and logs one warning with the row count. A row that cannot be parsed may reference any blob, so the sweep cannot prove which blobs are safe. Claim cleanup still runs.
  3. Owner erase is ungated. eraseInstanceData still calls deleteUnclaimedBlobs directly. deleteConnection and resetInstanceBinding both reach it. It deletes the erased instance's history first, so the collector can free its blobs.

Index (v15)

Schema v15 adds idx_pdpp_record_changes_blob_ref on

CAST(CASE WHEN json_valid(data) THEN json_extract(data, '$.blob_ref.blob_id') END AS TEXT)
  • json_valid guard: json_extract raises malformed JSON on a non-JSON history row. Without the guard, CREATE INDEX fails, the store refuses to start, and later inserts of such a row fail too. The guard keeps the index buildable and inserts working.
  • Outer CAST: without it the planner keeps SCAN even with the index present, because the outer pdpp_blobs.blob_id column has TEXT affinity. A CASE alone does not provide the affinity.

Raw EXPLAIN QUERY PLAN, correlated history subquery only:

before (v14, no index):  SCAN h
after  (v15, guarded):   SEARCH pdpp_record_changes USING COVERING INDEX idx_pdpp_record_changes_blob_ref (<expr>=?)

The DELETE statement plans the same way.

Before / after

Tests:

  • pdpp-history-blob-gc.test.ts (3 cases): a history-only blob survives migration and startup until owner erase.
  • pdpp-history-blob-ref-index.test.ts (4 cases):
    • the planner uses the index;
    • a v14 store migrates to v15 without changing history rows;
    • with a malformed history row present, startup collection deletes nothing (including the orphan) and warns once with the count, on every startup;
    • with a malformed row present, owner erase still frees the blobs, and the other instance's malformed row survives.

Results:

  • Before the fix (store reverted to origin/main, GC test kept): 3 failed / 3. Example: AssertionError: expected undefined to be '98fcd9c1…'.
  • Mutation: the unguarded expression makes the malformed-row test fail with SqliteError: malformed JSON at open.
  • Mutation: bypassing the startup gate fails the skip test.
  • Mutation: removing the outer CAST leaves the planner on SCAN.
  • packages/server/src/storage (28 files): 694 passed / 694.
  • tsc --build packages/server: exit 0.
  • ESLint and Prettier clean on the changed files.

Review focus

  • Storage growth: history-referenced blobs stay until owner erase. This is intended, but it means more bytes are retained.
  • While any history row is unreadable, startup no longer collects blobs at all. The warning repeats on every startup until the row is removed. This is the intended trade-off.
  • Owner erase is ungated by design. Its collector can still free a blob that only an unreadable history row of a different instance references. That row cannot be parsed, so the reference is unknowable. Gating erase too would mean an owner erase of one connection cannot free its own blobs while any other connection holds bad history.
  • Migration cost: CREATE INDEX runs once, at first open after upgrade, and scans all history rows.
  • Not measured on a large real Desktop store; the plan change is the evidence here.

🤖 Generated with Claude Code

The v4 migration reconstructs blob claims only from current records, and
blob GC never consulted pdpp_record_changes. A pre-v4 blob referenced only
by a history row was therefore collected during migration or startup while
the history row still pointed at it.

Blob GC now also skips any blob referenced by retained history. Explicit
owner erase paths delete history before calling the collector, so they
still free blobs.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
tnunamak and others added 3 commits October 24, 2026 07:46
The blob GC's history check scanned every pdpp_record_changes row for each
candidate blob, at every writable open. EXPLAIN QUERY PLAN showed
`SCAN pdpp_record_changes` inside that correlated subquery.

Schema v15 adds an expression index on the blob reference. The GC predicate
is built by one helper, so the query and the index expression cannot drift.
The CAST gives the expression TEXT affinity: without it the planner does not
search the index for the TEXT blob_id comparison.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
…JSON

json_extract raises "malformed JSON" on a non-JSON history row. The plain
expression index would therefore fail CREATE INDEX in the v15 migration, so
the Personal Server would refuse to start, and every later insert of such a
row would fail too.

The indexed expression and the GC predicate now both guard with json_valid.
The outer CAST keeps TEXT affinity; a CASE alone stops the planner from
searching the index. A malformed row has no parseable blob reference, so it
does not protect a blob.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
…adable

A history row whose JSON cannot be parsed may reference any blob, and the
startup sweep cannot tell which. While any pdpp_record_changes row holds
invalid JSON, startup skips blob collection and logs one warning with the
row count. Claim cleanup still runs.

Explicit owner erasure calls the collector directly and is not gated.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
@tnunamak
tnunamak merged commit 5397c61 into main Oct 24, 2026
5 checks passed
@tnunamak
tnunamak deleted the tnunamak/history-blob-gc-1024 branch October 24, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant