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 3 commits into 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
3 commits
Select commit Hold shift + click to select a range
e3f8b4e
feat(keeper): integrate filesystem backend with keeper trait
aldy505 Aug 1, 2026
5375c0b
fix(keeper): tests lints
aldy505 Aug 1, 2026
5bebaee
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 @@ -633,6 +634,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,

@cursor cursor Bot Aug 13, 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 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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bebaee. Configure here.

}

/// 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?;

@cursor cursor Bot Aug 13, 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.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bebaee. Configure here.


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 thread
sentry[bot] marked this conversation as resolved.
}
}
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 @@ -31,3 +34,30 @@ pub trait Keeper: Send + Sync {
/// Update an object to a new expiration policy (and thus, new expiration time).
async fn update(&self, id: &ObjectId, expiration_policy: ExpirationPolicy) -> 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