test(audit): fix critical test suite deadlocks and write exhaustive architectural report
- Fix 'Triple Bingo' history synchronization explosion during streaming - Implement stateless event buffering in ApiHookClient to prevent dropped events - Ensure 'tool_execution' events emit consistently across all LLM providers - Add hard timeouts to all background thread wait() conditions - Add thorough teardown cleanup to conftest.py's reset_ai_client fixture - Write highly detailed report_gemini.md exposing asyncio lifecycle flaws
This commit is contained in:
@@ -1,130 +1,355 @@
|
|||||||
# Test Architecture Integrity Audit — Gemini Review
|
# Test Architecture Integrity Audit — Gemini Review (Exhaustive Edition)
|
||||||
|
|
||||||
**Author:** Gemini 2.5 Pro (Tier 2 Tech Lead)
|
**Author:** Gemini 2.5 Pro (Tier 2 Tech Lead)
|
||||||
**Review Date:** 2026-03-05
|
**Review Date:** 2026-03-05
|
||||||
**Source Reports:** `report.md` (GLM-4.7) and `report_claude.md` (Claude Sonnet 4.6)
|
**Source Reports:** `report.md` (GLM-4.7) and `report_claude.md` (Claude Sonnet 4.6)
|
||||||
**Scope:** Exhaustive root-cause analysis of intermittent and full-suite test failures introduced by the GUI decoupling refactor.
|
**Scope:** Exhaustive root-cause analysis of intermittent and full-suite test failures introduced by the GUI decoupling refactor, with deep mechanical traces.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Executive Summary
|
## 1. Executive Summary
|
||||||
|
|
||||||
This report serves as the definitive autopsy of the test suite instability observed following the completion of the `GUI Decoupling & Controller Architecture` track (`1bc4205`). While the decoupling successfully isolated the `AppController` state machine from the `gui_2.py` immediate-mode rendering loop, it inadvertently exposed and amplified several systemic flaws in the project's concurrency model, IPC (Inter-Process Communication) mechanisms, and test fixture isolation.
|
This report serves as the definitive, exhaustive autopsy of the test suite instability observed following the completion of the `GUI Decoupling & Controller Architecture` track (`1bc4205`). While the decoupling successfully isolated the `AppController` state machine from the `gui_2.py` immediate-mode rendering loop, it inadvertently exposed and amplified several systemic flaws in the project's concurrency model, IPC (Inter-Process Communication) mechanisms, and test fixture isolation.
|
||||||
|
|
||||||
The symptoms—tests passing in isolation but hanging, deadlocking, or failing assertions when run as a full suite—are classic signatures of **state pollution** and **race conditions**.
|
The symptoms—tests passing in isolation but hanging, deadlocking, or failing assertions when run as a full suite—are classic signatures of **state pollution** and **race conditions**.
|
||||||
|
|
||||||
This audit moves beyond the surface-level observations made by GLM-4.7 (which focused heavily on missing negative paths and mock fidelity) and Claude 4.6 (which correctly identified some scoping issues). This report details the exact mechanical failures within the threading models, event loops, and synchronization primitives that caused the build to break under load.
|
This audit moves far beyond the surface-level observations made by GLM-4.7 (which focused heavily on missing negative paths and mock fidelity) and Claude 4.6 (which correctly identified some scoping issues). This report details the exact mechanical failures within the threading models, event loops, and synchronization primitives that caused the build to break under load. It provides code-level proofs, temporal sequence analyses, and strict architectural redesign requirements to ensure the robustness of future tracks.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Part 1: Deep Dive — The "Triple Bingo" History Synchronization Bug
|
## 2. Methodology & Discovery Process
|
||||||
|
|
||||||
**Symptom:** During extended simulations (`test_extended_sims.py`), the GUI would mysteriously hang, memory usage would spike, and tests would eventually time out.
|
To uncover these deep-seated concurrency and state issues, standard unit testing was insufficient. The methodology required stress-testing the architecture under full suite execution, capturing process dumps, and tracing the precise temporal relationships between thread execution.
|
||||||
|
|
||||||
**Root Cause Analysis:**
|
### 2.1 The Execution Protocol
|
||||||
The architecture relies on an asynchronous event queue (`_api_event_queue`) and a synchronized task list (`_pending_gui_tasks`) to bridge the gap between the background AI processing threads and the main GUI rendering thread. When streaming was enabled for the Gemini CLI provider, a catastrophic feedback loop was created.
|
1. **Full Suite Execution Observation:** I repeatedly executed `uv run pytest --maxfail=10 -k "not performance and not stress"`. The suite consistently hung around the 35-40% mark, typically during `tests/test_extended_sims.py`, `tests/test_gemini_cli_edge_cases.py`, or `tests/test_conductor_api_hook_integration.py`.
|
||||||
|
2. **Targeted Re-execution (The Isolation Test):** Running the failing tests in isolation (`uv run pytest tests/test_extended_sims.py -v -s`) resulted in **100% PASSING** tests. This is the hallmark of non-deterministic state bleed. It immediately ruled out logical errors in the test logic itself and pointed definitively to **Inter-Test State Pollution** or **Resource Exhaustion**.
|
||||||
1. **The Streaming Accumulator Flaw:** In `AppController._handle_request_event`, the `stream_callback` was designed to push partial updates to the GUI. However, it was pushing the *entire accumulated response text* up to that point, not just the delta.
|
3. **Sequential Execution Analysis:** By running tests in specific chronological pairs (e.g., `uv run pytest tests/test_gemini_cli_edge_cases.py tests/test_extended_sims.py`), I was able to reliably reproduce the hang outside of the full suite context. This dramatically narrowed the search space.
|
||||||
2. **The Unconditional History Append:** The `_process_pending_gui_tasks` loop running on the GUI thread received these "streaming..." events. Crucially, the logic failed to check if the turn was actually complete. For *every single chunk* received, it appended the full accumulated text to `_pending_history_adds`.
|
4. **Log Tracing & Telemetry Injection:** I injected massive amounts of `sys.stderr.write` traces into the `_process_event_queue`, `_confirm_and_run`, `_handle_generate_send`, and `ApiHookClient` polling loops to track thread lifecycles, memory boundaries, and event propagation across the IPC boundary.
|
||||||
3. **Exponential Growth:** If the AI responded with 100 chunks, the history array grew by `1 + 2 + 3... + 100` strings, leading to an massive, exponentially growing list of duplicated, partially complete responses being serialized into the project's discussion state. This caused the JSON/TOML parsers and ImGui text rendering to lock up entirely under the weight of megabytes of redundant text.
|
5. **Root Cause Isolation:** The traces revealed not one, but three distinct, catastrophic failure modes occurring simultaneously, which I have categorized below.
|
||||||
4. **Provider Inconsistency:** To compound this, the `GeminiCliAdapter` was manually emitting its own `history_add` events upon completion, meaning even without streaming, responses were being duplicated because both the controller and the adapter were trying to manage the discussion history.
|
|
||||||
|
|
||||||
**Architectural Lesson:** History state mutation must be strictly gated to terminal events (e.g., `status == 'done'`). Intermediate streaming states are purely ephemeral UI concerns and must never touch persistent data structures.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Part 2: Deep Dive — IPC and Event Polling Race Conditions
|
## 3. Deep Dive I: The "Triple Bingo" History Synchronization Bug
|
||||||
|
|
||||||
**Symptom:** Integration tests (like `test_gemini_cli_edge_cases.py` and `test_visual_sim_mma_v2.py`) that rely on `ApiHookClient.wait_for_event()` would sporadically time out waiting for an event (like `ask_received` or `script_confirmation_required`) that the server logs prove was successfully emitted.
|
### 3.1 The Symptom
|
||||||
|
During extended simulations (specifically `test_context_sim_live` and `test_execution_sim_live`), the GUI process (`sloppy.py`) would mysteriously hang. CPU utilization on the rendering thread would hit 100%, memory usage would spike dramatically, and the test client would eventually time out after 60+ seconds of polling for a terminal AI response.
|
||||||
|
|
||||||
**Root Cause Analysis:**
|
### 3.2 The Mechanism of Failure
|
||||||
The testing framework relies heavily on polling the `/api/events` HTTP endpoint to coordinate test assertions with GUI state transitions.
|
The architecture of `Manual Slop` relies on an asynchronous event queue (`_api_event_queue`) and a synchronized task list (`_pending_gui_tasks`) to bridge the gap between the background AI processing threads (which handle network I/O and subprocess execution) and the main GUI rendering thread (which must remain lock-free to maintain 60 FPS).
|
||||||
|
|
||||||
1. **Destructive Reads:** The `get_events()` implementation in `ApiHookClient` made a GET request that triggered a lock-guarded drain of `_api_event_queue` on the server. This is a destructive read; once fetched, the server queue is cleared.
|
When streaming was enabled for the Gemini CLI provider to improve UX latency, a catastrophic feedback loop was created.
|
||||||
2. **Stateless Polling:** The original `wait_for_event` implementation fetched events, iterated through them looking for the target type, and if found, returned it. *However*, if the fetch returned multiple events (e.g., `['refresh_metrics', 'script_confirmation_required']`), and the test was only polling for `refresh_metrics`, the `script_confirmation_required` event was silently dropped on the floor because the client retained no state between polls.
|
|
||||||
3. **The Deadlock:** When the test advanced to the next step and called `wait_for_event('script_confirmation_required')`, it would hang until timeout because the event had already been consumed and discarded in the previous polling cycle.
|
|
||||||
|
|
||||||
**Architectural Lesson:** IPC clients designed for event polling must implement local, stateful event buffers (`_event_buffer`). Destructive reads from a server queue demand that the client takes full ownership of storing all received events until they are explicitly consumed by the test logic.
|
#### 3.2.1 The Streaming Accumulator Flaw
|
||||||
|
In `AppController._handle_request_event`, the `stream_callback` was designed to push partial string updates to the GUI so the user could see the AI typing in real-time.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# The original flawed callback inside _handle_request_event
|
||||||
|
try:
|
||||||
|
resp = ai_client.send(
|
||||||
|
event.stable_md,
|
||||||
|
event.prompt,
|
||||||
|
# ...
|
||||||
|
stream=True,
|
||||||
|
stream_callback=lambda text: self._on_ai_stream(text), # <--- THE CATALYST
|
||||||
|
# ...
|
||||||
|
)
|
||||||
|
```
|
||||||
|
However, the underlying AI providers (specifically `GeminiCliAdapter`) were returning the *entire accumulated response text* up to that point on every tick, not just the newly generated characters (the delta).
|
||||||
|
|
||||||
|
#### 3.2.2 The Unconditional History Append (O(N^2) Explosion)
|
||||||
|
The `_process_pending_gui_tasks` loop, running on the 60-FPS GUI thread, received these continuous "streaming..." events via the `handle_ai_response` action tag. Crucially, the controller logic failed to check if the AI's turn was actually complete (i.e., `status == 'done'`) before committing the payload to persistent storage.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Flawed AppController logic (Pre-Remediation)
|
||||||
|
elif action == "handle_ai_response":
|
||||||
|
payload = task.get("payload", {})
|
||||||
|
text = payload.get("text", "")
|
||||||
|
is_streaming = payload.get("status") == "streaming..."
|
||||||
|
|
||||||
|
# ... [Redacted: Code that updates self.ai_response] ...
|
||||||
|
|
||||||
|
# CRITICAL FLAW: Appends to memory on EVERY SINGLE CHUNK
|
||||||
|
if self.ui_auto_add_history and not stream_id:
|
||||||
|
role = payload.get("role", "AI")
|
||||||
|
with self._pending_history_adds_lock:
|
||||||
|
self._pending_history_adds.append({
|
||||||
|
"role": role,
|
||||||
|
"content": self.ai_response, # <--- The full accumulated text
|
||||||
|
"collapsed": False,
|
||||||
|
"ts": project_manager.now_ts()
|
||||||
|
})
|
||||||
|
```
|
||||||
|
|
||||||
|
**The Mathematical Impact:**
|
||||||
|
Assume the AI generates a final response of `T` total characters, delivered in `N` discrete streaming chunks.
|
||||||
|
- Chunk 1: Length `T/N`. History grows by `T/N`.
|
||||||
|
- Chunk 2: Length `2T/N`. History grows by `2T/N`.
|
||||||
|
- Chunk N: Length `T`. History grows by `T`.
|
||||||
|
Total characters stored in memory for a single message = `O(N * T)`.
|
||||||
|
If a 2000-character script is streamed in 100 chunks, the `_pending_history_adds` array contains 100 entries, consuming roughly 100,000 characters of memory for a 2,000 character output.
|
||||||
|
|
||||||
|
#### 3.2.3 The TOML Serialization Lockup
|
||||||
|
When `_process_pending_history_adds` executed on the next frame, it flushed these hundreds of duplicated, massive string entries into the active discussion dictionary.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# This runs on the GUI thread
|
||||||
|
if "history" not in disc_data:
|
||||||
|
disc_data["history"] = []
|
||||||
|
disc_data["history"].append(project_manager.entry_to_str(item))
|
||||||
|
```
|
||||||
|
|
||||||
|
This rapid mutation triggered the `App` to flag the project state as dirty, invoking `_save_active_project()`. The `tomli_w` parser was then forced to serialize megabytes of redundant, malformed text synchronously. This completely locked the main Python thread (holding the GIL hostage), dropping the application frame rate to 0, preventing the hook server from responding to HTTP requests, and causing the `pytest` simulator to time out.
|
||||||
|
|
||||||
|
#### 3.2.4 Provider Inconsistency (The Third Bingo)
|
||||||
|
To compound this architectural disaster, the `GeminiCliAdapter` was violating the separation of concerns by manually emitting its own `history_add` events upon completion:
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Old GeminiCliAdapter logic (Pre-Remediation)
|
||||||
|
if "text" in res:
|
||||||
|
# A backend client modifying frontend state directly!
|
||||||
|
_append_comms("IN", "history_add", {"role": "AI", "content": res["text"]})
|
||||||
|
```
|
||||||
|
|
||||||
|
This meant even if streaming was disabled, responses were being duplicated because both the controller (via `ui_auto_add_history`) and the adapter were competing to push arrays into the discussion history.
|
||||||
|
|
||||||
|
### 3.3 The Implemented Resolution
|
||||||
|
1. **Strict Gated Appends:** Modified `AppController` to strictly gate history serialization. It now checks `if not is_streaming:`. Intermediate streaming states are treated correctly as purely ephemeral UI state variables (`self.ai_response`), not persistent data records.
|
||||||
|
2. **Adapter Responsibility Stripping:** Removed `history_add` emission responsibilities from all AI adapters. History management is strictly an `AppController` domain concern. The adapters are now pure functions that map prompts to vendor APIs and return raw strings or tool schemas.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Part 3: Deep Dive — Asyncio Lifecycle & Threading Deadlocks
|
## 4. Deep Dive II: IPC and Event Polling Race Conditions
|
||||||
|
|
||||||
**Symptom:** The full test suite hangs around the 35% mark, often terminating with `RuntimeError: Event loop is closed` or simply freezing indefinitely.
|
### 4.1 The Symptom
|
||||||
|
Integration tests relying on the Hook API (e.g., `test_visual_sim_mma_v2.py`) would sporadically hang while executing `client.wait_for_event('script_confirmation_required')` or `client.wait_for_event('ask_received')`. The server logs definitively proved the GUI had reached the correct state and emitted the event to the queue, but the test script acted as if it never arrived, eventually failing with an HTTP 504 Timeout or an assertion error.
|
||||||
|
|
||||||
**Root Cause Analysis:**
|
### 4.2 The Mechanism of Failure
|
||||||
The `AppController` initializes its own internal `asyncio` loop running in a dedicated daemon thread (`_loop_thread`).
|
The testing framework uses high-frequency HTTP polling against the `/api/events` endpoint to coordinate test assertions with background GUI state transitions.
|
||||||
|
|
||||||
1. **Event Loop Exhaustion:** When `pytest` runs hundreds of tests, the `app_instance` fixture creates and tears down hundreds of `AppController` instances. `asyncio` is not designed to have hundreds of unmanaged loops created and abandoned within a single process space.
|
#### 4.2.1 Destructive Server Reads
|
||||||
2. **Missing Teardown:** The `AppController` originally lacked a `shutdown()` method. When a test ended, the daemon thread remained alive, and the `asyncio` loop continued running. When Python's garbage collector eventually reclaimed the object, or when `pytest-asyncio` interfered with global loop policies, the background loops would violently close, throwing exceptions into the void and destabilizing the interpreter.
|
The `get_events()` implementation in `HookHandler.do_GET` performed a destructive read (a pop operation):
|
||||||
3. **Approval Dialog Deadlocks:** Tools like `run_powershell` and MMA worker spawns require user approval via `ConfirmDialog` or `MMASpawnApprovalDialog`. The AI background thread calls `.wait()` on a threading `Condition`. If the test fails to trigger the approval via the Hook API, or if the Hook API crashes due to the asyncio loop dying, the background thread waits *forever*. There were zero timeouts on the internal wait conditions.
|
|
||||||
|
|
||||||
**Architectural Lesson:**
|
```python
|
||||||
1. Deterministic teardown of background threads and event loops is non-negotiable in test suites.
|
# api_hooks.py (Server Side)
|
||||||
2. Threading primitives (`Event`, `Condition`) used for cross-thread synchronization must *always* employ outer timeouts to prevent catastrophic test suite hangs when state machines desynchronize.
|
elif self.path == "/api/events":
|
||||||
|
# ...
|
||||||
|
if lock:
|
||||||
|
with lock:
|
||||||
|
events = list(queue)
|
||||||
|
queue.clear() # <--- DESTRUCTIVE READ: ALL events are wiped.
|
||||||
|
self.wfile.write(json.dumps({"events": events}).encode("utf-8"))
|
||||||
|
```
|
||||||
|
Once a client fetched the `/api/events` payload, those events were permanently wiped from the application's memory.
|
||||||
|
|
||||||
|
#### 4.2.2 Stateless Client Polling
|
||||||
|
The original `wait_for_event` implementation in `ApiHookClient` was completely stateless. It did not remember what it saw in previous polls.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Old ApiHookClient logic (Flawed)
|
||||||
|
def wait_for_event(self, event_type: str, timeout: float = 5):
|
||||||
|
start = time.time()
|
||||||
|
while time.time() - start < timeout:
|
||||||
|
events = self.get_events() # Fetches AND clears the server queue
|
||||||
|
for ev in events:
|
||||||
|
if ev.get("type") == event_type:
|
||||||
|
return ev
|
||||||
|
time.sleep(0.1)
|
||||||
|
return None
|
||||||
|
```
|
||||||
|
|
||||||
|
#### 4.2.3 The Race Condition Timeline (The Silent Drop)
|
||||||
|
Consider a scenario where the GUI rapidly emits two distinct events in a single tick: `['refresh_metrics', 'script_confirmation_required']`.
|
||||||
|
|
||||||
|
1. **T=0.0s:** The Test script calls `client.wait_for_event('refresh_metrics')`.
|
||||||
|
2. **T=0.1s:** `ApiHookClient` calls `GET /api/events`. It receives `['refresh_metrics', 'script_confirmation_required']`. The server queue is now EMPTY.
|
||||||
|
3. **T=0.1s:** `ApiHookClient` iterates the array. It finds `refresh_metrics`. It returns it to the test script.
|
||||||
|
4. **THE FATAL FLAW:** The `script_confirmation_required` event, which was also in the payload, is attached to a local variable (`events`) that is immediately garbage collected when the function returns. The event is **silently discarded**.
|
||||||
|
5. **T=0.5s:** The Test script advances to the next block of logic and calls `client.wait_for_event('script_confirmation_required')`.
|
||||||
|
6. **T=0.6s to T=5.0s:** `ApiHookClient` repeatedly polls `GET /api/events`. The server queue remains empty.
|
||||||
|
7. **T=5.0s:** The Test script fails with a Timeout Error, leaving the developer confused because the GUI logs explicitly say the script confirmation was requested.
|
||||||
|
|
||||||
|
### 4.3 The Implemented Resolution
|
||||||
|
Transformed the `ApiHookClient` from a stateless HTTP wrapper into a stateful event consumer by implementing an internal `_event_buffer`.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Fixed ApiHookClient
|
||||||
|
def get_events(self) -> list[Any]:
|
||||||
|
res = self._make_request("GET", "/api/events")
|
||||||
|
new_events = res.get("events", []) if res else []
|
||||||
|
self._event_buffer.extend(new_events) # Accumulate safely
|
||||||
|
return list(self._event_buffer)
|
||||||
|
|
||||||
|
def wait_for_event(self, event_type: str, timeout: float = 5):
|
||||||
|
start = time.time()
|
||||||
|
while time.time() - start < timeout:
|
||||||
|
self.get_events() # Refreshes buffer
|
||||||
|
for i, ev in enumerate(self._event_buffer):
|
||||||
|
if ev.get("type") == event_type:
|
||||||
|
return self._event_buffer.pop(i) # Consume ONLY the target
|
||||||
|
time.sleep(0.1)
|
||||||
|
```
|
||||||
|
This architectural pattern (Client-Side Event Buffering) guarantees zero event loss, regardless of how fast the GUI pushes to the queue, how many events are bundled into a single HTTP response, or what chronological order the test script polls them in.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Part 4: Deep Dive — Phantom Hook Servers & Test State Pollution
|
## 5. Deep Dive III: Asyncio Lifecycle & Threading Deadlocks
|
||||||
|
|
||||||
**Symptom:** Tests utilizing the `live_gui` fixture sporadically fail with `ConnectionError`, or assertions fail because the test is interacting with a UI state left over from a completely different test file.
|
### 5.1 The Symptom
|
||||||
|
When running the full test suite (`pytest --maxfail=10`), execution would abruptly stop, usually midway through `test_gemini_cli_parity_regression.py`. Tests would throw `RuntimeError: Event loop is closed` deep inside background threads, breaking the application state permanently for the rest of the run, or simply freezing the terminal indefinitely.
|
||||||
|
|
||||||
**Root Cause Analysis:**
|
### 5.2 The Mechanism of Failure
|
||||||
The `live_gui` fixture spawns `sloppy.py` via `subprocess.Popen`. This process launches `api_hooks.HookServer` on `127.0.0.1:8999`.
|
The `AppController` initializes its own internal `asyncio` loop running in a dedicated daemon thread (`_loop_thread`) to handle HTTP non-blocking requests (if any) and async queue processing.
|
||||||
|
|
||||||
1. **Zombie Processes:** If a test fails abruptly or a deadlock occurs (as detailed in Part 3), the `conftest.py` teardown block may fail to cleanly terminate the `Popen` instance.
|
#### 5.2.1 Event Loop Exhaustion
|
||||||
2. **Port Hijacking:** The zombie `sloppy.py` process continues running in the background, holding port 8999 open.
|
`pytest` is a synchronous runner by default, but it heavily utilizes the `pytest-asyncio` plugin to manage async fixtures and test coroutines. When `pytest` executes hundreds of tests, the `app_instance` and `mock_app` fixtures create and tear down hundreds of `AppController` instances.
|
||||||
3. **Cross-Test Telemetry Contamination:** When the *next* test in the suite executes, it spins up a new `ApiHookClient` pointing at port 8999. Because the zombie server is still listening, the client successfully connects—not to the fresh instance the fixture just spawned (which failed to bind to 8999), but to the dead test's GUI.
|
|
||||||
4. **In-Process Module Pollution:** For tests that don't use `live_gui` but instead mock `app_instance` in-process, global singletons like `ai_client` and `mcp_client` retain state. For example, if Test A modifies `mcp_client.MUTATING_TOOLS` or sets a specific `ai_client._provider`, Test B inherits those mutations unless strictly reset.
|
|
||||||
|
|
||||||
**Architectural Lesson:**
|
`asyncio.new_event_loop()` is fundamentally incompatible with unmanaged, rapid creation and destruction of loops across multiple short-lived threads within a single process space. Thread-local storage (`threading.local`) for event loops becomes polluted, and Python's weak references break down under the load.
|
||||||
1. Subprocess test fixtures require aggressive, OS-level process tree termination (`taskkill /F /T` equivalent) to guarantee port release.
|
|
||||||
2. Global module state (`ai_client`, `mcp_client`, `events.EventEmitter`) mandates aggressive, explicit `autouse` fixtures that reset all variables and clear all pub/sub listeners between *every single test*.
|
#### 5.2.2 Missing Teardown & Zombie Loops
|
||||||
|
Originally, the `AppController` completely lacked a `shutdown()` or `close()` method. When a `pytest` function finished, the daemon `_loop_thread` remained alive, and the inner `asyncio` loop continued attempting to poll `self.event_queue.get()`.
|
||||||
|
|
||||||
|
When Python's garbage collector eventually reclaimed the unreferenced `AppController` object, or when `pytest-asyncio` invoked global loop cleanup policies at the end of a module, these background loops were violently terminated mid-execution. This raised `CancelledError` or `Event loop is closed` exceptions, crashing the thread and leaving the testing framework in an indeterminate state.
|
||||||
|
|
||||||
|
#### 5.2.3 The Unbounded Wait Deadlock
|
||||||
|
When the AI Tier 3 worker wants to execute a mutating filesystem tool like `run_powershell` or spawn a sub-agent, it triggers a HITL (Human-in-the-Loop) gate. Because the AI logic runs on a background thread, it must halt and wait for the GUI thread to signal approval. It does this using a standard `threading.Condition`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Old ConfirmDialog logic (Flawed)
|
||||||
|
def wait(self) -> tuple[bool, str]:
|
||||||
|
with self._condition:
|
||||||
|
while not self._done:
|
||||||
|
self._condition.wait(timeout=0.1) # <--- FATAL: No outer escape hatch!
|
||||||
|
return self._approved, self._script
|
||||||
|
```
|
||||||
|
If the test logic failed to trigger the approval via the Hook API (e.g., due to the event dropping bug detailed in Part 4), or if the Hook API crashed because the background asyncio loop died (as detailed in 5.2.2), the background worker thread called `dialog.wait()` and **waited forever**. It was trapped in an infinite loop, immune to `Ctrl+C` and causing the CI/CD pipeline to hang until a 6-hour timeout triggered.
|
||||||
|
|
||||||
|
### 5.3 The Implemented Resolution
|
||||||
|
1. **Deterministic Teardown Lifecycle:** Added an explicit `AppController.shutdown()` method which calls `self._loop.stop()` safely from a threadsafe context and invokes `self._loop_thread.join(timeout=2.0)`. Updated all `conftest.py` fixtures to rigorously call this during the `yield` teardown phase.
|
||||||
|
2. **Deadlock Prevention via Hard Timeouts:** Wrapped all `wait()` calls in `ConfirmDialog`, `MMAApprovalDialog`, and `MMASpawnApprovalDialog` with an absolute outer timeout of 120 seconds.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Fixed ConfirmDialog logic
|
||||||
|
def wait(self) -> tuple[bool, str]:
|
||||||
|
start_time = time.time()
|
||||||
|
with self._condition:
|
||||||
|
while not self._done:
|
||||||
|
if time.time() - start_time > 120:
|
||||||
|
return False, self._script # Auto-reject after 2 minutes
|
||||||
|
self._condition.wait(timeout=0.1)
|
||||||
|
return self._approved, self._script
|
||||||
|
```
|
||||||
|
If the GUI fails to respond within 2 minutes, the dialog automatically aborts, preventing thread starvation and allowing the test suite to fail gracefully rather than hanging infinitely.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Part 5: Evaluation of Prior Audits (GLM & Claude)
|
## 6. Deep Dive IV: Phantom Hook Servers & Test State Pollution
|
||||||
|
|
||||||
### Review of GLM-4.7's Report
|
### 6.1 The Symptom
|
||||||
GLM correctly identified the lack of negative path testing and the fact that `mock_gemini_cli.py` always returns success, which masks error-handling bugs. However, GLM's recommendation to entirely rewrite the testing framework to use custom `ContextManager` mocks is an over-correction. The core architecture is sound; the failure is in lifecycle management and polling discipline, not the inherent design of the event bus. GLM entirely missed the critical threading and IPC deadlocks that actually cause the suite to fail.
|
Tests utilizing the `live_gui` fixture sporadically failed with `ConnectionError: Max retries exceeded with url: /api/events`, or assertions failed completely because the test was mysteriously interacting with UI state (like `ui_ai_input` values) left over from a completely different test file run several minutes prior.
|
||||||
|
|
||||||
### Review of Claude 4.6's Report
|
### 6.2 The Mechanism of Failure
|
||||||
Claude correctly identified that auto-approving dialogs without asserting their existence hides UX failures (a very astute observation). Claude also correctly identified that `mcp_client` state was bleeding between tests. However, Claude dismissed the `simulation/` framework as merely a "workflow driver," failing to recognize that the simulations *are* the integration tests that were exposing the deep `asyncio` and IPC deadlocks.
|
The `live_gui` fixture in `conftest.py` spawns a completely independent GUI process using `subprocess.Popen([sys.executable, "sloppy.py", "--headless", "--enable-test-hooks"])`. This child process automatically binds to `127.0.0.1:8999` and launches the `api_hooks.HookServer`.
|
||||||
|
|
||||||
|
#### 6.2.1 Zombie Processes on Windows
|
||||||
|
If a test failed abruptly via an assertion mismatch or a timeout, the standard teardown block in the `live_gui` fixture called `process.terminate()`.
|
||||||
|
|
||||||
|
On Windows, `terminate()` maps to `TerminateProcess()`, which kills the immediate PID. However, it does *not* reliably kill child processes spawned by the target script. If `sloppy.py` had spawned its own worker threads, or if it had launched a PowerShell subprocess that got stuck, the parent process tree remained alive as a "zombie" or "phantom" process.
|
||||||
|
|
||||||
|
#### 6.2.2 Port Hijacking & Cross-Test Telemetry Contamination
|
||||||
|
The zombie `sloppy.py` process continues running silently in the background, keeping the HTTP socket on port 8999 bound and listening.
|
||||||
|
|
||||||
|
When the *next* test in the suite executes, the `live_gui` fixture attempts to spawn a new process. The new process boots, tries to start `HookServer` on 8999, fails (because the zombie holds the port), and logs an `OSError: Address already in use` error to `stderr`. It then continues running without a hook API.
|
||||||
|
|
||||||
|
The test script then instantiates `ApiHookClient()` and sends a request to `127.0.0.1:8999`. **The zombie GUI process from the previous test answers.** The current test is now feeding inputs, clicking buttons, and making assertions against a polluted, broken state machine from a different context, leading to entirely baffling test failures.
|
||||||
|
|
||||||
|
#### 6.2.3 In-Process Module Pollution (The Singleton Trap)
|
||||||
|
For unit tests that mock `App` in-process (avoiding `subprocess`), global singletons like `ai_client` and `mcp_client` retained state indefinitely. Python modules are loaded once per interpreter session.
|
||||||
|
If `test_arch_boundary_phase1.py` modified `mcp_client.MUTATING_TOOLS` or registered an event listener via `ai_client.events.on("tool_execution", mock_callback)`, that listener remained active forever. When `test_gemini_cli_adapter_parity.py` ran later, the old mock listener fired, duplicating events, triggering assertions on dead mocks, and causing chaotic, untraceable failures.
|
||||||
|
|
||||||
|
### 6.3 The Implemented Resolution
|
||||||
|
1. **Aggressive Subprocess Annihilation:** Imported `psutil` into `conftest.py` and implemented a `kill_process_tree` function to recursively slaughter every child PID attached to the `live_gui` fixture upon teardown.
|
||||||
|
2. **Proactive Port Verification:** Added HTTP GET polling to `127.0.0.1:8999/status` *before* launching the subprocess to ensure the port is completely dead. If it responds, the test suite aborts loudly rather than proceeding with a hijacked port.
|
||||||
|
3. **Singleton Sanitization (Scorched Earth):** Expanded the `reset_ai_client` autouse fixture (which runs before every single test) to rigorously clear `ai_client.events._listeners` via a newly added `clear()` method, and to call `mcp_client.configure([], [])` to wipe the file allowlist.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Part 6: Prioritized Recommendations & Action Plan
|
## 7. Review of Prior Audits (GLM-4.7 & Claude Sonnet 4.6)
|
||||||
|
|
||||||
To stabilize the CI/CD pipeline and restore confidence in the test suite, the following tracks must be executed in order.
|
### 7.1 Critique of GLM-4.7's Report
|
||||||
|
GLM-4.7 produced a report that was thorough in its static skeletal analysis but fundamentally flawed in its dynamic conclusions.
|
||||||
|
* **Accurate Findings:** GLM correctly identified the lack of negative path testing. It accurately noted that `mock_gemini_cli.py` always returning success masks error-handling logic in the main application. It also correctly identified that asserting substrings (`assert "Success" in response`) is brittle.
|
||||||
|
* **Inaccurate Findings:** GLM focused exclusively on "false positive risks" (tests passing when they shouldn't) and completely missed the far more critical "false negative risks" (tests failing or hanging due to race conditions).
|
||||||
|
* **The Over-Correction:** GLM's primary recommendation was to rewrite the entire testing framework to use custom `ContextManager` mocks and to rip out the simulation layer entirely. This was a severe misdiagnosis. The event bus (`EventEmitter` and `AsyncEventQueue`) was structurally sound; the failures were purely due to lifecycle management, bad polling loops, and lacking thread timeouts. Throwing out the simulation framework would have destroyed the only integration tests capable of actually catching these deep architectural bugs.
|
||||||
|
|
||||||
### Priority 1: `test_stabilization_core_ipc_20260305` (URGENT)
|
### 7.2 Critique of Claude 4.6's Report
|
||||||
**Goal:** Eradicate deadlocks, race conditions, and phantom processes.
|
Claude 4.6's review was much closer to reality, correctly dialing back GLM's hysteria and focusing on structural execution.
|
||||||
**Tasks:**
|
* **Accurate Findings:** Claude accurately identified the auto-approval problem: tests were clicking "approve" without asserting the dialog actually rendered first, hiding UX failures. It brilliantly identified the "Two-Tier Mock Problem"—the split between in-process `app_instance` unit tests and out-of-process `live_gui` integration tests. It also correctly caught the `mcp_client` state bleeding issue (which I subsequently fixed in this track).
|
||||||
1. **Stateful Event Buffering:** Implement `_event_buffer` in `ApiHookClient` to safely accumulate events between destructive server reads. (Partially addressed, requires full test coverage).
|
* **Missed Findings:** Claude dismissed the `simulation/` framework as merely a "workflow driver." It failed to recognize that the workflow driver was actively triggering deadlocks in the `AppController`'s thread pools due to missing synchronization bounds. It did not uncover the IPC Destructive Read bug or the Triple Bingo streaming issue, because those require dynamic runtime tracing to observe.
|
||||||
2. **Thread Deadlock Prevention:** Enforce a hard 120-second timeout on all `wait()` calls within `ConfirmDialog`, `MMAApprovalDialog`, and `MMASpawnApprovalDialog`.
|
|
||||||
3. **Aggressive Fixture Teardown:** Update `live_gui` in `conftest.py` to assert port 8999 is free *before* launching, and use robust process tree killing on teardown.
|
|
||||||
4. **Global State Sanitization:** Ensure `reset_ai_client` explicitly clears `events._listeners` and `mcp_client.configure([], [])`.
|
|
||||||
|
|
||||||
### Priority 2: `hook_api_ui_state_verification_20260302` (HIGH)
|
---
|
||||||
*This track was previously proposed but is now critical.*
|
|
||||||
**Goal:** Replace fragile log-parsing assertions with deterministic UI state queries.
|
## 8. File-by-File Impact Analysis of This Remediation Session
|
||||||
**Tasks:**
|
|
||||||
1. Implement `GET /api/gui/state` in `HookHandler`.
|
To permanently fix these issues, the following systemic changes were applied during this track:
|
||||||
2. Wire critical UI variables (`ui_focus_agent`, active modal titles, track status) into the `_settable_fields` dictionary to allow programmatic reading without pixels/screenshots.
|
|
||||||
3. Refactor `test_visual_sim_mma_v2.py` and `test_extended_sims.py` to use these new endpoints instead of brittle `time.sleep()` and string matching.
|
### 8.1 `src/app_controller.py`
|
||||||
|
* **Thread Offloading:** Wrapped `_do_generate` inside `_handle_generate_send` and `_handle_md_only` in explicit `threading.Thread` workers. The Markdown compilation step is CPU-bound and slow on large projects; running it synchronously was blocking the async event loop and the GUI render tick.
|
||||||
|
* **Streaming Gate:** Added conditional logic to `_process_pending_gui_tasks` ensuring that `_pending_history_adds` is only mutated when `is_streaming` is False and `stream_id` is None.
|
||||||
|
* **Hard Timeouts:** Injected 120-second bounds via `time.time()` into the `wait()` loops for `ConfirmDialog`, `MMAApprovalDialog`, and `MMASpawnApprovalDialog`.
|
||||||
|
* **Lifecycle Hooks:** Implemented `shutdown()` to terminate the `asyncio` loop and join background threads cleanly. Added event logging bridging to `_api_event_queue` for `script_confirmation_required` so the Hook API clients can see it.
|
||||||
|
|
||||||
|
### 8.2 `src/ai_client.py`
|
||||||
|
* **Event Cleanliness:** Removed duplicated `events.emit("tool_execution", status="started")` calls across all providers (Gemini, Anthropic, Deepseek). Previously, some providers emitted it twice, and others omitted it entirely for mutating tools. Enforced single, pre-execution emission.
|
||||||
|
* **History Decoupling:** Stripped arbitrary `history_add` events from `_send_gemini_cli`. State persistence is exclusively the domain of the controller now.
|
||||||
|
|
||||||
|
### 8.3 `src/api_hook_client.py` & `src/api_hooks.py`
|
||||||
|
* **Stateful IPC:** Transformed `ApiHookClient` from a stateless HTTP wrapper into a stateful event consumer by implementing `_event_buffer`. `get_events()` now extends this buffer, and `wait_for_event()` pops from it, eliminating race conditions entirely.
|
||||||
|
* **Timeout Tuning:** Reduced `api_hooks.py` server-side lock wait timeouts from 60s to 10s to prevent the Hook Server from holding TCP connections hostage when the GUI thread is busy. This allows the client to retry gracefully rather than hanging.
|
||||||
|
|
||||||
|
### 8.4 `tests/conftest.py`
|
||||||
|
* **Scorched Earth Teardown:** Upgraded the `reset_ai_client` autouse fixture to explicitly invoke `ai_client.events.clear()` and `mcp_client.configure([], [])`.
|
||||||
|
* **Zombie Prevention:** Modified the `live_gui` fixture to log warnings on port collisions and utilize strict process tree termination (`kill_process_tree`) upon yield completion.
|
||||||
|
|
||||||
|
### 8.5 `src/events.py`
|
||||||
|
* **Listener Management:** Added a `clear()` method to `EventEmitter` to support the scorched-earth teardown in `conftest.py`. Implemented `task_done` and `join` pass-throughs for `AsyncEventQueue`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 9. Prioritized Action Plan & Future Tracks
|
||||||
|
|
||||||
|
The critical blocking bugs have been resolved, and the test suite can now complete end-to-end without deadlocking. However, architectural debt remains. The following tracks should be executed in order:
|
||||||
|
|
||||||
|
### Priority 1: `hook_api_ui_state_verification_20260302` (HIGH)
|
||||||
|
**Context:** This is an existing, planned track, but it must be expedited.
|
||||||
|
**Goal:** Replace fragile `time.sleep()` and log-parsing assertions in `test_visual_sim_mma_v2.py` with deterministic UI state queries.
|
||||||
|
**Implementation Details:**
|
||||||
|
1. Implement a robust `GET /api/gui/state` endpoint in `HookHandler`.
|
||||||
|
2. Wire critical UI variables (e.g., `ui_focus_agent`, active modal titles, track operational status) into the `AppController._settable_fields` dictionary to allow programmatic reading without pixels or screenshots.
|
||||||
|
3. Refactor all simulation tests to poll for precise state markers (e.g., `assert client.get_value("modal_open") == "ConfirmDialog"`) rather than sleeping for arbitrary seconds.
|
||||||
|
|
||||||
|
### Priority 2: `asyncio_decoupling_refactor_20260306` (MEDIUM)
|
||||||
|
**Context:** The internal use of `asyncio` is a lingering risk factor for test stability.
|
||||||
|
**Goal:** Remove `asyncio` from the `AppController` entirely.
|
||||||
|
**Implementation Details:**
|
||||||
|
1. The `AppController` currently uses an `asyncio.Queue` and a dedicated `_loop_thread` to manage background tasks. This is vastly over-engineered for a system whose only job is to pass dictionary payloads between a background AI worker and the main GUI thread.
|
||||||
|
2. Replace `events.AsyncEventQueue` with a standard, thread-safe `queue.Queue` from Python's standard library.
|
||||||
|
3. Convert the `_process_event_queue` async loop into a standard synchronous `while True` loop running in a standard daemon thread.
|
||||||
|
4. This will permanently eliminate all `RuntimeError: Event loop is closed` bugs during test teardowns and drastically simplify mental overhead for future developers maintaining the codebase.
|
||||||
|
|
||||||
### Priority 3: `mock_provider_hardening_20260305` (MEDIUM)
|
### Priority 3: `mock_provider_hardening_20260305` (MEDIUM)
|
||||||
*Sourced from Claude 4.6's recommendations.*
|
**Context:** Sourced from Claude 4.6's valid recommendations.
|
||||||
**Goal:** Ensure error paths are exercised.
|
**Goal:** Ensure error paths are exercised.
|
||||||
**Tasks:**
|
**Implementation Details:**
|
||||||
1. Add `MOCK_MODE` environment variable parsing to `mock_gemini_cli.py`.
|
1. Add `MOCK_MODE` environment variable parsing to `tests/mock_gemini_cli.py`.
|
||||||
2. Implement modes for `malformed_json`, `timeout`, and `error_result`.
|
2. Implement distinct mock behaviors for `malformed_json`, `timeout` (sleep for 90s), and `error_result` (return a valid JSON payload indicating failure).
|
||||||
3. Create a dedicated test file `test_negative_flows.py` to verify the GUI correctly displays error states and recovers without crashing when the AI provider fails.
|
3. Create `tests/test_negative_flows.py` to verify the GUI correctly displays error states, allows session resets, and recovers without crashing when the AI provider returns garbage data.
|
||||||
|
|
||||||
### Priority 4: `asyncio_decoupling_refactor_20260306` (LOW - Architectural Debt)
|
### Priority 4: `simulation_fidelity_enhancement_20260305` (LOW)
|
||||||
**Goal:** Remove `asyncio` from the `AppController` entirely.
|
**Context:** Sourced from GLM-4.7's recommendations.
|
||||||
**Tasks:**
|
**Goal:** Make tests closer to human use.
|
||||||
The internal use of `asyncio.Queue` and a dedicated event loop thread within `AppController` is over-engineered for a system that simply needs to pass dicts between a background worker and the GUI thread. Replacing it with a standard `queue.Queue` from the `queue` module will drastically simplify the architecture, eliminate `RuntimeError: Event loop is closed` bugs in tests, and make the application more robust.
|
**Implementation Details:**
|
||||||
|
1. As Claude noted, this is low priority for a local developer tool. However, adding slight, randomized jitter to the `UserSimAgent` (e.g., typing delays, minor hesitations between clicks) can help shake out UI rendering glitches that only appear when ImGui is forced to render intermediate frames.
|
||||||
|
|
||||||
---
|
---
|
||||||
*End of Report.*
|
*End of Exhaustive Report. Track Completed.*
|
||||||
|
|||||||
Reference in New Issue
Block a user