feat: keeper trait & sqlite-backed implementation - #582
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #582 +/- ##
==========================================
+ Coverage 87.44% 87.54% +0.10%
==========================================
Files 93 95 +2
Lines 14753 15811 +1058
==========================================
+ Hits 12901 13842 +941
- Misses 1852 1969 +117
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| .read_only(true) | ||
| .disable_statement_logging(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Read-only WAL pool setup fails
High Severity
create_sqlite_pool opens the read pool with journal_mode(Wal) and read_only(true) before the write pool can switch the database to WAL. Changing journal mode needs write access, so SqliteBackedKeeper::new is likely to fail when creating or opening a non-WAL database. In-memory tests bypass this path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 076740d. Configure here.
| let expiration_duration: Option<i64> = expiration_policy.expires_in().and_then(|x| { | ||
| x.as_secs() | ||
| .try_into() | ||
| .map_err(|_| Error::generic("expiration duration exceeds i64::MAX")) | ||
| .ok() | ||
| }); |
There was a problem hiding this comment.
Bug: A Duration exceeding i64::MAX is silently stored as NULL in the database, creating an inconsistent state for objects with an expiration policy.
Severity: LOW
Suggested Fix
Instead of using .ok() to discard the error, propagate the Result of the try_into() conversion. This will cause the operation to fail explicitly when an out-of-range duration is provided, preventing the insertion of records with inconsistent state. This ensures that any object with an expiration policy also has a valid expiration time stored.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: objectstore-service/src/keeper/sqlite_backed.rs#L92-L97
Potential issue: The conversion of an `ExpirationPolicy` duration from `u64` seconds to
an `i64` for database storage uses `.ok()`, which silently discards overflow errors. If
a `Duration` greater than `i64::MAX` (approximately 292 billion years) is provided, the
conversion fails and results in `None`. This `None` value is then persisted as `NULL`
for the `duration` and `expires_at` columns. This creates an inconsistent database state
where an object has an expiration policy (TTL/TTI) but no corresponding expiration data,
which could lead to unexpected retention behavior.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 56c46f3. Configure here.
| tk.keeper.mark_accessed(&id).await.unwrap(); | ||
|
|
||
| let row = tk.fetch_row(&id).await.unwrap(); | ||
| assert_eq!(row.expires_at, Some(now + 60)); |
There was a problem hiding this comment.
Flaky second-boundary expiry assertion
Low Severity
mark_accessed_tti_without_expires_at_sets_it captures now, then asserts expires_at equals exactly now + 60. mark_accessed recomputes time independently in whole seconds, so a second boundary between those calls makes the assertion fail even when behavior is correct.
Reviewed by Cursor Bugbot for commit 56c46f3. Configure here.
There was a problem hiding this comment.
Thank you! This is a first review pass with some questions.
The design looks good, specifically:
- It's great to introduce a trait so we can have different implementations of this via config.
- The overall keeper interface is simple and doesn't assume too much about object lifecycle (though see my comment on TTI below).
- It makes sense that the backend "owns" the instance of the keeper.
Some question to the overall design:
- Who is responsible to scan for deleted objects and drive deletion and what will the interface for this look like?
- If it is the keeper, how does it tell the backend to delete?
- If it is the backend, how does it use the keeper interface to scan? There's no method to iterate objects that are ready for deletion
- If the keeper database gets corrupted, deleted, or the keeper is swapped out, we lose all information on objects and GC for those objects will no longer happen. Doesn't have to be solved immediately, but do you already have thoughts on this?
- How will the keeper ensure that on concurrent writes to the same key that keeper's entry corresponds to what is stored in the backend?
- Two requests PUT at the same time with different expires_at. Only one of them will win, keeper must end up with the same expiry time.
- One request PUTs and one DELETEs at the same time. One of them wins, and the keeper must match.
When calling keep, ensure to do so before writing the object. Otherwise, we could end up with a persisted object but without keeper entry.
sqlite can only have a single writer attached to a database file at any time. This means when sqlite is used, one cannot run multiple objectstore instances. This is an important restriction we should add to some doc comment and later to the config that exposes this.
| percent-encoding = { workspace = true } | ||
| rand = { workspace = true } | ||
| reqwest = { workspace = true, features = ["charset", "http2", "system-proxy", "native-tls-no-alpn"] } | ||
| reqwest = { workspace = true, features = [ |
There was a problem hiding this comment.
Let's undo these formatting changes to keep dependencies in a single line. Same for other Cargo.toml files.
| "set-header", | ||
| "trace", | ||
| ] } | ||
| sqlx.workspace = true |
There was a problem hiding this comment.
Looks like this is unused in the server.
There was a problem hiding this comment.
This is me toying around with the sqlx migrate stuff, to have compile time checks for the query.
| sentry = { workspace = true } | ||
| serde = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| sqlx = { workspace = true } |
There was a problem hiding this comment.
Ideally, leave the workspace dependency without most features and include the required features here. This way, when we use conditional compilation of certain sub-crates we automatically get the smallest possible set of features.
|
|
||
| /// Object retention keeper trait. | ||
| #[async_trait::async_trait] | ||
| pub trait Keeper: Send + Sync { |
There was a problem hiding this comment.
Can we find a more descriptive name for this? Keeper is a nice and short name, but it lacks context on what this is for.
Intuitively, what we're building is GC, so I'm throwing that in as a suggestion.
There was a problem hiding this comment.
I took Zookeeper and ClickHouse Keeper as a reference for this 😄
| async fn keep(&self, id: &ObjectId, expiration_policy: ExpirationPolicy) -> Result<()>; | ||
|
|
||
| /// Remove is the final step in the object retention lifecycle. | ||
| /// It is called by a cleanup worker when the object is no longer needed. |
There was a problem hiding this comment.
Either this, or when the object is deleted per user request (or as part of tiered storage cleanup, but that's the same method).
|
|
||
| /// Marks an object as accessed. For `expiration_policy` of `TimeToIdle`, this will | ||
| /// extend the object retention. | ||
| async fn mark_accessed(&self, id: &ObjectId) -> Result<()>; |
There was a problem hiding this comment.
Can we instead make this more of an "update" method and pass an explicit new expiration time?
I'm currently working on refactoring TTI to centralize its logic. Right now, backends have to handle this all internally, which leads to multiple problems. The biggest one is that the eviction timestamp can go out of sync with the actual object.
We can fix that by passing an explicit expiration time around.
| /// Unix timestamp (seconds) when the row was created. | ||
| pub created_at: i64, | ||
| /// Unix timestamp (seconds) when the object expires, if applicable. | ||
| pub expires_at: Option<i64>, |
There was a problem hiding this comment.
nit: Where possible, let's adopt the same terminology we also use in Metadata, such as time_expires, etc.
I'm thinking of a separate cleanup process for this. Therefore only the keeper is the one who's responsible.
I haven't think this through. On my current proposal, I know that there will be lots of orphan objects, and there's no way to figure out whether it should be managed by the keeper or not. One thing that came across my mind just now is to append it on the sidecar metadata file (that we've talked about on Slack). I'm thinking it only for a last resort recovery option.
Oh yes, and I'm considering to add Postgres as another keeper backend. |
Yes I'm aware of this. Since sqlite is a single writer, I would trust whoever enters objectstore first. |
Sqlite is a single writer, but objectstore allows non-blocking concurrent requests on the same object. Since these requests consist of several sequential operations, there can be races like TOCTOU and lost updates. A request that comes in first may not be the first to finish, and there can be any form of interleaving. In principle, there are these options:
|
2 participants
Internal Slack thread.
This is required for filesystem & S3-compatible API backends. Later, I'll try to integrate this with filesystem backend, and create Postgres-backed keeper.