fix(sdk): stamp writer metadata on every store mutation#477
Conversation
The last_writer/last_write_at metadata was stamped only at store construction, so after the first connection it stopped reflecting the most recent write. Stamp it inside the same transaction as every Insert, Update, and Delete in the SQLite and PostgreSQL stores so a shared multi-agent store can identify which SDK wrote last. stampWriter becomes a free function taking an execer interface, satisfied by both a connection/pool and a transaction, so it can run within a mutation's transaction. The PostgreSQL Delete is wrapped in a transaction so the stamp is atomic with the delete. Refs #470
The last_writer/last_write_at metadata was stamped only at store construction, so after the first connection it stopped reflecting the most recent write. Stamp it inside the same transaction as every insert, update, and delete in the SQLite and PostgreSQL stores so a shared multi-agent store can identify which SDK wrote last. _stamp_writer no longer commits on its own and runs within the caller's transaction; the PostgreSQL delete is wrapped in a transaction so the stamp is atomic with the delete. Fixes #470
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughWriter metadata stamping now runs during SQLite and PostgreSQL insert, update, and delete operations in the Go and Python SDKs. Schema initialisation also writes the metadata within transactional scope, and tests verify the writer keys change after mutations. ChangesWriter metadata stamping across SDK stores
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@sdk/go/store_sqlite_test.go`:
- Around line 914-937: The mutation metadata test only verifies keyLastWriteAt,
but it should also confirm keyLastWriter is restamped after each mutation. In
the store_sqlite_test.go test around the Insert, Update, and Delete paths, reuse
the existing overwrite-and-read pattern for keyLastWriter alongside
keyLastWriteAt, then assert that both values change after s.Insert, s.Update,
and s.Delete. Use the existing readTimestamp helper pattern as a guide and
locate the relevant logic by the keyLastWriteAt/keyLastWriter constants and the
s.Insert, s.Update, and s.Delete calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb4f7650-cd97-49fe-bfe0-e480d8442665
📒 Files selected for processing (6)
sdk/go/store_sqlite.gosdk/go/store_sqlite_test.gosdk/go/stores/postgres/postgres.gosdk/python/src/cq/store.pysdk/python/src/cq/stores/postgres.pysdk/python/tests/test_store.py
There was a problem hiding this comment.
Pull request overview
This PR fixes stale store metadata (last_writer, last_write_at) by stamping writer/timestamp on every persistent-store mutation (insert/update/delete) and ensuring the stamp is performed in the same transaction as the mutation for atomicity (notably for PostgreSQL deletes).
Changes:
- Python: Move writer stamping into the mutation transactions for both SQLite (
SqliteStore) and PostgreSQL (PostgresStore), and stop_stamp_writerfrom committing independently. - Go: Make PostgreSQL writer stamping transaction-friendly via a small
execerinterface and stamp on every mutation; ensure deletes are wrapped in a transaction for atomic stamping. - Tests: Add mutation-level writer-stamp refresh coverage for Python SQLite and Go SQLite.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/python/tests/test_store.py | Adds tests asserting last_write_at is refreshed on insert/update/delete for the local (SQLite) store. |
| sdk/python/src/cq/stores/postgres.py | Stamps writer metadata on insert/update/delete and wraps delete in a transaction for atomicity. |
| sdk/python/src/cq/store.py | Makes _stamp_writer transaction-scoped (no internal commit) and calls it on each SQLite mutation. |
| sdk/go/stores/postgres/postgres.go | Refactors writer stamping to work with both pool/tx and stamps within mutation transactions (including delete). |
| sdk/go/store_sqlite.go | Stamps metadata within SQLite mutation transactions by making stampWriter accept either DB or tx executors. |
| sdk/go/store_sqlite_test.go | Adds a regression test ensuring last_write_at changes on insert/update/delete. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The SQLite stampWriter was the only statement in the mutation transactions still using a context-less Exec, so the metadata write ignored the caller's cancellation while every other statement in the same transaction honored it. Switch the execer interface to ExecContext and thread the caller's context through Insert, Update, and Delete. Inline the single-caller ensureMetadata into newSQLiteStore so the construction-time stamp, which has no caller context, lives at the constructor with an explicit background context rather than fabricating one inside a helper. Also assert in the Go and Python tests that both last_writer and last_write_at are restamped on every mutation, so a regression that drops either write is caught. Refs #470
What
The store metadata (
last_writer,last_write_at) was stamped only at store construction, duringensureSchema/ensureMetadata. After the first connection, every insert/update/delete left the metadata stale — lying about the most recent write. This affected all persistent store implementations across both SDKs (SQLite and PostgreSQL).For a shared multi-agent PostgreSQL store, operators want this metadata to reflect which agent actually wrote last. This PR stamps the writer on every mutation, inside the same transaction as the mutation itself, so the metadata stays accurate and the stamp is atomic with the write.
How
stampWriterbecomes a free function taking a smallexecerinterface, satisfied by both a connection/pool and a transaction, so it can run within a mutation's transaction. The PostgreSQLDeleteis now wrapped in a transaction so the stamp is atomic with the delete._stamp_writerno longer commits on its own and runs within the caller's transaction. The PostgreSQLdeleteis wrapped in a transaction for the same atomicity.Testing
make lint: clean (0 issues)make test: Go 456, Python 308 (+11 skipped), frontend 11 — all passFixes #470
Summary by CodeRabbit
New Features
Bug Fixes
Tests