From 54501099e53baac044d6b564031f7f3876278392 Mon Sep 17 00:00:00 2001
From: Adam Moussa <adam@seahavenind.com>
Date: Fri, 10 Jul 2026 16:28:01 -0400
Subject: [PATCH] fix: roll back weekly repost when its ts can't be persisted
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The Monday rollover deletes the old post then reposts a fresh one, but only
saved the new ts as its last step. If the save failed (or the Lambda died)
after the post landed, the async retry would read the stale, already-deleted
ts, no-op its delete, and post a second schedule — orphaning the first at the
bottom of the channel.

Wrap the save so a failure after a successful repost best-effort deletes the
fresh message before re-raising, letting the retry start clean. Mirrors the
orphan-avoidance the activity bump already has.
---
 docs/sticky-schedule-post.md      |  4 ++++
 src/weekly-post/app.py            | 24 +++++++++++++++++++++++-
 tests/weekly_post/test_handler.py | 23 +++++++++++++++++++++++
 3 files changed, 50 insertions(+), 1 deletion(-)

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.