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.
1 participant
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.