fix(server): release declared-key ownership on delete and null (ARN-238) - #398
fix(server): release declared-key ownership on delete and null (ARN-238)#398rita-aga wants to merge 3 commits into
Conversation
…te (ARN-238) RED: assert delete releases key ownership (reclaim by a new entity), and that boot key-index backfill heals legacy stale rows for tombstoned entities and living entities whose declared keys no longer resolve.
ADR-0172: EntityKeyRow.key_hash == \"\" is a RELEASE marker. The entity actor emits one row per declared key on every write — a real hash when living and resolvable, a release marker when the write tombstones the entity or the key no longer resolves. Postgres and sim skip empty hashes on uniqueness/insert and always drop the entity's prior row for that key_name. Key backfill heals legacy stale rows for tombstones and fully-nulled keys (pre-watermark). Verified: cargo test -p temper-server --test dst_entity_key_index → 6/6 pass.
Grok independent review — PR #398 (ARN-238 / ADR-0172)Scope: Release declared-key ownership on delete and null via empty-hash RELEASE markers; heal legacy stale rows in key-index backfill. What was done well
FindingsImportant (should fix / track before relying on production heal)
Suggestions (nice to have)
Plan alignmentMatches ADR-0172: release markers, store skip-on-claim, backfill heal, non-goals honored. Residual risks are disclosed rather than hidden. VerdictNo correctness gaps in the primary live delete path relative to the stated bug; residuals are operational/follow-up, not silent wrongness on the new write path. Verdict: PASS |
|
@greptile review |
ARENA SHIPPABLE · Grok · 2026-07-14 11:57 PDTPR: #398 Checklist
Summarydeclared-key release |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| EntityLoadOutcome::Tombstoned => { | ||
| // ARN-238 healing pass: a deleted entity must not keep its | ||
| // declared keys. Emit a release marker per declared key so | ||
| // rows written before release-on-delete are purged. | ||
| let release_rows: Vec<temper_runtime::persistence::EntityKeyRow> = keys | ||
| .iter() | ||
| .map(|key| temper_runtime::persistence::EntityKeyRow { | ||
| key_name: key.name.clone(), | ||
| key_hash: String::new(), | ||
| }) | ||
| .collect(); | ||
| match store | ||
| .backfill_entity_keys( | ||
| tenant.as_str(), | ||
| entity_type, | ||
| entity_id, | ||
| &release_rows, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(()) => healed += 1, | ||
| Err(e) => { | ||
| failed += 1; | ||
| tracing::warn!( | ||
| error = %e, entity_type = %entity_type, entity_id = %entity_id, | ||
| "key index backfill: tombstone release failed" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The Tombstoned healing arm is only reachable during the backfill, which is skipped for any entity type that already carries a watermark. For production deployments where a keyed type's watermark was written before this fix landed, every entity deleted in that window still holds its stale entity_key_index row. New entities trying to claim the same key values will continue to receive uniqueness-violation rejections forever — the exact "durable damage" ARN-238 describes. Live release prevents new staleness going forward, but the pre-existing stale rows are never cleared unless the watermark is manually dropped and the backfill re-runs. Consider whether the fix plan needs a one-shot migration or a safe watermark-reset mechanism before this merges.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/state/projection_backfill/key_index.rs
Line: 226-254
Comment:
**Watermarked types are not re-healed; stale rows persist indefinitely**
The `Tombstoned` healing arm is only reachable during the backfill, which is skipped for any entity type that already carries a watermark. For production deployments where a keyed type's watermark was written before this fix landed, every entity deleted in that window still holds its stale `entity_key_index` row. New entities trying to claim the same key values will continue to receive uniqueness-violation rejections forever — the exact "durable damage" ARN-238 describes. Live release prevents new staleness going forward, but the pre-existing stale rows are never cleared unless the watermark is manually dropped and the backfill re-runs. Consider whether the fix plan needs a one-shot migration or a safe watermark-reset mechanism before this merges.
How can I resolve this? If you propose a fix, please make it concise.
rita-aga
changed the title
|
@greptile review |
|
Addressed: |
1 participant
Summary
Full fix for ARN-238: declared-key ownership no longer stays stale after an entity is deleted or a declared key is fully nulled.
ADR-0172 (unique vs competitor ADR-0159):
EntityKeyRow.key_hash == ""is a RELEASE marker co-committed with the journal.Root cause
ADR-0153 co-commit derived key rows only from post-transition fields. Tombstone writes still held key values in fields, so the dead entity re-claimed the hash. Fully nulled keys emitted no row, so the store never deleted the prior hash.
Fix
status/to_status == Deleted) or key unresolvable.EntityLoadOutcome::Tombstonedadded.Commits (RED → GREEN)
3aba7ae6— RED: failing DST for delete-release + backfill heal (delete + null legs)ab875229— GREEN: release markers + stores + backfill + ADR-0172Tests
Residuals
append_batchstill carries no key rows (pre-existing ADR-0153 gap).Linear
ARN-238
Greptile Summary
This PR fixes ARN-238 by introducing an empty-string RELEASE marker (
EntityKeyRow.key_hash == \"\") that causes stores to drop an entity's declared-key ownership atomically with the journal append, preventing tombstoned or null-keyed entities from holding stale rows forever and blocking new claimants.EntityKeyRowper declared key on every write — a real hash when the entity is living and the key resolves, an empty RELEASE marker when the write tombstones the entity (state.status == \"Deleted\"orevent.to_status == \"Deleted\") or when key components are all null/absent.EntityLoadOutcome::Tombstonedvariant routes deleted entities to a release-only pass; living entities with unresolvable keys emit mixed real-hash + RELEASE rows; three new DST tests cover all three legs.Confidence Score: 5/5
The write path, both stores, and the backfill all handle release markers consistently; correctness is well-covered by the three new DST tests.
The RELEASE marker protocol is applied uniformly across the live write path (actor), both store implementations, and the backfill; no code path can silently re-claim a key after tombstoning or nulling. The one comment left is a logging/performance note on the backfill Tombstoned arm and does not affect the correctness of key ownership.
crates/temper-server/src/state/projection_backfill/key_index.rs — the Tombstoned arm early-out and healed counter, though no correctness issue.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Entity write (actor.rs)"] --> B{"tombstoned?\nstate.status==Deleted\nOR event.to_status==Deleted"} B -- yes --> C["key_hash = RELEASE marker (empty)"] B -- no --> D{"key components resolvable?"} D -- yes --> E["key_hash = canonical hash"] D -- no --> C C --> F["push EntityKeyRow with key_hash=empty"] E --> F F --> G["append_with_index_rows"] G --> H{"per key_row in store"} H --> I["DELETE entity prior row for key_name"] I --> J{"key_hash empty?"} J -- yes RELEASE --> K["done - row deleted, nothing inserted"] J -- no --> L["INSERT key_name, key_hash, entity_id"] M["Boot backfill (key_index.rs)"] --> N["load entity state"] N --> O{"EntityLoadOutcome?"} O -- Tombstoned --> P["emit RELEASE markers for all declared keys"] O -- "Fields: already_keyed AND all keys resolve" --> Q["skip - already clean"] O -- "Fields: any key unresolvable" --> R["emit RELEASE for null keys, real hash for resolvable"] O -- Skip --> S["count skipped"] P --> T["backfill_entity_keys - DELETE stale rows"] R --> T T --> U["healed or newly_keyed counter"] U --> V{"failed == 0?"} V -- yes --> W["set watermark"] V -- no --> X["log warning - resume next boot"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["Entity write (actor.rs)"] --> B{"tombstoned?\nstate.status==Deleted\nOR event.to_status==Deleted"} B -- yes --> C["key_hash = RELEASE marker (empty)"] B -- no --> D{"key components resolvable?"} D -- yes --> E["key_hash = canonical hash"] D -- no --> C C --> F["push EntityKeyRow with key_hash=empty"] E --> F F --> G["append_with_index_rows"] G --> H{"per key_row in store"} H --> I["DELETE entity prior row for key_name"] I --> J{"key_hash empty?"} J -- yes RELEASE --> K["done - row deleted, nothing inserted"] J -- no --> L["INSERT key_name, key_hash, entity_id"] M["Boot backfill (key_index.rs)"] --> N["load entity state"] N --> O{"EntityLoadOutcome?"} O -- Tombstoned --> P["emit RELEASE markers for all declared keys"] O -- "Fields: already_keyed AND all keys resolve" --> Q["skip - already clean"] O -- "Fields: any key unresolvable" --> R["emit RELEASE for null keys, real hash for resolvable"] O -- Skip --> S["count skipped"] P --> T["backfill_entity_keys - DELETE stale rows"] R --> T T --> U["healed or newly_keyed counter"] U --> V{"failed == 0?"} V -- yes --> W["set watermark"] V -- no --> X["log warning - resume next boot"]Reviews (2): Last reviewed commit: "fix(server): count newly_keyed only for ..." | Re-trigger Greptile