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

Aperta
#841 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
agent-runtime bug
Lingua principale
TypeScript
Stelle
143
Fork
46
Merge medio
3g 10h
PR unite (30g)
24

Descrizione

## 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.

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Inizia in agent/tests/test_server.py, in reset_server_state e nel pattern esistente test_background_thread_failure_503_and_backup_terminal_write; esamina server.py:_active_threads e _spawn_background per il ciclo di vita sottoposto a test. Esegui i test pertinenti di test_server.py, quindi aggiungi la copertura per un’attività in background lenta e verifica che il setup e il teardown del fixture attendano i thread attivi e falliscano in caso di timeout senza contaminare il test successivo.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
python
Ambito
testing-qa
Tipo di issue
Bug
Difficoltà
3/5
Tempo stimato
1-2 giorni
Stato di attività
Attiva
Chiarezza
Specificata chiaramente
Idoneità per principianti
72/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.