feat(cryptify): persist upload-session state to SQLite (write-through) - #319
Conversation
Upload sessions lived only in `Store`'s in-memory map, so any restart lost every in-flight upload. This makes the state durable without changing a single response: every transition is written through to SQLite, and nothing reads the rows back yet (restore-on-boot is postguard#303). `usage_db` now names cryptify's whole state database rather than just the quota table: one file, one rusqlite connection behind one mutex (`StateDb`), two tables. `upload_sessions` is created with `CREATE TABLE IF NOT EXISTS` at startup, so a database already holding usage rows gains it in place, and `usage_db` unset still means in-memory only. `Store::persist_session` writes one upsert and runs before the handler responds, at each of the three transitions: `Store::create` for init, `upload_chunk` once the rolling token has advanced, and `upload_finalize` right after the unsealed sender is known. The reverse edges matter as much, so `Store::remove` and the purge task both delete the row — an unresumable session must not leave one behind, or the table would grow without bound and a future restore would resurrect sessions the purge already killed. Database errors are logged rather than propagated: persistence must not add a way for a healthy upload to fail. Three schema decisions worth stating. The recovery token is stored as a hex SHA-256, never as the plaintext bearer credential, so a restore path compares hashes. `created_at` is left out of the upsert's `DO UPDATE SET` list, which is what keeps it recording when the session began. And there is no expected-size column, because chunk PUTs carry `Content-Range: bytes <s>-<e>/*` and the total is unknown to the server until finalize declares it. Tests cover the layer directly and through the `build_rocket` seam, reading rows back over a second connection so they assert what a restarting process would find on disk: the full row after init, progress and a preserved `created_at` after a chunk, the sender after finalize, deletion on remove and on eviction, the no-database no-op path, and a usage-only database gaining the table. Two integration tests drive real HTTP init/chunk/finalize and read the columns with plain SQL.
|
Thanks for the PR! I'm on it — taking a look at the upload session persistence changes (SQLite write-through for the three transitions, the |
There was a problem hiding this comment.
Review arrived after the merge (opened 07:41Z, merged 07:45Z, no reviews on it), so this is post-hoc — everything below is on main now. Posting as a comment because there is nothing left to block; the two bugs are worth follow-up tickets rather than a revert.
The design decisions in the description hold up. Write-through before the response, created_at omitted from the DO UPDATE SET list, the recovery token stored only as a hex SHA-256, and no expected-size column are all the right calls, and the tests reading rows back over a second connection is the detail that makes them mean anything. The CREATE TABLE IF NOT EXISTS in-place upgrade and leaving the usage_db key name alone are both correctly reasoned as ops-visible-change avoidance.
Two things I would not have merged without a second look, both verified against this branch:
1. The purge/persist race resurrects rows that nothing will ever delete again (store.rs). purge_expired now removes from state.files/expirations under the state lock, drops the lock, and only then calls delete_session. A chunk PUT that took its Arc from store.get before eviction is still awaiting the body read, and calls persist_session after — which goes straight to the INSERT ... ON CONFLICT without ever consulting state.files. The row comes back, but the session is gone from both state.files and expirations, so nothing deletes it a second time. That is exactly the unbounded-growth and resurrect-a-killed-session pair the description says the delete exists to prevent. Store::remove has the same shape and a wider window — it deletes the row before taking the state lock.
2. usage_db inside data_dir makes every session row publicly downloadable (main.rs). GET /filedownload/<filename> serves any name without a slash straight out of data_dir with no credential; is_safe_download_segment only blocks traversal and ../.. Not a live exposure — prod's conf/config.toml sets data_dir but never sets usage_db, so persistence is off there — but this PR is what turns that file from (email, timestamp, bytes) quota rows into recipients, mail_content, sender, api_key_tenant and recovery_token_hash, and the new test helper models the DB-inside-data_dir layout while cryptify/CLAUDE.md never says it must live outside.
Three smaller notes inline: an fsync per chunk PUT under the tokio guard, touch not persisting last_active_at, and secure_delete for a database that now holds message bodies.
None of this is a revert — the feature works and the response surface is genuinely unchanged. But #1 should land before #303 reads these rows back, because restore is the point at which a resurrected row stops being a leak and starts being a wrong answer.
|
|
||
| // Outside the `state` mutex: an evicted session is unresumable, so its | ||
| // row goes with it. Without this the table would grow without bound | ||
| // and a restore would resurrect sessions the purge already killed. |
There was a problem hiding this comment.
An evicted session's row is resurrected and then never deleted again. purge_expired removes the entry from state.files/expirations under the state lock, drops the lock, then deletes the row here. But a chunk PUT that took its Arc from store.get before eviction is still awaiting the body read and the file write, and calls persist_session afterwards — and upsert_session is an INSERT ... ON CONFLICT, so it re-inserts the row. The session is now gone from state.files and from expirations, so nothing will ever delete that row again: the table grows without bound and #303 would restore a session the purge already killed — precisely the two failure modes the PR description says this delete exists to prevent. Store::remove (store.rs:646) has the same race, and deletes before removing from the map.
Confirmed on this branch with a scratch test: Store::with_idle_ttl(20ms, .., Some(db)), create, hold the handle from store.get, poll until the row disappears, then persist_session — the row is back while store.get returns None. In prod the window is the whole chunk body-read, so a slow large chunk arriving near the 1h idle deadline hits it.
A fix needs the write and the liveness check to agree — e.g. have persist_session take the state lock and skip the upsert when state.files no longer contains the id, or keep a short-lived tombstone set that upsert_session consults.
| setup: &TestSetup, | ||
| ) -> (Client, std::path::PathBuf, std::path::PathBuf) { | ||
| let (figment, dir) = test_figment(); | ||
| let db_path = dir.join("state.db"); |
There was a problem hiding this comment.
Putting the state DB inside data_dir makes every session row publicly downloadable, and this helper models exactly that layout. GET /filedownload/<filename> (main.rs:1395) serves any name without a slash straight out of data_dir, with no credential — is_safe_download_segment only blocks traversal. Verified against this branch: GET /filedownload/state.db → 200 (4096 bytes), and GET /filedownload/state.db-wal → 200 with 37 KB containing the session UUID (WAL mode means the live rows are in the -wal file). That is recipients, mail_content, sender, api_key_tenant and recovery_token_hash for every in-flight upload, readable by anyone who guesses the filename.
Not a live prod exposure — prod's conf/config.toml doesn't set usage_db, and config.dev.toml points it at /app/data/usage.db while data_dir is /tmp/data. But this PR is what turns that file from (email, timestamp, bytes) quota rows into full session state including a bearer-credential hash and the message body, and the new cryptify/CLAUDE.md text ('one file to mount and back up') never says it must live outside data_dir — while this test helper writes it inside. Worth doing here: point the test's DB somewhere outside data_dir so it isn't a template, and refuse (or loudly warn) at startup when usage_db resolves inside data_dir. Restricting is_safe_download_segment to a UUID shape would close it for good — data_dir files are always Uuid::new_v4().hyphenated() — but that's a bigger call than this PR.
| // the client treats the token it gets back as committed, so the durable | ||
| // row must not lag behind it. A database failure is logged, not | ||
| // propagated — the chunk is already on disk and in memory. | ||
| store.persist_session(uuid, &state); |
There was a problem hiding this comment.
This is a synchronous rusqlite commit — a global std::sync::Mutex plus, under WAL with SQLite's default synchronous=FULL, an fsync — running on a Rocket worker thread with the FileState tokio guard still held (drop(state) is the next line). record_usage has the same shape but runs once per upload at finalize; this one runs on every chunk PUT, so with chunk_size = 5000000 that's an fsync per 5 MB per upload, and all concurrent uploads serialise on the one connection mutex while blocking the async executor.
Nothing here is wrong today at current traffic, but two cheap mitigations are worth considering: PRAGMA synchronous = NORMAL (safe under WAL — it risks only the last commit on power loss, and the row is a recovery aid, not the source of truth), and/or doing the write outside the state guard so a slow disk can't hold the per-session lock.
| /// Set when the row is first inserted and never overwritten afterwards | ||
| /// (the upsert's `DO UPDATE` list omits it on purpose). | ||
| created_at: i64, | ||
| /// Refreshed on every persisted transition. |
There was a problem hiding this comment.
last_active_at is refreshed only where persist_session is called, but the other thing that counts as activity isn't a persisted transition: GET /fileupload/{uuid}/status calls store.touch (main.rs:1109), which extends the in-memory expirations deadline and writes nothing. So a session kept alive across a page refresh by status polls has a durable last_active_at arbitrarily far behind its real idle deadline. Harmless now since nothing reads the column, but if #303 reconstructs the idle deadline from it, restore will evict exactly the cross-refresh-resume sessions the status endpoint exists to keep alive. Either persist on touch too, or say in this field doc (and cryptify/CLAUDE.md) that last_active_at tracks persisted transitions only and is a lower bound on activity.
| )?; | ||
| Ok(UsageDb { | ||
| conn.execute( | ||
| "CREATE TABLE IF NOT EXISTS upload_sessions ( |
There was a problem hiding this comment.
StateDb::open sets journal_mode=WAL but not secure_delete, so when the purge task or remove deletes a session row its plaintext — recipient addresses, mail_content, sender — stays in the database's free pages, and in the WAL until checkpoint, indefinitely. Before this PR the file only ever held (email, timestamp, bytes). For the database that now holds message bodies and recipient lists, PRAGMA secure_delete = ON is cheap at this write volume. Related and worth a line in cryptify/CLAUDE.md: a successfully finalized session's row is only removed by the idle purge, so recipient addresses and the mail body outlive the completed transfer by up to idle_ttl (1h).
|
Heads up — this landed before the review could run (opened 07:41Z, merged 07:45Z, no reviews), so I couldn't block anything. I reviewed it anyway and left a consolidated comment with 5 inline notes on #319. Overall the write-through design holds up well: write-before-respond,
Plus three smaller notes inline (fsync-per-chunk while holding the tokio guard, |
Closes #302. First half of #300 (epic #247, workstream E); restore-on-boot is #303 and is deliberately not here.
What this changes
Upload sessions lived only in
Store's in-memory map, so any restart lost every in-flight upload. This makes the state durable without changing a single response: every transition is written through to SQLite, and nothing reads the rows back yet.usage_dbnow names cryptify's whole state database rather than just the quota table: one file, onerusqlite::Connectionbehind oneMutex(StateDb), two tables.upload_sessionsis created withCREATE TABLE IF NOT EXISTSat startup, so a database already holding usage rows gains it in place, andusage_dbunset still means in-memory only. The key name was left alone on purpose — renaming it would be an ops-visible change for zero behavioural gain.Store::persist_sessionwrites one upsert and runs before the handler responds, at each of the three transitions:Store::create(before the entry is visible in memory)upload_chunk, once the rolling token has advancedupload_finalize, right after the unsealedsenderis known, before the emailThe reverse edges matter as much, so
Store::removeand the purge task both delete the row — an unresumable session must not leave one behind, or the table would grow without bound and a future restore would resurrect sessions the purge already killed. Database errors are logged rather than propagated: persistence must not add a way for a healthy upload to fail.Three schema decisions worth calling out
GET /fileupload/{uuid}/status. A restore path comparessha256(presented)against the column.created_atis absent from the upsert'sDO UPDATE SETlist. That omission is what keeps it recording when the session began;last_active_atmoves on every transition.Content-Range: bytes <s>-<e>/*on chunk PUTs, so the total is genuinely unknown to the server until finalize declares it (and finalize rejects a declaration that disagrees withuploaded).Coverage
12 new tests, all reading rows back over a second connection to the same file so they assert what a restarting process would actually find on disk rather than what the writing connection holds:
store.rs: the full row after init; progress plus a preservedcreated_atafter a chunk (and that it upserts rather than appends); the sender after finalize; deletion onremoveand on eviction; the recovery token present only as a hash; the no-database no-op path; schema creation on first boot and on reopen; a usage-only database gaining the table without disturbing its usage rows.main.rsmod integration, through thebuild_rocketseam withusage_dbpointed inside the per-testdata_dir: real HTTP init → chunk → finalize, with the columns read back using plain SQL.session_rows_are_visible_in_sqlite_after_init_and_chunkis the ticket's done bar, and it asserts the persisted token is the one the client was handed.upload_happy_path_is_unchanged_with_persistence_enabledpins the no-visible-change claim.email.rs:language_code_matches_serde_representationpins the newLanguage::code()(what themail_langcolumn stores) to the serde/wire form.cargo test -p cryptify --all-targets→ 170 passed, 0 failed.cargo clippy -p cryptify --all-targets -- -D warningsandcargo fmt --all -- --checkclean. No dependency changes — this uses therusqlitealready in the lockfile (the sqlx/rusqlitelinks = "sqlite3"coupling in rootCLAUDE.mdis untouched).cryptify/CLAUDE.mdis updated in the same commit, including the fact that prod'sconf/config.tomldoes not setusage_dbat all — so session persistence is off in any deployment that never set it, which #303 will need to know.