diff --git a/docs/sticky-schedule-post.md b/docs/sticky-schedule-post.md index 5039b1f..f14c6a5 100644 --- a/docs/sticky-schedule-post.md +++ b/docs/sticky-schedule-post.md @@ -80,13 +80,16 @@ is therefore not a substitute for bottom-stickiness. ### Weekly rollover — `src/weekly-post/app.py` -- **Drop the unconditional `chat_delete` + always-repost.** On Monday, - `chat_update` the stored post to roll the two-week window forward, keeping the - same `ts` (the activity bump is what moves it to the bottom; the rollover just - refreshes content). -- **First-run / recovery fallback:** when there is no stored post or the - `chat_update` fails (e.g. the message was deleted manually), `chat_postMessage` - a fresh message and store its `ts`. +- **Delete + repost on Monday** — the previous week's stored post is + `chat_delete`d and a fresh schedule is `chat_postMessage`d so the message + lands at the bottom of the channel every Monday regardless of in-week + activity. The delete failure is non-fatal; the handler always reposts. +- **First-run:** when there is no stored post to delete, the handler + `chat_postMessage`s a fresh message and stores its `ts`. +- **Repost rollback:** if the new `ts` can't be persisted after the repost has + landed, the fresh message is `chat_delete`d before the error propagates, so an + async retry (which would read the stale, already-deleted `ts`) can't orphan a + duplicate schedule at the bottom of the channel. - The `pins:write` scope and the `_pin_schedule_post` helper are removed — the pin is fully replaced by bottom-stickiness. @@ -123,16 +126,18 @@ request URL. On a `message.channels` event in the schedule channel All three paths converge on the **single stored `ts`** and never fight: -- **Monday rollover** — `chat_update` the stored `ts` (content refresh; same - message), clearing `last_bump_ts` so the next activity is free to bump. -- **In-week shift edits** — `_refresh_schedule_post` `chat_update`s the same +- **Monday rollover** — `chat_delete`s the old stored `ts`, `chat_postMessage`s + a fresh post at the bottom, and saves the new `ts` (clearing `last_bump_ts` so + the next activity is free to bump). +- **In-week shift edits** — `_refresh_schedule_post` `chat_update`s the current stored `ts`; it does not change the `ts` or touch `last_bump_ts`. - **Activity bump** — deletes the stored `ts`, reposts the same content at the bottom, and saves the new `ts`. Subsequent rollovers and edits then operate on that new `ts`. -Because only the bump ever mints a new `ts` (and it always re-saves it -immediately), the other two paths always read the current `ts` from the record. +Both the rollover and the bump mint a new `ts` on repost and re-save it +immediately, so `_refresh_schedule_post` always reads the current `ts` from the +record. ## Scope / manifest impact diff --git a/src/slack-bot/app.py b/src/slack-bot/app.py index 50abcca..a8c6aad 100644 --- a/src/slack-bot/app.py +++ b/src/slack-bot/app.py @@ -461,15 +461,25 @@ def handle_channel_message(event, client, schedule, schedule_channel, retry_num= logger.info("Ignoring retried message event (retry %s)", retry_num) return if not schedule_channel or event.get("channel") != schedule_channel: + logger.info( + "Skipping bump — channel mismatch (expected %s, got %s)", + schedule_channel, + event.get("channel"), + ) return # Ignore the bot's own posts and non-user message events (edits, deletes, # 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"): + logger.info( + "Skipping bump — bot message, subtype %s, or thread reply", + event.get("subtype"), + ) return post = schedule.get_schedule_post(schedule_channel) if not post or not post.get("message_ts"): + logger.info("Skipping bump — no stored schedule post for %s", schedule_channel) return now = time.time() @@ -478,6 +488,11 @@ def handle_channel_message(event, client, schedule, schedule_channel, retry_num= last_bump is not None and now - float(last_bump) < SCHEDULE_BUMP_DEBOUNCE_SECONDS ): + logger.info( + "Skipping bump — debounced (last bump %.0fs ago, window %ds)", + now - float(last_bump), + SCHEDULE_BUMP_DEBOUNCE_SECONDS, + ) return # Optimistically stamp the debounce window *before* the delete/repost, so a diff --git a/src/weekly-post/app.py b/src/weekly-post/app.py index 74b67c9..45d5744 100644 --- a/src/weekly-post/app.py +++ b/src/weekly-post/app.py @@ -318,41 +318,55 @@ def handler(event, context): f"to {end_date.strftime('%b %-d')}" ) - # Roll the two-week window forward by editing the stored message in place - # (same ts) so the Monday rollover, in-week shift edits, and the activity - # bump all converge on a single stored ts. Fall back to a fresh post when - # there is no stored message or the edit fails (e.g. it was deleted). + # Delete the previous week's schedule post + repost a fresh one so the + # message lands at the bottom of the channel every Monday. The activity + # bump (handle_channel_message) handles in-week bottom-stickiness; this + # ensures the rollover itself re-places the post at the bottom. old_post = schedule.get_schedule_post(channel_id) - message_ts = None if old_post and old_post.get("message_ts"): try: - slack.chat_update( - channel=channel_id, - ts=old_post["message_ts"], - blocks=blocks, - text=fallback_text, - ) - message_ts = old_post["message_ts"] + slack.chat_delete(channel=channel_id, ts=old_post["message_ts"]) logger.info( - "Rolled schedule post %s forward to week of %s", - message_ts, - week_start, + "Deleted previous schedule post %s for weekly rollover", + old_post["message_ts"], ) except Exception: logger.warning( - "Could not update existing schedule post; reposting", exc_info=True + "Could not delete old schedule post for rollover; continuing", + exc_info=True, ) - if message_ts is None: - result = slack.chat_postMessage( - channel=channel_id, blocks=blocks, text=fallback_text - ) - message_ts = result["ts"] - logger.info( - "Posted new weekly schedule to channel %s (ts=%s)", channel_id, message_ts - ) + result = slack.chat_postMessage( + channel=channel_id, blocks=blocks, text=fallback_text + ) + message_ts = result["ts"] + logger.info( + "Posted new weekly schedule to channel %s (ts=%s)", channel_id, message_ts + ) - schedule.save_schedule_post(channel_id, message_ts, week_start) + # Persist the new ts immediately. If the write fails after the post has + # already landed, roll the fresh message back before letting the error + # propagate — an async retry would otherwise read the stale (already + # deleted) ts, no-op its delete, and post a *second* schedule, orphaning + # this one at the bottom of the channel. + try: + schedule.save_schedule_post(channel_id, message_ts, week_start) + except Exception: + logger.warning( + "Failed to persist schedule post %s; rolling it back to avoid an " + "orphaned duplicate on retry", + message_ts, + exc_info=True, + ) + try: + slack.chat_delete(channel=channel_id, ts=message_ts) + except Exception: + logger.warning( + "Could not roll back orphaned schedule post %s", + message_ts, + exc_info=True, + ) + raise return { "posted": True, "channel": channel_id, diff --git a/tests/weekly_post/test_handler.py b/tests/weekly_post/test_handler.py index b98c33a..82f31cc 100644 --- a/tests/weekly_post/test_handler.py +++ b/tests/weekly_post/test_handler.py @@ -83,24 +83,26 @@ def test_pay_email_failure_does_not_block_schedule_post( @freeze_time(MON_0800) def test_rolls_existing_post_forward(weeklypost_app, schedule, seed, slack, env): - # An existing post is edited in place (stable permalink), never deleted. + # An existing post is deleted + reposted so it lands at the bottom every + # Monday. The old ts is cleaned up and a new one is stored. seed.schedule_post("C_TEST", "111.111") result = weeklypost_app.handler({"force": True}, None) - slack.chat_delete.assert_not_called() - slack.chat_update.assert_called_once() - assert slack.chat_update.call_args.kwargs["ts"] == "111.111" - # The stored ts is unchanged, so the permalink stays stable. - assert result["message_ts"] == "111.111" - assert schedule.get_schedule_post("C_TEST")["message_ts"] == "111.111" + slack.chat_delete.assert_called_once() + assert slack.chat_delete.call_args.kwargs["ts"] == "111.111" + slack.chat_update.assert_not_called() + slack.chat_postMessage.assert_called_once() + # New ts from the repost is stored. + assert result["message_ts"] == "999.000" + assert schedule.get_schedule_post("C_TEST")["message_ts"] == "999.000" @freeze_time(MON_0800) -def test_reposts_when_update_fails(weeklypost_app, schedule, seed, slack, env): - # A stored post that can no longer be edited (e.g. deleted manually) falls - # back to a fresh post and re-saves the new ts. +def test_reposts_when_delete_fails(weeklypost_app, schedule, seed, slack, env): + # A stored post that can no longer be deleted (e.g. was already removed + # manually) still reposts — the delete failure is non-fatal. seed.schedule_post("C_TEST", "111.111") - slack.chat_update.side_effect = Exception("message_not_found") + slack.chat_delete.side_effect = Exception("message_not_found") result = weeklypost_app.handler({"force": True}, None) @@ -109,6 +111,29 @@ def test_reposts_when_update_fails(weeklypost_app, schedule, seed, slack, env): assert schedule.get_schedule_post("C_TEST")["message_ts"] == "999.000" +@freeze_time(MON_0800) +def test_rolls_back_repost_when_save_fails( + weeklypost_app, schedule, seed, slack, env, monkeypatch +): + # If persisting the fresh post's ts fails after the repost has already + # landed, the just-posted message is deleted so an async retry can't leave + # an orphaned duplicate. The error still propagates. + seed.schedule_post("C_TEST", "111.111") + monkeypatch.setattr( + weeklypost_app.ShiftSchedule, + "save_schedule_post", + MagicMock(side_effect=Exception("dynamo down")), + ) + + with pytest.raises(Exception, match="dynamo down"): + weeklypost_app.handler({"force": True}, None) + + # Old post deleted for the rollover, then the fresh (999.000) post rolled + # back when its ts couldn't be persisted. + deleted_ts = [c.kwargs["ts"] for c in slack.chat_delete.call_args_list] + assert deleted_ts == ["111.111", "999.000"] + + @freeze_time(MON_0800) def test_skips_when_not_7am_and_not_forced(weeklypost_app, schedule, slack, env): # Frozen hour is 08:00 ET, not 07:00 → skip unless forced.