fix(stores): uniform deleted-entity listing + non-poisoning Redis appends (ARN-192)#350
Open
rita-aga wants to merge 1 commit into
Open
fix(stores): uniform deleted-entity listing + non-poisoning Redis appends (ARN-192)#350rita-aga wants to merge 1 commit into
rita-aga wants to merge 1 commit into
Conversation
…Redis append (ARN-192) Two EventStore-backend divergences implemented the same operation with different semantics. (a) Deleted-entity visibility. Whole-tenant `list_entity_ids` filtered tombstoned entities only on Turso, so after a restart a deleted entity reappeared in `GET /EntitySet` on Postgres/Redis/Sim. Deletion is now decided once, in the shared cold-path index population (`populate_index_from_store` / `populate_index_from_store_by_type`), via the canonical `is_deleted_envelope` predicate — the same check the hot-path `ensure_entity_loaded` already applies — so all four backends exclude deleted entities identically, including the `payload.action == "Deleted"` case a plain SQL filter misses. The Turso-specific whole-tenant `event_type='Deleted'` subquery is dropped; the store returns raw distinct pairs and the server layer is the single deletion authority. (b) Redis reported failure for a committed write. `append` committed the events and advanced the sequence atomically inside the Lua script, then ran non-atomic post-commit segment bookkeeping (each step `?`-propagating). A transient error there returned `Err` for an already-durable write, so the actor kept a stale sequence and every later append hit a permanent `ConcurrencyViolation` (wedged actor), while a retry re-applied the command and duplicated events. The segment bookkeeping — never read back for the Redis backend — is removed; the Lua commit is the sole authority on whether an append succeeded. Starts with ADR-0155. Tests (red-green): - deleted_entity_index_parity: a deleted entity is absent from a listing after a restart on Sim (deterministic) and Redis (gated on REDIS_URL). - Redis committed_append_survives_broken_segment_metadata: a forced post-commit failure no longer wedges the actor or duplicates events. - Turso store-contract and replay-parity tests updated to the now-uniform behavior. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Collaborator
Author
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
rita-aga has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
rita-aga has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Collaborator
Author
ARN-165 principle audit — request changesThe Redis Lua-append change correctly avoids returning failure after a successful commit. Deleted-entity parity and the broader store contract remain incomplete. Blocking findings
Required directionAdd a bounded canonical The Integrity failure is also real: this branch raises the production |
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
1 participant
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.
Closes ARN-192 (High, Bug) under epic ARN-165. Draft for review — do not merge.
Two store-backend divergences
(a) Deleted-entity visibility. Turso
list_entity_idsfiltered tombstoned entities at the DB layer, but Postgres/Redis/Sim did not — so after a restart, deleted entities reappeared inGET /EntitySeton those backends. Fix: deletion is now decided once, in the shared cold pathpopulate_index_from_store, vialatest_event_is_tombstoneapplying the canonicalis_deleted_envelopepredicate (identical across all four backends, payload-action-aware); the Turso-specific SQL subquery is dropped.(b) Redis reported committed writes as failures. Redis
appendcommitted the event + advanced seq atomically via Lua, then did non-atomic post-commit segment bookkeeping that?-propagated transient errors — turning a durable append into an Err → wedged actor (permanent ConcurrencyViolation). Fix: removed the post-commit bookkeeping; the Lua commit is the sole authority.Red→green
deleted_entity_absent_from_list_after_restart_sim: RED (deleted entity reappeared) → GREEN. Redis variant GREEN against live Redis. Turso restart-delete test stays GREEN with the subquery dropped.committed_append_survives_broken_segment_metadata: RED (append returned Err for a committed event) → GREEN.Beneficial side effect
Dropping the Turso subquery makes
migrate_turso_to_postgresmigrate deleted entities' full history (previously silently skipped) — documented in the ADR.🤖 Generated with Claude Code