-
Notifications
You must be signed in to change notification settings - Fork 385
Slackbot: ignore edits while initial response is in progress #1224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
06be2f5
92b2f54
41bea1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| """Thread status tracking for Slack events (cross-process safe). | ||
| This module encapsulates a tiny SQLite-backed status mechanism to avoid duplicate | ||
| processing of the same Slack thread when users edit their original post or when | ||
| Slack delivers multiple events. It keeps the rest of the app simple and avoids | ||
| sprawling status logic across files. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Literal | ||
|
|
||
| from slackbot.core import Database | ||
|
|
||
| Status = Literal["in_progress", "completed"] | ||
|
|
||
|
|
||
| async def ensure_schema(db: Database) -> None: | ||
| """Ensure the status table exists. | ||
| Using CREATE TABLE IF NOT EXISTS is idempotent and cheap enough to call | ||
| whenever we touch the status table, avoiding module-level state or locks. | ||
| """ | ||
|
|
||
| def _create() -> None: | ||
| db.con.execute( | ||
| """ | ||
| CREATE TABLE IF NOT EXISTS slack_thread_status ( | ||
| thread_ts TEXT PRIMARY KEY, | ||
| status TEXT NOT NULL CHECK (status IN ('in_progress','completed')), | ||
| updated_at REAL NOT NULL | ||
| ); | ||
| """ | ||
| ) | ||
| db.con.commit() | ||
|
|
||
| await db.loop.run_in_executor(db.executor, _create) | ||
|
|
||
|
|
||
| async def try_acquire(db: Database, thread_ts: str) -> bool: | ||
| """Attempt to mark a thread as in_progress; returns True if acquired. | ||
| Uses an atomic INSERT OR IGNORE on a PRIMARY KEY to prevent duplicates across | ||
| concurrent processes or tasks. | ||
| """ | ||
| await ensure_schema(db) | ||
|
|
||
| def _insert() -> int: | ||
| cur = db.con.cursor() | ||
| cur.execute( | ||
| """ | ||
| INSERT OR IGNORE INTO slack_thread_status (thread_ts, status, updated_at) | ||
| VALUES (?, 'in_progress', strftime('%s','now')) | ||
| """, | ||
| (thread_ts,), | ||
| ) | ||
| db.con.commit() | ||
| return cur.rowcount | ||
|
|
||
| rowcount: int = await db.loop.run_in_executor(db.executor, _insert) | ||
| return rowcount == 1 | ||
|
|
||
|
|
||
| async def get_status(db: Database, thread_ts: str) -> Status | None: | ||
| await ensure_schema(db) | ||
|
|
||
| def _query() -> Status | None: | ||
| cur = db.con.cursor() | ||
| cur.execute( | ||
| "SELECT status FROM slack_thread_status WHERE thread_ts = ?", | ||
| (thread_ts,), | ||
| ) | ||
| row = cur.fetchone() | ||
| return row[0] if row else None # type: ignore[return-value] | ||
|
|
||
| return await db.loop.run_in_executor(db.executor, _query) | ||
|
|
||
|
|
||
| async def mark_completed(db: Database, thread_ts: str) -> None: | ||
| await ensure_schema(db) | ||
|
|
||
| def _update() -> None: | ||
| cur = db.con.cursor() | ||
| cur.execute( | ||
| """ | ||
| UPDATE slack_thread_status | ||
| SET status = 'completed', updated_at = strftime('%s','now') | ||
| WHERE thread_ts = ? | ||
| """, | ||
| (thread_ts,), | ||
| ) | ||
| db.con.commit() | ||
|
|
||
| await db.loop.run_in_executor(db.executor, _update) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,15 @@ | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| from slackbot._internal.constants import WORKSPACE_TO_CHANNEL_ID | ||||||||||||||||||||||
| from slackbot._internal.templates import CHANNEL_REDIRECT_MESSAGE, WELCOME_MESSAGE | ||||||||||||||||||||||
| from slackbot._internal.thread_status import ( | ||||||||||||||||||||||
| get_status as get_thread_status, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| from slackbot._internal.thread_status import ( | ||||||||||||||||||||||
| mark_completed as mark_thread_completed, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| from slackbot._internal.thread_status import ( | ||||||||||||||||||||||
| try_acquire as try_acquire_thread, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| from slackbot.assets import summarize_thread | ||||||||||||||||||||||
| from slackbot.core import ( | ||||||||||||||||||||||
| Database, | ||||||||||||||||||||||
|
|
@@ -40,6 +49,9 @@ | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| logger = get_logger(__name__) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Duplicate handling is coordinated via a small SQLite table in | ||||||||||||||||||||||
| # _internal.thread_status to work across processes. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def get_designated_channel_for_workspace(team_id: str) -> str | None: | ||||||||||||||||||||||
| """Get the designated channel ID for a given workspace team ID.""" | ||||||||||||||||||||||
|
|
@@ -137,6 +149,37 @@ async def handle_message(payload: SlackPayload, db: Database): | |||||||||||||||||||||
| return Completed(message="Message too long", name="SKIPPED") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if re.search(BOT_MENTION, user_message) and payload.authorizations: | ||||||||||||||||||||||
| # Use the root thread timestamp as our idempotency key | ||||||||||||||||||||||
| root_ts = thread_ts | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Cross-process acquire; only one handler should proceed | ||||||||||||||||||||||
| acquired = await try_acquire_thread(db, root_ts) | ||||||||||||||||||||||
| if not acquired: | ||||||||||||||||||||||
| status = await get_thread_status(db, root_ts) | ||||||||||||||||||||||
| if status == "in_progress": | ||||||||||||||||||||||
| assert event.channel is not None, ( | ||||||||||||||||||||||
| "Event channel is None when posting edit-ignored notice" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
Comment on lines
+160
to
+162
|
||||||||||||||||||||||
| assert event.channel is not None, ( | |
| "Event channel is None when posting edit-ignored notice" | |
| ) | |
| if event.channel is None: | |
| logger.error("Event channel is None when posting edit-ignored notice") | |
| return Completed( | |
| message="Cannot post edit-ignored notice: event channel is None", | |
| name="SKIPPED_NO_CHANNEL", | |
| data=dict(thread_ts=root_ts), | |
| ) |
Copilot
AI
Sep 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message should include the specific error details for better debugging. Consider logging the exception: logger.warning('Failed to mark thread as completed', exc_info=True) or logger.warning('Failed to mark thread as completed: %s', e).
| logger.warning("Failed to mark thread as completed") | |
| logger.warning("Failed to mark thread as completed", exc_info=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type ignore comment is unnecessary. The return type annotation already handles the None case correctly, and mypy should understand that
row[0]will be aStatuswhenrowis not None.