🤖 feat(relay): carry the deleted original's slot in deletion notices - #7884
wpfleger96 wants to merge 1 commit into
Conversation
Relay-signed message_deleted notices now record where the original lived (created_at, parent, root, depth, broadcast) so clients can draw the tombstone in place. One builder serves both the NIP-29 DELETE_EVENT path and the admin outbox tombstone. The NIP-29 soft-delete and its notice now commit in one transaction, so a failed notice insert can no longer lose the marker. The admin path freezes the slot on the action row with the mutation marker (migration 0050), and delivery rebuilds from that payload, so retries sign identical bytes even after purge. Rows from before the column existed freeze their slot once at finalization. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
GitHub Actions
Error
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one deletion-recovery defect and two stale migration-test expectations (grouped in the second inline finding).
Merge criteria: make failed NIP-29 deletion recover through the real ingest/same-event retry boundary, cover that failure path, and update the migration fixtures so the PostgreSQL gate passes. Preserve atomic target/counter/notice persistence.
Reviewed d19dbe7652643664c7e7d95e8d233259ae35b103 against 20131488528e35e6c50f4ccdb0490a9135c28edf. Source-only review on Blox, with independent wire/security and NIP-29 lanes; no code executed or tests rerun. Existing CI ran merge f3ed1b9664c58fcb8bbc8372d01e61c7b2d8a521; the new handler/DB tests passed but bypass the failed-command replay boundary. Admin freeze/outbox recovery and legacy wire compatibility produced no blockers. Desktop rendering is explicitly deferred to the stacked PRs.
| channel_id, | ||
| ) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("soft_delete_event failed: {e}"))?; |
There was a problem hiding this comment.
[P1] Keep a failed delete retryable through ingest
If the notice INSERT fails after the target UPDATE, soft_delete_event_with_notice rolls back the deletion and counters, but the kind-9005 command has already committed in ingest.rs:3315-3347. The caller only logs this side-effect error (3361-3368), then dispatches the command and returns accepted: true (3415-3431). Replaying the same signed command returns duplicate: before side effects (3349-3355), leaving the original live indefinitely for that request. BASE committed the target deletion before emitting the notice; this PR newly makes notice-storage failure restore the target without giving that rollback a retry owner.
Keep command acceptance and the target/counter/notice transaction recoverable together: preferably include command persistence in the atomic boundary, or provide durable recovery. If using caller retry, both propagate failure and allow an authorized duplicate to finish an incomplete deletion; changing only the ACK is insufficient. Add an ingest-level regression that fails notice persistence, retries the identical signed 9005 event, and verifies the target, counters and one notice converge. The new DB rollback test directly invokes the DB function again, so it cannot catch this duplicate-command boundary.
| .sql | ||
| .as_str() | ||
| .contains("idx_thread_metadata_window")); | ||
| assert_eq!(migrations[49].version, 50); |
There was a problem hiding this comment.
[P2] Advance the existing migration fixtures to include 0050
The new migration is registered and the desired-state schema contains original_slot, but two existing tests still describe the previous schema. admin_schema_parity_between_desired_state_and_migrations runs only through migration 47 (runtime/migration.rs:2268-2271), so its migrated database lacks the column now present in schema.sql. Separately, thread_window_postgres_tests.rs:314 still asserts migration version 49 after running the production migrator, which now returns 50.
Both fail in this head’s PostgreSQL CI job. Update the parity fixture to apply the current migrations and the latest-version assertion. This is stale test setup, not a missing production migration; do not remove the column from the desired-state schema to satisfy the parity failure.
|
Pausing tombstone B for now, so closing this. Leaving the branch in place in case we pick it back up later. |
This branch was successfully deployed
2 participants
Stack: this PR → PR 2 (desktop channel projection, to follow) → PR 3 (desktop threads, to follow)
Relay-signed
message_deletednotices (kind40099) now carry the deleted original's structural slot, so clients can render the tombstone where the original lived, including thread replies after a reload. This PR is relay-only; desktop consumption comes in the stacked PRs.Wire contract
content.original(optional, all-or-nothing,version: 1):created_at,parent_event_id,root_event_id,depth,broadcast. No content, author, or tags of the original.h, plus unmarkedetags for target, root, and parent (deduplicated). Noreply/rootmarkers, noptags.created_atstays the deletion time and is never backdated, so livesince-filtered subscribers still receive it.Changes
handlers/deletion_tombstone.rs: onebuild_delete_tombstoneused by both emitters, replacing the NIP-29 inline JSON and the test-onlydelete_tombstone_contentduplicate.DELETE_EVENT: soft-delete, thread counters, and notice insert commit in one transaction (soft_delete_event_with_notice); fan-out happens after commit. A failed notice insert rolls the deletion back instead of losing the marker.0050adds nullablerelay_admin_actions.original_slotJSONB.execute_delete_with_markerfreezes the slot, or "unavailable" if the row is gone, in the lease-fenced transaction.finalize_successcopies it into the tombstone outbox payload, anddeliver_tombstonerebuilds from the payload. Retries sign identical bytes even after the target is purged. Actions committed before0050freeze their slot once at finalization. Outbox rows withoutoriginaldeliver the legacy notice.