feat(keeper): integrate filesystem backend with keeper trait - #583
feat(keeper): integrate filesystem backend with keeper trait#583aldy505 wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| keeper: KeeperConfig { | ||
| backend: KeeperBackend::Sqlite, | ||
| connection_url: "sqlite:///opt/objectstore/keeper.db".into(), | ||
| }, |
There was a problem hiding this comment.
Broken default keeper database path
High Severity
The default connection_url points at /opt/objectstore/keeper.db, a path that does not exist elsewhere in this project. Local runs, Docker (/data), README env setup, and e2e tests all rely on this default when keeper is unset, so filesystem backend startup will fail creating the SQLite database.
Reviewed by Cursor Bugbot for commit 499ea21. Configure here.
| _ => Err(Error::generic(format!("unknown backend {}", s))), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Keeper backend config case mismatch
Medium Severity
KeeperBackend deserializes as Sqlite, but the filesystem config docs and FromStr accept sqlite. Config written as documented (or via env) will fail to parse. Other config enums in this repo use rename_all = "lowercase", and the new FromStr impl is never wired into serde.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 499ea21. Configure here.
| { | ||
| objectstore_log::debug!("Object not found"); | ||
| } | ||
| self.keeper.remove(id).await?; |
There was a problem hiding this comment.
Keeper removed on failed deletes
Medium Severity
delete_object always calls keeper.remove before returning the filesystem delete result. If remove_file fails for a reason other than not found, the keeper entry is dropped while the object file remains, so TTL/TTI tracking is permanently lost for that object.
Reviewed by Cursor Bugbot for commit 499ea21. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| self.keeper.keep(id, metadata.expiration_policy).await?; | ||
|
|
There was a problem hiding this comment.
Bug: Overwriting an object with a TTL/TTI policy fails due to a database UNIQUE constraint violation, causing data inconsistency despite the file being successfully updated.
Severity: HIGH
Suggested Fix
Modify the keeper.keep() method to handle cases where an object ID already exists. Instead of a simple INSERT, use an INSERT OR REPLACE or an equivalent UPSERT statement. This will ensure that overwriting an object also correctly updates its expiration policy in the keeper database without causing a constraint violation.
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/backend/local_fs.rs#L136-L137
Potential issue: When an object with a non-manual expiration policy (e.g., TTL or TTI)
is overwritten, the file is successfully updated on the filesystem, but the overall
operation fails. This occurs because the `keeper.keep()` method attempts to `INSERT` a
new record for the object's expiration into the SQLite database. Since a record for that
`object_id` already exists, this action violates the `PRIMARY KEY` constraint. The
resulting SQL error is propagated, causing the `put_object` request to fail, leading to
an inconsistency between the file storage and the expiration keeper database.
Also affects:
objectstore-service/src/backend/local_fs.rs:457~458
| file.sync_data().await?; | ||
| drop(file); | ||
|
|
||
| self.keeper.keep(id, metadata.expiration_policy).await?; |
There was a problem hiding this comment.
Overwrite breaks keeper retention tracking
High Severity
put_object and complete_multipart always call keeper.keep, but keep only inserts and rejects duplicates. Overwriting an object with a client-supplied key fails after the file is already replaced, or silently leaves a stale retention row when the new policy is Manual.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eba4ba5. Configure here.
bad7565 to
5bebaee
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 6 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5bebaee. Configure here.
| } | ||
| err => err?, | ||
| }; | ||
| self.keeper.mark_accessed(id).await?; |
There was a problem hiding this comment.
GET does not refresh TTI
High Severity
get_object calls keeper.mark_accessed before metadata is read. That method is not on Keeper. Time-to-idle refresh is implemented as update, which needs the expiration policy. Access therefore cannot extend TTI, so idle objects expire on the original deadline even when they are read.
Reviewed by Cursor Bugbot for commit 5bebaee. Configure here.
|
|
||
| /// Configuration for the keeper backend. | ||
| /// See [`KeeperConfig`] for details. | ||
| pub keeper: KeeperConfig, |
There was a problem hiding this comment.
Keeper config has no default
Medium Severity
FileSystemConfig.keeper is required and KeeperConfig has no Default or serde(default). Top-level filesystem can inherit keeper from Config::default(), but nested filesystem (for example tiered long_term via env with only type and path) cannot. Those configs fail to deserialize after this change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5bebaee. Configure here.
1 participant
Refs FS-482