Skip to content

feat: bookmarks events handler partial recovery#793

Open
aintnostressin wants to merge 1 commit intomainfrom
feat/bookmark-handler-fix-state
Open

feat: bookmarks events handler partial recovery#793
aintnostressin wants to merge 1 commit intomainfrom
feat/bookmark-handler-fix-state

Conversation

@aintnostressin
Copy link
Copy Markdown
Contributor

Prepares bookmark handlers for partial recovery.

  • Reorders sync_del to read the bookmark target from the graph before deleting the edge, ensuring data survives for retries if Redis ops fail (graph-last pattern)
  • Guards counter decrement behind a Redis index existence check, preventing double-decrement on retry
  • Adds get_bookmark_target graph query and Bookmark::get_target_from_graph to read without side effects

@aintnostressin aintnostressin requested review from ok300 and tipogi April 3, 2026 16:30
@aintnostressin aintnostressin changed the title feat: fix bookmarks partial recovery feat: bookmarks partial recovery Apr 3, 2026
@aintnostressin aintnostressin changed the title feat: bookmarks partial recovery feat: bookmarks events handler partial recovery Apr 3, 2026
Comment on lines +63 to 67
let (post_id, author_id) = match Bookmark::get_target_from_graph(&user_id, &bookmark_id).await?
{
Some(info) => info,
None => return Err(EventProcessorError::SkipIndexing),
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (post_id, author_id) = match Bookmark::get_target_from_graph(&user_id, &bookmark_id).await?
{
Some(info) => info,
None => return Err(EventProcessorError::SkipIndexing),
};
let (post_id, author_id) = Bookmark::get_target_from_graph(&user_id, &bookmark_id)
.await?
.ok_or(EventProcessorError::SkipIndexing)?;

None => return Err(EventProcessorError::SkipIndexing),
};

// 2. Guard counter decrement: only decrement if bookmark still exists in Redis index
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The guard is designed to prevent double-decrement on retry, it it only handles the case where both the Redis index removal and the counter decrement succeed before the graph delete failed.

If del_from_index (step 3) succeeds, but the process crashes before UserCounts::decrement runs, then on retry existed_in_index will be false and the counter will never be decremented.

test.del(&user_kp, &bookmark_path).await?;

// Verify fully deleted state
assert!(find_post_bookmark(&user_id, &post_id, &user_id)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test conflates the post author and the bookmarker, which means it doesn't cover the case when post author != bookmarker, which is a more realistic scenario. It would be better if the test creates 2 separate users, author_kp and bookmarker_kp.

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.

2 participants