Skip to content

feat: tag events handlers partial recovery#792

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

feat: tag events handlers partial recovery#792
aintnostressin wants to merge 1 commit intomainfrom
feat/tag-handler-fix-state

Conversation

@aintnostressin
Copy link
Copy Markdown
Contributor

Prepares tag handlers for partial recovery.

  • DEL handler: graph-last ordering — Reads tag target from graph first (new get_tag_target query), performs Redis index cleanup, then deletes the graph edge last. This ensures the TAGGED edge survives for retry if Redis ops fail mid-way.
  • DEL handler: guarded decrements — Uses check_set_member to verify the tagger is still in the Redis set before decrementing counters/sending notifications, preventing double-decrement on retry.
  • PUT handler: retry recovery — When put_to_graph returns Updated (edge already exists from a prior attempt), re-runs idempotent Redis index writes to recover from partial failures where graph succeeded but Redis didn't.
  • Idempotent no-op on replay — Tag DEL no longer returns SkipIndexing when the graph edge is already gone; it returns Ok(()) silently.

@aintnostressin aintnostressin requested review from ok300 and tipogi April 3, 2026 16:25
@aintnostressin aintnostressin changed the title feat: tag handlers partial recovery feat: tag events handlers partial recovery Apr 3, 2026
Comment on lines +274 to +277
let tagger_in_index =
TagUser::check_set_member(&[tagged_id.as_str(), label.as_str()], &user_id)
.await?
.1;
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 tagger_in_index =
TagUser::check_set_member(&[tagged_id.as_str(), label.as_str()], &user_id)
.await?
.1;
let (_, tagger_in_index) =
TagUser::check_set_member(&[&tagged_id, &label], &user_id).await?;

Comment on lines +282 to +287
let tagger_in_index = TagPost::check_set_member(
&[author_id.as_str(), post_id.as_str(), label.as_str()],
&user_id,
)
.await?
.1;
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 tagger_in_index = TagPost::check_set_member(
&[author_id.as_str(), post_id.as_str(), label.as_str()],
&user_id,
)
.await?
.1;
let (_, tagger_in_index) =
TagPost::check_set_member(&[&author_id, &post_id, &label], &user_id).await?;


// Read the target details for a tag without deleting the TAGGED edge.
// Used in tag del to read before graph-last deletion.
pub fn get_tag_target(user_id: &str, tag_id: &str) -> Query {
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.

This is almost identical (1 line diff) to delete_tag in del.rs. This means any future logic change in the query has to reflect equally across both methods.

Maybe there's a way to avoid query duplication? Like have both methods internally use the same "query builder fn" with one boolean flag include_delete_call?

Alternatively, both methods should have a comment pointing at each other, saying "query logic must be kept in-sync with (other fn)". Although this is more error prone than the 1st suggestion.

let user_id: Option<String> = row.get("user_id").unwrap_or(None);
let author_id: Option<String> = row.get("author_id").unwrap_or(None);
let post_id: Option<String> = row.get("post_id").unwrap_or(None);
let label: String = row.get("label").expect("Query should return tag label");
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.

Maybe this shouldn't panic, but instead throw a GraphError?

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