Skip to content

Commit 8e31ac1

Browse files
committed
fix(agent): kill ECS baseline-build hang + early Linear ACK (ABCA-707)
A Linear task against the full ABCA monorepo wedged silently for 50+ min: the pre-agent baseline build (mise run build → the agent pytest suite) hung on the ECS substrate, and the issue showed no reaction/comment/state the whole time. Root-caused live on ABCA-707. Three fixes: 1. Kill the hang at its root (agent/tests/conftest.py). The ECS agent task def sets AGENT_SESSION_ROLE_ARN. With it set, aws_session resolves a *scoped* session and tenant_client returns session.client(...), which BYPASSES a @patch("boto3.client") mock. test_attachments then makes a REAL S3 get_object that blocks forever on the ECS network (no egress) in a socket read SIGALRM can't interrupt. Add AGENT_SESSION_ROLE_ARN to the _clean_env scrub list so every test resolves the unscoped path where the mock intercepts. reset_session_cache() alone was insufficient — a cold get_session() re-resolves scoped while the var is still set. + regression test (TestConftestScrubsScopingEnv). 2. Make the hang watchdog actually reap (agent/tests/conftest.py). The prior faulthandler.dump_traceback_later(1200, exit=True) never fired: pytest's faulthandler_timeout re-arms faulthandler's single timer per-test WITHOUT exit=True, cancelling the session-level exit timer. Replace with an independent daemon-thread reaper that dumps all stacks and os._exit(1)s at 600s, so a future hang fails the build fast instead of burning to the 3600s ceiling. 3. Early Linear ACK (agent/src/pipeline.py). Move token resolve + 👀 reaction + Backlog→In Progress + start comment to BEFORE setup_repo() so a large-repo task shows immediate feedback during the multi-minute baseline build instead of looking dead. As a side benefit, a setup-phase failure now has a 👀 for the existing outer crash handler to swap to ❌ — closing the silent-stuck-issue gap. configure_channel_mcp stays after the clone (it needs the repo dir).
1 parent 4466a02 commit 8e31ac1

3 files changed

Lines changed: 132 additions & 41 deletions

File tree

agent/src/pipeline.py

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -953,28 +953,23 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
953953
if prompt_version:
954954
os.environ["PROMPT_VERSION"] = prompt_version
955955

956-
# Setup repo (deterministic pre-hooks)
957-
with task_span("task.repo_setup") as setup_span:
958-
setup = setup_repo(config, progress=progress)
959-
setup_span.set_attribute("build.before", setup.build_before)
960-
progress.write_agent_milestone(
961-
"repo_setup_complete",
962-
f"branch={setup.branch} build_before={setup.build_before}",
963-
)
964-
965-
system_prompt = build_system_prompt(config, setup, hc, system_prompt_overrides)
966-
967-
# Channel-specific MCP wiring. Must happen before
968-
# discover_project_config so the scan picks up the file we just
969-
# wrote. Resolve the per-channel access token from Secrets
970-
# Manager *before* writing .mcp.json so the child SDK process
971-
# inherits the env var that the MCP server entry references
972-
# (${LINEAR_API_TOKEN} / ${JIRA_API_TOKEN}).
956+
# ── Early ACK (ABCA-707) ─────────────────────────────────────────
957+
# Acknowledge the task is picked up BEFORE the (potentially long)
958+
# pre-agent baseline build in setup_repo(). On a large repo that
959+
# baseline is minutes (up to the build-verify ceiling); posting the
960+
# 👀 only *after* it left the issue looking dead for the whole phase
961+
# (the ABCA-707 symptom: no reaction, comment, or state change for
962+
# 30+ min). None of these calls needs the cloned repo — they act on
963+
# the channel issue via its API token + issue id from channel
964+
# metadata — so they belong before the clone/build.
965+
#
966+
# Resolve the per-channel access token from Secrets Manager first
967+
# (react_task_started/comment_task_started read the env var it sets).
968+
# configure_channel_mcp DOES need setup.repo_dir, so it stays below.
973969
if config.channel_source == "linear":
974970
resolve_linear_api_token(config.channel_metadata)
975971
elif config.channel_source == "jira":
976972
resolve_jira_oauth_token(config.channel_metadata)
977-
configure_channel_mcp(setup.repo_dir, config.channel_source)
978973

979974
# 👀 on the Linear issue — acknowledges the task is picked up.
980975
# No-op for non-Linear tasks. Best-effort; failures are logged
@@ -1000,6 +995,31 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
1000995
config.channel_metadata,
1001996
)
1002997

