mirror of
https://github.com/Sea-Haven-Industries/afterhours-shift-manager.git
synced 2026-09-30 07:53:11 +00:00
Harden schedule bump against Slack retries
The message listener runs synchronously before the 200 ack, so a slow delete+repost can blow Slack's 3s window and be retried with no guard. Ignore retried deliveries (X-Slack-Retry-Num), stamp last_bump_ts before the delete/repost so a mid-bump retry is debounced, and skip thread replies that don't move the main timeline. Refs: #135
This commit is contained in:
parent
bf359100a1
commit
d84c14d031
3 changed files with 91 additions and 12 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue