From 3c63ecc1ad9579a2167e53808f81c59876ba5df9 Mon Sep 17 00:00:00 2001 From: Mix Date: Thu, 15 Oct 2026 01:19:18 +0300 Subject: [PATCH] store: migrations under BEGIN IMMEDIATE, the version read again inside MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D-S3. A deferred BEGIN starts nothing until the first access (SQLite, lang_transaction.html § 2.2), and migrate() read user_version outside the transaction: after an upgrade the panel and dmarc-ingest, opening the file at once, could both read the old version, both begin, and the second applied the next migration again on the migrated file - "already exists", that Open failing (nothing damaged: its transaction rolled back). The DSN now has _txlock=immediate (modernc.org/sqlite v1.60.1, sqlite.go applyQueryParams() sets beginMode, tx.go newTx() issues "begin immediate" for any transaction not read-only), so a migration holds the write lock from its BEGIN, and migrate() reads user_version again inside it and skips one another process applied meanwhile. Test TestTwoOpensMigrateOnce: a raw connection holds BEGIN IMMEDIATE on a new file while two Opens read version 0 and queue on their first BEGIN; released, both succeed and user_version is the number of migrations. Without the fix it fails as the register said: "apply migration 0001_init.sql: SQL logic error: table settings already exists (1)". Stand (2026-10-15): go vet and go test ./... pass on the server (24 packages); make e2e exit 0. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++ docs/audit-2026-10-12.md | 4 ++- docs/roadmap.md | 2 +- internal/store/schema_test.go | 55 +++++++++++++++++++++++++++++++++++ internal/store/store.go | 27 +++++++++++++++-- 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c50d29d..4249cbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -141,6 +141,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version - **A password shown once is emptied when its page is left,** so a browser that keeps the page for its Back button shows an empty field; the meaningless `Pragma` header goes (assumptions D-B13). +- **A database update runs once when two programs open the database at + the same moment.** After an upgrade, the panel and the DMARC report + intake could both find the database one version behind and both apply + the next update; the second then failed, and with it that start or that + report (nothing was damaged). Each update now holds the database from + its start and checks the version again (assumptions D-S3). - **Words for what happens to mail match Postfix's.** A message refused with a temporary error while the panel or the spam filter does not answer was called deferred, which in Postfix's words is mail waiting in its queue; diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md index 5208398..d154283 100644 --- a/docs/audit-2026-10-12.md +++ b/docs/audit-2026-10-12.md @@ -999,10 +999,12 @@ those after it move up one patch number each. app's browser): a `no-store` page is not restored from the back/forward cache (pageshow persisted=false, a fresh load); the `pagehide` handler is for browsers that do restore one. Firefox and Safari not measured. -- [ ] **Two processes opening an old database** (D-S3): `user_version` is +- [x] **Two processes opening an old database** (D-S3): `user_version` is read outside the transaction, so the panel and dmarc-ingest can both run one migration; `_txlock=immediate` and the version re-read inside it; a test with two concurrent opens behind a held lock. - **Opus** + *Done 2026-10-15:* the test fails without the fix exactly as the + register said ("table settings already exists") and passes with it. - [ ] **Certificates from certbot** (C-A1, C-Q4, D-guide:258, :291, :722): certbot's `live/` files are relative links into `archive/`, so mounting `live/` alone, or linking `certs` to it, leaves Postfix without a diff --git a/docs/roadmap.md b/docs/roadmap.md index c631a2e..0f48113 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -42,7 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | -| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`; next stage 2c → `2.5.2`) | 42/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | +| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`; next stage 2c → `2.5.2`) | 43/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | **Recommended order** (not binding; owner, 2026-10-09: feature order follows development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and diff --git a/internal/store/schema_test.go b/internal/store/schema_test.go index 0ae1b7f..0449d24 100644 --- a/internal/store/schema_test.go +++ b/internal/store/schema_test.go @@ -8,6 +8,7 @@ import ( "sort" "strings" "testing" + "time" ) // 2.0 starts from one baseline: the 1.x chain is gone from the binary, not @@ -383,3 +384,57 @@ func TestBaselineConstraints(t *testing.T) { t.Errorf("after deleting the user %d inbound assignment(s) stayed (%v)", n, err) } } + +// Two processes opening one file that needs migrating - the panel and +// dmarc-ingest after an upgrade - each apply every migration at most once: +// both read the old version, both wait for the write lock, and the second +// finds the work done (assumptions D-S3). Before _txlock=immediate and the +// re-read, the second ran 0001 again and failed with "already exists". +func TestTwoOpensMigrateOnce(t *testing.T) { + path := filepath.Join(t.TempDir(), "selfpost.db") + // The blocker holds the write lock while both opens read user_version 0 + // and queue on their first BEGIN IMMEDIATE. + blocker, err := sql.Open("sqlite", "file:"+path+"?_pragma=journal_mode(WAL)") + if err != nil { + t.Fatal(err) + } + defer blocker.Close() + blocker.SetMaxOpenConns(1) + if _, err := blocker.Exec("BEGIN IMMEDIATE"); err != nil { + t.Fatal(err) + } + + errs := make(chan error, 2) + for i := 0; i < 2; i++ { + go func() { + st, err := Open(path) + if err == nil { + err = st.Close() + } + errs <- err + }() + } + time.Sleep(500 * time.Millisecond) // both are waiting on the lock by now + if _, err := blocker.Exec("COMMIT"); err != nil { + t.Fatal(err) + } + for i := 0; i < 2; i++ { + if err := <-errs; err != nil { + t.Fatalf("open %d: %v", i, err) + } + } + + st, err := Open(path) + if err != nil { + t.Fatal(err) + } + defer st.Close() + var version int + if err := st.db.QueryRow("PRAGMA user_version").Scan(&version); err != nil { + t.Fatal(err) + } + entries, _ := fs.ReadDir(migrationsFS, "migrations") + if version != len(entries) { + t.Fatalf("user_version %d, want %d", version, len(entries)) + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 13abd76..3b195c7 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -34,7 +34,17 @@ func Open(path string) (*Store, error) { // busy_timeout first. SQLite 3.53.4: foreign_keys is per connection // (https://sqlite.org/foreignkeys.html § 2), journal_mode=WAL stays in the // file (https://sqlite.org/wal.html § 3.3). - dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)&_pragma=foreign_keys(ON)", path) + // + // _txlock=immediate: every transaction that may write takes the write + // lock at BEGIN (modernc.org/sqlite v1.60.1, sqlite.go applyQueryParams() + // sets beginMode, tx.go newTx() issues "begin immediate"; read-only + // transactions keep a plain begin). A deferred BEGIN starts nothing + // until the first access (lang_transaction.html § 2.2), so two processes + // - the panel and dmarc-ingest after an upgrade - could each read the + // schema version and then both run the next migration; an immediate one + // waits through busy_timeout, holding no lock that the other needs, and + // migrate() reads the version again under it (assumptions D-S3). + dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)&_pragma=foreign_keys(ON)&_txlock=immediate", path) db, err := sql.Open("sqlite", dsn) if err != nil { return nil, fmt.Errorf("open database: %w", err) @@ -70,7 +80,10 @@ const applicationID = 0x53503230 var ErrForeignSchema = errors.New("database was not created by SelfPost 2.x; 2.0 starts from an empty data directory (docs/schema-migrations.md)") // migrate applies embedded migrations in filename order, tracking progress via -// SQLite's PRAGMA user_version so each migration runs at most once. +// SQLite's PRAGMA user_version so each migration runs at most once: the +// version is read again inside each migration's transaction, which holds the +// write lock from its BEGIN (Open, _txlock), and a migration another process +// applied in the meantime is skipped. func (s *Store) migrate() error { entries, err := fs.ReadDir(migrationsFS, "migrations") if err != nil { @@ -123,6 +136,16 @@ func (s *Store) migrate() error { if err != nil { return fmt.Errorf("begin migration %s: %w", name, err) } + var now int + if err := tx.QueryRow("PRAGMA user_version").Scan(&now); err != nil { + tx.Rollback() + return fmt.Errorf("read schema version for %s: %w", name, err) + } + if now >= target { + // Applied by another process while this one waited for the lock. + tx.Rollback() + continue + } // One Exec runs the whole file: modernc.org/sqlite v1.60.1, stmt.go // stmt.exec(), "FALLBACK PATH: Multi-statement script" prepares and // steps each statement of the tail in turn. It runs with foreign_keys