abuse-enforcement-service: fail closed when the allowlist store errors - #103
Open
Pitchfork-and-Torch wants to merge 3 commits into
Open
abuse-enforcement-service: fail closed when the allowlist store errors#103Pitchfork-and-Torch wants to merge 3 commits into
Pitchfork-and-Torch wants to merge 3 commits into
Conversation
* Add meritocratic author-size IPS to RankingScorer. * Make For You rank by merit, not reach. Add size-aware OON relief for small creators, origin-author diversity so viral originals cannot flood via many retweeters, and mute/block symmetry for quotes and reposts. Complements author-size IPS; docs in FEED_FAIRNESS. Co-authored-by: Jon Bailey <Pitchfork-and-Torch@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Jon Bailey <Pitchfork-and-Torch@users.noreply.github.com>
…Hydrator (#6) VFCandidateHydrator asks visibility filtering twice per request: once at TimelineHome for in-network candidates (plus repost sources) and once at TimelineHomeRecommendations for out-of-network candidates (plus ancestors and quoted posts). It then merged both answers into one HashMap keyed by tweet id, with the recommendations map applied last. A tweet id can be in both sets. The common case is a followed author's own thread: the root post is an in-network candidate, and the reply in the same thread lists the root as an ancestor. The same happens whenever another selected candidate quotes or replies to an in-network post. In every such case the recommendations verdict overwrote the in-network verdict, so the in-network post was judged under the rules that are meant to apply only to recommendations from accounts the viewer does not follow (SpamHighRecall, NsfwHighRecall, DoNotAmplify, NsfwText, FosnrAbuseInsults, the NSFW author/tweet flags, DMCA and geo-restricted media, and the OON-only user labels in visibility-filtering/rules/registry.rs). VFFilter then removed the post from the viewer's For You feed even though the viewer follows the author and README.md states that "the same post is allowed to a follower". The same collision runs the other way for an out-of-network candidate that is also the source of a followed account's repost: the merge order decides which verdict wins, and neither order is right for both cases. Keep the two result maps separate and route every lookup to the map matching how the id was requested: a candidate's own verdict comes from the map for its in_network flag; ancestors and quoted posts read the recommendations map; repost sources read the in-network map. No VF rule changes and no extra RPCs. Tests cover both collision directions, the ancillary routing, tombstoned ancestors, interstitials, error propagation, and an end-to-end hydrate() run with a client that answers Allow at TimelineHome and Drop at TimelineHomeRecommendations. The end-to-end test fails on the previous code with the root post carrying the recommendations-only drop reason. Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Jon Bailey <Pitchfork-and-Torch@users.noreply.github.com>
#7) The allowlist is an exemption check that runs before any rule can fire. Until now a Manhattan GET failure during that check was swallowed inside ManhattanAllowlist::get_entity and returned as None, which the fetch helpers read as is_allowlisted: false. Enforcement then continued into the rule pipeline for an account or post that may have been exempt, while the neighbouring Gizmoduck and credibility fetches on the same path abort with `?` and are retried. Make the allowlist lookup behave like those fetches: - ManhattanAllowlist::get / get_entity return anyhow::Result<Option<_>>. Ok(None) means the store confirmed the key is absent. A GET error or an undecodable stored entry is returned as Err instead of None. - fetch_user_allowlist / fetch_entity_allowlist return Result and propagate the error. Only a confirmed absence maps to "not allowlisted". - run_enforcement_inner uses `?` on the allowlist lookups, so a store error aborts the attempt and the score lands in the existing retry queue (backoff, then dropped without enforcing) rather than proceeding to rules. - Admin handlers: GET /allowlist/{id} and GET /allowlist/{type}/{id} return 500 on a read error instead of 404; bulk upsert reports a failed pre-read as a per-row error; the DELETE audit snapshot stays best-effort. Adds unit tests for the lookup-to-facts conversion, including one that asserts a store error is not turned into is_allowlisted=false. Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Jon Bailey <Pitchfork-and-Torch@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The allowlist is the exemption check before enforcement rules fire. A Manhattan GET failure during that check was returned as None and treated as not allowlisted, so rules could run (including toward suspend) against accounts that may have been exempt. Neighbouring Gizmoduck and credibility fetches on the same path abort and retry; only the exemption lookup failed toward enforcement.
Change
Make the allowlist lookup fail closed toward retry, like the other critical fetches.
No change when the store is healthy. No rule or action changes.
Tests
Unit tests for the lookup-to-facts conversion, including asserting a store error is not turned into is_allowlisted=false.
Related fork PR: Pitchfork-and-Torch#7