fix(agent): stop scoped-session S3 hang in baseline build + reap hung suites + early ACK

agent/src/pipeline.py

@@ -842,28 +842,22 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
             if prompt_version:
                 os.environ["PROMPT_VERSION"] = prompt_version
 
-            # Setup repo (deterministic pre-hooks)
-            with task_span("task.repo_setup") as setup_span:
-                setup = setup_repo(config, progress=progress)
-                setup_span.set_attribute("build.before", setup.build_before)
-            progress.write_agent_milestone(
-                "repo_setup_complete",
-                f"branch={setup.branch} build_before={setup.build_before}",
-            )
-
-            system_prompt = build_system_prompt(config, setup, hc, system_prompt_overrides)
-
-            # Channel-specific MCP wiring. Must happen before
-            # discover_project_config so the scan picks up the file we just
-            # wrote. Resolve the per-channel access token from Secrets
-            # Manager *before* writing .mcp.json so the child SDK process
-            # inherits the env var that the MCP server entry references
-            # (${LINEAR_API_TOKEN} / ${JIRA_API_TOKEN}).
+            # ── Early ACK ────────────────────────────────────────────────────
+            # Acknowledge the task is picked up BEFORE the (potentially long)
+            # pre-agent baseline build in setup_repo(). On a large repo that
+            # baseline is minutes (up to the build-verify ceiling); posting the
+            # 👀 only *after* it left the issue looking dead for the whole phase
+            # (no reaction, comment, or state change). None of these calls needs
+            # the cloned repo — they act on the channel issue via its API token +
+            # issue id from channel metadata — so they belong before the build.
+            #
+            # Resolve the per-channel access token from Secrets Manager first
+            # (react_task_started/comment_task_started read the env var it sets).
+            # configure_channel_mcp DOES need setup.repo_dir, so it stays below.
             if config.channel_source == "linear":
                 resolve_linear_api_token(config.channel_metadata)
             elif config.channel_source == "jira":
                 resolve_jira_oauth_token(config.channel_metadata)
-            configure_channel_mcp(setup.repo_dir, config.channel_source)
 
             # 👀 on the Linear issue — acknowledges the task is picked up.
             # No-op for non-Linear tasks. Best-effort; failures are logged
@@ -883,13 +877,38 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
             )
 
             # Move the Jira card To Do → In Progress so the board reflects that
-            # work has started (issue #572). No-op for non-Jira tasks.
-            # Best-effort; failures are logged and never block the pipeline.
+            # work has started (issue #572). No-op for non-Jira tasks. Part of
+            # the early ACK (before setup_repo) so the board reflects "started"
+            # during the baseline build, not only after it. Best-effort; failures
+            # are logged and never block the pipeline.
             transition_task_started(
                 config.channel_source,
                 config.channel_metadata,
             )
 
+            # Setup repo (deterministic pre-hooks). A failure/timeout/OOM in the
+            # pre-agent baseline build raises here; it needs no local handler —
+            # the outer ``except Exception`` at the bottom of this ``try`` writes
+            # the task FAILED, swaps the 👀 (posted above) to ❌, and posts the
+            # failure comment. Posting the 👀 earlier is what makes the outer
+            # handler's ❌-swap actually visible for setup failures.
+            with task_span("task.repo_setup") as setup_span:
+                setup = setup_repo(config, progress=progress)
+                setup_span.set_attribute("build.before", setup.build_before)
+            progress.write_agent_milestone(
+                "repo_setup_complete",
+                f"branch={setup.branch} build_before={setup.build_before}",
+            )
+
+            system_prompt = build_system_prompt(config, setup, hc, system_prompt_overrides)
+
+            # Channel-specific MCP wiring. Must happen before
+            # discover_project_config so the scan picks up the file we just
+            # wrote — and after the clone, since it writes .mcp.json into the
+            # repo dir. (Token resolution + the 👀/start ACK moved earlier so
+            # the user gets immediate feedback; see the Early ACK block above.)
+            configure_channel_mcp(setup.repo_dir, config.channel_source)
+
             # Download attachments from S3 (version-pinned, integrity-verified)
             prepared_attachments: list = []
             if config.attachments:

agent/tests/conftest.py

@@ -1,11 +1,73 @@
 """Shared fixtures for agent unit tests."""
 
+import faulthandler
+import os
+import sys
+import threading
 from types import SimpleNamespace
 
 import pytest
 
 from models import TaskConfig
 
