feat(gc): Implement SQLite-backed GcStorage by aldy505 · Pull Request #653 · getsentry/objectstore

aldy505 · 2026-09-26T04:54:54Z

Introduces GcStorage trait, that will be implemented by various backend. Also implemented SQLite here.

Supersedes #582
Refs FS-482

Supersedes #582

cursor

Stale Bugbot comment from a previous run.

It's so hard to develop this on Windows. I need to grab my Linux laptop.

codecov · 2026-09-27T12:41:18Z

Codecov Report

❌ Patch coverage is 97.20930% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.93%. Comparing base (1c2db3b) to head (5a7f734).
⚠️ Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
...bjectstore-service/src/garbage_collector/sqlite.rs	97.20%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
+ Coverage   91.52%   91.93%   +0.41%     
==========================================
  Files         116      120       +4     
  Lines       22175    23274    +1099     
==========================================
+ Hits        20295    21397    +1102     
+ Misses       1880     1877       -3

Components	Coverage Δ
Rust Backend	`94.88% <97.20%> (+0.19%)`	⬆️
Rust Client	`89.13% <ø> (+4.71%)`	⬆️
Python Client	`91.64% <ø> (-2.25%)`	⬇️

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

linear-code · 2026-09-29T09:07:48Z

FS-482

matt-codecov

a couple questions:

do we have performance goals for self-hosted deployments? i have been burned by poor single-insert sqlite performance before. if we need high throughput, we may want to introduce batched queries, but it introduces some risk of inventory corruption if we crash without flushing batches
do we have scaling requirements for self-hosted deployments? i've seen a few different "distributed sqlite" projects (https://rqlite.io/ / https://docs.fly.io/litefs/how-it-works/) but generally consider sqlite to be node-local

aldy505 · 2026-09-30T04:20:49Z

do we have performance goals for self-hosted deployments? i have been burned by poor single-insert sqlite performance before. if we need high throughput, we may want to introduce batched queries, but it introduces some risk of inventory corruption if we crash without flushing batches

If they're using k8s (the unofficial helm chart), they want to use the postgres implementation of this garbage collector. In which I will implement after SQLite is correctly implemented. This is how Taskbroker is doing as well, they provide both options, with SQLite as default.

do we have scaling requirements for self-hosted deployments? i've seen a few different "distributed sqlite" projects (https://rqlite.io/ / https://docs.fly.io/litefs/how-it-works/) but generally consider sqlite to be node-local

I've contributed to rqlite before, but no thanks, let's not consider that approach.

lcian

The biggest point to resolve will be #653 (comment) but in the meantime we can get the rest of the things in order before we find a solution for that.

matt-codecov

#677 changed the ChangeStream trait to help write GC correctly. unfortunately you will need to rebase and tweak your PR to implement:

begin_write(): create the GC record before committing in the storage backend
begin_update(): lengthen the expiration in GC before committing in the storage backend
commit_delete(): delete the GC record after deleting the stored object

if begin_write() or begin_update() fail, it's not safe to allow the backend commit to happen. otherwise we can permanently leak objects or cause GC to delete them prematurely. so those functions have to .await rather than spawn and they have to surface failure to the caller

if commit_delete() fails it's kind of harmless. GC will just have a ghost record floating around until its expiry. if the ghost record has manual expiration then it'll float around forever, but that's... probably a small edge case.

…tream/sqlite-garbage-collector

…tream trait

cursor

Stale Bugbot comment from a previous run.

sentry-junior

sqlx is a hard dependency of objectstore-service, and it's pulled in with features this code doesn't need. tls-rustls-ring-native-roots and chrono aren't needed for a local SQLite file, and nothing in garbage_collector_sqlite.rs uses chrono. Trimming the features to runtime-tokio + sqlite avoids compiling the TLS stack for this. It's also worth putting it behind a feature (like storage-cogs), so builds that only use GCS or Bigtable don't compile bundled SQLite.

Posted by mistake; feedback was meant to be shared privately.

github-actions · 2026-11-01T12:32:21Z

Changelog Preview

This PR lands in these workspaces:

objectstore-server: CalVer

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).

New Features

Implement SQLite-backed GcStorage by aldy505 in #653

_{🤖 This preview updates automatically when you update the PR.}

…tream/sqlite-garbage-collector

lcian · 2026-11-03T16:54:20Z

+                let mut ids = Vec::new();
+                for row in rows {
+                    let id = row.clone();
+                    ids.push(ObjectId::from_storage_path(&id).unwrap());


This should never fail but if it does, let's please log the error at appropriate level (you can use the macros in objectstore_log to do that, please check how it's done elsewhere) and continue instead of panicking.

lcian · 2026-11-03T17:05:22Z

+        let pool = self.pool.clone();
+        let id = id.clone();
+
+        let query_result = sqlx::query("INSERT INTO garbage_collector (object_id, expires_at) VALUES (?, ?) ON CONFLICT (object_id) DO UPDATE SET expires_at = excluded.expires_at WHERE excluded.expires_at IS NULL OR garbage_collector.expires_at IS NULL OR garbage_collector.expires_at < excluded.expires_at")


