Skip to content

fix(watcher): gate delete side effects with a SETNX tombstone that read-through cannot resurrect - #970

Open
SHAcollision wants to merge 2 commits into
mainfrom
fix/delete-tombstone-gate
Open

fix(watcher): gate delete side effects with a SETNX tombstone that read-through cannot resurrect#970
SHAcollision wants to merge 2 commits into
mainfrom
fix/delete-tombstone-gate

Conversation

@SHAcollision

@SHAcollision SHAcollision commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

post::sync_del and follow::sync_del gate their non-idempotent side effects (count decrements, engagement updates, notifications) on index entries. Those entries are ordinary read-through keys: since the graph delete runs last, any concurrent read (GET /v0/post, post_relationships_is_reply, UserFollows::get_by_id) re-creates the gate from the still-present graph node during a retry window, and the retry re-runs every decrement and re-fires the notifications.

Fix: a SETNX tombstone (Deleting:Post:{author}:{post} / Deleting:Follow:{follower}:{followee}, 6h TTL) acquired immediately before the first mutation; side effects run only on gate && first_attempt. No read path can re-create it, it is only acquired when there is something to protect, and a failure before any side effect ran releases it. Released best-effort after the graph delete; a new PUT for the same key also drops any stale tombstone, and the TTL backstops the rest.

tag::del has the same shape and gets the same treatment once #962 lands (it edits the same lines).

Resurrection tests for both handlers fail on main (double decrement) and pass here.

Fixes #960

Pre-submission Checklist

  • I manually reviewed the PR
  • I asked one or more LLMs to review the PR
  • I asked one or more LLMs to check if this PR can be simplified
  • If appropriate, I added tests for the changes in this PR
  • If appropriate, I added performance benchmarks for the APIs added in this PR (n/a)

…ad-through cannot resurrect

Delete handlers used the presence of an index entry (PostRelationships,
Followers set) as their retry-idempotency gate, but read-through caches
repopulate those keys from the still-present graph node during the retry
window (the graph delete runs last by design). A retried delete could
therefore re-run non-idempotent decrements and re-fire notifications.

Add kv::guards::{try_acquire, release} (SET key 1 NX EX ttl / DEL) and
acquire a Deleting:Post:{author}:{post} or Deleting:Follow:{follower}:{followee}
tombstone in sync_del just before the first non-idempotent mutation,
after all gate reads, so a transient read failure stays retryable with
side effects intact. Non-idempotent side effects now require both the
existing index gate and first_attempt from the tombstone, which no read
path can recreate. The tombstone is released (best-effort, logged on
failure) after the final graph delete and expires after 6 hours if the
delete dead-letters.

tag::del has the same shape but is left for a follow-up after #962 lands,
since that fix edits the same lines.

Fixes #960

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec607b8f46

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread nexus-watcher/src/events/handlers/post.rs Outdated
Comment thread nexus-watcher/src/events/handlers/follow.rs
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a double-decrement bug in post::sync_del and follow::sync_del where the existing index-gate (PostRelationships / Followers::check_in_index) could be resurrected by a read-through cache-miss between retry attempts, causing non-idempotent side effects (count decrements, engagement updates, notifications) to re-fire on every retry.

  • New guards moduletry_acquire (SETNX + EX) and release (DEL) wrap a lightweight Redis tombstone that, unlike regular index entries, can never be recreated by cache population.
  • sync_del in both handlers — tombstone is acquired only when the index gate is present; all non-idempotent side effects are gated on run_side_effects = still_indexed && first_attempt; tombstone is released before first mutation on SREM/PostRelationships::delete failure (so retry can re-acquire), preserved through the retry window otherwise, and dropped unconditionally after a successful graph delete.
  • sync_put cleanup — a new PUT drops any stale tombstone so a re-follow or re-publish doesn't have its subsequent delete silently swallowed.
  • Resurrection tests — new integration tests confirm that counts are decremented exactly once across both attempts when the index is resurrected between them.

Confidence Score: 5/5

Safe to merge. The tombstone protocol is correctly structured: acquired only when the index gate is present, released only by the attempt that owns it (or after a successful graph delete), and backstopped by a 6 h TTL. All previous review comments have been applied as suggested.

The core invariants hold across every failure scenario traced: a guard released on pre-side-effect failure lets the retry re-acquire and re-run; a guard kept after partial side effects causes the retry to skip them; the unconditional final release is gated behind the ? on the graph delete so it only fires on success. Tests are structurally sound — the second-resource trick (followee Y / second post) ensures the floor at 0 does not mask a double-decrement.

No files require special attention. The two handler files carry the most logic but both implement the same well-reasoned pattern consistently.

Important Files Changed

