MySql implementation for atuin-server#3541
Conversation
Greptile SummaryThis PR adds MySQL support for the Atuin server database layer. The main changes are:
Confidence Score: 2/5These issues should be fixed before merging.
Focus on
|
| Filename | Overview |
|---|---|
| crates/atuin-server-mysql/src/lib.rs | Adds the MySQL database implementation; write handling, cache rollout parsing, and cleanup need fixes. |
| crates/atuin-server-database/src/lib.rs | Adds MySQL URI detection but leaves MySQL credentials unredacted in debug output. |
| crates/atuin-server/server.toml | Changes the embedded generated config to active MySQL and open-registration defaults. |
Comments Outside Diff (1)
-
crates/atuin-server-database/src/lib.rs, line 100-111 (link)Redact MySQL URIs
MySQL is now a supported URI type, butDebugonly redacts Postgres settings. Startup failures formatsettings.db_settingswith{:?}, so a failed MySQL connection can printmysql://user:password@...or the read replica password into logs. Redact all URL-based database URIs here.
Reviews (1): Last reviewed commit: "MySql implementation for atuin-server" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| open_registration = true | ||
|
|
||
| ## URI for postgres (using development creds here) | ||
| # db_uri="postgres://username:password@localhost/atuin" | ||
| # db_uri="sqlite:///config/atuin-server.db" | ||
| db_uri = "mysql://root:foobar@127.0.0.1/atuin" |
There was a problem hiding this comment.
This file is embedded as the generated server config for new installs. With these active values, a fresh server writes open_registration = true and a local MySQL root URI by default, so it can start with public registration enabled and a development database target unless the operator edits the file first. Keep these as commented examples and leave registration disabled by default.
| let idx_cache_rollout = std::env::var("IDX_CACHE_ROLLOUT").unwrap_or("0".to_string()); | ||
| let idx_cache_rollout = idx_cache_rollout.parse::<f64>().unwrap_or(0.0); | ||
| let use_idx_cache = rand::thread_rng().gen_bool(idx_cache_rollout / 100.0); |
There was a problem hiding this comment.
gen_bool panics when the probability is outside 0.0..=1.0. If IDX_CACHE_ROLLOUT is set to 101 or -1, every status call that reaches this code can crash the request task instead of falling back safely. Clamp the parsed value to 0..=100 or reject invalid values before dividing.
| let idx_cache_rollout = std::env::var("IDX_CACHE_ROLLOUT").unwrap_or("0".to_string()); | |
| let idx_cache_rollout = idx_cache_rollout.parse::<f64>().unwrap_or(0.0); | |
| let use_idx_cache = rand::thread_rng().gen_bool(idx_cache_rollout / 100.0); | |
| let idx_cache_rollout = std::env::var("IDX_CACHE_ROLLOUT").unwrap_or("0".to_string()); | |
| let idx_cache_rollout = idx_cache_rollout.parse::<f64>().unwrap_or(0.0).clamp(0.0, 100.0); | |
| let use_idx_cache = rand::thread_rng().gen_bool(idx_cache_rollout / 100.0); |
| let result = sqlx::query( | ||
| "insert ignore into store | ||
| (id, client_id, host, idx, timestamp, version, tag, data, cek, user_id) | ||
| values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| ", | ||
| ) | ||
| .bind(id) | ||
| .bind(i.id) | ||
| .bind(i.host.id) | ||
| .bind(i.idx as i64) | ||
| .bind(i.timestamp as i64) // throwing away some data, but i64 is still big in terms of time | ||
| .bind(&i.version) | ||
| .bind(&i.tag) | ||
| .bind(&i.data.data) | ||
| .bind(&i.data.content_encryption_key) | ||
| .bind(user.id) | ||
| .execute(&mut *tx) | ||
| .await?; |
There was a problem hiding this comment.
INSERT IGNORE suppresses more than duplicate-key conflicts in MySQL. If a record has an out-of-range value, truncated tag, or another data error, MySQL can skip or coerce the row and this method still returns Ok(()); the sync client then believes the encrypted record was stored. Use duplicate-key-only handling and let other write errors fail the upload.
| sqlx::query( | ||
| "insert ignore into history | ||
| (client_id, user_id, hostname, timestamp, data) | ||
| values (?, ?, ?, ?, ?) | ||
| ", | ||
| ) | ||
| .bind(client_id) | ||
| .bind(i.user_id) | ||
| .bind(hostname) | ||
| .bind(i.timestamp) | ||
| .bind(data) | ||
| .execute(&mut *tx) | ||
| .await?; |
There was a problem hiding this comment.
INSERT IGNORE can hide non-duplicate MySQL errors here too. A history row with invalid or truncated data can be skipped while add_history commits and returns success, so the client drops local retry state for data the server did not actually store. Limit the ignore behavior to duplicate client_id conflicts and propagate other database errors.
| .await?; | ||
|
|
||
| sqlx::query("delete from users where id = ?") | ||
| .bind(u.id) | ||
| .execute(&self.pool) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[instrument(skip_all)] | ||
| async fn delete_history(&self, user: &User, id: String) -> DbResult<()> { | ||
| sqlx::query( |
There was a problem hiding this comment.
Deleting a user removes store rows but leaves that user's store_idx_cache rows behind. When the cache rollout is enabled, stale cache entries can be returned for a reused or restored user id, and the table will also grow with deleted accounts. Delete store_idx_cache for the user in this cleanup path.
1 participant
For $reasons, I cannot run Postgres at my company. This PR implements MySQL support for atuin-server.
The
test_full_db_storytest introduced in #3514 passes.Running a local server and syncing my history does result in
Checks