kayg/calternal
Watch
1
0
Fork
You've already forked calternal
 0
Code Issues 190 Pull requests 61 Projects Releases Packages Wiki Activity Actions 26

perf: reduce attachment index flushes during upload saves #831

Closed
kayg wants to merge 0 commits from perf/attachment-index-flush into dev
pull from: perf/attachment-index-flush
merge into: kayg:dev
Conversation 3 Commits - Files changed -
kayg commented 2026-11-01 17:18:36 +00:00
Owner
Copy link

Plan and cost

Keep a bounded LRU cache of up to 64 per-user attachment-index connections. Duplicate lookup and optional row insert share the cached connection. WAL with synchronous=FULL makes a changed row durable in the WAL before acknowledgement; SQLite checkpoints later at its normal page threshold, cache eviction or shutdown. Attachment bytes and parent-directory durability barriers are unchanged.

Index lookup remains O(log F) for F files. Cache lookup is O(1) plus an O(64) LRU update. If all entries are busy at capacity, an action uses a scoped connection and may checkpoint at close. No file bytes are retained.

Failure Effect Detection Recovery Gate
Index open/lookup fails Add fails before placement Action error Retry with same idempotency key Attachment action tests
Index write fails after placement Durable file remains; index is a hint Existing warning Retry the hint on a later upload Upload/retry tests
No attachment change 0 attachment-index sync calls Syscalls between save markers None needed Linux strace test
One attachment change 1 durable WAL sync Syscalls between save markers Replay the FULL-synced WAL Linux strace test
Cache full and every entry busy Scoped connection may checkpoint at close Debug event and trace Reopen index next action Bounded-cache test

Current-dev measurements

Base origin/dev: 55f35fcde3bc91ca3781055577b93e93819f6cd4. Seed 75931004, one 1k-note Home, four upload clients, 256 KiB, 60 seconds, and overlapping note saves. Same untraced matrix cell on dev-01:

Run Uploads Save samples p50 p95 Attachment-index syncs/upload
Before 696 60 17.88 ms 56.26 ms 4.017 (trace run)
After 1,520 59 13.29 ms 40.62 ms 1.017 (trace run)

The single shared-host p95 diagnostic improved 27.8%; it is not qualification. Trace counts were 1,868 syncs/465 uploads before (932 WAL, 469 DB, 467 directory) and 821/807 after (814 WAL, 5 DB, 2 directory). Traced latency is excluded. Linux test asserts 0 syncs for duplicate/no-change and 1 WAL sync for one new attachment; cache bound test passed with 65 busy users.

Exact commands, binary hashes and run notes are in docs/perf/attachment-index-flush.md. Dev-01 gates passed: cargo fmt --all --check, cargo test --locked -p calternal-attachments -j4 -- --nocapture (21 passed, 0 failed), cargo clippy --locked -p calternal-attachments --all-targets -j4 -- -D warnings, and cargo build --release --locked -p calternal -j4.

Perf-01 small all-SSD comparison

Five alternating matched pairs used the same 1k Home, 4 upload clients, 256 KiB attachments, 60-second window, and seed 75931004. All ten runs exited 0 with 59 or 60 save samples. Baseline was current dev 55f35fc; fixed was eef64732.

Metric Current dev Fixed
Median run save-ack p50 11.70 ms 12.20 ms
Median run save-ack p95 18.94 ms 18.33 ms
Median paired save-ack p95 ratio — 98.8%
Median paired upload response p95 ratio — 68.0%
Median paired throughput ratio — 148.9%

The cell meets its existing matrix limits (throughput at least 95%, response p95 and overlapping save-ack p95 at most 105%). Pair 1 and 5 save-ack p95 ratios were noisy and above 105%; the acceptance statistic is the median of all five paired ratios. The unchanged perf/budgets.json was used; it has no direct entry for uploads.256KiB.save_ack, so this result is judged against the existing relative matrix limits. No budgets were edited. The full 18-cell matrix still needs requalification after rebasing #788. Per-pair results are in docs/perf/attachment-index-flush-perf01.json.