Filename Overview
nexus-common/src/db/kv/index/guards.rs New module: thin wrappers around SET NX EX (try_acquire) and DEL (release). Both are correct — SETNX semantics are atomic, and DEL on a missing key is a no-op so release is idempotent.
nexus-watcher/src/events/handlers/follow.rs sync_del: tombstone acquired only when still_indexed=true; SREM failure releases the guard only when first_attempt; unconditional release after graph delete is gated by ? propagation so it only fires on success. sync_put: stale tombstone cleared best-effort.
nexus-watcher/src/events/handlers/post.rs sync_del: mirrors the follow handler; tombstone gated on post_in_index; PostRelationships::delete failure releases guard only on first_attempt; run_side_effects replaces all post_in_index guards on non-idempotent operations; unconditional final release is safe because it only executes after a successful graph delete.
nexus-watcher/src/events/handlers/utils.rs Adds DELETION_GUARD_TTL_SECS (6h, ~2.4x the documented worst-case retry window of ~2.5h) and the two guard-key helper functions. Well-documented with rationale for the TTL choice.
nexus-watcher/tests/event_processor/follows/del_idempotent.rs New resurrection test: uses a second followee (Y) to keep the following count above 0 floor so a double-decrement is observable; manually acquires the tombstone, simulates partial cleanup, triggers read-through, then calls sync_del and asserts exactly one decrement.
nexus-watcher/tests/event_processor/posts/idempotent/del.rs New resurrection test: second post keeps posts count above floor; manually acquires tombstone, triggers read-through via PostRelationships::get_by_id, then runs the full del handler and asserts exactly one decrement and tombstone released.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant W as Watcher (sync_del)
    participant R as Redis
    participant G as Graph DB
    participant RT as Read-through (GET /v0/*)

    Note over W,G: Attempt 1
    W->>R: "get_from_index (post_in_index = true)"
    W->>R: "SETNX Deleting:Post:X:Y EX 21600 OK (first_attempt=true)"
    W->>R: PostRelationships::delete (gate removed)
    W->>R: UserCounts::decrement posts/replies
    W->>G: delete_post FAIL
    W-->>W: return Err (guard NOT released, side effects ran)

    Note over RT,R: Between retries
    RT->>G: get post (graph still has it)
    RT->>R: re-populate PostRelationships index (gate resurrected!)

    Note over W,G: Attempt 2
    W->>R: "get_from_index (post_in_index = true, resurrected!)"
    W->>R: "SETNX Deleting:Post:X:Y EX 21600 nil (first_attempt=false)"
    Note right of W: run_side_effects = false, skip decrements
    W->>R: PostRelationships::delete (idempotent, still runs)
    W->>G: delete_post OK
    W->>R: DEL Deleting:Post:X:Y (tombstone released)
Loading
%%{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"}}}%%
sequenceDiagram
    participant W as Watcher (sync_del)
    participant R as Redis
    participant G as Graph DB
    participant RT as Read-through (GET /v0/*)

    Note over W,G: Attempt 1
    W->>R: "get_from_index (post_in_index = true)"
    W->>R: "SETNX Deleting:Post:X:Y EX 21600 OK (first_attempt=true)"
    W->>R: PostRelationships::delete (gate removed)
    W->>R: UserCounts::decrement posts/replies
    W->>G: delete_post FAIL
    W-->>W: return Err (guard NOT released, side effects ran)

    Note over RT,R: Between retries
    RT->>G: get post (graph still has it)
    RT->>R: re-populate PostRelationships index (gate resurrected!)

    Note over W,G: Attempt 2
    W->>R: "get_from_index (post_in_index = true, resurrected!)"
    W->>R: "SETNX Deleting:Post:X:Y EX 21600 nil (first_attempt=false)"
    Note right of W: run_side_effects = false, skip decrements
    W->>R: PostRelationships::delete (idempotent, still runs)
    W->>G: delete_post OK
    W->>R: DEL Deleting:Post:X:Y (tombstone released)
Loading

Reviews (2): Last reviewed commit: "fix(watcher): scope deletion guard acqui..." | Re-trigger Greptile

Comment thread nexus-watcher/src/events/handlers/follow.rs Outdated
Comment thread nexus-watcher/src/events/handlers/post.rs Outdated
Review follow-ups for the SETNX deletion tombstone:

- Acquire the guard only when the index gate is present, so a no-op
  delete never leaves a tombstone that would suppress the decrements of
  a later re-delete.
- If an idempotent cleanup step fails before any side effect ran (gate
  deletion in post::sync_del, SREM join in follow::sync_del), release a
  guard acquired by this attempt before propagating, so the retry does
  not skip side effects that never ran. Guards held by previous attempts
  are kept, since their side effects may have run.
- Release the guard at the end of post::sync_put and follow::sync_put:
  a new PUT proves any earlier delete cycle is finished, so a leftover
  tombstone (failed release or dead-letter) is stale and must not
  suppress a later legitimate delete of the re-created entity.
@SHAcollision

Copy link
Copy Markdown
Collaborator Author

@greptile review again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read-through cache can resurrect the delete-idempotency gate, causing double-decremented counts and duplicate notifications on a retried delete

1 participant