998+
# Setup repo (deterministic pre-hooks). A failure/timeout/OOM in the
999+
# pre-agent baseline build raises here; it needs no local handler —
1000+
# the outer ``except Exception`` at the bottom of this ``try`` writes
1001+
# the task FAILED, swaps the 👀 (posted above) to ❌, and posts the
1002+
# failure comment. Before the Early-ACK move the 👀 didn't exist yet
1003+
# at this point, so a setup failure left the issue silently stuck
1004+
# (the ABCA-707 symptom); posting the 👀 earlier is what makes the
1005+
# outer handler's ❌-swap actually visible for setup failures.
1006+
with task_span("task.repo_setup") as setup_span:
1007+
setup = setup_repo(config, progress=progress)
1008+
setup_span.set_attribute("build.before", setup.build_before)
1009+
progress.write_agent_milestone(
1010+
"repo_setup_complete",
1011+
f"branch={setup.branch} build_before={setup.build_before}",
1012+
)
1013+
1014+
system_prompt = build_system_prompt(config, setup, hc, system_prompt_overrides)
1015+
1016+
# Channel-specific MCP wiring. Must happen before
1017+
# discover_project_config so the scan picks up the file we just
1018+
# wrote — and after the clone, since it writes .mcp.json into the
1019+
# repo dir. (Token resolution + the 👀/start ACK moved earlier so
1020+
# the user gets immediate feedback; see the Early ACK block above.)
1021+
configure_channel_mcp(setup.repo_dir, config.channel_source)
1022+
10031023
# Download attachments from S3 (version-pinned, integrity-verified)
10041024
prepared_attachments: list = []
10051025
if config.attachments:

agent/tests/conftest.py

Lines changed: 65 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,55 @@
11
"""Shared fixtures for agent unit tests."""
22

33
import faulthandler
4+
import os
45
import sys
6+
import threading
57
from types import SimpleNamespace
68

79
import pytest
810

911
from models import TaskConfig
1012

11-
# Session-wide hang backstop, immune to what pytest-timeout's SIGALRM cannot
12-
# reach. SIGALRM fires only in the MAIN thread during a test's *call* phase, so a
13-
# deadlock in a WORKER thread, a fixture, collection, or a C-level socket read the
14-
# main thread never returns from stalls the whole `mise run build` silently — up
15-
# to the platform's 3600s build-verify ceiling (the ECS-only stall seen on
16-
# ABCA-684/686/688 and the warm-cache run: 53 min of dead air, signal never
17-
# fired). faulthandler runs a dedicated C watchdog thread that the GIL and blocked
18-
# syscalls can't stall: at 1200s (20 min — comfortably inside the 3600s ceiling,
19-
# far above the whole suite's normal ~3 min) it dumps EVERY thread's Python stack
20-
# to stderr and hard-exits, converting a blind stall into a self-diagnosing crash
21-
# that names the exact file:line. Complements the per-test faulthandler_timeout in
22-
# pyproject.toml (which dumps but does not exit). ``exit=True`` guarantees the
23-
# non-zero exit even if the hang is uninterruptible.
24-
faulthandler.dump_traceback_later(1200, exit=True, file=sys.stderr)
13+
# Session-wide hang backstop. SIGALRM (pytest-timeout method="signal") fires only
14+
# in the MAIN thread during a test's *call* phase, so a deadlock in a WORKER
15+
# thread, a fixture, collection, or a C-level socket read the main thread never
16+
# returns from stalls the whole `mise run build` silently — up to the platform's
17+
# 3600s build-verify ceiling (the ECS-only stall on ABCA-684/686/688, and again
18+
# on ABCA-707: 40+ min of dead air, container never reaped).
19+
#
20+
# The obvious instrument — ``faulthandler.dump_traceback_later(1200, exit=True)``
21+
# — does NOT work here: faulthandler has a SINGLE internal timer, and pytest's
22+
# ``faulthandler_timeout`` (pyproject.toml) RE-ARMS it at the start of every test
23+
# WITHOUT ``exit=True``. So a session-level exit timer is cancelled by the first
24+
# test, the per-test timer only DUMPS, and the suite hangs forever anyway
25+
# (exactly what happened on ABCA-707: a 300s dump fired, no exit followed).
26+
#
27+
# So own the reaper on a dedicated daemon thread pytest cannot touch. A blocked
28+
# socket read (the ABCA-707 failure mode) releases the GIL, so this thread runs;
29+
# it dumps every thread's stack for diagnosis and then HARD-EXITS the process, so
30+
# `mise run build` returns non-zero within seconds of the deadline instead of
31+
# burning to the 3600s ceiling. Deadline 600s: ~200x the whole suite's normal
32+
# ~3s… well above any legitimate run (per-test cap is 300s) yet far under the
33+
# build ceiling.
34+
_HANG_REAP_DEADLINE_S = 600
35+
36+
37+
def _reap_on_hang() -> None:
38+
faulthandler.dump_traceback(all_threads=True, file=sys.stderr)
39+
print(
40+
f"\nCONFTEST HANG WATCHDOG: test session exceeded {_HANG_REAP_DEADLINE_S}s "
41+
"— dumped all thread stacks above and hard-exiting so the build fails "
42+
"fast instead of stalling to the build-verify ceiling.",
43+
file=sys.stderr,
44+
flush=True,
45+
)
46+
os._exit(1)
47+
48+
49+
# daemon=True so a clean, fast suite exit is never blocked waiting on this timer.
50+
_hang_watchdog = threading.Timer(_HANG_REAP_DEADLINE_S, _reap_on_hang)
51+
_hang_watchdog.daemon = True
52+
_hang_watchdog.start()
2553

2654

