Skip to content

feat(keeper): integrate filesystem backend with keeper trait #583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aldy505 wants to merge 4 commits into
base: aldy505/feat/ttl-keeper
Choose a base branch
from
+106 −22
Open

feat(keeper): integrate filesystem backend with keeper trait #583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
499ea21
feat(keeper): integrate filesystem backend with keeper trait
aldy505 Aug 1, 2026
eba4ba5
Merge branch 'aldy505/feat/ttl-keeper' into aldy505/feat/integrate-fi…
aldy505 Aug 1, 2026
9db675f
fix(keeper): tests lints
aldy505 Aug 1, 2026
bad7565
test(localfs): use unwrap
aldy505 Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions objectstore-server/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ use std::time::Duration;
use anyhow::Result;
use figment::providers::{Env, Format, Serialized, Yaml};
use objectstore_service::backend::local_fs::FileSystemConfig;
use objectstore_service::keeper::{KeeperBackend, KeeperConfig};
use objectstore_types::auth::Permission;
use secrecy::{CloneableSecret, SecretBox, SerializableSecret, zeroize::Zeroize};
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -586,6 +587,10 @@ impl Default for Config {

storage: StorageConfig::FileSystem(FileSystemConfig {
path: PathBuf::from("data"),
keeper: KeeperConfig {
backend: KeeperBackend::Sqlite,
connection_url: "sqlite:///opt/objectstore/keeper.db".into(),
},

@cursor cursor Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 499ea21. Configure here.

}),

runtime: Runtime::default(),
Expand Down
83 changes: 65 additions & 18 deletions objectstore-service/src/backend/local_fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ use crate::backend::common::{
};
use crate::error::{Error, Result};
use crate::id::ObjectId;
use crate::keeper::sqlite_backed::SqliteBackedKeeper;
use crate::keeper::{Keeper, KeeperBackend, KeeperConfig};
use crate::multipart::{
AbortMultipartResponse, CompleteMultipartResponse, CompletedPart, InitiateMultipartResponse,
ListPartsResponse, Part, PartNumber, UploadId, UploadPartResponse,
Expand All @@ -34,6 +36,9 @@ use crate::stream::{self, ClientStream};
/// storage:
/// type: filesystem
/// path: /data
/// keeper:
/// backend: sqlite
/// connection_url: sqlite://data/keeper.db
/// ```
#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)]
pub struct FileSystemConfig {
Expand All @@ -51,18 +56,31 @@ pub struct FileSystemConfig {
/// - `OS__STORAGE__TYPE=filesystem`
/// - `OS__STORAGE__PATH=/path/to/storage`
pub path: PathBuf,

/// Configuration for the keeper backend.
/// See [`KeeperConfig`] for details.
pub keeper: KeeperConfig,
}

/// Local filesystem backend for development and testing.
#[derive(Debug)]
pub struct LocalFsBackend {
path: PathBuf,
keeper: Box<dyn Keeper>,
}

impl LocalFsBackend {
/// Creates a new [`LocalFsBackend`] rooted at the directory in `config`.
pub fn new(config: FileSystemConfig) -> Self {
Self { path: config.path }
pub async fn new(config: FileSystemConfig) -> Result<Self> {
let keeper = match config.keeper.backend {
KeeperBackend::Sqlite => {
Box::new(SqliteBackedKeeper::new(&config.keeper.connection_url).await?)
}
};
Ok(Self {
path: config.path,
keeper,
})
}
}

Expand Down Expand Up @@ -115,6 +133,8 @@ impl Backend for LocalFsBackend {
file.sync_data().await?;
drop(file);

self.keeper.keep(id, metadata.expiration_policy).await?;

@cursor cursor Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit eba4ba5. Configure here.


Comment on lines +136 to +137

@sentry sentry Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ok(())
}

Expand All @@ -131,6 +151,7 @@ impl Backend for LocalFsBackend {
}
err => err?,
};
self.keeper.mark_accessed(id).await?;

let mut reader = BufReader::new(file);
let mut metadata_line = String::new();
Expand Down Expand Up @@ -174,6 +195,7 @@ impl Backend for LocalFsBackend {
{
objectstore_log::debug!("Object not found");
}
self.keeper.remove(id).await?;

@cursor cursor Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 499ea21. Configure here.

Ok(result?)
Comment thread
sentry[bot] marked this conversation as resolved.
Comment on lines 195 to 199

@sentry sentry Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: In delete_object, a failure in self.keeper.remove(id) can mask the result of the filesystem deletion, potentially causing the caller to believe an object exists when it has been deleted.
Severity: HIGH

Suggested Fix

Handle the result of the filesystem deletion before attempting the keeper database operation. For example, if the file is not found, return the NotFound error immediately. If the file is deleted successfully, proceed with the keeper operation but ensure its potential failure does not obscure the successful deletion.

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#L195-L199

Potential issue: In the `delete_object` function, the result of the filesystem deletion
is stored but not immediately returned. An unconditional call to
`self.keeper.remove(id).await?` follows. If this database operation fails (e.g., due to
an I/O error or the database being unavailable), the `?` operator will propagate the
database error, and the original result from the filesystem operation will be discarded.
This can lead to an inconsistent state where a file is successfully deleted from the
filesystem, but the caller receives a database error, leading it to believe the object
still exists.

}
}
Expand Down Expand Up @@ -432,6 +454,8 @@ impl MultipartUploadBackend for LocalFsBackend {
file.sync_data().await?;
drop(file);

self.keeper.keep(id, metadata.expiration_policy).await?;

// Clean up multipart state
tokio::fs::remove_dir_all(dir).await?;

Expand All @@ -458,7 +482,13 @@ mod tests {
let tempdir = tempfile::tempdir().unwrap();
let backend = LocalFsBackend::new(FileSystemConfig {
path: tempdir.path().to_path_buf(),
});
keeper: KeeperConfig {
backend: KeeperBackend::Sqlite,
connection_url: "sqlite::memory:".into(),
},
})
.await
.unwrap();

let id = ObjectId::random(ObjectContext {
usecase: "testing".into(),
Expand Down Expand Up @@ -499,7 +529,13 @@ mod tests {
let tempdir = tempfile::tempdir().unwrap();
let backend = LocalFsBackend::new(FileSystemConfig {
path: tempdir.path().to_path_buf(),
});
keeper: KeeperConfig {
backend: KeeperBackend::Sqlite,
connection_url: "sqlite::memory:".into(),
},
})
.await
.unwrap();

let id = ObjectId::random(ObjectContext {
usecase: "testing".into(),
Expand Down Expand Up @@ -533,7 +569,13 @@ mod tests {
let tempdir = tempfile::tempdir().unwrap();
let backend = LocalFsBackend::new(FileSystemConfig {
path: tempdir.path().to_path_buf(),
});
keeper: KeeperConfig {
backend: KeeperBackend::Sqlite,
connection_url: "sqlite::memory:".into(),
},
})
.await
.unwrap();

let id = ObjectId::random(ObjectContext {
usecase: "testing".into(),
Expand All @@ -551,17 +593,22 @@ mod tests {
})
}

fn make_backend() -> (tempfile::TempDir, LocalFsBackend) {
async fn make_backend() -> Result<(tempfile::TempDir, LocalFsBackend)> {
let tempdir = tempfile::tempdir().unwrap();
let backend = LocalFsBackend::new(FileSystemConfig {
path: tempdir.path().to_path_buf(),
});
(tempdir, backend)
keeper: KeeperConfig {
backend: KeeperBackend::Sqlite,
connection_url: "sqlite::memory:".into(),
},
})
.await?;
Ok((tempdir, backend))
}

#[tokio::test]
async fn multipart_single_part() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata {
content_type: "text/plain".into(),
Expand Down Expand Up @@ -613,7 +660,7 @@ mod tests {

#[tokio::test]
async fn multipart_multiple_parts() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand Down Expand Up @@ -687,7 +734,7 @@ mod tests {

#[tokio::test]
async fn multipart_list_parts() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand Down Expand Up @@ -750,7 +797,7 @@ mod tests {

#[tokio::test]
async fn get_object_range_bounded() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand All @@ -777,7 +824,7 @@ mod tests {

#[tokio::test]
async fn get_object_range_from() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand All @@ -804,7 +851,7 @@ mod tests {

#[tokio::test]
async fn get_object_range_last() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand All @@ -831,7 +878,7 @@ mod tests {

#[tokio::test]
async fn get_object_range_unsatisfiable() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand All @@ -849,7 +896,7 @@ mod tests {

#[tokio::test]
async fn multipart_abort() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand All @@ -875,7 +922,7 @@ mod tests {

#[tokio::test]
async fn multipart_invalid_etag() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand Down Expand Up @@ -924,7 +971,7 @@ mod tests {

#[tokio::test]
async fn multipart_missing_part() {
let (_tempdir, backend) = make_backend();
let (_tempdir, backend) = make_backend().await.unwrap();
let id = make_id();
let metadata = Metadata::default();

Expand Down
6 changes: 4 additions & 2 deletions objectstore-service/src/backend/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ pub async fn from_config(config: StorageConfig) -> Result<Box<dyn common::Backen

async fn from_leaf_config(config: StorageConfig) -> Result<Box<dyn common::Backend>> {
Ok(match config {
StorageConfig::FileSystem(c) => Box::new(local_fs::LocalFsBackend::new(c)),
StorageConfig::FileSystem(c) => Box::new(local_fs::LocalFsBackend::new(c).await?),
StorageConfig::S3Compatible(c) => {
Box::new(s3_compatible::S3CompatibleBackend::without_token(c))
}
Expand Down Expand Up @@ -127,7 +127,9 @@ async fn lt_from_config(
config: MultipartUploadStorageConfig,
) -> Result<Box<dyn common::MultipartUploadBackend>> {
Ok(match config {
MultipartUploadStorageConfig::FileSystem(c) => Box::new(local_fs::LocalFsBackend::new(c)),
MultipartUploadStorageConfig::FileSystem(c) => {
Box::new(local_fs::LocalFsBackend::new(c).await?)
}
MultipartUploadStorageConfig::Gcs(c) => Box::new(gcs::GcsBackend::new(c).await?),
})
}
34 changes: 32 additions & 2 deletions objectstore-service/src/keeper/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
//! Keeper defines a trait to support extending an object retention for backends
//! that does not support them natively (e.g., S3-compatible backend and filesystem).

use std::str::FromStr;

use objectstore_types::metadata::ExpirationPolicy;
use serde::{Deserialize, Serialize};

use crate::error::Result;
use crate::error::{Error, Result};
use crate::id::ObjectId;

/// SQLite-backed implementation of the [`Keeper`] trait.
Expand All @@ -18,7 +21,7 @@ pub struct ObjectExpiry<'a> {

/// Object retention keeper trait.
#[async_trait::async_trait]
pub trait Keeper: Send + Sync {
pub trait Keeper: Send + Sync + std::fmt::Debug {
/// Keep is the first step in the object retention lifecycle.
/// Practically speaking, it would not be kept if the `expiration_policy` is `Manual`.
/// The `expiration_policy` is set by the client at upload time via the supplied Metadata.
Expand All @@ -32,3 +35,30 @@ pub trait Keeper: Send + Sync {
/// extend the object retention.
async fn mark_accessed(&self, id: &ObjectId) -> Result<()>;
}

/// Keeper backend of choice.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum KeeperBackend {
/// SQLite-backed keeper.
Sqlite,
}

impl FromStr for KeeperBackend {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"sqlite" => Ok(KeeperBackend::Sqlite),
_ => Err(Error::generic(format!("unknown backend {}", s))),
}
}
}

@cursor cursor Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 499ea21. Configure here.


/// Configuration for the keeper backend.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub struct KeeperConfig {
/// Specifies the backend to use.
pub backend: KeeperBackend,
/// Connection URL for the backend.
/// Refer to each backend's documentation for details.
pub connection_url: String,
}
Loading