Move CLI auth sessions from SlateDB to SQLite - #648
Draft
brynary wants to merge 8 commits into
Draft
Conversation
`Automation::to_toml_string` and the `to_persisted` helper it wrapped have had no production callers since automations moved from `<storage>/automations/*.toml` into SQLite. Writes now serialize through `canonical_bytes` for revision hashing; nothing renders an `Automation` back to a TOML document. Repoint the canonicalization test at `parse_persisted` + `canonical_bytes` so it exercises the production path that actually produces the bytes the revision hash is computed over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A CLI auth session is a rotation chain, but the SlateDB records that back it today store identity and profile per token, so a chain has no owner and nothing stops its rows from disagreeing. These two tables give the chain a home: `auth_sessions` holds the identity and profile once, `refresh_tokens` holds only per-token facts. Two invariants the current code relies on but never states become constraints. The partial unique index on `(session_id) WHERE used_at_ms IS NULL` enforces that rotation leaves exactly one live token per chain -- which is what makes the session listing an indexed lookup instead of a scan-and-group. The foreign key with `ON DELETE CASCADE` makes revoking a session remove its tokens without a second statement. Tokens are retained after rotation until they expire so a replayed token stays distinguishable from a forgery. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every operation the SlateDB store answers with a full keyspace scan becomes an indexed query here: listing a user's sessions joins one row per session via the partial unique index instead of scanning every token ever issued and grouping by chain, and revoking one is a single DELETE that cascades. Rotation is the structural win. Claiming the presented token is one `UPDATE ... WHERE used_at_ms IS NULL ... RETURNING`, and it is the transaction's first statement, so SQLite takes the write lock before anything is read. A concurrent caller blocks on that lock and then sees the token already spent, which is exactly the replay signal -- so the store needs no `KeyedMutex` to serialise rotation, and the guarantee survives more than one server process. Expiry is checked ahead of reuse on the cold path, preserving the ordering callers depend on: only replaying a still-live token revokes its chain. Drops the ordering CHECKs between a session's timestamps and its tokens'. Rotation stamps `now` from the process clock against rows written by an earlier request, so an NTP step backwards would have turned a harmless clock anomaly into refresh failing outright for every affected session. The store is not wired into the server yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Points the session listing, revocation, refresh, and logout paths at
`AuthSessionStore`. Listing a user's sessions and revoking one stop scanning
the whole refresh-token keyspace; both are now indexed queries.
Fixes two timestamps that were wrong by construction. `created_at` was fed
from the newest token's `issued_at`, so a session's reported start drifted
forward on every refresh, and `last_seen_at` read a field only ever set at
issue -- so both rendered the same value. They now come from the session row,
where they mean what they say.
Deletes `next_refresh_row`, which had to fabricate an identity of
("https://github.com", "0") and empty profile strings for the no-existing-row
case, because a token was required to carry chain-level fields. Rotation now
takes just the new hash, expiry, and user agent. That also removes the
pre-read it existed to feed, closing the window between that read and the
one `consume_and_rotate` did itself.
Opening the store per request is gone with it: five handlers each had a
500-response arm for "could not open the store", which field access on
AppStores cannot fail.
Drops the replay-revocation cache. Its only effect was reporting `revoked`
rather than `expired` for the third and later presentations in a concurrent
burst, and `fabro-client` (client.rs:508-513) matches both codes in one arm
and treats them identically. Replay detection itself is unaffected: it is
`Reused` into `delete_session`, which lives in the database. The concurrency
test now accepts either code, since losers that arrive after the winner's
revocation find the row already cascaded away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removes `slate/auth_tokens.rs`, `Database::refresh_tokens()`, and its `OnceCell` now that nothing reads them, and clears the retired `auth/refresh` prefix once at startup. That sweep is not housekeeping we could skip: the reaper that used to collect those records went with the store, so without it they would sit in the object store forever. A later boot finds the prefix empty and does nothing. `record/transaction.rs` goes too -- rotation was its only caller, and SQLite transactions replaced it. `KeyedMutex` stays; `AuthCodeStore` still uses it until auth codes move. Existing refresh tokens are not migrated. Everyone re-authenticates once on upgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records the two new tables in the server configuration reference, and adds a changelog entry leading with the operator-visible consequence: this upgrade signs everyone out once, because existing refresh tokens are not migrated. Also notes the error-code change on concurrent replay, since it is observable even though the CLI handles both codes identically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows the type rename: a rotation chain is now an auth session with its own row, so the local names and the Repository doc comment should say so rather than referring to a store that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`git add -A` over lib/ and docs/ swept in work that was already untracked in the working tree before this branch started: lib/crates/, and three docs files. None of it belongs to this change. Removed from the index only, so the files stay on disk as the untracked work they were. They net out of the branch diff entirely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines
+452
to
+460
| let mut batch = slatedb::WriteBatch::new(); | ||
| let mut deletes = 0_u64; | ||
| while let Some(entry) = iter.next().await? { | ||
| batch.delete(entry.key); | ||
| deletes += 1; | ||
| } | ||
| if deletes > 0 { | ||
| db.write(batch).await?; | ||
| } |
| --- | ||
|
|
||
| <Warning> | ||
| **Everyone signs in again after this upgrade.** Existing refresh tokens are not migrated, so every signed-in browser and CLI is logged out the moment the new server binary starts. Run `fabro auth login` again on each machine. There is no staged rollout for this — avoid upgrading mid-task. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves refresh tokens out of the
auth/refreshSlateDB keyspace into two SQLite tables, continuing the storage consolidation from #537/#539/#570/#571/#572/#573.Warning
This upgrade signs everyone out once. Existing refresh tokens are not migrated, so every signed-in browser and CLI is logged out the moment the new binary starts. There is no staged rollout — this should not ship in a nightly people are mid-task on.
Why
RefreshTokenStorestored one record per token, but the unit the API and users care about is the chain — a session that survives rotation. Three consequences, all visible in the code before this change:active_cli_sessionsanddelete_active_chain_for_identitybothscan_stream()d the entire prefix and filtered in Rust, on everyGET/DELETE /api/v1/auth/sessions. Same problem Add SQLite runs read model #572 fixed for run lists.created_atcame from the newest token'sissued_at, so a session's reported start drifted forward on every refresh;last_seen_atread a field only ever set at issue, so both rendered the same value.next_refresh_rowhad to fabricate an identity —("https://github.com", "0")plus empty profile strings — because a token row was required to carry chain-level fields. That fake row was handed toconsume_and_rotateand discarded on theNotFoundpath.What changed
auth_sessionsholds the identity and profile once;refresh_tokensholds only per-token facts. Two invariants the old code relied on but never stated are now constraints:(session_id) WHERE used_at_ms IS NULL— at most one live token per chain. The oldHashMap+max(last_used_at)grouping was defending against a state rotation already prevented.ON DELETE CASCADE, so revoking a session removes its tokens in the same statement.Rotation no longer needs an application mutex. Claiming the presented token is one
UPDATE ... WHERE used_at_ms IS NULL ... RETURNING, and it is the transaction's first statement, so SQLite takes the write lock before anything is read. A concurrent caller blocks, then sees the token already spent — which is exactly the replay signal.KeyedMutexis gone from this path, and unlike a per-process mutex the guarantee holds across more than one server process.The replay-revocation cache is deleted, not migrated. Its only effect was reporting
revokedrather thanexpiredfor the third and later presentations in a concurrent burst, andfabro-client(client.rs:508-513) matches both codes in one arm and treats them identically. Replay detection is unaffected — it isReused→delete_session, in the database.Deliberate deviations from the plan
nowfrom the process clock against rows written by an earlier request, so an NTP step backwards would have turned a harmless clock anomaly into refresh failing outright for every affected session. A test caught this; the constraint was the wrong trade.AppState::test_auth_session_store()added behind#[cfg(any(test, feature = "test-support"))]—TestAppStateBuilderowns its pool internally, so integration tests had no other way to seed against the pool the router reads.record/transaction.rsdeleted — rotation was its only caller.KeyedMutexstays;AuthCodeStorestill uses it.Note for reviewers
delete_active_chain_for_identity_requires_active_owned_tokencould not port. Its fixture built two live tokens in one chain and a same-chain token owned by a different identity — both now unrepresentable. Thefabro-dbschema tests assert those rejections instead, and each was checked to fire on its intended constraint rather than incidentally.auth/codeis a deliberate follow-up: same pattern, much smaller, and folding it in would roughly double this diff.Test plan
cargo nextest run --workspace— 7,416 passed, 0 failedcargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings— cleancargo +nightly-2026-04-14 fmt --check --all— cleanNot yet done: manual verification against a live server (log in, list, refresh, revoke).
🤖 Generated with Claude Code