Fix follower cleanup semantics#26
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes issues across multiple repositories (MemoryRepository, KvRepository, MemoryCachedRepository, SqliteRepository, and PostgresRepository) where removing one of multiple active follow requests for the same actor would prematurely delete the follower or fail to clean up stale followers during reassignment. The changes ensure that a follower is only removed when no active follow requests remain. Feedback on these changes highlights a potential performance bottleneck in KvRepository due to an BEGIN IMMEDIATE TRANSACTION in SQLite to prevent potential SQLITE_BUSY errors during write transactions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14937cdc87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/botkit-sqlite/src/mod.ts (1)
418-466: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove the check inside the transaction and delete by both IDs. A concurrent
addFollower()can reassign the samefollow_request_idafter the pre-transaction lookup, so this code may delete the newly assigned request while cleaning up the staleactorId. Make theDELETEconstrain bothfollow_request_idandfollower_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit-sqlite/src/mod.ts` around lines 418 - 466, The pre-transaction existence check in removeFollower can race with addFollower, allowing a reused follow_request_id to be deleted for the wrong follower. Move the validation into the transaction in removeFollower and make the DELETE in deleteRequestStmt constrain both follow_request_id and follower_id. Use the same row lookup and cleanup flow, but only commit after confirming the row still matches the expected actorId, so cleanupFollower cannot act on a reassigned request.packages/botkit/src/repository.ts (2)
569-592: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake follower cleanup atomic and indexed
cleanupFollowercan still delete a follower after another request has already reassigned that follower ID, because the follow-request check happens before the lock/delete section. The follow-request lookup is also a full scan on every add/remove path; a reverse index would avoid that linear work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/repository.ts` around lines 569 - 592, The follower cleanup flow is not atomic and can still delete a follower after it has been reassigned, because cleanupFollower checks hasFollowRequestForFollower before entering the lock/delete loop. Move the follow-request existence check into the locked section and re-validate the follower state immediately before deleting, using the existing lockKey/listKey logic in cleanupFollower to prevent stale deletes. Also replace the full scan in hasFollowRequestForFollower with a reverse index keyed by followerId so add/remove paths can do direct lookups instead of iterating all followRequests entries.
511-531: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard
addFollowerwith a CAS/lock. Concurrent writes for the samefollowRequestIdcan leave the losing actor persisted only in the followers list/JSON, sincepreviousFollowerIdis read before any write and the cleanup only applies to that stale value. Mirror the PostgreslockFollowRequestpath or use the existingkv.caspattern here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/botkit/src/repository.ts` around lines 511 - 531, Guard addFollower against concurrent updates by using the same CAS/lock approach used elsewhere in Repository, since reading previousFollowerId before writing can leave a losing follower partially persisted. Update addFollower to serialize per followRequestId (or use kv.cas) around the read/write/cleanup sequence, and ensure the cleanup only runs for the actor that actually won the write. Refer to Repository.addFollower, cleanupFollower, and the existing lockFollowRequest/kv.cas patterns when implementing the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/botkit-sqlite/src/mod.ts`:
- Around line 418-466: The pre-transaction existence check in removeFollower can
race with addFollower, allowing a reused follow_request_id to be deleted for the
wrong follower. Move the validation into the transaction in removeFollower and
make the DELETE in deleteRequestStmt constrain both follow_request_id and
follower_id. Use the same row lookup and cleanup flow, but only commit after
confirming the row still matches the expected actorId, so cleanupFollower cannot
act on a reassigned request.
In `@packages/botkit/src/repository.ts`:
- Around line 569-592: The follower cleanup flow is not atomic and can still
delete a follower after it has been reassigned, because cleanupFollower checks
hasFollowRequestForFollower before entering the lock/delete loop. Move the
follow-request existence check into the locked section and re-validate the
follower state immediately before deleting, using the existing lockKey/listKey
logic in cleanupFollower to prevent stale deletes. Also replace the full scan in
hasFollowRequestForFollower with a reverse index keyed by followerId so
add/remove paths can do direct lookups instead of iterating all followRequests
entries.
- Around line 511-531: Guard addFollower against concurrent updates by using the
same CAS/lock approach used elsewhere in Repository, since reading
previousFollowerId before writing can leave a losing follower partially
persisted. Update addFollower to serialize per followRequestId (or use kv.cas)
around the read/write/cleanup sequence, and ensure the cleanup only runs for the
actor that actually won the write. Refer to Repository.addFollower,
cleanupFollower, and the existing lockFollowRequest/kv.cas patterns when
implementing the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb4caa19-e934-44f1-b5ef-7c669be4f6be
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGES.mdpackages/botkit-postgres/src/mod.test.tspackages/botkit-postgres/src/mod.tspackages/botkit-sqlite/src/mod.test.tspackages/botkit-sqlite/src/mod.tspackages/botkit/src/bot-impl.test.tspackages/botkit/src/repository.test.tspackages/botkit/src/repository.ts
Move follower removal checks into the protected update path so stale follow request state cannot delete or report the wrong follower. SQLite now validates and deletes by both follow_request_id and follower_id in one transaction, while the KV repository serializes updates per follow request and revalidates follower state before cleanup. fedify-dev#26 (review) Assisted-by: Codex:GPT-5
Use immediate transactions for follower add and remove paths so SQLite reserves the write lock before reading rows that the same transaction will update. This avoids deferred transaction upgrades failing with SQLITE_BUSY under concurrent writers. fedify-dev#26 (comment) Assisted-by: Codex:GPT-5
Place the follower cleanup changelog entries under the unreleased 0.4.4 section instead of the already released 0.4.3 section. This keeps the release history intact and leaves the upcoming release notes populated. fedify-dev#26 (comment) Assisted-by: Codex:GPT-5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves issues across multiple repository implementations (MemoryRepository, KvRepository, MemoryCachedRepository, SqliteRepository, and PostgresRepository) where removing one of multiple active follow requests for the same actor could prematurely delete the follower or trigger an early unfollow event. It also ensures that reassigning a follow request does not leave stale followers behind. Key improvements include adding an index in SQLite, introducing a robust locking mechanism in KV storage, and adding comprehensive unit tests to cover these scenarios. I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 644bff122c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Store follower lock keys with a short TTL so a process crash after lock acquisition cannot leave persistent CAS-backed stores blocked forever. This keeps the existing lock protocol but lets abandoned lock entries age out without manual cleanup. fedify-dev#26 (comment) Assisted-by: Codex:GPT-5
Move follower removal checks into the protected update path so stale follow request state cannot delete or report the wrong follower. SQLite now validates and deletes by both follow_request_id and follower_id in one transaction, while the KV repository serializes updates per follow request and revalidates follower state before cleanup. fedify-dev#26 (review) Assisted-by: Codex:gpt-5.5
Use immediate transactions for follower add and remove paths so SQLite reserves the write lock before reading rows that the same transaction will update. This avoids deferred transaction upgrades failing with SQLITE_BUSY under concurrent writers. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
Place the follower cleanup changelog entries under the unreleased 0.4.4 section instead of the already released 0.4.3 section. This keeps the release history intact and leaves the upcoming release notes populated. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
Store follower lock keys with a short TTL so a process crash after lock acquisition cannot leave persistent CAS-backed stores blocked forever. This keeps the existing lock protocol but lets abandoned lock entries age out without manual cleanup. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
Preserve follower records while any accepted follow request still points at the actor, and only report an unfollow when the last active request is removed. This keeps repository state and Bot.onUnfollow delivery aligned across the memory, KV, SQLite, and PostgreSQL backends. Also clean up stale followers when a follow request is reassigned, add regression tests for duplicate and reassigned follow requests, and record the change in the changelog. Fixes fedify-dev#25 Assisted-by: Codex:gpt-5.5
Move follower removal checks into the protected update path so stale follow request state cannot delete or report the wrong follower. SQLite now validates and deletes by both follow_request_id and follower_id in one transaction, while the KV repository serializes updates per follow request and revalidates follower state before cleanup. fedify-dev#26 (review) Assisted-by: Codex:gpt-5.5
Use immediate transactions for follower add and remove paths so SQLite reserves the write lock before reading rows that the same transaction will update. This avoids deferred transaction upgrades failing with SQLITE_BUSY under concurrent writers. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
Place the follower cleanup changelog entries under the unreleased 0.4.4 section instead of the already released 0.4.3 section. This keeps the release history intact and leaves the upcoming release notes populated. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
Store follower lock keys with a short TTL so a process crash after lock acquisition cannot leave persistent CAS-backed stores blocked forever. This keeps the existing lock protocol but lets abandoned lock entries age out without manual cleanup. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
Collapse duplicated follower-index pruning branches into one update path while preserving the same cleanup result. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes issues across several repositories (MemoryRepository, KvRepository, MemoryCachedRepository, SqliteRepository, and PostgresRepository) where removing one of multiple active follow requests for the same actor could prematurely delete the follower or trigger incorrect unfollow events, and where reassigning follow requests left stale followers. The changes introduce robust follower cleanup logic, indexing, and transaction/locking improvements. The review feedback highlights a valuable optimization opportunity in KvRepository.addFollower where checking if a follower is brand new can prevent an expensive O(N) scan of the follow requests space.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request addresses issues across @fedify/botkit and its database adapters (SQLite and PostgreSQL) where removing one of multiple active follow requests for the same actor would prematurely delete the follower or trigger an unfollow event. It introduces a robust locking mechanism and index management in KvRepository, updates MemoryRepository and SQL repositories to only clean up followers when no active follow requests remain, and adds extensive test coverage. The review feedback correctly notes that setting "sideEffects": true in @fedify/botkit's package.json prevents tree-shaking and suggests moving this configuration to the PostgreSQL package if it was meant for the Temporal polyfill.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Avoid scanning the follow-request keyspace when adding the first request for a brand new follower. New follower rows can start from an empty reverse index, while existing followers still rebuild missing legacy indexes as needed. fedify-dev#26 (comment) fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes issues in MemoryRepository, KvRepository, MemoryCachedRepository, SqliteRepository, and PostgresRepository where removing one of multiple active follow requests for the same actor would prematurely delete the follower or fire an unfollow event, and reassigning a follow request left stale followers behind. It introduces robust locking mechanisms, indexing of follow requests per follower, and updates database queries to safely handle conflicts and cleanups. Comprehensive unit tests have been added across the packages to verify these behaviors. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6fb587221
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Rebuild a missing follower reverse index even when the follower row is being created. Upgraded stores can still have legacy follow request mappings for that actor, so treating every missing follower row as a clean slate can recreate the premature-unfollow case. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes issues in @fedify/botkit, @fedify/botkit-sqlite, and @fedify/botkit-postgres repositories where removing one of multiple active follow requests for the same actor would prematurely delete the follower or trigger a premature unfollow event. The repositories have been updated to only clean up followers when no active follow requests remain. Additionally, KvRepository has been refactored to use expiring follower locks and index follow requests. The review feedback highlights a potential ReferenceError due to module-level Temporal duration definitions in repository.ts, suggesting they be initialized lazily or inside the methods where they are used to ensure safety with polyfills.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Avoid evaluating Temporal.Duration at repository module load time. Some source-loading environments register the Temporal polyfill after import ordering is already underway, so the lock durations now initialize only when the CAS lock path runs. fedify-dev#26 (comment) fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves issues across @fedify/botkit repositories (Memory, SQLite, Postgres, and KV) where removing one of multiple active follow requests for the same actor could prematurely delete the follower or trigger unfollow events, and where reassigning follow requests left stale followers. It introduces a cleanupFollower mechanism to ensure followers are only deleted when no active follow requests remain, adds database indexes, and improves transaction and locking safety. Review feedback highlights a potential compilation error under strictNullChecks in the SQLite repository due to a possibly undefined row object, and advises reverting the sideEffects change to false in packages/botkit/package.json to preserve tree-shaking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Guard the removed follower row explicitly before reading actor_json. This keeps the post-transaction parse path easy for strict null checking to follow without changing the removal behavior. fedify-dev#26 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc0c83b526
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
This pull request fixes how follower records are maintained when the same actor has more than one accepted
Followrequest, or when an existing follow request ID is reassigned to another actor.The old behavior treated a single follow request as if it owned the follower record. That broke down when a remote server sent duplicate follows, replayed a follow under a new ID, or otherwise left multiple accepted follow requests pointing at the same actor. Removing one request could delete the shared follower record even though another request still made the actor a follower. It could also make
Bot.onUnfollowfire too early, becauseremoveFollower()returned an actor even when the follower was still present.This PR changes the repository contract so
removeFollower()returns an actor only when the follower record is actually removed. If an actor still has another active follow request, the specific request is removed but the method returnsundefined, which keepsBot.onUnfollowaligned withhasFollower()andcountFollowers().The same semantics are now applied across packages/botkit/src/repository.ts, packages/botkit-sqlite/src/mod.ts, and packages/botkit-postgres/src/mod.ts.
MemoryRepository,KvRepository,MemoryCachedRepository,SqliteRepository, andPostgresRepositoryall keep the follower row until no remaining follow request references it, and they clean up stale follower rows when a follow request is reassigned to a different actor.For
KvRepository, the fix usesKvStore.list()to scan existing follow request entries instead of introducing a new reverse index, so existing KV data remains usable without a migration. For SQLite, the follow request table now has an index onfollower_id, andINSERT OR REPLACEwas replaced with conflict updates to avoid deleting parent rows that still have valid references.Regression tests were added in packages/botkit/src/repository.test.ts, packages/botkit-sqlite/src/mod.test.ts, packages/botkit-postgres/src/mod.test.ts, and packages/botkit/src/bot-impl.test.ts. The tests cover duplicate follow requests, reassigned follow request IDs, and the bot-level unfollow event behavior that made the return value bug visible.
Fixes #25.
Summary by CodeRabbit