perf: reduce DuckDB push write amplification - #992
Merged
Conversation
DuckDB mirror pushes rewrote a session's dependent rows whenever a modified session reached the push window, even when only metadata changed or an active transcript had only appended a suffix. That caused avoidable single-file churn and left reclaimed blocks for later DuckDB maintenance. Add DuckDB push fingerprints for messages, tool calls, tool-result events, and usage events; skip unchanged message dependents; append only verified suffix messages for active-session growth; and fall back to full replacement on any historical or dependent-row mismatch. Keep PostgreSQL skip guards in parity by including tool-result events there too. Run threshold-gated DuckDB CHECKPOINT maintenance after committed mutating pushes but before advancing local push state. Cover the lifecycle with tests that assert preserved row IDs, usage refresh, result-event fallback, and checkpoint gating.
roborev: Combined Review (
|
DuckDB append and skip decisions now include the mirrored message id stream in the local-vs-target fingerprint. That prevents curation refreshes from copying pins with newly assigned local message ids while DuckDB messages still carry an older id assignment. The regression test rewrites a local session with identical transcript content but new message ids, then verifies the incremental push falls back to a dependent-row rewrite and leaves pinned messages joined to mirrored rows.
DuckDB checkpoint maintenance is a local-file concern. Quack pushes route storage through a remote server, so running pragma_database_size or CHECKPOINT on the client DuckDB handle can target the wrong catalog or fail after mutations have committed but before the push watermark advances. Gate the checkpoint hook for remote targets and cover the gate with a regression test. Also remove misleading timestamp plumbing from the fast-path lifecycle test helper; DataVersion writes already stamp local_modified_at through the local store.
The quack checkpoint gate needs Push-level coverage because the failure mode happens after remote mutations commit and before local sync state is finalized. Extend the existing real quack push smoke test to assert that a successful remote push persists the watermark.
Legacy or manually repaired DuckDB mirrors can contain message rows where id is NULL even though normal pushes write the mirrored local primary key. The new message-id fingerprint should treat that as a mismatch to repair, not as a scan error that aborts incremental push. Scan message ids as nullable and add regression coverage that corrupts a mirrored id to NULL, then verifies the next incremental push rewrites the session and restores the local id.
roborev: Combined Review (
|
roborev: Combined Review (
|
DuckDB incremental push tests rewrite local sessions after the initial push watermark has advanced. Leaving fixture LocalModifiedAt values at their original timestamps meant those tests could skip the session selection path they were intended to exercise. Set fresh local modification times in the rewrite helpers so the follow-up incremental pushes deterministically cover the message-id repair and lifecycle fallback paths.
roborev: Combined Review (
|
The previous review fix wrote Session.LocalModifiedAt in helpers that use WriteSessionBatchAtomic, but that field is ignored by the session upsert. These helper writes already pass DataVersion, which is the path that stamps local_modified_at for incremental push selection. Remove the inert assignments and document the DataVersion behavior at the write sites so the tests continue to rely on the persisted timestamp mechanism they actually exercise.
roborev: Combined Review (
|
A checkpoint failure can happen after hard-deleted DuckDB rows have already been removed but before the push boundary fingerprints are finalized. On the next retry those rows are no longer reported as stale, so old cached fingerprints could be copied forward and later suppress a restored or missing-row session push. Delete reconciliation now returns the mirror session IDs that remain after stale cleanup, and incremental skip filtering runs only after cached fingerprints have been pruned against that mirror state. A regression covers the checkpoint-failure retry path without advancing the watermark.
roborev: Combined Review (
|
Incremental DuckDB pushes could prune a cached fingerprint after discovering a missing mirror row, then advance the watermark without adding the unchanged local session back to the push candidate set. That left the mirror missing the row until a full push or later local modification. Treat pruned fingerprints that still have in-scope local sessions as repair candidates before fingerprint filtering, so the normal push path reinserts the session and dependents. The regression test deletes a mirrored row after a successful full push and verifies the next incremental push restores it.
roborev: Combined Review (
|
DuckDB skip and append fast paths updated the session, usage, and secret rows but depended on the later global curation refresh to mirror pin changes. If another session failed in the same push, that refresh was skipped while successful sessions could still have their fingerprints persisted. Refresh pinned_messages for the session inside the skip and append transactions, and reuse that session-scoped helper for filtered curation refreshes. The new regressions call the fast paths directly after changing a pin so the global curation pass cannot mask stale mirrored pins.
roborev: Combined Review (
|
cpcloud
added a commit
that referenced
this pull request
Jul 7, 2026
Session push fingerprints included local stat fields that can churn on no-op rewrites or file touches, making unchanged sessions push candidates and rewriting mirror rows. Exclude file_mtime/local_modified_at in PostgreSQL and file_size/file_mtime/file_inode/file_device/local_modified_at in DuckDB while keeping file_path and the content-derived file_hash. Mirror write paths still write every stat column when a session really pushes, but those mirror stat columns can now go stale while session content is unchanged. Stored fingerprints from older builds use the previous field set, so the first push after upgrade will re-push sessions once before the new fingerprints settle, matching the upgrade shape of the earlier #992 fingerprint change.
cpcloud
added a commit
that referenced
this pull request
Jul 7, 2026
Session push fingerprints included local stat fields that can churn on no-op rewrites or file touches, making unchanged sessions push candidates and rewriting mirror rows. Exclude file_mtime/local_modified_at in PostgreSQL and file_size/file_mtime/file_inode/file_device/local_modified_at in DuckDB while keeping file_path and the content-derived file_hash. Mirror write paths still write every stat column when a session really pushes, but those mirror stat columns can now go stale while session content is unchanged. Stored fingerprints from older builds use the previous field set, so the first push after upgrade will re-push sessions once before the new fingerprints settle, matching the upgrade shape of the earlier #992 fingerprint change.
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.
DuckDB mirror pushes now avoid rewriting session dependents when the target already matches local message, tool, result-event, and usage fingerprints. Suffix-only active-session growth appends just the new messages and dependent rows, while historical or dependent-row mismatches still fall back to a full replacement.
The skip and append fast paths refresh per-session pinned messages and secret findings in the same mutation transaction, so a later partial push failure cannot leave those dependent rows stale while persisting fresh fingerprints.
Incremental DuckDB pushes also repair cached fingerprints whose mirror rows are missing by re-adding the unchanged local session to the push candidates before skip filtering. That keeps retry and partial-failure cleanup from advancing the watermark while the mirror is still missing a session.
The DuckDB backend runs threshold-gated checkpoint maintenance after committed mutating pushes, before local watermark advancement. PostgreSQL skip fingerprints now include tool-result events so both push backends agree on stale event detection.
The main review points are
internal/duckdb/sync.go,internal/duckdb/push.go,internal/duckdb/push_fingerprint.go,internal/duckdb/checkpoint.go, and the lifecycle coverage ininternal/duckdb/sync_fastpath_test.go. Checkpointing is intentionally partial reclamation; large historical rewrites may still require full-copy compaction outside the normal push path.