diff --git a/docs/sticky-schedule-post.md b/docs/sticky-schedule-post.md index 88ae188..5039b1f 100644 --- a/docs/sticky-schedule-post.md +++ b/docs/sticky-schedule-post.md @@ -101,15 +101,23 @@ request URL. On a `message.channels` event in the schedule channel stored as `last_bump_ts` on the schedule-post record, so the debounce check reads no channel history. - **Idempotent / safe** — the bump is skipped when: + - the delivery is a Slack **retry** (`X-Slack-Retry-Num` set) — under + `process_before_response=True` the delete+repost runs before the 200 ack, so + a slow run can be retried; we never bump on a retry (the next real message + bumps anyway), + - the triggering message is a **thread reply** (`thread_ts`) — a threaded reply + doesn't push the schedule down the main timeline, - there is no stored post, - the triggering message is the bot's own (`bot_id`) or a non-user message subtype (edits, deletes, joins, …), - the debounce window has not elapsed, or - the bot isn't in the channel (`not_in_channel`). -- Otherwise it `chat_delete`s the stored post and `chat_postMessage`s the current - `build_week_schedule` blocks, then `save_schedule_post`s the new `ts` (with - `last_bump_ts`). If the old message is already gone, it still reposts so the - channel always ends with the schedule. +- Otherwise it **stamps `last_bump_ts` optimistically before** the delete/repost + (so a retry fired mid-bump — or after the run dies before the final save — is + debounced and can't orphan a duplicate), `chat_delete`s the stored post, + `chat_postMessage`s the current `build_week_schedule` blocks, then + `save_schedule_post`s the new `ts`. If the old message is already gone, it + still reposts so the channel always ends with the schedule. ### Interaction with `_refresh_schedule_post` and the rollover diff --git a/src/slack-bot/app.py b/src/slack-bot/app.py index 14cc4be..8d99b3c 100644 --- a/src/slack-bot/app.py +++ b/src/slack-bot/app.py @@ -440,19 +440,27 @@ def _slack_error_code(exc) -> str | None: return None -def handle_channel_message(event, client, schedule, schedule_channel): +def handle_channel_message(event, client, schedule, schedule_channel, retry_num=None): """Bump the schedule post to the bottom of the channel on new activity. Debounced delete+repost of the tracked post so it stays at the bottom of - the channel timeline. Idempotent/safe: skips when there is no stored post, - when the triggering message is the bot's own / an edit-delete subtype, when - debounced inside the window, or when the bot isn't in the channel. + the channel timeline. Idempotent/safe: skips retried Slack deliveries + (``retry_num``), thread replies, the bot's own / edit-delete messages, when + there is no stored post, when debounced inside the window, or when the bot + isn't in the channel. """ + # A retry means our first delivery already (likely) did the work but the + # 200 ack didn't reach Slack in time. Never bump again on a retry — the next + # real channel message bumps anyway — so a slow run can't double-post. + if retry_num: + logger.info("Ignoring retried message event (retry %s)", retry_num) + return if not schedule_channel or event.get("channel") != schedule_channel: return # Ignore the bot's own posts and non-user message events (edits, deletes, - # joins, …) — only genuine new user messages should bump. - if event.get("bot_id") or event.get("subtype"): + # joins, …), plus thread replies — a threaded reply doesn't push the + # schedule down the main timeline, so it isn't worth a delete+repost. + if event.get("bot_id") or event.get("subtype") or event.get("thread_ts"): return post = schedule.get_schedule_post(schedule_channel) @@ -467,6 +475,16 @@ def handle_channel_message(event, client, schedule, schedule_channel): ): return + # Optimistically stamp the debounce window *before* the delete/repost, so a + # retry fired while this run is still in flight (or after it dies mid-bump) + # is suppressed and can't orphan a duplicate schedule message. + schedule.save_schedule_post( + schedule_channel, + post["message_ts"], + post.get("week_start", ""), + last_bump_ts=now, + ) + blocks = build_week_schedule(schedule) text = _schedule_fallback_text() try: @@ -2199,7 +2217,10 @@ def create_app( publish_home(client, event["user"], _changelog_text()) @app.event("message") - def handle_message_event(event, client): - handle_channel_message(event, client, schedule, schedule_channel) + def handle_message_event(event, client, request): + retry_num = request.headers.get("x-slack-retry-num") if request else None + handle_channel_message( + event, client, schedule, schedule_channel, retry_num=retry_num + ) return app diff --git a/tests/slack_bot/test_channel_bump.py b/tests/slack_bot/test_channel_bump.py index 3e282ea..fb44eb7 100644 --- a/tests/slack_bot/test_channel_bump.py +++ b/tests/slack_bot/test_channel_bump.py @@ -105,6 +105,56 @@ def test_skips_other_channel(slackbot_app, schedule, seed, client): client.chat_postMessage.assert_not_called() +@freeze_time(MON) +def test_retried_event_is_a_noop(slackbot_app, schedule, seed, client): + # A Slack retry (X-Slack-Retry-Num set) must never trigger a second bump. + seed.schedule_post(CHANNEL, "111.111") + + slackbot_app.handle_channel_message( + _msg(), client, schedule, CHANNEL, retry_num="1" + ) + + client.chat_delete.assert_not_called() + client.chat_postMessage.assert_not_called() + # The record is left untouched (no optimistic debounce stamp either). + post = schedule.get_schedule_post(CHANNEL) + assert post["message_ts"] == "111.111" + assert post.get("last_bump_ts") is None + + +@freeze_time(MON) +def test_thread_reply_is_a_noop(slackbot_app, schedule, seed, client): + # A threaded reply doesn't push the schedule down the main timeline. + seed.schedule_post(CHANNEL, "111.111") + + slackbot_app.handle_channel_message( + _msg(thread_ts="050.000"), client, schedule, CHANNEL + ) + + client.chat_delete.assert_not_called() + client.chat_postMessage.assert_not_called() + assert schedule.get_schedule_post(CHANNEL)["message_ts"] == "111.111" + + +@freeze_time(MON) +def test_stamps_debounce_before_repost(slackbot_app, schedule, seed, client): + # The debounce window is stamped optimistically before the delete/repost so + # a retry mid-bump is suppressed even if this run dies before the final save. + seed.schedule_post(CHANNEL, "111.111") + captured = {} + + def _delete(**kwargs): + # Mid-bump snapshot: the record already carries last_bump_ts. + captured["post"] = schedule.get_schedule_post(CHANNEL) + + client.chat_delete.side_effect = _delete + client.chat_postMessage.return_value = {"ts": "222.222"} + + slackbot_app.handle_channel_message(_msg(), client, schedule, CHANNEL) + + assert captured["post"].get("last_bump_ts") is not None + + @freeze_time(MON) def test_skips_when_bot_not_in_channel(slackbot_app, schedule, seed, client): seed.schedule_post(CHANNEL, "111.111")