Due to the fact that backend writes are not synchronized with the ChangeStream, and that we decided that our invariant is gonna be to store the maximum here, unfortunately we're gonna have to preserve a NULL expires_at (infinite duration) over a finite expires_at. It seems that this query is doing the opposite.

Concretely, we need to use something like:

INSERT INTO garbage_collector (object_id, expires_at) VALUES (?, ?) ON CONFLICT (object_id) DO UPDATE SET expires_at = excluded.expires_at WHERE excluded.expires_at IS NULL OR garbage_collector.expires_at < excluded.expires_at

This means that objects written with infinite expiry are stored forever, and that's the tradeoff we accept by storing the maximum.
Fear not, because this won't be a problem anymore after we refactor the service to mandate TieredStorage, after that we can synchronize the ChangeStream too, so conflicts should never happen.
Moreover, no usecase currently uses infinite expiry.

Is it possible to "update" the object to have null value on the expired_at field?

lcian · 2026-11-03T17:08:02Z

+        let pool = self.pool.clone();
+        let id = id.clone();
+
+        let query_result = sqlx::query("INSERT INTO garbage_collector (object_id, expires_at) VALUES (?, ?) ON CONFLICT (object_id) DO UPDATE SET expires_at = excluded.expires_at WHERE excluded.expires_at IS NULL OR garbage_collector.expires_at IS NULL OR garbage_collector.expires_at < excluded.expires_at")


[nit] You could use a raw string r#"INSERT INTO ..."# here to force this to format more nicely, as shown in https://github.com/getsentry/objectstore/pull/653/changes/f95fc6a39d1e0558c24ce9b2131f3b068046bba6..1fe4d5983b3def23fda5850026141baf24ff4ac5#r4457955970 for example.

Or we could use sqlx::query! but that introduces some requirements at build-time and I think that's overkill for our 3 small queries.

lcian · 2026-11-03T17:08:37Z

 serde_yaml = "0.9.34-deprecated"
 sqlx = "0.9.0"
-sketches-ddsketch = "0.3.1"
+sketches-ddsketch = "0.4.2"


There are many changes to the Cargo.toml/.lock which should go away when you rebase the stack.
Then [nit] look also at which sqlx features we're bringing in, we might only need a subset of the default.

I believe.. Jan, told me to only select the features on the package level instead of workspace level.

lcian · 2026-11-03T17:12:27Z

[nit] You could add an objectstore-service/build.rs to make sure that a rebuild applies changed or new migrations, see https://docs.rs/sqlx/0.9.0/sqlx/macro.migrate.html#triggering-recompilation-on-migration-changes.

lcian · 2026-11-03T17:13:57Z

@@ -0,0 +1,5 @@
+CREATE TABLE IF NOT EXISTS garbage_collector (
+    object_id TEXT NOT NULL PRIMARY KEY,
+    expires_at INTEGER,


It would likely be useful to add an index on expires_at given that we're going to iterate rows using ORDER BY expires_at ....

lcian · 2026-11-03T17:20:58Z

+            .await;
+
+        if query_result.is_err() {
+            let error = query_result.err();
+            match error {
+                Some(err) => return Err(ChangeStreamError::Failure(Box::new(err))),
+                None => {
+                    return Err(ChangeStreamError::Failure(
+                        "failed to insert row into garbage_collector".into(),
+                    ));
+                }
+            }
+        }
+
+        Ok(())


I think this can be rewritten as follows:

Suggested change

.await;

if query_result.is_err() {

let error = query_result.err();

match error {

Some(err) => return Err(ChangeStreamError::Failure(Box::new(err))),

None => {

return Err(ChangeStreamError::Failure(

"failed to insert row into garbage_collector".into(),

));

}

}

}

Ok(())

.await

.map_err(|e| ChangeStreamError::Failure(Box::new(e)))?;

Ok(())

Same for the analogous snippet in list_expired.

lcian

Left some minor comments.

lcian · 2026-11-03T17:26:52Z

+        let pool = self.pool.clone();
+        let id = id.clone();


[nit] In some places, such as here, it seems that using clone could be avoided, for instance here you only need a borrow of the pool which you can get through self.pool, similarly id.as_storage_path() only needs &self which we already have as argument.

Based on Lorenzo's comments

cursor

Stale Bugbot comment from a previous run.

cursor · 2026-11-08T10:51:25Z

+                DO UPDATE
+                SET expires_at = excluded.expires_at
+                WHERE garbage_collector.expires_at IS NOT NULL
+                    AND garbage_collector.expires_at < excluded.expires_at"#,


NULL expiry cannot replace a deadline

High Severity

The conflict WHERE clause requires garbage_collector.expires_at < excluded.expires_at. A comparison against SQL NULL is unknown, so an upsert with None (infinite duration) never replaces a stored finite deadline. That breaks the store-the-maximum invariant and the GcStorage contract that None marks an object as never expiring, so a later rewrite without a TTL can still be garbage-collected.

^{Reviewed by Cursor Bugbot for commit 7e221a8. Configure here.}

See https://docs.rs/sqlx/0.9.0/sqlx/macro.migrate.html#triggering-recompilation-on-migration-changes

cursor

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 5a7f734. Configure here.}

