fix: delete+repost schedule on weekly rollover for bottom placement (#167)

* fix: delete+repost schedule on weekly rollover for bottom placement

The Monday rollover was chat_update-ing in place, which only refreshes
content without moving the message to the bottom. Now it chat_deletes
the old post and chat_postMessages a fresh one so the schedule lands
at the bottom every Monday, independent of in-week activity.

Also added info-level logging to the bump handler silent return paths
so skipped bumps are observable at runtime.

* fix: roll back weekly repost when its ts can't be persisted

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.

---------

Co-authored-by: seahaven-openswe[bot] <296972425+seahaven-openswe[bot]@users.noreply.github.com>
Co-authored-by: Adam Moussa <adam@seahavenind.com>

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,
+- **Delete + repost on Monday** — the previous week's stored post is
-  `chat_update` the stored post to roll the two-week window forward, keeping the
+  `chat_delete`d and a fresh schedule is `chat_postMessage`d so the message
-  same `ts` (the activity bump is what moves it to the bottom; the rollover just
+  lands at the bottom of the channel every Monday regardless of in-week
-  refreshes content).
+  activity. The delete failure is non-fatal; the handler always reposts.
-- **First-run / recovery fallback:** when there is no stored post or the
+- **First-run:** when there is no stored post to delete, the handler
-  `chat_update` fails (e.g. the message was deleted manually), `chat_postMessage`
+  `chat_postMessage`s a fresh message and stores its `ts`.
-  a fresh message and store 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
+- **Monday rollover** — `chat_delete`s the old stored `ts`, `chat_postMessage`s
-  message), clearing `last_bump_ts` so the next activity is free to bump.
+  a fresh post at the bottom, and saves the new `ts` (clearing `last_bump_ts` so
-- **In-week shift edits** — `_refresh_schedule_post` `chat_update`s the same
+  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
+Both the rollover and the bump mint a new `ts` on repost and re-save it
-immediately), the other two paths always read the current `ts` from the record.
+immediately, so `_refresh_schedule_post` always reads the current `ts` from the
+record.
 
 ## Scope / manifest impact
 

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

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
+    # Delete the previous week's schedule post + repost a fresh one so the
-    # (same ts) so the Monday rollover, in-week shift edits, and the activity
+    # message lands at the bottom of the channel every Monday. The activity
-    # bump all converge on a single stored ts. Fall back to a fresh post when
+    # bump (handle_channel_message) handles in-week bottom-stickiness; this
-    # there is no stored message or the edit fails (e.g. it was deleted).
+    # 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(
+            slack.chat_delete(channel=channel_id, ts=old_post["message_ts"])
-                channel=channel_id,
-                ts=old_post["message_ts"],
-                blocks=blocks,
-                text=fallback_text,
-            )
-            message_ts = old_post["message_ts"]
             logger.info(
-                "Rolled schedule post %s forward to week of %s",
+                "Deleted previous schedule post %s for weekly rollover",
-                message_ts,
+                old_post["message_ts"],
-                week_start,
             )
         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(
-        result = slack.chat_postMessage(
+        channel=channel_id, blocks=blocks, text=fallback_text
-            channel=channel_id, blocks=blocks, text=fallback_text
+    )
-        )
+    message_ts = result["ts"]
-        message_ts = result["ts"]
+    logger.info(
-        logger.info(
+        "Posted new weekly schedule to channel %s (ts=%s)", channel_id, message_ts
-            "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,

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_delete.assert_called_once()
-    slack.chat_update.assert_called_once()
+    assert slack.chat_delete.call_args.kwargs["ts"] == "111.111"
-    assert slack.chat_update.call_args.kwargs["ts"] == "111.111"
+    slack.chat_update.assert_not_called()
-    # The stored ts is unchanged, so the permalink stays stable.
+    slack.chat_postMessage.assert_called_once()
-    assert result["message_ts"] == "111.111"
+    # New ts from the repost is stored.
-    assert schedule.get_schedule_post("C_TEST")["message_ts"] == "111.111"
+    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):
+def test_reposts_when_delete_fails(weeklypost_app, schedule, seed, slack, env):
-    # A stored post that can no longer be edited (e.g. deleted manually) falls
+    # A stored post that can no longer be deleted (e.g. was already removed
-    # back to a fresh post and re-saves the new ts.
+    # 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.