Merge pull request #51 from PostHog/tom/race-condition

Clear permission set list immediately on account change

Author: Piccirello

src/main.py

@@ -5,6 +5,7 @@
 import jmespath as jp
 from slack_bolt import Ack, App, BoltContext
 from slack_bolt.adapter.aws_lambda import SlackRequestHandler
+import slack_sdk.errors
 from slack_sdk import WebClient
 from slack_sdk.web.slack_response import SlackResponse
 
@@ -808,7 +809,7 @@ def _get_cached_user_info(view_key: str, user_id: str, client: "WebClient") -> t
 
 
 @app.action(slack_helpers.RequestForAccessView.ACCOUNT_ACTION_ID)
-def handle_account_selection(ack: Ack, body: dict, client: WebClient) -> SlackResponse:
+def handle_account_selection(ack: Ack, body: dict, client: WebClient) -> SlackResponse | None:
     ack()
     logger.info("Handling account selection")
 
@@ -824,11 +825,35 @@ def handle_account_selection(ack: Ack, body: dict, client: WebClient) -> SlackRe
     logger.info(f"Selected accounts: {account_ids}")
 
     view_id = body["view"]["id"]
+    view_hash = body["view"].get("hash")
+
+    def safe_views_update(view) -> SlackResponse | None:  # noqa: ANN001, ANN202
+        nonlocal view_hash
+        try:
+            response = client.views_update(view_id=view_id, view=view, hash=view_hash)
+        except slack_sdk.errors.SlackApiError as e:
+            error = e.response.get("error") if e.response else None
+            # hash_conflict / view_expired = a newer handler already updated the view; skip silently.
+            if error in {"hash_conflict", "view_expired", "not_found"}:
+                logger.info(f"Skipping stale views_update: {error}")
+                return None
+            raise
+        new_hash = response.data.get("view", {}).get("hash") if response.data else None  # type: ignore[union-attr]
+        if new_hash:
+            view_hash = new_hash
+        return response
 
     # No accounts selected → show placeholder, disable submit
     if not account_ids:
         updated_view = slack_helpers.RequestForAccessView.build_no_permission_sets_view(view_blocks=body["view"]["blocks"])
-        return client.views_update(view_id=view_id, view=updated_view)
+        return safe_views_update(updated_view)
+
+    # Immediately replace stale permission set list with a loading placeholder before any AWS calls.
+    loading_response = safe_views_update(slack_helpers.RequestForAccessView.show_permission_set_loading(body["view"]["blocks"]))
+    if loading_response is None:
+        # A newer handler is already in flight; let it produce the final view.
+        return None
+    current_blocks = loading_response.data["view"]["blocks"]  # type: ignore[index]
 
     # Get cached user info, re-fetching from Identity Center on cache miss
     # (e.g. when Lambda container was recycled between form load and account selection)
@@ -842,8 +867,8 @@ def handle_account_selection(ack: Ack, body: dict, client: WebClient) -> SlackRe
     logger.info(f"Valid permission sets for selected accounts and user: {valid_ps_names}")
 
     if not valid_ps_names:
-        updated_view = slack_helpers.RequestForAccessView.build_no_permission_sets_view(view_blocks=body["view"]["blocks"])
-        return client.views_update(view_id=view_id, view=updated_view)
+        updated_view = slack_helpers.RequestForAccessView.build_no_permission_sets_view(view_blocks=current_blocks)
+        return safe_views_update(updated_view)
 
     if "*" in valid_ps_names:
         permission_sets = sso.get_permission_sets_from_config_with_cache(sso_client=sso_client, s3_client=s3_client, cfg=cfg)
@@ -853,8 +878,8 @@ def handle_account_selection(ack: Ack, body: dict, client: WebClient) -> SlackRe
 
     # Handle case where filtered list is empty (configured names don't exist in SSO)
     if not permission_sets:
-        updated_view = slack_helpers.RequestForAccessView.build_no_permission_sets_view(view_blocks=body["view"]["blocks"])
-        return client.views_update(view_id=view_id, view=updated_view)
+        updated_view = slack_helpers.RequestForAccessView.build_no_permission_sets_view(view_blocks=current_blocks)
+        return safe_views_update(updated_view)
 
     # Classify permission sets as auto-approved vs requires-approval
     auto_approved_arns: set[str] | None = None
