Slackbot: dedupe only the root message; process replies normally#1225
Merged
Conversation
…e try_acquire/mark_completed only when event is the root (no thread_ts)\n- Prevent 'SKIPPED_DUPLICATE' on legitimate follow-ups in the same thread
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where Slack thread replies were incorrectly being treated as duplicates and skipped after the root message was processed. The fix modifies the deduplication logic to only gate root messages while allowing replies to be processed normally.
- Modified deduplication to only apply to root messages (when
event.thread_tsis None) - Added conditional logic to only acquire/mark completion for root messages
- Updated error messages to clarify they apply specifically to root events
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| except Exception: | ||
| logger.warning("Failed to mark thread as completed") | ||
| # Only mark completion for the root message; do not block replies | ||
| if "is_root_message" in locals() and is_root_message: |
There was a problem hiding this comment.
Using 'is_root_message' in locals() is fragile and could break if the variable declaration is moved. Consider restructuring the code to avoid this check, perhaps by moving the completion logic inside the if is_root_message: block or using a different approach to track whether completion is needed.
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.
Problem\n- After merging the previous PR, follow-up messages in the same thread were skipped as duplicates.\n- Root cause: the idempotency key (root thread_ts) was used to gate all events in the thread, so once a status row existed, later events (including legitimate replies) failed acquisition and were treated as duplicates.\n\nFix\n- Dedupe ONLY the root message (when event.thread_ts is None).\n - Acquire/mark-completed only for the root.\n - While in progress: edits to the root get a polite "still working" note.\n - After completion: duplicate root deliveries are skipped.\n- Replies (messages with event.thread_ts present) are not gated and are processed normally.\n\nNotes\n- Keeps the tiny SQLite status in _internal/thread_status.\n- Minimal changes to api.py; retains the existing user-facing behavior for edit notices.\n- Lint/format pass.\n\nHappy to adjust behavior if you want replies during in-progress to be queued or coalesced instead of processed immediately.