## Plan and cost Keep a bounded LRU cache of up to 64 per-user attachment-index connections. Duplicate lookup and optional row insert share the cached connection. WAL with `synchronous=FULL` makes a changed row durable in the WAL before acknowledgement; SQLite checkpoints later at its normal page threshold, cache eviction or shutdown. Attachment bytes and parent-directory durability barriers are unchanged. Index lookup remains O(log F) for F files. Cache lookup is O(1) plus an O(64) LRU update. If all entries are busy at capacity, an action uses a scoped connection and may checkpoint at close. No file bytes are retained. | Failure | Effect | Detection | Recovery | Gate | |---|---|---|---|---| | Index open/lookup fails | Add fails before placement | Action error | Retry with same idempotency key | Attachment action tests | | Index write fails after placement | Durable file remains; index is a hint | Existing warning | Retry the hint on a later upload | Upload/retry tests | | No attachment change | 0 attachment-index sync calls | Syscalls between save markers | None needed | Linux strace test | | One attachment change | 1 durable WAL sync | Syscalls between save markers | Replay the FULL-synced WAL | Linux strace test | | Cache full and every entry busy | Scoped connection may checkpoint at close | Debug event and trace | Reopen index next action | Bounded-cache test | ## Current-dev measurements Base `origin/dev`: `55f35fcde3bc91ca3781055577b93e93819f6cd4`. Seed `75931004`, one 1k-note Home, four upload clients, 256 KiB, 60 seconds, and overlapping note saves. Same untraced matrix cell on dev-01: | Run | Uploads | Save samples | p50 | p95 | Attachment-index syncs/upload | |---|---:|---:|---:|---:|---:| | Before | 696 | 60 | 17.88 ms | 56.26 ms | 4.017 (trace run) | | After | 1,520 | 59 | 13.29 ms | 40.62 ms | 1.017 (trace run) | The single shared-host p95 diagnostic improved 27.8%; it is not qualification. Trace counts were 1,868 syncs/465 uploads before (932 WAL, 469 DB, 467 directory) and 821/807 after (814 WAL, 5 DB, 2 directory). Traced latency is excluded. Linux test asserts 0 syncs for duplicate/no-change and 1 WAL sync for one new attachment; cache bound test passed with 65 busy users. Exact commands, binary hashes and run notes are in `docs/perf/attachment-index-flush.md`. Dev-01 gates passed: `cargo fmt --all --check`, `cargo test --locked -p calternal-attachments -j4 -- --nocapture` (21 passed, 0 failed), `cargo clippy --locked -p calternal-attachments --all-targets -j4 -- -D warnings`, and `cargo build --release --locked -p calternal -j4`. ## Perf-01 small all-SSD comparison Five alternating matched pairs used the same 1k Home, 4 upload clients, 256 KiB attachments, 60-second window, and seed `75931004`. All ten runs exited 0 with 59 or 60 save samples. Baseline was current dev `55f35fc`; fixed was `eef64732`. | Metric | Current dev | Fixed | |---|---:|---:| | Median run save-ack p50 | 11.70 ms | 12.20 ms | | Median run save-ack p95 | 18.94 ms | 18.33 ms | | Median paired save-ack p95 ratio | — | 98.8% | | Median paired upload response p95 ratio | — | 68.0% | | Median paired throughput ratio | — | 148.9% | The cell meets its existing matrix limits (throughput at least 95%, response p95 and overlapping save-ack p95 at most 105%). Pair 1 and 5 save-ack p95 ratios were noisy and above 105%; the acceptance statistic is the median of all five paired ratios. The unchanged `perf/budgets.json` was used; it has no direct entry for `uploads.256KiB.save_ack`, so this result is judged against the existing relative matrix limits. No budgets were edited. The full 18-cell matrix still needs requalification after rebasing #788. Per-pair results are in `docs/perf/attachment-index-flush-perf01.json`.
docs(perf): record attachment index flush baseline
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
Markdown extraction contract / markdown-native-contract (pull_request) Has been cancelled
Details
Markdown extraction contract / markdown-editor-contract (pull_request) Has been cancelled
Details
8f57b68941
kayg commented 2026-11-01 18:11:59 +00:00
Author
Owner
Copy link

Independent review, round 1

Reviewed #831 at d2a2b8776ab87cba21b3862896ddcff4d51a5378 against 55f35fc.

P2: release the global cache mutex before opening or closing SQLite connections. In crates/attachments/src/lib.rs:197–202, Attachments::index holds indexes across all of IndexCache::get. Both Index::open paths (:112, :116) perform filesystem/SQLite work under that lock. Eviction (:108) also drops the last idle Arc<Index> under it, which can checkpoint and sync the database on close. Consequently, a cold user or an eviction stalled on storage prevents every other user's add/check from even obtaining an already cached connection. Moving the caller to spawn_blocking does not remove this cross-user serialization. This introduces a shared I/O bottleneck where the prior opens were independent. Remove an eviction candidate under the lock and drop it after unlocking; perform opens outside the global lock, using a per-user initialization guard/recheck to preserve the 64-entry bound. Add a gate that pauses one user's open/eviction and verifies another user's warm cache lookup completes before it resumes.

