feat: keeper trait & sqlite-backed implementation - #582
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #582 +/- ##
==========================================
- Coverage 87.99% 87.93% -0.07%
==========================================
Files 96 97 +1
Lines 15956 16189 +233
==========================================
+ Hits 14041 14235 +194
- Misses 1915 1954 +39
☔ 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}
| 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.
| 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.
There was a problem hiding this comment.
Therefore we need to trust whatever the server is giving to the keeper?
| /// 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:
|
| expiration_policy INTEGER NOT NULL DEFAULT 0, | ||
| duration INTEGER, | ||
| created_at INTEGER NOT NULL, | ||
| expires_at INTEGER |
There was a problem hiding this comment.
We will likely need an index on this.
| .try_into() | ||
| .map_err(|_| Error::generic("current time exceeds i64::MAX"))?; | ||
|
|
||
| let time_expires = expiration_duration.map(|duration| current_time + duration); |
There was a problem hiding this comment.
Bug: The addition current_time + duration to calculate time_expires can overflow an i64, causing the object to be considered permanently expired and preventing future updates.
Severity: MEDIUM
Suggested Fix
Replace the direct addition (+) with checked_add() when calculating time_expires. This will allow handling the overflow case explicitly, for example by returning an error or capping the expiration time at i64::MAX.
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#L113
Potential issue: The calculation of `time_expires` via `current_time + duration` uses
unchecked `i64` addition. While both `current_time` and `duration` are individually
validated to be less than `i64::MAX`, their sum can still exceed this limit. This can
happen if a client provides a very large but valid duration (e.g., hundreds of years).
When an overflow occurs, the resulting `time_expires` wraps around to a large negative
number. This causes the system to treat the object as immediately and permanently
expired, preventing any future updates to it. The issue is present in the `keep` method
and also in the `update` method for both `TimeToLive` and `TimeToIdle` policies.
Also affects:
objectstore-service/src/keeper/sqlite_backed.rs:207objectstore-service/src/keeper/sqlite_backed.rs:210
2918f9c to
2a8235d
Compare
| let time_expires = match expiration_policy { | ||
| ExpirationPolicy::Manual => None, | ||
| ExpirationPolicy::TimeToLive(_) => { | ||
| expiration_duration.map(|duration| keeper_row.time_created + duration) |
There was a problem hiding this comment.
Bug: The addition of keeper_row.time_created and duration is unchecked. With an extremely large duration, this can overflow, causing incorrect expiration behavior.
Severity: LOW
Suggested Fix
Use checked arithmetic, such as .checked_add(), to handle the potential overflow gracefully. Alternatively, add validation to ensure the provided duration is within a reasonable range before performing the addition.
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#L207
Potential issue: In the `update` method, the calculation of `time_expires` involves
adding two `i64` values: `keeper_row.time_created` and `duration`. This addition is not
checked for overflow. While `duration` is validated to fit within an `i64`, their sum
can exceed `i64::MAX` if an extremely large, albeit valid, duration is provided by a
client. In release builds, this would cause the value to wrap around to a negative
number, making the object appear to be immediately expired. In debug builds, this would
cause a panic.
| ) | ||
| .bind(match expiration_policy { | ||
| ExpirationPolicy::Manual => SQLITE_POLICY_MANUAL, | ||
| ExpirationPolicy::TimeToLive(_) => SQLITE_POLICY_TIME_TO_LIVE, | ||
| ExpirationPolicy::TimeToIdle(_) => SQLITE_POLICY_TIME_TO_IDLE, | ||
| }) | ||
| .bind(expiration_duration) | ||
| .bind(time_expires) | ||
| .bind(id.as_storage_path().to_string()) | ||
| .execute(&mut *atomic) | ||
| .await?; |
There was a problem hiding this comment.
Bug: The update() method can create database rows for Manual expiration policies, while the keep() method intentionally avoids this, leading to an inconsistency.
Severity: MEDIUM
Suggested Fix
To ensure consistent behavior, the update() method should remove the database row when an object's policy is changed to Manual, similar to calling remove(). Alternatively, the design should be updated to consistently handle Manual rows.
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#L216-L235
Potential issue: There is a logical inconsistency in how manual expiration policies are
handled. The `keep()` method does not create a database entry for objects with a
`Manual` policy, following a design principle that they should not be persisted.
However, the `update()` method can change an existing object's policy to `Manual`, which
leaves a row in the database with `expiration_policy = 0`. This creates an 'orphan' row
that violates the intended design and could be mishandled by future cleanup logic.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 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 2a8235d. Configure here.
| .execute(&mut *atomic) | ||
| .await?; | ||
|
|
||
| atomic.commit().await?; |
There was a problem hiding this comment.
Update races on stale keeper row
Medium Severity
update reads from read_pool, then later writes on write_pool without holding that snapshot in the same transaction. A concurrent remove plus keep (or another update) can land a newer row that this UPDATE then overwrites with the stale policy and expiry.
Reviewed by Cursor Bugbot for commit 2a8235d. Configure here.
| .unwrap(); | ||
|
|
||
| let row = tk.fetch_row(&id).await.unwrap(); | ||
| assert_eq!(row.time_expires, Some(now + 60)); |
There was a problem hiding this comment.
TTI update test can flake
Low Severity
update_tti_without_time_expires_sets_it snapshots now in seconds, then asserts time_expires equals now + 60. update computes expiry from a later SystemTime::now(), so crossing a second boundary makes the assertion fail despite correct behavior.
Reviewed by Cursor Bugbot for commit 2a8235d. Configure here.
In metadata, the duration for expiring policies is always positive (see Is there a place where you're seeing negative durations (other than for expired objects, of course)? |
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.
Refs FS-482