Fix Issue cache over- and under-invalidation - #605
Merged
Conversation
Production showed an isolated X-Cache HIT->MISS flip with no edit to the issue in question. Root cause: ModelLabel.SERIES (used as an IssueViewSet.cache_detail_dependent_labels entry to catch Series renames) is also bumped by update_series_modified_on_issue_save() on every issue write anywhere on the site, so any unrelated issue edit was invalidating every cached issue detail response. Same problem existed for Arc/Character/Team's issue_list via ModelLabel.ISSUE/SERIES. Both now drop those dependencies and accept bounded (24h) staleness on Series renames instead, consistent with the tradeoff already made for Creator/Universe. Auditing every writer of Issue.modified to confirm no other over-broad dependency existed surfaced the opposite problem: several fields IssueReadSerializer embeds don't bump Issue.modified at all when they change. update_related_modified() now also bumps the specific Issue's own `modified` (in addition to the parent Arc/Character/Team's, scoped by pk so it can't cross-contaminate other issues) when arcs/characters/teams are added or removed. New handlers cover the same gap for universes, reprints, and Variant create/update/delete. issue_ratings is deliberately left as accepted staleness -- ratings are common enough that bumping on every one would undermine the cache for popular issues.
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
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.
Description
Production testing of the
X-Cacheheader from #604 turned up an isolatedHIT→MISSflip on an issue that hadn't been edited. Root cause:ModelLabel.SERIES— used as anIssueViewSet.cache_detail_dependent_labelsentry specifically to catch Series renames — is also bumped byupdate_series_modified_on_issue_save()on every issue write anywhere on the site (PublisherViewSet.series_listneeds that for its embeddednum_issues). So any unrelated issue edit elsewhere on the site was invalidating every cached issue detail response, not just the one that changed. The same problem existed forArc/Character/Team'sissue_listviaModelLabel.ISSUE/SERIES. Both now drop those dependencies and accept bounded (24h) staleness on Series renames instead — the same tradeoff already made deliberately for Creator/Universe names.Auditing every writer of
Issue.modifiedto make sure no other over-broad dependency was hiding turned up the opposite problem: several fieldsIssueReadSerializerembeds don't bumpIssue.modifiedat all when they change, so edits to them could go unreflected in a cached issue detail for up to 24h:modified(needed for its ownissue_listcache) — the issue's ownmodifiedwas never touched.universesandreprints(a symmetric self-referential M2M) had no invalidation signal wired up at all.Variantdidn't touch its parent Issue'smodifiedeither.All of these are now fixed with handlers scoped precisely to the pk(s) actually affected — never a blanket update, so this can't reintroduce the cross-contamination bug above.
issue_ratings(average_rating/rating_count) is deliberately left as accepted staleness — ratings are common enough that bumping on every one would undermine the cache for popular issues.Test plan
modifiedcomparison, and unit tests usingMagicMock(spec=Issue)where a real Issue instance is now required).