diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index 191fd7ee..dfe97463 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -823,28 +823,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 @@ -863,6 +857,29 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: 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: diff --git a/agent/tests/conftest.py b/agent/tests/conftest.py index da43271c..334dd4a4 100644 --- a/agent/tests/conftest.py +++ b/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,40 @@ def make_task_config(**overrides) -> TaskConfig: "LOG_GROUP_NAME", "MEMORY_ID", "ENABLE_CLI_TELEMETRY", + # Per-session IAM scoping (PR #209). When this is set, ``aws_session`` resolves + # a *scoped* session and ``tenant_client`` returns ``session.client(...)`` — + # which BYPASSES a ``@patch("boto3.client")`` mock. On the ECS substrate the + # task def sets this, so a test that mocks ``boto3.client`` (e.g. + # ``test_attachments``) instead makes a REAL S3 call that hangs on the network + # (no egress). Stripping it here forces every test onto the unscoped path, + # where the ``boto3.client`` mock actually intercepts. Resetting the session + # cache below is NOT enough on its own — a fresh ``get_session()`` re-resolves + # as scoped while this var is still set. + "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() diff --git a/agent/tests/test_aws_session.py b/agent/tests/test_aws_session.py index 96578d4d..c57b1e23 100644 --- a/agent/tests/test_aws_session.py +++ b/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 # --------------------------------------------------------------------------- diff --git a/agent/tests/test_conftest_env_scrub.py b/agent/tests/test_conftest_env_scrub.py new file mode 100644 index 00000000..9c515b98 --- /dev/null +++ b/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