[#739] Fix plot_count +1 bug: reconcile after genesis plot#741
[#739] Fix plot_count +1 bug: reconcile after genesis plot#741realproject7 merged 3 commits intomainfrom
Conversation
Root cause: StorylineCreated handler (indexer + backfill) hardcodes plot_count: 1 but never calls reconcileStorylinePlotCount(), unlike the PlotChained handler which does. This causes drift when genesis plots are processed, leading to plot_count being off by +1. Fix: Add reconcileStorylinePlotCount() call after genesis plot upsert in both the indexer (index/storyline/route.ts) and the backfill cron (cron/backfill/route.ts), matching the existing pattern used by the PlotChained handler. Reading Mode was unaffected because it counts actual chapter objects rather than relying on the cached plot_count field. Fixes #739 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
project7-interns
left a comment
There was a problem hiding this comment.
T2b APPROVE — root cause and fix verified:
Root cause: StorylineCreated hardcodes plot_count:1 then inserts genesis plot, but never reconciles — unlike PlotChained which calls reconcileStorylinePlotCount(). Result: cached plot_count drifts +1.
Fix: Add reconcileStorylinePlotCount() after genesis plot upsert in both:
- src/app/api/index/storyline/route.ts (live indexer) ✓
- src/app/api/cron/backfill/route.ts (backfill cron) ✓
Import added only where missing (storyline route); backfill already had it. Reuses existing reconcile function — no new abstractions. Reading Mode unaffected (counts chapters directly). +7 lines, 0 deletions. Minimal and correct.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The root-cause analysis looks right, but this patch only fixes future StorylineCreated ingests. Existing storylines with inflated plot_count values will stay wrong after merge.
Findings
- [high] The backfill cron still resumes from
backfill_cursor.last_block, so the newreconcileStorylinePlotCount()only runs for brand-new or explicitly replayedStorylineCreatedevents. Production storylines that were already indexed before this patch are never revisited, which means the globally wrongstoryline.plot_countvalues on profile/story/card surfaces remain wrong even after deployment.- File:
src/app/api/cron/backfill/route.ts:110 - Suggestion: add an explicit one-off reconciliation for existing storylines, or intentionally replay the affected historical range as part of the fix so current rows are corrected, not just future inserts.
- File:
Decision
Requesting changes because issue #739 is a global existing-data bug, and this PR currently prevents new drift without repairing the rows that are already wrong.
Adds POST /api/cron/reconcile-plots that iterates all storylines and reconciles plot_count from the actual plots table. Idempotent and safe to run multiple times. Fixes inflated plot_count on already-indexed storylines that were affected before the genesis reconciliation fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The new reconcile endpoint repairs existing bad rows, but it is exposed without the cron authorization guard used by the other maintenance routes.
Findings
- [high]
POST /api/cron/reconcile-plotsis unauthenticated. Unlike the existing cron/backfill route, this new endpoint does not checkCRON_SECRETor any auth header before iterating every storyline and issuing server-side updates. That means any external caller can trigger a full-database reconciliation job on demand.- File:
src/app/api/cron/reconcile-plots/route.ts:12 - Suggestion: apply the same cron authorization pattern used in
src/app/api/cron/backfill/route.tsbefore running the reconciliation loop.
- File:
Decision
Requesting changes because the data-fix direction is right, but the new maintenance endpoint needs the existing cron auth guard before it is safe to merge.
Matches the same auth pattern used by the backfill cron route. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The code-level blockers are resolved, but issue #739 still has no ticket comment documenting the root cause.
Findings
- [medium] Issue #739 explicitly requires the root cause to be documented in the ticket comments, but the issue currently has no comments. The PR summary explains it, but the acceptance criterion is still unmet.
- File:
issue #739 comments - Suggestion: post the root-cause summary to issue #739 before merge so the fix is documented where the ticket asked for it.
- File:
Decision
Keeping this as request changes because the implementation looks correct now, but the ticket’s documentation requirement is still incomplete.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The remaining documentation blocker is resolved. Issue #739 now has the required root-cause comment, and the code changes cover both future ingests and existing storyline reconciliation.
Findings
- None.
Decision
Approving because the review blockers are addressed and the ticket requirements are now complete.
Summary
StorylineCreatedhandler hardcodesplot_count: 1but never callsreconcileStorylinePlotCount(), unlike thePlotChainedhandler which does. This causes the cachedplot_countto drift by +1.reconcileStorylinePlotCount()after genesis plot upsert in both:src/app/api/index/storyline/route.ts(live indexer)src/app/api/cron/backfill/route.ts(backfill cron)plot_countfield.storyline.plot_countfrom DB — no UI changes needed.Fixes #739
Test plan
npm run buildpasses🤖 Generated with Claude Code