Skip to content

perf: batch message inserts in a per-session transaction during sync (#59) - #64

Closed
elecnix wants to merge 1 commit into
mainfrom
batch-insert-tx-l
Closed

perf: batch message inserts in a per-session transaction during sync (#59)#64
elecnix wants to merge 1 commit into
mainfrom
batch-insert-tx-l

Conversation

@elecnix

@elecnix elecnix commented Aug 9, 2026

Copy link
Copy Markdown
Owner

What this does

When the sync step reads a session file, it used to write every message to the database
as its own little "save", with the file's resume marker as a separate save afterwards.
Now, all of a session's messages — plus that resume marker — are written in a single save
(one transaction per session). Fewer saves means the sync finishes faster, and it also
makes sync safe if it's interrupted partway: either the whole session is recorded, or none
of it is. You never end up with half a conversation in the database.

Acceptance bar (from issue #59)

  • Same rows on disk: a re-sync of untouched files is still a no-op; message counts and
    stats are identical. Covered by the existing sync tests plus the new ones.
  • Interrupted sync leaves no partial session: each session's upsert, message inserts,
    cursor, and count all commit atomically. A mid-session failure rolls the whole session
    back — and because the resume cursor is updated inside the same transaction, it can
    never advance past messages that were rolled back, so a failed session is re-runnable and
    nothing is skipped.

What I measured (and against what)

Synthetic corpus on a scratch /tmp SQLite DB (WAL): 20 sessions × 5,000 messages =
100,000 message rows, single-threaded, zero LLM calls. Same corpus, same machine, the only
variable being the transaction boundary.

  • Baseline (per-row autocommit, prior code): 5,146 ms
  • This change (per-session transaction): 1,893 ms — about 2.7× faster

The full-text trigger still fires once per message (that cost is unchanged, matching the
issue's note), so the speedup is the commit amortisation, not the trigger work.

Technical detail

runSync dispatches to syncPiSession and syncClaudeSession. Both now build a
db.transaction(...) that wraps, in order: the upsertSession, the message insert loop,
and the updateCursor + message_count updates. The transaction returns the number of
messages inserted so result counters only reflect sessions that actually committed; a
throw rolls the whole session back and surfaces via runSync's per-session error handling.

The two load-bearing guarantees are covered by new tests in
tests/component/sync.test.ts that arm a real BEFORE INSERT trigger raising a
mid-transaction error:

  1. A session that fails mid-sync leaves no partial session (no session row, no
    messages — the good message that preceded the failure is rolled back too).
  2. An appended line that fails does not advance the resume cursor past the rolled-back
    rows — last_line and message_count stay at the last committed state, so the next
    sync retries from the right place.

Full suite passes: 438 unit/component tests and the 21-test integration run.
— steady-lynx-13

Wrap the Pi and Claude session insert loops (plus the upsert, cursor, and
message-count updates) in a single per-session db.transaction instead of
autocommitting every insertMessage row. Committing once per session
amortises commit cycles. The cursor update lives inside the same
transaction so a rollback can never advance it past rows that were
discarded — an interrupted sync leaves no partial session and is
re-runnable (issue #59).

Co-authored-by: steady-lynx-13 <steady-lynx-13@pi-agent.local>
@elecnix

elecnix commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Closing this in favour of #63and the most valuable thing in this PR is being carried over, not discarded.

Both PRs implemented issue #59 independently: same task, same brief, two different model configurations, neither aware of the other. Both are green, and both found the real subtlety unaided — that batching messages without moving the resume cursor inside the same transaction leaves a session partially imported with its cursor advanced past rows that were never written, so the next run silently skips them.

Where this PR was better: it tested the property. #63 states atomicity in its description; this PR proves it, with two cases that are exactly the right ones — a mid-sync failure leaving zero session and message rows, and a partial failure not advancing the resume cursor past rolled-back rows. That second case is the reason the issue exists, and a change of this kind without it is a claim rather than a guarantee.

That requirement has been handed to #63's author, specified as behaviour rather than as code — its implementation differs (+98/−84 against this PR's +120/−96), and a test shaped for this one could pass there without ever touching its rollback path, which is worse than no test because it looks like coverage. They have also been asked to falsify each test by temporarily moving the cursor update outside the transaction and confirming it goes red.

On cost, #63 was produced for $0.0525 against $0.0678 here — 29% less, in fewer calls, for a smaller diff.

Thank you — the tests were the sharpest contribution of the pair, and they are why the merged change will be verified rather than merely asserted.

@elecnix elecnix closed this Aug 9, 2026
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.

1 participant