aws-samples / aws-samples/sample-autonomous-cloud-coding-agents

bug(agent): test_server.py leaks live pipeline threads — reset fixture clears _active_threads without joining, causing cross-test mock contamination

Open
#841 0 comments 0 reactions 0 assignees View on GitHub
agent-runtime bug
Dominant language
TypeScript
Stars
143
Forks
46
Avg merge
3d 9h
Merged PRs (30d)
20

Description

## Problem

`agent/tests/test_server.py` intermittently fails with a mock being called by a test that never made the call. Observed twice on PR #680 (a **CLI-only** PR that cannot touch `agent/`), each time on a *different* test:

| Run | Test |
| --- | --- |
| [33446493920](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/actions/runs/33446493920) | `TestMicrovmRunHookPlatformConfig::test_no_platform_config_and_no_baked_env_is_refused_not_run_unscoped` |
| earlier run | `TestMicrovmRunHookPreInstallAwsSilence::test_no_cloudwatch_or_credential_seam_is_touched_before_the_install[no-config]` |

```
FAILED tests/test_server.py::TestMicrovmRunHookPlatformConfig::test_no_platform_config_and_no_baked_env_is_refused_not_run_unscoped
AssertionError: Expected 'mock' to not have been called. Called 1 times.
Calls: [call(repo_url='org/repo', task_description='do it', github_token='ghp_x',
anthropic_model='', aws_region='', task_id='t-pc', max_turns=100, ...)]
```

`main` is green (verified by manual `workflow_dispatch` [33510225606](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/actions/runs/33510225606): agent 1755 passed, cdk 4322 passed, cli 768 passed), so this is a **race**, not a regression — a single green run does not clear it.

## Root cause

Three facts combine:

1. **`run_task` is resolved at call time, in a background thread.** `server.py:35` does `from pipeline import run_task`; `_run_task_background` calls it bare at `server.py:478`. A bare global resolves *when executed*, not when the thread starts. That function is the `threading.Thread` target in `_spawn_background` (`server.py:736`, `target=` at `:748`).

2. **The reset fixture drops thread references without joining them.** `test_server.py:21-29`:

```python
@pytest.fixture(autouse=True)
def reset_server_state():
...
with server._threads_lock:
server._active_threads.clear() # <-- drops the handle; thread keeps running
```

`_active_threads` (`server.py:255`) is only a tracking list. Clearing it does not stop anything.

3. **Every test in `TestMicrovmRunHookPlatformConfig` shares `task_id: "t-pc"`** (the `_payload()` default at `test_server.py:2364`), and six of them return `200`, each spawning a real `pipeline-t-pc` thread while `monkeypatch.setattr(server, "run_task", MagicMock())` is in effect.

**The sequence:** an accepting test spawns `pipeline-t-pc` → the fixture clears the list and pytest advances → the still-live thread reaches `server.py:478`, looks up `server.run_task`, and receives **the next test's mock** → that test's `assert_not_called()` fails.

The captured call args confirm it rather than merely fitting it: `task_id='t-pc'` is the class-wide default that the refusal test would never start, and `aws_region=''` / `anthropic_model=''` are env-derived values already stripped — a sibling test's environment, not this one's. Note also that the refusal test's own asserts (`400`, `MICROVM_RUN_PLATFORM_CONFIG_INCOMPLETE`, `missing_env`) all **passed**; only `assert_not_called()` failed. The request path is correct. The contamination is external.

## Why it surfaced now

Not new code — new *volume*. MicroVM P1 (#645 / `f3cfb4e3b`, Aug 6) and P2 (#733 / `4d53a73b0`, Aug 28) added several accepting `/run` hook tests that all share one `task_id`, widening the window. `build (agentcore)` has no `push:` trigger (`pull_request`, `merge_group`, `workflow_dispatch` only) and last ran on `main` on 2026-06-04, so nothing re-verifies `main` between merges either — filing separately.

Same family as #615 (scoped-session bypassing a `boto3.client` mock): a per-test mock defeated by state the fixture does not actually reset. #615 fixed the *env* half by adding `AGENT_SESSION_ROLE_ARN` to `_AGENT_ENV_VARS` (`conftest.py:162`); the *thread* half is still open.

## Fix

The file already contains the correct pattern — `test_background_thread_failure_503_and_backup_terminal_write` (`test_server.py:60-76`) polls until no thread `is_alive()`, with a comment explaining that joining "eliminates the race." Promote that into the autouse fixture.

- [ ] `reset_server_state` joins (with timeout) every live thread in `_active_threads` before clearing, on both setup and teardown
- [ ] Fail loudly if a thread outlives its timeout, rather than clearing and continuing — a silent leak is what produced this
- [ ] Give each accepting test in `TestMicrovmRunHookPlatformConfig` a distinct `task_id` so a leak is attributable to its origin instead of anonymous
- [ ] Regression test: spawn a deliberately slow `run_task`, let the fixture tear down, assert the next test's mock is untouched (should fail before the fix)
- [ ] Re-run #680's build once merged and confirm it goes green with no code change to that PR

## Not in scope

`_spawn_background`'s production behaviour is fine — daemon-free named threads tracked in a list is reasonable for the runtime. This is purely test lifecycle.

Contributor guide

Open the contributing guide

Research direction

Start in agent/tests/test_server.py at reset_server_state and the existing test_background_thread_failure_503_and_backup_terminal_write pattern; inspect server.py:_active_threads and _spawn_background for the lifecycle being tested. Run the relevant test_server.py tests, then add coverage for a slow background task and verify fixture setup and teardown wait for live threads and fail on timeout without contaminating the next test.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.