Durability and lifecycle checks:

  • The base revision already sets journal_mode=WAL; this change strengthens synchronous=NORMAL to FULL and retains connections. A successful FULL WAL commit is durable before return, assuming the filesystem/device honors sync. Attachment bytes still go through file sync_all, placement, and directory fsync. Index-write errors remain logged and acknowledged because the index is a hint; the attachment file's durability does not depend on that row.
  • SQLite's default auto-checkpoint remains enabled (normally 1,000 WAL pages). A normal last-connection close also attempts a checkpoint. FULL checkpoints sync WAL before copying and sync the database before WAL reuse. A failed/incomplete checkpoint leaves recovery in the WAL; durable acknowledgement does not require immediate checkpoint completion. Process termination need not run a destructor for committed WAL recovery to work.
  • docs/storage.md explicitly keeps WAL/SHM companions beside databases, requires stopped-server coordinated copies or SQLite backup plus coordinated snapshots, includes the entire attachments metadata directory in moves, and requires current state for rollback. docs/backup.md also warns against copying the live DB alone. I found no production path at this head that copies/deletes just this attachment DB. SHM is reconstructible coordination state; WAL must not be omitted from a raw live-state snapshot.
  • The cache retains at most 64 entries, evicting the least-recent idle entry (strong_count == 1). Busy entries are retained; all-busy capacity falls back to scoped connections, so 64 bounds retained cache entries, not total in-flight connections. There is no idle expiry or explicit per-user invalidation. A cached connection can outlive an externally moved/replaced/deleted state directory. At this head, storage moves/restores are documented offline, state is separate from Home content, and I found no implemented live Home/account deletion or tiering move of this state DB. This is therefore a future lifecycle integration requirement, not a demonstrated data-loss path in this revision: any such live operation must drain leases and invalidate before replacing/removing state.
  • Mutex<Connection> serializes access across blocking workers; rusqlite's Connection is Send and does not need to be Sync directly. Independent SQLite connections/processes use WAL file locking and the busy timeout. Full server instances are additionally excluded by both storage-root locks; per-process attachment placement locking alone is not a multi-server guarantee.
  • Older rollback-journal databases can convert on open when SQLite obtains the required locks; failure returns an error. There is no schema change. Downgrading to the base keeps WAL support, although its NORMAL setting restores the older, weaker index-commit durability policy.

Validation: source review plus a small Python SQLite probe via netbird ssh claude@calternal-dev-01-compute02.vpn.kayg.org, in the uncommitted ~/Developer/Worktrees/review-831-r1 checkout. Rollback-journal seed -> WAL/FULL insert -> child os._exit(0) without connection close -> reopen retained both rows and passed integrity_check; FULL wal_checkpoint(TRUNCATE) and reopen also passed (system SQLite 3.46.1). This checks SQLite recovery semantics, not the bundled rusqlite build or physical power loss. No builds or full Rust gates rerun; no pushes or committed changes.

Verdict: request changes for the P2 cross-user I/O lock above. No P0/P1 findings; no additional P3 findings.

