diff --git a/docs/sticky-schedule-post.md b/docs/sticky-schedule-post.md index a4978d6..f14c6a5 100644 --- a/docs/sticky-schedule-post.md +++ b/docs/sticky-schedule-post.md @@ -86,6 +86,10 @@ is therefore not a substitute for bottom-stickiness. 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. diff --git a/src/weekly-post/app.py b/src/weekly-post/app.py index c0c9ed7..45d5744 100644 --- a/src/weekly-post/app.py +++ b/src/weekly-post/app.py @@ -344,7 +344,29 @@ def handler(event, context): "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 cb141c7..82f31cc 100644 --- a/tests/weekly_post/test_handler.py +++ b/tests/weekly_post/test_handler.py @@ -111,6 +111,29 @@ def test_reposts_when_delete_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.