refactor: checkpoint API cleanup#5318
Conversation
…ders - Rename CheckpointConfig.directory to location — the field means a directory for JsonProvider and a database file path for SqliteProvider, so the generic name fits both - Add max_checkpoints to BaseProvider protocol - Add max_checkpoints and pruning to JsonProvider, matching SqliteProvider - Remove prune logic from checkpoint_listener since providers own cleanup
iris-clawd
left a comment
There was a problem hiding this comment.
Review: Checkpoint API Cleanup
Clean refactor — directory → location is the right abstraction, and moving prune logic into providers is good separation of concerns.
🟡 Prune is now a separate transaction (SqliteProvider)
Previously, _PRUNE ran inside the same sqlite3.connect() context as the insert (same transaction). Now prune() opens a new connection. Two implications:
- If the process crashes between
checkpoint()andprune(), you accumulate an extra row. Minor — alpha-level acceptable. - More importantly: two separate connections means two separate transactions. Under concurrent writes (e.g., multiple crews checkpointing to the same DB), there's a window where a prune could race with an insert from another thread. WAL mode helps, but the old atomic approach was safer.
If this matters, prune could accept an optional connection/cursor to reuse the write transaction. Not blocking for alpha.
🟡 No aprune on BaseProvider
BaseProvider defines both checkpoint and acheckpoint, but prune is sync-only. The listener always calls it synchronously via _do_checkpoint, so it works today. But if someone calls acheckpoint → prune in an async context, they'll block the event loop on SQLite I/O. Worth adding aprune to the protocol for symmetry, even if the default impl just calls prune.
🟢 Looks good
- Breaking changes are fine for alpha (
CheckpointConfig.directory→location,SqliteProvider.__init__droppingmax_checkpoints) - JsonProvider prune logic is a clean lift from the old
_prunefunction - SqliteProvider prune reuses the existing
_PRUNESQL correctly - Docstrings updated consistently across all files
Minor stuff, nothing blocking. 💬 163
iris-clawd
left a comment
There was a problem hiding this comment.
Approving — already reviewed. Notes on separate prune transaction and missing aprune are suggestions for later. Good to land. ✅
iris-clawd
left a comment
There was a problem hiding this comment.
Re-approving after new commits. ✅
Summary
CheckpointConfig.directorytolocation— the field means a directory for JsonProvider and a database file path for SqliteProvider, so the generic name fits bothprunemethod toBaseProviderprotocol so each provider owns its own cleanup logiccheckpoint_listenerinto the providers where it belongsmax_checkpointsstays onCheckpointConfigas the single place users configure it; the listener passes it through toprovider.pruneafter each writeTest plan
CheckpointConfig(location=..., max_checkpoints=5)works for both providersNote
Medium Risk
Medium risk because it renames a public
CheckpointConfigfield and changes pruning responsibilities, which could break existing integrations or custom providers if not updated.Overview
Checkpointing API cleanup. Renames
CheckpointConfig.directorytolocationacross code and docs to reflect that the value may be either a filesystem directory (JsonProvider) or a SQLite DB file path (SqliteProvider).Provider-owned retention. Adds
prune(location, max_keep)to theBaseProviderprotocol and removes file-pruning logic fromcheckpoint_listener, delegating retention toJsonProvider.prune(delete old JSON files) andSqliteProvider.prune(delete old rows), with tests updated accordingly.Reviewed by Cursor Bugbot for commit c61a40e. Bugbot is set up for automated code reviews on this repo. Configure here.