Used atomic query for updating repost count outside of manual updates #1172
Conversation
ref https://linear.app/ghost/issue/PROD-2422 When concurrent `announce` / `undo` (`announce`) activities are processed for external posts, the repost count could become negative due to using stale in-memory values Example: - Post does not exist in the db - We receive an `announce` and an `undo` for the post very close together - Both handlers try to create the post (as it does not exist) - One handler succeeds in creating the post and gets the result (a post with a repost count of 0) - The other handler fails gracefully (as we ignore duplicate inserts) and gets the same post, with a repost count of 0 - `announce` handler adds repost and saves: 0 + 1 = 1 - `undo` handler removes repost (which exists because `announce` has already saved), but calculates: 0 - 1 = -1 To resolve we use atomic SQL operations (repost_count + delta) for external posts when doing add/remove operations, preventing stale reads. Manual count updates via `setRepostCount()` are still preserved when the dirty flag is set
WalkthroughA new integration test was added to the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/post/post.repository.knex.integration.test.ts (1)
2338-2338: Consider using more specific assertions instead of deep equality.The
toEqual()comparison between Post objects might be fragile if the Post class contains methods, computed properties, or complex internal state that doesn't compare reliably.Consider replacing the deep equality check with specific property assertions:
- expect(postClone).toEqual(post); + expect(postClone.id).toBe(post.id); + expect(postClone.repostCount).toBe(post.repostCount); + expect(postClone.uuid).toBe(post.uuid);This approach is more explicit about what properties should match and less likely to break due to internal object structure changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/post/post.repository.knex.integration.test.ts(1 hunks)src/post/post.repository.knex.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: allouis
PR: TryGhost/ActivityPub#615
File: src/post/post.service.ts:254-278
Timestamp: 2025-05-05T07:34:59.855Z
Learning: The moderation check for the `repostByApId` method (using `moderationService.canInteractWithAccount()`) will be implemented in a separate commit, as part of the phased approach to adding moderation restrictions to reposts.
Learnt from: allouis
PR: TryGhost/ActivityPub#737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
Learnt from: allouis
PR: TryGhost/ActivityPub#615
File: src/http/api/repost.ts:29-42
Timestamp: 2025-05-05T07:33:50.790Z
Learning: The PostService.repostByApId method currently only returns 'upstream-error', 'not-a-post', 'missing-author', and 'already-reposted' errors. The 'cannot-interact' error for moderation restrictions hasn't been implemented yet, though PR #615 is preparing the repost controller to handle such restrictions in the future.
📚 Learning: the moderation check for the `repostbyapid` method (using `moderationservice.caninteractwithaccount(...
Learnt from: allouis
PR: TryGhost/ActivityPub#615
File: src/post/post.service.ts:254-278
Timestamp: 2025-05-05T07:34:59.855Z
Learning: The moderation check for the `repostByApId` method (using `moderationService.canInteractWithAccount()`) will be implemented in a separate commit, as part of the phased approach to adding moderation restrictions to reposts.
Applied to files:
src/post/post.repository.knex.tssrc/post/post.repository.knex.integration.test.ts
📚 Learning: in the outboxes table migration for ghost activitypub, the author_id field always refers to the auth...
Learnt from: allouis
PR: TryGhost/ActivityPub#737
File: migrate/migrations/000054_populate-outboxes-table.up.sql:56-63
Timestamp: 2025-05-27T09:09:22.110Z
Learning: In the outboxes table migration for Ghost ActivityPub, the author_id field always refers to the author of the original post, not the person performing the outbox action. For reposts (outbox_type = 1), author_id should be posts.author_id (original author), while user_id captures who made the repost via the joins through accounts and users tables.
Applied to files:
src/post/post.repository.knex.tssrc/post/post.repository.knex.integration.test.ts
📚 Learning: the postservice.repostbyapid method currently only returns 'upstream-error', 'not-a-post', 'missing-...
Learnt from: allouis
PR: TryGhost/ActivityPub#615
File: src/http/api/repost.ts:29-42
Timestamp: 2025-05-05T07:33:50.790Z
Learning: The PostService.repostByApId method currently only returns 'upstream-error', 'not-a-post', 'missing-author', and 'already-reposted' errors. The 'cannot-interact' error for moderation restrictions hasn't been implemented yet, though PR #615 is preparing the repost controller to handle such restrictions in the future.
Applied to files:
src/post/post.repository.knex.tssrc/post/post.repository.knex.integration.test.ts
📚 Learning: in the tryghost/activitypub project, mysql automatically handles the `updated_at` column when updati...
Learnt from: allouis
PR: TryGhost/ActivityPub#583
File: src/account/account.repository.knex.ts:18-29
Timestamp: 2025-04-29T07:58:17.648Z
Learning: In the TryGhost/ActivityPub project, MySQL automatically handles the `updated_at` column when updating records, so there's no need to set it manually in update queries.
Applied to files:
src/post/post.repository.knex.ts
📚 Learning: in src/http/api/webhook.controller.ts, the handlepostunpublished, handlepostupdated, and handlepostd...
Learnt from: vershwal
PR: TryGhost/ActivityPub#1033
File: src/http/api/webhook.controller.ts:103-107
Timestamp: 2025-07-14T11:41:50.880Z
Learning: In src/http/api/webhook.controller.ts, the handlePostUnpublished, handlePostUpdated, and handlePostDeleted methods in PR #1033 are intentionally implemented as dummy APIs that return 200 status with null body. These are placeholder implementations to prevent URL errors while the actual backend logic will be built in upcoming PRs, following the incremental development approach used in this codebase.
Applied to files:
src/post/post.repository.knex.tssrc/post/post.repository.knex.integration.test.ts
🔇 Additional comments (6)
src/post/post.repository.knex.ts (3)
413-418: Excellent implementation of the atomic update logic!The
shouldUseAtomicUpdateflag correctly addresses the race condition issue described in the PR objectives. The logic properly distinguishes between:
- Internal posts and external posts without manual modifications → use atomic SQL updates
- External posts with manual modifications → preserve manual count while applying delta
The comments clearly explain the business reasoning.
436-437: Good addition for observability.Including the
shouldUseAtomicUpdateflag in the logging provides valuable debugging information to understand which update strategy is being applied in different scenarios.
443-454: Implementation correctly applies the atomic update strategy.The conditional logic properly implements the two update strategies:
- Atomic SQL increment for race condition prevention when
shouldUseAtomicUpdateis true- Manual count preservation with delta application when false
The implementation is consistent with the like count update pattern used earlier in the file (lines 352-361), maintaining code consistency. The math for applying the delta (
insertedRepostsCount - removedRepostsCount) is correct.src/post/post.repository.knex.integration.test.ts (3)
2305-2357: Excellent test coverage for concurrent repost count updates!This test effectively simulates the race condition scenario described in the PR objectives, where concurrent
announceandundoactivities could cause negative repost counts due to stale in-memory values. The test structure properly validates that atomic SQL operations prevent data corruption during concurrent updates.
2312-2336: Post clone creation is thorough and realistic.The manual construction of the Post clone effectively simulates how concurrent handlers would have separate instances with identical initial state. The use of all constructor parameters ensures the clone has the same state as the original post before the concurrent modifications begin.
2349-2352: Proper validation of atomic update behavior.The test correctly expects that the save operation should not throw an error despite the clone having a stale repost count of 0. This validates that the repository uses atomic SQL operations (likely
UPDATE posts SET repost_count = repost_count - 1 WHERE id = ?) instead of direct assignment based on in-memory values.
ref https://linear.app/ghost/issue/PROD-2422
When concurrent
announce/undo(announce) activities are processed for external posts, the repost count could become negative due to using stale in-memory valuesExample:
announceand anundofor the post very close togetherannouncehandler adds repost and saves: 0 + 1 = 1undohandler removes repost (which exists becauseannouncehas already saved), but calculates: 0 - 1 = -1To resolve we use atomic SQL operations (repost_count + delta) for external posts when doing add/remove operations, preventing stale reads. Manual count updates via
setRepostCount()are still preserved when the dirty flag is set