2755
class FakeRunCmd:
@@ -111,6 +139,16 @@ def make_task_config(**overrides) -> TaskConfig:
111139
"LOG_GROUP_NAME",
112140
"MEMORY_ID",
113141
"ENABLE_CLI_TELEMETRY",
142+
# Per-session IAM scoping (PR #209). When this is set, ``aws_session`` resolves
143+
# a *scoped* session and ``tenant_client`` returns ``session.client(...)`` —
144+
# which BYPASSES a ``@patch("boto3.client")`` mock. On the ECS substrate the
145+
# task def sets this, so a test that mocks ``boto3.client`` (e.g.
146+
# ``test_attachments``) instead makes a REAL S3 call that hangs on the network
147+
# (no egress). Stripping it here forces every test onto the unscoped path,
148+
# where the ``boto3.client`` mock actually intercepts. Resetting the session
149+
# cache below is NOT enough on its own — a fresh ``get_session()`` re-resolves
150+
# as scoped while this var is still set. (Live-caught on ABCA-707, 2026-07-15.)
151+
"AGENT_SESSION_ROLE_ARN",
114152
]
115153

116154

@@ -121,15 +159,19 @@ def _clean_env(monkeypatch):
121159
The env cleanup keeps host state from leaking into agent code that reads
122160
``os.environ`` at import/call time.
123161
124-
The session reset closes a cross-test leak: ``aws_session`` caches the
125-
resolved boto3 session in a MODULE GLOBAL (``_session``/``_scoped``), and
126-
``tenant_client`` returns ``session.client(...)`` from that cache when
127-
``_scoped`` is True — bypassing a downstream ``@patch("boto3.client")``. If an
128-
earlier test left a scoped session cached (only ``test_aws_session`` cleans up
129-
after itself), a later test like ``test_attachments`` would get a REAL client
130-
despite its mock → a live S3 call that hangs on the ECS network (no route/no
131-
creds) in a socket read SIGALRM can't interrupt. Resetting the cache before
132-
every test guarantees each test resolves the session under its own patches.
162+
The env cleanup + session reset TOGETHER close a scoped-session leak that
163+
hangs the suite on the ECS substrate: ``aws_session`` caches the resolved
164+
boto3 session in a MODULE GLOBAL (``_session``/``_scoped``), and
165+
``tenant_client`` returns ``session.client(...)`` when ``_scoped`` is True —
166+
bypassing a downstream ``@patch("boto3.client")``. Two things make a test
167+
resolve *scoped*: a stale cached session (fixed by ``reset_session_cache``),
168+
OR ``AGENT_SESSION_ROLE_ARN`` still being set when the cache is cold (fixed by
169+
stripping it in ``_AGENT_ENV_VARS`` above — the ECS task def sets it, so on
170+
that substrate the reset alone re-resolves scoped and the leak persists). With
171+
the var gone AND the cache reset, every test resolves the unscoped path where
172+
its ``boto3.client`` mock intercepts. Otherwise a mocked test (e.g.
173+
``test_attachments``) makes a REAL S3 call that hangs on the ECS network
174+
(no egress) in a socket read SIGALRM can't interrupt. (Live-caught ABCA-707.)
133175
"""
134176
for var in _AGENT_ENV_VARS:
135177
monkeypatch.delenv(var, raising=False)

agent/tests/test_aws_session.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,35 @@ def test_blank_role_arn_treated_as_unset(self, monkeypatch):
7979
assert is_scoped() is False
8080

8181

82+
class TestConftestScrubsScopingEnv:
83+
"""Regression guard for the ECS test-hang (ABCA-707).
84+
85+
The bug: the ECS agent task def sets ``AGENT_SESSION_ROLE_ARN``. If that var
86+
leaks into the test process, ``get_session()`` resolves a *scoped* session and
87+
``tenant_client`` returns ``session.client(...)`` — bypassing a
88+
``@patch("boto3.client")`` mock. A mocked test (e.g. ``test_attachments``)
89+
then makes a REAL S3 call that hangs on the ECS network (no egress) in a
90+
socket read SIGALRM can't interrupt → the whole ``mise run build`` stalls
91+
silently for 40+ min. The ``_clean_env`` autouse fixture in conftest MUST
92+
strip the var (resetting the session cache alone is not enough — a cold
93+
``get_session()`` re-resolves scoped while the var is still set).
94+
"""
95+
96+
def test_session_role_arn_not_visible_to_tests(self):
97+
import os
98+
99+
# No monkeypatch here: this asserts the AUTOUSE fixture already scrubbed
100+
# the var, even if the parent (ECS) environment had it set.
101+
assert os.environ.get(SESSION_ROLE_ARN_ENV) is None
102+
103+
def test_session_resolves_unscoped_by_default(self):
104+
# With the var scrubbed and the cache reset (both by _clean_env), a bare
105+
# get_session() must be unscoped — the path where boto3.client mocks work.
106+
with patch("boto3.Session", return_value=MagicMock()):
107+
get_session()
108+
assert is_scoped() is False
109+
110+
82111
# ---------------------------------------------------------------------------
83112
# Scoped: SessionRole ARN set → refreshable assumed-role session
84113
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)