+# Session-wide hang backstop. SIGALRM (pytest-timeout method="signal") fires only
+# in the MAIN thread during a test's *call* phase, so a deadlock in a WORKER
+# thread, a fixture, collection, or a C-level socket read the main thread never
+# returns from stalls the whole `mise run build` silently — up to the platform's
+# 3600s build-verify ceiling (the ECS-only stall on ABCA-684/686/688, and the
+# scoped-session S3 hang the _clean_env reset below guards against: 40+ min of
+# dead air, container never reaped).
+#
+# The obvious instrument — ``faulthandler.dump_traceback_later(1200, exit=True)``
+# — does NOT work here: faulthandler has a SINGLE internal timer, and pytest's
+# ``faulthandler_timeout`` (pyproject.toml) RE-ARMS it at the start of every test
+# WITHOUT ``exit=True``. So a session-level exit timer is cancelled by the first
+# test, the per-test timer only DUMPS, and the suite hangs forever anyway.
+#
+# So own the reaper on a dedicated daemon thread pytest cannot touch. A blocked
+# socket read releases the GIL, so this thread runs; it dumps every thread's
+# stack for diagnosis and then HARD-EXITS the process, so `mise run build`
+# returns non-zero within seconds of the deadline instead of burning to the
+# ceiling. Deadline 600s: a SESSION backstop for the whole-suite hangs SIGALRM
+# can't interrupt — sized well above the longest healthy run (the suite normally
+# finishes in seconds; the per-test pytest-timeout cap is 120s, see
+# pyproject.toml) yet far under the 3600s build-verify ceiling.
+#
+# ``pytest_sessionfinish`` cancels the timer on a clean finish (below), so a
+# legitimately slow-but-passing run that lands near 600s — e.g. still in teardown
+# / coverage write — is NOT hard-exited into a bewildering red (#616 review N2).
+# ``os._exit`` skips atexit + buffer flush, so it must only fire on a TRUE hang.
+_HANG_REAP_DEADLINE_S = 600
+
+
+def _reap_on_hang() -> None:
+    faulthandler.dump_traceback(all_threads=True, file=sys.stderr)
+    print(
+        f"\nCONFTEST HANG WATCHDOG: test session exceeded {_HANG_REAP_DEADLINE_S}s "
+        "— dumped all thread stacks above and hard-exiting so the build fails "
+        "fast instead of stalling to the build-verify ceiling.",
+        file=sys.stderr,
+        flush=True,
+    )
+    os._exit(1)
+
+
+# daemon=True so a clean, fast suite exit is never blocked waiting on this timer.
+_hang_watchdog = threading.Timer(_HANG_REAP_DEADLINE_S, _reap_on_hang)
+_hang_watchdog.daemon = True
+_hang_watchdog.start()
+
+
+def pytest_sessionfinish(session, exitstatus):
+    """Cancel the hang watchdog on a clean session finish (#616 review N2).
+
+    Without this, a legitimately slow-but-passing suite that finishes just after
+    the 600s deadline (e.g. during teardown / coverage write) would be hard-exited
+    by ``_reap_on_hang`` and turn green red with a thread-dump uncorrelated to any
+    failed test. ``Timer.cancel()`` is a no-op if the timer already fired (a true
+    hang), so this only prevents the false-positive kill."""
+    _hang_watchdog.cancel()
+
 
 class FakeRunCmd:
     """Shared fake for ``shell.run_cmd``: records argv and returns scripted results.
@@ -94,11 +156,34 @@ def make_task_config(**overrides) -> TaskConfig:
     "LOG_GROUP_NAME",
     "MEMORY_ID",
     "ENABLE_CLI_TELEMETRY",
+    # Per-session IAM scoping (PR #209) — the scoped-session S3-hang guard.
+    # See the ``_clean_env`` docstring below for the full rationale (why the
+    # env strip AND the cache reset are both required).
+    "AGENT_SESSION_ROLE_ARN",
 ]
 
 
 @pytest.fixture(autouse=True)
 def _clean_env(monkeypatch):
-    """Remove agent-related env vars before every test."""
+    """Remove agent-related env vars and reset the AWS session cache each test.
+
+    The env cleanup + session reset TOGETHER close a scoped-session leak that
+    hangs the suite on the ECS substrate: ``aws_session`` caches the resolved
+    boto3 session in a MODULE GLOBAL (``_session``/``_scoped``), and
+    ``tenant_client`` returns ``session.client(...)`` when ``_scoped`` is True —
+    bypassing a downstream ``@patch("boto3.client")``. Two things make a test
+    resolve *scoped*: a stale cached session (fixed by ``reset_session_cache``),
+    OR ``AGENT_SESSION_ROLE_ARN`` still being set when the cache is cold (fixed by
+    stripping it in ``_AGENT_ENV_VARS`` above — the ECS task def sets it, so on
+    that substrate the reset alone re-resolves scoped and the leak persists). With
+    the var gone AND the cache reset, every test resolves the unscoped path where
+    its ``boto3.client`` mock intercepts. Otherwise a mocked test (e.g.
+    ``test_attachments``) makes a REAL S3 call that hangs on the ECS network
+    (no egress) in a socket read SIGALRM can't interrupt.
+    """
     for var in _AGENT_ENV_VARS:
         monkeypatch.delenv(var, raising=False)
+
+    from aws_session import reset_session_cache
+
+    reset_session_cache()

agent/tests/test_aws_session.py

@@ -79,6 +79,15 @@ def test_blank_role_arn_treated_as_unset(self, monkeypatch):
         assert is_scoped() is False
 
 
+# NOTE: the conftest-scrub regression guard for AGENT_SESSION_ROLE_ARN lives in
+# tests/test_conftest_env_scrub.py — deliberately in its OWN module with NO local
+# autouse fixture. This file's `_reset` fixture (above) itself does
+# `monkeypatch.delenv(SESSION_ROLE_ARN_ENV)`, which would MASK whether conftest's
+# `_clean_env` performs the scrub — a guard placed here passes even if the
+# conftest line is deleted (#616 review B1). Only a fixture-free module truly
+# guards the fix.
+
+
 # ---------------------------------------------------------------------------
 # Scoped: SessionRole ARN set → refreshable assumed-role session
 # ---------------------------------------------------------------------------

agent/tests/test_conftest_env_scrub.py

@@ -0,0 +1,50 @@
+"""Fixture-free regression guard for the ECS test-hang scrub (#615 / #616 B1).
+
+This module deliberately has NO local autouse fixture. It exists to prove that
+conftest's ``_clean_env`` autouse fixture strips ``AGENT_SESSION_ROLE_ARN`` from
+the environment before each test — the exact line the #615 fix adds to
+``_AGENT_ENV_VARS``.
+
+Why a separate module: ``test_aws_session.py`` has its OWN autouse ``_reset``
+fixture that does ``monkeypatch.delenv(SESSION_ROLE_ARN_ENV)``, so a guard placed
+there passes even if the conftest scrub is deleted (it asserts what the local
+fixture guarantees, not what the fix does — #616 review B1). Here, only conftest's
+``_clean_env`` is in play.
+
+Why set the var at MODULE IMPORT time (not in the test body): pytest autouse
+fixtures run during test *setup*, before the body executes — so a
+``monkeypatch.setenv`` inside the test can't be scrubbed by them. Poisoning the
+process env at import time (before any fixture runs) means the conftest scrub is
+the only thing that can clear it by the time a test body reads it. Remove the
+``AGENT_SESSION_ROLE_ARN`` line from conftest's ``_AGENT_ENV_VARS`` and BOTH tests
+below fail — a true bidirectional guard.
+
+The bug this guards: with ``AGENT_SESSION_ROLE_ARN`` set, ``aws_session``
+resolves a *scoped* session and ``tenant_client`` returns ``session.client(...)``,
+bypassing a ``@patch("boto3.client")`` mock. A mocked test (e.g.
+``test_attachments``) then makes a REAL S3 call that hangs forever on the ECS
+network (no egress) in a socket read SIGALRM can't interrupt — stalling the whole
+``mise run build``.
+"""
+
+import os
+
+from aws_session import SESSION_ROLE_ARN_ENV, is_scoped
+
+# Poison the process env at import time — BEFORE any fixture runs. The conftest
+# `_clean_env` autouse must scrub this for the assertions below to hold.
+os.environ[SESSION_ROLE_ARN_ENV] = "arn:aws:iam::123456789012:role/leaked-from-parent-env"
+
+
+def test_conftest_scrubs_session_role_arn():
+    # If conftest's _clean_env strips AGENT_SESSION_ROLE_ARN, it is gone here even
+    # though the module poisoned it at import time. If the scrub line is removed,
+    # this fails — the guard the #615 fix actually needs.
+    assert os.environ.get(SESSION_ROLE_ARN_ENV) is None
+
+
+def test_session_resolves_unscoped_when_scrubbed():
+    # With the var scrubbed (and the cache reset, both by conftest _clean_env) a
+    # bare get_session() must be unscoped — the path where boto3.client mocks
+    # intercept, so no real S3 call is made and nothing hangs.
+    assert is_scoped() is False