update_streak performs a non-transactional read-modify-write, allowing duplicate active streaks and permanent streak loss

[forkwright]

Finding

update_streak runs a sequence of independent SQL statements — a date(-1 day) SELECT, a current-streak SELECT, and a conditional INSERT/UPDATE — each as a separate round-trip against the pool, with no enclosing transaction. The single read-modify-write spans multiple .await points, and between each one the pool connection is released. This one missing transaction produces two distinct defects:

1. TOCTOU race (duplicate active rows). Two concurrent calls for the same user_id on the same today can both observe current = None and both execute the is_current = 1 INSERT, leaving two simultaneously-active streak rows for one user.
2. Partial-failure data loss. In the streak-gap arm the function first runs UPDATE ... SET is_current = 0, then a separate INSERT to start a new streak. If the INSERT fails after the UPDATE has already committed (unique-constraint violation, disk pressure, pool error), every row is left is_current = 0 — the user has no active streak, permanently, until some later play event re-triggers update_streak.

Evidence

crates/apotheke/src/repo/play_history/mod.rs:405 — the whole function body is a bare sequence of pool-level calls, never pool.begin():

pub async fn update_streak(pool: &SqlitePool, user_id: UserId, today: &str) -> Result<(), DbError> {

crates/apotheke/src/repo/play_history/mod.rs:415 — the current-streak read is a free-standing fetch_optional with no BEGIN; the value it returns can be stale by the time the match arm writes:

    let current = sqlx::query_as::<_, StreakRow>(
        "SELECT streak_start, streak_end, days
         FROM play_streaks
         WHERE user_id = ? AND is_current = 1",
    )
    .bind(user_id.as_bytes().as_ref())
    .fetch_optional(pool)
    .await

crates/apotheke/src/repo/play_history/mod.rs:429 — the None arm INSERT acquires the connection separately, so two racing callers can both reach it:

            sqlx::query(
                "INSERT INTO play_streaks
                 (user_id, streak_start, streak_end, days, is_current)
                 VALUES (?, ?, ?, 1, 1)",
            )

crates/apotheke/src/repo/play_history/mod.rs:462 — the gap arm clears the active flag in its own committed statement:

                "UPDATE play_streaks SET is_current = 0 WHERE user_id = ? AND is_current = 1",

crates/apotheke/src/repo/play_history/mod.rs:471 — the follow-up INSERT is a second, independent execute; a failure here strands the table in the cleared-but-not-recreated state:

            sqlx::query(
                "INSERT INTO play_streaks
                 (user_id, streak_start, streak_end, days, is_current)
                 VALUES (?, ?, ?, 1, 1)",
            )

Why this matters

current_streak reads WHERE user_id = ? AND is_current = 1 with fetch_optional. With two active rows the database returns whichever the cursor yields first, so the streak value becomes non-deterministic, and later update_streak calls extend or duplicate an arbitrary row, silently corrupting that user's history. The partial-failure path is worse: sqlx does not retry SQLite write errors, so a single transient failure between the UPDATE and the INSERT wipes the active streak with no surfaced error and no recovery path. Both are silent, unrecoverable corruptions of persisted state that the system reports as success — exactly the class of integrity failure that erodes trust in an auditable store, since the data diverges from reality while every call returns Ok.

Desired correction

Wrap the entire update_streak body in a single transaction: open with pool.begin(), run the date SELECT, the current-streak SELECT, and the INSERT/UPDATE arms against the &mut *tx borrow, and tx.commit() once at the end (an explicit failure path rolls back). The three-step read-modify-write must be atomic so concurrent callers serialize and any mid-function error leaves the table untouched. Done when: concurrent calls for the same (user_id, today) produce exactly one is_current = 1 row, and a forced failure at any statement inside update_streak leaves the play_streaks table in its pre-call state.