Run SQL persistence cleanup in bounded batches#6564
Open
AVtheking wants to merge 1 commit into
Open
Conversation
🦋 Changeset detectedLatest commit: 4af19bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughShared SQL persistence now creates an expiration index and performs periodic bounded cleanup of expired rows across SQL dialects. MySQL, PostgreSQL, and SQLite tests verify deletion of expired records, retention of live records, and index creation. ChangesPersistence cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PersistenceLayer
participant TestClock
participant SQLDatabase
PersistenceLayer->>SQLDatabase: create expiration index
PersistenceLayer->>TestClock: schedule periodic cleanup
TestClock-->>PersistenceLayer: advance cleanup time
PersistenceLayer->>SQLDatabase: delete expired rows in batches
SQLDatabase-->>PersistenceLayer: return cleanup result
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
AVtheking
marked this pull request as ready for review
July 23, 2026 22:39
hsubra89
reviewed
Jul 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/effect/src/unstable/persistence/Persistence.ts (2)
606-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDialect-specific batched delete queries look sound.
- pg: CTE with
ctid, bounded byLIMIT, atomic single statement.- mysql: single-table
DELETE ... ORDER BY ... LIMIT, wrapped insql.withTransactionso the follow-upSELECT ROW_COUNT()reads from the same session/connection as theDELETE— this is the right call given connection pooling.- mssql:
UPDLOCK, READPAST, READCOMMITTEDLOCKhints reduce lock contention with concurrent readers/writers, and the whole thing is one statement.- sqlite:
rowid IN (...)bounded subquery +RETURNING, avoiding interactive transactions.Minor nit on the mysql branch (Lines 619-631): wrapping an inner
Effect.gen(...).pipe(sql.withTransaction)inside an outerEffect.fnUntraced(function*(...) { return yield* ... })is a bit redundant — the outer generator only does oneyield*and returns. Could flatten to a single function.♻️ Flatten the mysql branch
- mysql: () => - Effect.fnUntraced(function*(expiresAtOrBefore: number) { - return yield* Effect.gen(function*() { - yield* sql` - DELETE FROM ${table} - WHERE expires IS NOT NULL AND expires <= ${expiresAtOrBefore} - ORDER BY expires - LIMIT ${sql.literal(String(cleanupBatchSize))} - ` - const rows = yield* sql<{ readonly count: number }>`SELECT ROW_COUNT() AS count` - return Number(rows[0].count) - }).pipe(sql.withTransaction) - }), + mysql: () => (expiresAtOrBefore: number) => + Effect.gen(function*() { + yield* sql` + DELETE FROM ${table} + WHERE expires IS NOT NULL AND expires <= ${expiresAtOrBefore} + ORDER BY expires + LIMIT ${sql.literal(String(cleanupBatchSize))} + ` + const rows = yield* sql<{ readonly count: number }>`SELECT ROW_COUNT() AS count` + return Number(rows[0].count) + }).pipe(sql.withTransaction),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/persistence/Persistence.ts` around lines 606 - 659, Flatten the mysql branch’s redundant Effect wrapper by replacing the outer Effect.fnUntraced generator with a single function that directly returns the existing Effect.gen(...).pipe(sql.withTransaction) flow. Preserve the DELETE, same-connection ROW_COUNT query, transaction wrapping, and returned count behavior.
602-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making cleanup tuning knobs configurable.
cleanupBatchSize,cleanupBatchDelay, andcleanupIntervalare hardcoded. Different deployments may want to tune batch size/interval for their table size or DB load; exposing these via layer options (with these as defaults) would avoid a code change for tuning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/persistence/Persistence.ts` around lines 602 - 604, Expose cleanupBatchSize, cleanupBatchDelay, and cleanupInterval as configurable layer options in the persistence setup, while retaining 1000, 10 milliseconds, and 5 minutes as their defaults. Update the cleanup scheduling and batching logic to use the configured option values instead of the hardcoded constants.packages/sql/mysql2/test/Persistence.test.ts (1)
27-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid cross-dialect cleanup test; a couple of minor robustness notes.
The dual-phase polling (real-time wait for async batch 1, then
TestClock.adjustto trigger the scheduled batch 2) correctly models the mix of virtual schedule delay and real DB I/O latency.Two minor points (shared with the pg/sqlite variants, flagged once here and noted at each site):
- The magic number
1001/expected remainder1is implicitly coupled to the source's hardcodedcleanupBatchSize = 1000. If that constant changes, three test files across dialects need matching updates independently.- The retry budget of
100 * 10ms(~1s of live wait) per polling loop could be flaky under slow CI/test containers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sql/mysql2/test/Persistence.test.ts` around lines 27 - 89, Update the “deletes expired entries in batches” test to avoid hardcoding the cleanup batch size and remainder values; reuse the shared cleanupBatchSize symbol or derive the inserted count and expected remainder from it. Also replace the fixed 100-iteration polling limits with a less flaky wait mechanism or sufficiently tolerant timeout while preserving both asynchronous cleanup phases.packages/sql/sqlite-node/test/Persistence.test.ts (1)
110-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistent with the mysql/pg variants; same duplication/coupling notes apply.
See the consolidated comment for the shared magic-number coupling and duplicated polling-loop pattern across dialect test files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sql/sqlite-node/test/Persistence.test.ts` around lines 110 - 172, Refactor the “deletes expired entries in batches” test to remove duplicated polling logic and shared magic-number coupling across dialect variants. Extract the polling behavior and batch/timing values into reusable test helpers or constants, then reuse them for both cleanup loops while preserving the existing assertions and expiration behavior.packages/sql/pg/test/Persistence.test.ts (1)
107-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistent with the mysql/sqlite variants; same duplication/coupling notes apply.
See the consolidated comment for the shared magic-number coupling and duplicated polling-loop pattern across dialect test files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sql/pg/test/Persistence.test.ts` around lines 107 - 169, Update the “deletes expired entries in batches” test to remove duplicated polling logic and shared magic-number coupling. Reuse the established polling helper/constants from the other persistence dialect tests for batch size, retry limits, and polling interval, while preserving the assertions for eventual deletion and live-entry retention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/effect/src/unstable/persistence/Persistence.ts`:
- Around line 606-659: Flatten the mysql branch’s redundant Effect wrapper by
replacing the outer Effect.fnUntraced generator with a single function that
directly returns the existing Effect.gen(...).pipe(sql.withTransaction) flow.
Preserve the DELETE, same-connection ROW_COUNT query, transaction wrapping, and
returned count behavior.
- Around line 602-604: Expose cleanupBatchSize, cleanupBatchDelay, and
cleanupInterval as configurable layer options in the persistence setup, while
retaining 1000, 10 milliseconds, and 5 minutes as their defaults. Update the
cleanup scheduling and batching logic to use the configured option values
instead of the hardcoded constants.
In `@packages/sql/mysql2/test/Persistence.test.ts`:
- Around line 27-89: Update the “deletes expired entries in batches” test to
avoid hardcoding the cleanup batch size and remainder values; reuse the shared
cleanupBatchSize symbol or derive the inserted count and expected remainder from
it. Also replace the fixed 100-iteration polling limits with a less flaky wait
mechanism or sufficiently tolerant timeout while preserving both asynchronous
cleanup phases.
In `@packages/sql/pg/test/Persistence.test.ts`:
- Around line 107-169: Update the “deletes expired entries in batches” test to
remove duplicated polling logic and shared magic-number coupling. Reuse the
established polling helper/constants from the other persistence dialect tests
for batch size, retry limits, and polling interval, while preserving the
assertions for eventual deletion and live-entry retention.
In `@packages/sql/sqlite-node/test/Persistence.test.ts`:
- Around line 110-172: Refactor the “deletes expired entries in batches” test to
remove duplicated polling logic and shared magic-number coupling across dialect
variants. Extract the polling behavior and batch/timing values into reusable
test helpers or constants, then reuse them for both cleanup loops while
preserving the existing assertions and expiration behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e780bf3-ce79-4f5c-96a8-18755a9ef732
📒 Files selected for processing (5)
.changeset/batch-persistence-expiration-cleanup.mdpackages/effect/src/unstable/persistence/Persistence.tspackages/sql/mysql2/test/Persistence.test.tspackages/sql/pg/test/Persistence.test.tspackages/sql/sqlite-node/test/Persistence.test.ts
AVtheking
force-pushed
the
codex/batch-persistence-expiration-cleanup
branch
from
July 23, 2026 22:53
0558e81 to
4af19bf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/effect/src/unstable/persistence/Persistence.ts (1)
661-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a short comment explaining the schedule's stop condition.
The combinator chain (
setInputType+passthrough+while+addDelay) is non-obvious: it stops draining once a batch deletes 0 rows and otherwise waitscleanupBatchDelaybetween batches. A one-line comment would save future readers from re-deriving this from the Schedule API.📝 Suggested comment
+ // Repeatedly delete a batch of expired rows, pausing cleanupBatchDelay between + // batches, until a batch deletes nothing (deletedCount === 0). const deleteExpiredSchedule = Schedule.forever.pipe( Schedule.setInputType<number>(), Schedule.passthrough, Schedule.while(({ input: deletedCount }) => deletedCount !== 0), Schedule.addDelay(() => Effect.succeed(cleanupBatchDelay)) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/persistence/Persistence.ts` around lines 661 - 666, Add a concise one-line comment immediately above deleteExpiredSchedule explaining that it repeatedly deletes batches, stops when a batch deletes zero rows, and waits cleanupBatchDelay between batches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/effect/src/unstable/persistence/Persistence.ts`:
- Around line 661-666: Add a concise one-line comment immediately above
deleteExpiredSchedule explaining that it repeatedly deletes batches, stops when
a batch deletes zero rows, and waits cleanupBatchDelay between batches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3d863a2-91da-45bf-8dcf-0fab51c36285
📒 Files selected for processing (5)
.changeset/batch-persistence-expiration-cleanup.mdpackages/effect/src/unstable/persistence/Persistence.tspackages/sql/mysql2/test/Persistence.test.tspackages/sql/pg/test/Persistence.test.tspackages/sql/sqlite-node/test/Persistence.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sql/mysql2/test/Persistence.test.ts
- .changeset/batch-persistence-expiration-cleanup.md
- packages/sql/pg/test/Persistence.test.ts
Contributor
Bundle Size Analysis
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Motivation
Previously, expiration cleanup was coupled to
BackingPersistence.make. There was no independent cleanup process, so expired entries for a store were only considered when that store was opened again. Ifmakewas never called again, those rows could remain ineffect_persistenceindefinitely.When cleanup did run, it issued one unbounded
DELETEfor every expired entry in that store. Theexpirescolumn also had no supporting index, so the database could scan a large part of the table and delete an arbitrary number of rows in a single statement. That combination could cause significant database load, longer-held locks, and unpredictable startup latency.Validation
pnpm lint-fixpnpm checkpnpm test packages/sql/pg/test/Persistence.test.ts(14 tests)pnpm test packages/sql/mysql2/test/Persistence.test.ts(12 tests)pnpm test packages/sql/sqlite-node/test/Persistence.test.ts(11 tests)Summary by CodeRabbit
New Features
Tests
This PR was raised with the help of Codex ( GPT-5.6-Sol-high )