## Independent review, round 1 Reviewed #831 at `d2a2b8776ab87cba21b3862896ddcff4d51a5378` against `55f35fc`. **P2: release the global cache mutex before opening or closing SQLite connections.** In `crates/attachments/src/lib.rs:197–202`, `Attachments::index` holds `indexes` across all of `IndexCache::get`. Both `Index::open` paths (`:112`, `:116`) perform filesystem/SQLite work under that lock. Eviction (`:108`) also drops the last idle `Arc<Index>` under it, which can checkpoint and sync the database on close. Consequently, a cold user or an eviction stalled on storage prevents every other user's add/check from even obtaining an already cached connection. Moving the caller to `spawn_blocking` does not remove this cross-user serialization. This introduces a shared I/O bottleneck where the prior opens were independent. Remove an eviction candidate under the lock and drop it after unlocking; perform opens outside the global lock, using a per-user initialization guard/recheck to preserve the 64-entry bound. Add a gate that pauses one user's open/eviction and verifies another user's warm cache lookup completes before it resumes. **Durability and lifecycle checks:** - The base revision already sets `journal_mode=WAL`; this change strengthens `synchronous=NORMAL` to `FULL` and retains connections. A successful FULL WAL commit is durable before return, assuming the filesystem/device honors sync. Attachment bytes still go through file `sync_all`, placement, and directory fsync. Index-write errors remain logged and acknowledged because the index is a hint; the attachment file's durability does not depend on that row. - SQLite's default auto-checkpoint remains enabled (normally 1,000 WAL pages). A normal last-connection close also attempts a checkpoint. FULL checkpoints sync WAL before copying and sync the database before WAL reuse. A failed/incomplete checkpoint leaves recovery in the WAL; durable acknowledgement does not require immediate checkpoint completion. Process termination need not run a destructor for committed WAL recovery to work. - `docs/storage.md` explicitly keeps WAL/SHM companions beside databases, requires stopped-server coordinated copies or SQLite backup plus coordinated snapshots, includes the entire attachments metadata directory in moves, and requires current state for rollback. `docs/backup.md` also warns against copying the live DB alone. I found no production path at this head that copies/deletes just this attachment DB. SHM is reconstructible coordination state; WAL must not be omitted from a raw live-state snapshot. - The cache retains at most 64 entries, evicting the least-recent idle entry (`strong_count == 1`). Busy entries are retained; all-busy capacity falls back to scoped connections, so 64 bounds retained cache entries, not total in-flight connections. There is no idle expiry or explicit per-user invalidation. A cached connection can outlive an externally moved/replaced/deleted state directory. At this head, storage moves/restores are documented offline, state is separate from Home content, and I found no implemented live Home/account deletion or tiering move of this state DB. This is therefore a future lifecycle integration requirement, not a demonstrated data-loss path in this revision: any such live operation must drain leases and invalidate before replacing/removing state. - `Mutex<Connection>` serializes access across blocking workers; rusqlite's `Connection` is Send and does not need to be Sync directly. Independent SQLite connections/processes use WAL file locking and the busy timeout. Full server instances are additionally excluded by both storage-root locks; per-process attachment placement locking alone is not a multi-server guarantee. - Older rollback-journal databases can convert on open when SQLite obtains the required locks; failure returns an error. There is no schema change. Downgrading to the base keeps WAL support, although its NORMAL setting restores the older, weaker index-commit durability policy. **Validation:** source review plus a small Python SQLite probe via `netbird ssh claude@calternal-dev-01-compute02.vpn.kayg.org`, in the uncommitted `~/Developer/Worktrees/review-831-r1` checkout. Rollback-journal seed -> WAL/FULL insert -> child `os._exit(0)` without connection close -> reopen retained both rows and passed `integrity_check`; FULL `wal_checkpoint(TRUNCATE)` and reopen also passed (system SQLite 3.46.1). This checks SQLite recovery semantics, not the bundled rusqlite build or physical power loss. No builds or full Rust gates rerun; no pushes or committed changes. **Verdict: request changes for the P2 cross-user I/O lock above. No P0/P1 findings; no additional P3 findings.**
kayg commented 2026-11-01 18:29:08 +00:00
Author
Owner
Copy link

Resolved the P2 in 316b43e9. The global mutex now only selects/inserts/removes per-user slots; opening is serialized by each user's slot after releasing the global lock, and evicted slots are dropped outside it. Added a test that stalls user A's open while user B completes a warm lookup.

Dev-01 gates passed via ~/bin/slot and the Nix dev shell: cargo fmt --all --check; cargo clippy --locked -p calternal-attachments --all-targets -j4 -- -D warnings; and nix shell nixpkgs#strace -c cargo test --locked -p calternal-attachments -j4 -- --nocapture (22 passed, 0 failed).

Resolved the P2 in `316b43e9`. The global mutex now only selects/inserts/removes per-user slots; opening is serialized by each user's slot after releasing the global lock, and evicted slots are dropped outside it. Added a test that stalls user A's open while user B completes a warm lookup. Dev-01 gates passed via `~/bin/slot` and the Nix dev shell: `cargo fmt --all --check`; `cargo clippy --locked -p calternal-attachments --all-targets -j4 -- -D warnings`; and `nix shell nixpkgs#strace -c cargo test --locked -p calternal-attachments -j4 -- --nocapture` (**22 passed, 0 failed**).
kayg commented 2026-11-04 13:04:54 +00:00
Author
Owner
Copy link

Closing: head 316b43e9 is already in dev (reached via 80dc2aed); nothing left to merge. Branch kept.

Closing: head 316b43e9 is already in dev (reached via 80dc2aed); nothing left to merge. Branch kept.
kayg closed this pull request 2026-11-04 13:04:56 +00:00
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
Markdown extraction contract / markdown-native-contract (pull_request) Has been cancelled
Details
Markdown extraction contract / markdown-editor-contract (pull_request) Has been cancelled
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backend
Rust server

  
design
Mockups, UI and TASTE

  
docs
ADRs, CONTEXT, TASTE

  
idea
Parked idea, never worked until promoted

  
infra
Deploy, CI, VMs

  
perf
Performance and budgets

  
reliability
Data integrity, corruption traps

  
web
Web client

No labels
backend
design
docs
idea
infra
perf
reliability
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kayg/calternal!831
No description provided.