Skip to content

Keep duplicate-reply clusters processing after per-tweet failures - #95

Open
jnadeau207-collab wants to merge 1 commit into
xai-org:mainfrom
jnadeau207-collab:fix/duplicate-reply-cluster-failures
Open

Keep duplicate-reply clusters processing after per-tweet failures#95
jnadeau207-collab wants to merge 1 commit into
xai-org:mainfrom
jnadeau207-collab:fix/duplicate-reply-cluster-failures

Conversation

@jnadeau207-collab

Copy link
Copy Markdown

Addresses #65

Bug

The public tree already has a dedicated BotMaker rule for duplicate reply clusters from both the unigram and CJK-character jobs: BBQDuplicateTextRepliesProd.

That rule evaluates each tweet through Filter and applies COPYPASTA_SPAM through a for/ForEach. Both BotMaker implementations aggregate each batch with Future.collect. A single failed per-tweet future therefore fails the whole filter or action batch. For clusters larger than the 50-item concurrency batch, later batches are reached only through the previous batch's successful flatMap, so one failed member prevents all later members from being processed.

A stale or temporarily unavailable tweet, safety-label lookup failure, exemption lookup failure, or label/marker write failure can therefore suppress healthy tweets in an already-detected duplicate-reply cluster.

Fix

  • Rescue eligibility failures per tweet, exclude only the uncertain tweet, and increment a fixed counter.
  • Rescue author/exemption failures per tweet, reject the existing -1 unavailable-author sentinel, and continue with the rest of the cluster.
  • Rescue each COPYPASTA_SPAM label plus BigQuery-to-BotMaker marker transaction independently, preserving label-before-marker ordering so a failed label is never marked complete.
  • Give the two RateLimited branches explicit Boolean results after the per-tweet loop.

The cluster job names, event condition, COPYPASTA_SPAM label, exemption policy, rate limit, marker namespace, and already-labeled checks are unchanged.

Scope

This hardens actioning for clusters that the existing unigram/CJK scheduled jobs have already produced. The scheduled BigQuery clustering query is not included in the public repository, so this PR does not claim to change text normalization or cluster formation.

Verification

  • The clean branch is one commit directly on current upstream main (85ac72a1bba41f21615e3f0bca56da75970a6633) and changes one production rule file.
  • Both the upstream and patched actions parse with the repository's exact ANTLR 3 BotMaker.g grammar.
  • A policy-preservation check confirms that the event condition and every existing literal/function call remain, with only three fixed metric names and IncrementStat added.
  • Source-level checks confirm the published Filter and ForEach fail-fast Future.collect boundaries, 50-item batching, TryOrElse rescue behavior, and sequential label-before-marker block execution.
  • A 120-member regression model proves one failure stops the baseline before later batches, while the patched per-item boundary processes every healthy member and leaves failed members unmarked.
  • git diff --check passes.

Verification run: https://github.com/jnadeau207-collab/x-algorithm/actions/runs/33702548512

The public export does not provide a runnable Scarecrow/BotMaker semantic compilation target, so the internal rule compiler remains required before deployment.

@jnadeau207-collab jnadeau207-collab left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Adversarial review (PR #95)

Claims vs code

Claim Evidence
Filter / for fail the whole batch via Future.collect, and batches >50 only continue through prior-batch flatMap Confirmed. Filter.java / ForEach.java: MAX_CONCURRENCY = 50, Future.collect per batch, later batches chained with flatMap.
Per-tweet TryOrElse isolates failures Confirmed. TryOrElse rescues exceptions (Future.rescue / sync catch) and returns the backup value, so Future.collect sees a successful FALSE/TRUE, not an exception.
Reject -1 unavailable-author sentinel Confirmed and necessary. GetTweetAuthorId.df is already TryOrElse(GetTweet(...).user_id, -1) — author lookup does not throw. Without :tweetAuthor != -1, IsUserGrayVerified(-1) does bare GetUser(:userId) and can throw, failing the whole Filter collect. Conjunction/&& short-circuits, so the new check prevents that call.
“Rescue author/exemption failures” Partially overstated. Author lookup is already soft-failed to -1; what this PR rescues is exemption-helper failures (IsUserGrayVerified, GetLongList, IsTestUser, etc.) plus the explicit -1 reject.
Label-before-marker so a failed label is never marked complete Confirmed for label→marker ordering inside one TryOrElse try-arm. Not solving the inverse: label succeeds and SetBigQuerytoBotmakerLabel fails → labeled without marker (pre-existing class of partial write).
“Addresses #65 Overclaim relative to the issue. #65 asks whether CJK near-duplicate detection works. This PR only hardens actioning for clusters the BBQ jobs already emit. Body scope disclaimer is honest; the “Addresses #65” one-liner is not.
Unchanged policy literals / job names / rate limit Looks true from the diff; only control-flow wrappers + three metric names + -1 check added.

Correctness

  • Fail-closed on eligibility/exemption lookup failure (exclude / do not label) is the right default for a spam labeler.
  • Action-loop isolation is the highest-value part of the change and matches runtime semantics.
  • -1 short-circuit returns FALSE on the try path, so it does not increment exemption_lookup_failed. Unavailable authors are silent in that counter and only show up in unactionable_tweets (mixed with real exemptions).
  • Exemption lookup failures now also land in unactionable_tweets, polluting that log vs “legitimately exempt.”
  • RateLimited success path always returns TRUE even if every per-tweet action TryOrElse returned FALSE (return value appears unused by this rule; mainly type uniformity).

Security

  • No new privilege surface. More cluster members may receive COPYPASTA_SPAM when a sibling previously aborted the batch — that is the intended behavior for an already-detected duplicate cluster.
  • Fail-closed on exemption uncertainty avoids labeling accounts that might be protected.

Missing tests / verification honesty

Gaps / asymmetry

  • Sibling BBQDuplicateTextProd.bot has the same Filter/for fail-fast shape and is untouched. Fine for reply-scoped #65 narrative; incomplete for the BBQ duplicate family.
  • Pre-existing: unused :downrankNote; bizarre far-future expiry on this rule.

Verdict: COMMENT

Core change is sound and evidence-backed against Filter/ForEach/TryOrElse/GetTweetAuthorId. Not blocking on code defects.

MUST_FIX

  • None for merge-blocking code. (Docs-only: drop or rephrase “Addresses #65” to “Related to #65 — hardens actioning only”.)

NIT

  1. Soften #65 framing in the PR description.
  2. Add a dedicated counter when :tweetAuthor == -1 (short-circuit currently skips exemption_lookup_failed).
  3. Consider not stuffing lookup failures into unactionable_tweets (or tag them) so ops logs stay meaningful.
  4. Same isolation pattern for BBQDuplicateTextProd.bot in a follow-up.
  5. Tone down “regression model proves” → “models / demonstrates under Future.collect assumptions.”
  6. Optional: nest marker write so label-success/marker-failure is distinguishable in metrics (still no perfect two-phase commit).

No fix commits pushed from this review (no clear code MUST_FIX). Write access exists on the PR head fork jnadeau207-collab/x-algorithm; upstream xai-org/x-algorithm is pull-only for this token.

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.

1 participant