@@ -882,12 +907,12 @@ def approver_group_resolver(group_ids: frozenset[str]) -> set[str]:
         )
 
     updated_view = slack_helpers.RequestForAccessView.update_with_permission_sets(
-        view_blocks=body["view"]["blocks"],
+        view_blocks=current_blocks,
         permission_sets=permission_sets,
         display_names=cfg.permission_set_display_names,
         auto_approved_arns=auto_approved_arns,
     )
-    return client.views_update(view_id=view_id, view=updated_view)
+    return safe_views_update(updated_view)
 
 
 # Early Revoke Handlers

src/slack_helpers.py

@@ -63,6 +63,7 @@ class RequestForAccessView:
 
     LOADING_BLOCK_ID = "loading"
     PERMISSION_SET_PLACEHOLDER_BLOCK_ID = "permission_set_placeholder"
+    PERMISSION_SET_LOADING_BLOCK_ID = "permission_set_loading"
 
     @classmethod
     def build(cls) -> View:
@@ -205,6 +206,39 @@ def build_permission_set_placeholder_block(cls) -> InputBlock:
             ),
         )
 
+    @classmethod
+    def build_permission_set_loading_block(cls) -> InputBlock:
+        return InputBlock(
+            block_id=cls.PERMISSION_SET_LOADING_BLOCK_ID,
+            label=PlainTextObject(text="Permission set"),
+            element=StaticSelectElement(
+                action_id=cls.PERMISSION_SET_ACTION_ID + "_loading",
+                placeholder=PlainTextObject(text=":hourglass: Loading permission sets..."),
+                options=[Option(text=PlainTextObject(text="—"), value="_loading")],
+            ),
+        )
+
+    @classmethod
+    def show_permission_set_loading(cls, view_blocks: list) -> View:
+        view = cls.build()
+        view.submit_disabled = True  # type: ignore[attr-defined]
+        blocks = remove_blocks(
+            view_blocks,
+            block_ids=[
+                cls.PERMISSION_SET_PLACEHOLDER_BLOCK_ID,
+                cls.PERMISSION_SET_BLOCK_ID,
+                cls.PERMISSION_SET_LOADING_BLOCK_ID,
+                "no_eligible_accounts",
+            ],
+        )
+        blocks = insert_blocks(
+            blocks=blocks,
+            blocks_to_insert=[cls.build_permission_set_loading_block()],
+            after_block_id=cls.ACCOUNT_BLOCK_ID,
+        )
+        view.blocks = blocks
+        return view
+
     @classmethod
     def update_with_accounts(cls, accounts: list[entities.aws.Account]) -> View:
         view = cls.build()
@@ -230,7 +264,10 @@ def update_with_permission_sets(
         view = cls.build()
         view.submit_disabled = False  # type: ignore[attr-defined]
         # Start from the current blocks, remove placeholder
-        blocks = remove_blocks(view_blocks, block_ids=[cls.PERMISSION_SET_PLACEHOLDER_BLOCK_ID, cls.PERMISSION_SET_BLOCK_ID])
+        blocks = remove_blocks(
+            view_blocks,
+            block_ids=[cls.PERMISSION_SET_PLACEHOLDER_BLOCK_ID, cls.PERMISSION_SET_BLOCK_ID, cls.PERMISSION_SET_LOADING_BLOCK_ID],
+        )
         # Insert permission set dropdown after account dropdown
         blocks = insert_blocks(
             blocks=blocks,
@@ -288,7 +325,7 @@ def build_no_permission_sets_view(cls, view_blocks: list) -> View:
         view.submit_disabled = True  # type: ignore[attr-defined]
         blocks = remove_blocks(
             view_blocks,
-            block_ids=[cls.PERMISSION_SET_PLACEHOLDER_BLOCK_ID, cls.PERMISSION_SET_BLOCK_ID],
+            block_ids=[cls.PERMISSION_SET_PLACEHOLDER_BLOCK_ID, cls.PERMISSION_SET_BLOCK_ID, cls.PERMISSION_SET_LOADING_BLOCK_ID],
         )
         blocks = insert_blocks(
             blocks=blocks,