Skip to content

AI WIP | Commonize SQLite store setup in atuin-common (sqlite feature) - #3661

Draft
markovejnovic wants to merge 8 commits into
mainfrom
feat/commonize-sqlite-store
Draft

AI WIP | Commonize SQLite store setup in atuin-common (sqlite feature)#3661
markovejnovic wants to merge 8 commits into
mainfrom
feat/commonize-sqlite-store

Conversation

@markovejnovic

Copy link
Copy Markdown
Contributor

What & why

Six SQLite-backed stores each hand-rolled the same new(path, timeout) pool setup — broken-symlink guard, parent-dir creation, the SqliteConnectOptions pragma chain, SqlitePoolOptions with acquire_timeout, and (for two) a 0o600 chmod. This extracts that into one feature-gated builder in atuin-common and migrates every store onto it.

The shared builder

New #[cfg(feature = "sqlite")] module atuin_common::sqlite:

let pool = atuin_common::sqlite::pool(path, timeout)
    .synchronous(SqliteSynchronous::Normal)
    .foreign_keys(true)
    .regexp(true)
    .open()
    .await?;

open() handles the filesystem prep (:memory: skip, broken-symlink guard, parent-dir creation), applies the configured pragmas, connects the pool, and optionally restricts the file to 0o600. There's also version(&pool) (consolidating the three identical sqlite_version() methods). Migrations stay per-store (they use the sqlx::migrate! macro, which must expand at the call site).

Feature: sqlite = ["sqlx/sqlite", "sqlx/regexp"] on atuin-common, enabled by the four consuming crates. This also closes a latent footgun: atuin-kv/atuin-scripts call .with_regexp() but never enabled sqlx's regexp feature themselves — it only worked via cross-crate feature unification. That capability is now explicitly owned by atuin-common.

Behavior preservation

The builder is faithful to which setters each store calledsynchronous is Option (unset ⇒ not called ⇒ sqlx default FULL), foreign_keys/regexp are opt-in, optimize_on_close/create_if_missing are universal. Each store's resulting SqliteConnectOptions is identical to before:

store journal synchronous foreign_keys regexp 0o600
history WAL Normal (sqlx default) yes
record WAL Normal yes
meta Delete (default) (default) yes
kv WAL Normal yes yes
scripts WAL Normal yes yes
ai WAL (default) (default) yes

Intentional changes (called out for review)

  1. Broken symlink: process::exit(1) → returned sqlx::Error. The old code called std::process::exit(1) from library code; the builder returns an error that propagates to the CLI's normal error handling. The meta and ai stores, which had no broken-symlink guard, gain this (strictly safer) check — harmless, they only ever pass real or :memory: paths.
  2. 0o600 restriction now runs right after connect, before migrations (meta/ai). Behavior-equivalent for the final on-disk permission (same single main-db file), a marginally tighter window.
  3. :memory: filesystem prep is skipped uniformly. history/record previously ran the fs-prep even for in-memory paths (which happened to succeed); now unified with meta/ai.

Verification

  • cargo build --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --lib all clean.
  • grep SqliteConnectOptions finds zero direct constructions across the six stores — all go through the builder.
  • 9 unit tests on the builder (pragmas, perms, broken-symlink, :memory:); each store's existing suite validates its own migration.
  • Not run end-to-end against a real production DB (a dev build migrates the real history.db); confidence comes from the per-store base-vs-head config comparison, the builder tests, and the full workspace suite.

🤖 Generated with Claude Code

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