cursor · 2026-11-08T10:54:46Z

@@ -0,0 +1,3 @@
+fn main() {
+    println!("cargo:rerun-if-changed=migrations");
+}


Build script watches wrong migrations path

Medium Severity

cargo:rerun-if-changed watches migrations under objectstore-service, but sqlx::migrate! embeds files from ../migrations/sqlite. Added or changed SQL there will not dirty this crate, so the compiled migrator can stay stale until some other source file forces a rebuild.

^{Reviewed by Cursor Bugbot for commit 5a7f734. Configure here.}

feat(change-stream): SQLite-backed garbage collector

7dbb16c

Supersedes #582

aldy505 requested a review from a team as a code owner September 26, 2026 04:54

This was referenced Sep 26, 2026

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

Closed

feat: keeper trait & sqlite-backed implementation #582

Closed

cursor Bot reviewed Sep 26, 2026

View reviewed changes

sentry Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

aldy505 added 3 commits September 26, 2026 12:03

fix: build issues

296786a

It's so hard to develop this on Windows. I need to grab my Linux laptop.

fix(change-stream): handle null expires_at

e9f46ff

fix: more missing imports

d8207ac

sentry Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

feat(change-stream): working implementation for sqlite garbage collector

20935a3

sentry Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

chore: make clippy happy

b2e54d4

matt-codecov reviewed Sep 30, 2026

View reviewed changes

lcian reviewed Sep 30, 2026

View reviewed changes

fix: code review comments

b1b0db6

sentry Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

aldy505 added 2 commits October 1, 2026 08:44

test(change-stream): updated tests for sqlite garbage collector

3a21138

feat(change-stream): take timeout into account for join operation

282c346

aldy505 requested review from lcian and matt-codecov October 1, 2026 01:46

matt-codecov reviewed Oct 9, 2026

View reviewed changes

aldy505 added 2 commits October 10, 2026 11:06

Merge remote-tracking branch 'origin/main' into aldy505/feat/change-s…

ff77829

…tream/sqlite-garbage-collector

feat(change-stream): update sqlite implementation to use new change s…

f95fc6a

…tream trait

cursor Bot reviewed Oct 17, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

aldy505 requested a review from matt-codecov October 17, 2026 07:36

aldy505 added this pull request to stack #691 October 17, 2026 16:11

sentry-junior Bot previously requested changes Oct 23, 2026

View reviewed changes

aldy505 mentioned this pull request Oct 24, 2026

feat(change-stream): ExpiryIndex for Sqlite backend #690

Closed

lcian reviewed Oct 27, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

lcian reviewed Oct 27, 2026

View reviewed changes

Comment thread objectstore-service/src/change_stream/garbage_collector_sqlite.rs Resolved

ref: standalone garbage collector storage trait and implementation

3ac0f5f

aldy505 added 2 commits November 1, 2026 19:36

Merge remote-tracking branch 'origin/main' into aldy505/feat/change-s…

ee81ab4

…tream/sqlite-garbage-collector

tests: sqlite garbage collector

1fe4d59

aldy505 requested a review from lcian November 1, 2026 13:46

aldy505 changed the title ~~feat(change-stream): SQLite-backed garbage collector~~ feat(garbage-collector): sqlite implementation Nov 1, 2026

aldy505 removed this pull request from stack #691 November 1, 2026 14:28

aldy505 added this pull request to stack #749 November 1, 2026 14:28

lcian changed the title ~~feat(garbage-collector): sqlite implementation~~ feat(gc): Implement SQLite-backed GcStorage Nov 3, 2026

lcian reviewed Nov 3, 2026

View reviewed changes

jan-auer mentioned this pull request Nov 4, 2026

meta(ai): Add a skill for writing changelog entries #761

Merged

feat(gc): improve query for handling object expiry

7e221a8

Based on Lorenzo's comments

cursor Bot reviewed Nov 8, 2026

View reviewed changes

chore(service): ensure migration is applied

5a7f734

See https://docs.rs/sqlx/0.9.0/sqlx/macro.migrate.html#triggering-recompilation-on-migration-changes

cursor Bot reviewed Nov 8, 2026

View reviewed changes

Conversation

aldy505 commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

linear-code Bot commented Sep 29, 2026

Uh oh!

matt-codecov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aldy505 commented Sep 30, 2026

Uh oh!

lcian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matt-codecov left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sentry-junior Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog Preview

New Features

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcian Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

aldy505 commented Sep 26, 2026 •

edited

Loading

cursor Bot left a comment •

edited

Loading

codecov Bot commented Sep 27, 2026 •

edited

Loading

lcian left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading

sentry-junior Bot left a comment •

edited

Loading

github-actions Bot commented Nov 1, 2026 •

edited

Loading

lcian Nov 3, 2026 •

edited

Loading

cursor Bot left a comment •

edited

Loading