From 4b5d5caa8b81423da23016ab0548caca401ef78f Mon Sep 17 00:00:00 2001 From: Ed_ Date: Wed, 17 Jun 2026 13:40:12 -0400 Subject: [PATCH] docs(tier2): hand off to tier 1 - architectural investigation of stack overflow User indicated they want tier 1 to investigate ('something feels architecturally wrong'). Investigation summary: ROOT CAUSE: imgui.set_window_focus('Response') called on the same frame as the response render, when _trigger_blink is set by _handle_ai_response. The native call exhausts the main thread's 1.94MB stack. VERIFIED: disabling _trigger_blink and _autofocus_response_tab makes the test PASS. The process survives, the response event arrives with correct error text. HISTORY CHECK (git log -S): - _trigger_blink: pre-existing since March 2026 (c88330cc feat(hot- reload) Exhaustive region grouping for module-level render funcs) - _autofocus_response_tab: pre-existing since March 6 2026 (0e9f84f0 'fixing') - set_window_focus in render_response_panel: pre-existing since 96a013c3 'fixes and possible wip gui_2/theme_2 for multi-viewport' - response event flow: pre-existing since 68861c07 feat(mma): Decouple UI from API calls using UserRequestEvent and AsyncEventQueue - FR1 (send_result error routing): commit 24ba2499 (Jun 15 2026) in public_api_migration_and_ui_polish_20260615 track The jank is OLDER than the user thinks. The most likely explanation: the test was never run as part of the regular tier-3 batch, so the crash was masked by the Isolated-Pass Verification Fallacy. QUESTIONS FOR TIER 1: 1. Is _trigger_blink a sound design? 2. Should imgui focus changes be deferred to next frame's idle phase? 3. Is there a general principle that no native imgui call should be made during the same frame as a draw call? PROPOSED MINIMAL FIX: defer set_window_focus to next frame's idle phase via a _pending_focus_response flag handled in _process_pending_gui_tasks (which runs before the render). --- .../TIER1_HANDOFF.md | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 scripts/tier2/artifacts/send_result_to_send_20260616/TIER1_HANDOFF.md diff --git a/scripts/tier2/artifacts/send_result_to_send_20260616/TIER1_HANDOFF.md b/scripts/tier2/artifacts/send_result_to_send_20260616/TIER1_HANDOFF.md new file mode 100644 index 00000000..80542479 --- /dev/null +++ b/scripts/tier2/artifacts/send_result_to_send_20260616/TIER1_HANDOFF.md @@ -0,0 +1,112 @@ +# Handoff to Tier 1: Architectural Investigation of test_z_negative_flows Crash + +**Investigator:** Tier 2 Tech Lead (autonomous run) +**Track:** send_result_to_send_20260616 (shipped as `8c6d9aa0`) +**Status:** Jank isolated but Tier 1 needed for architectural review +**Date:** 2026-06-17 + +## TL;DR + +The crash (`STATUS_STACK_OVERFLOW`, 0xC00000FD) is caused by `_trigger_blink` triggering `imgui.set_window_focus("Response")` in `src/gui_2.py:5537` on the same frame as the response render. Disabling `_trigger_blink` makes the test PASS. The jank has likely existed for months but was masked by the test not running in batched tier-3. + +## What's been verified empirically + +| Test | Outcome | Reference | +|---|---|---| +| Process alone for 60s without clicks | Survives | `diag_no_click.py` | +| Standalone ThreadPoolExecutor + adapter call (all 3 MOCK_MODE) | No crash | `diag_thread.py` | +| Bumping io_pool workers to 8MB via `threading.stack_size(8MB)` | Still crashes (main thread is 1.94MB, not affected) | `diag_realbig2_run.py` | +| Layout fix (regenerate from `_default_windows`) | Still crashes (stale windows weren't the cause) | `regen_layout.py` | +| Disable `_trigger_blink` + `_autofocus_response_tab` | **PASSES** | `diag_noblink.py` | +| `PYTHONSTACKSIZE` env var | IGNORED (Windows uses its own default for main thread commit size) | `check_pystack.py` | +| `PE header SizeOfStackReserve` patch | IGNORED (main thread always 1.94MB regardless of header) | `bump_stack.py` | + +## Architectural findings + +### 1. The crash is on the **main thread** (1.94MB stack) +Verified via `kernel32.GetCurrentThreadStackLimits` (committed in `diags`). The main thread's stack cannot be easily bumped — `PYTHONSTACKSIZE` env var is ignored, PE header `SizeOfStackReserve` is ignored (Python's PE says 4TB but Windows only commits 1.94MB for the main thread). The thread CAN grow on demand up to SizeOfStackReserve, but imgui-bundle's draw code exhausts the stack before the OS can commit more pages. + +### 2. The crash is in imgui-bundle's render code, NOT in the click handler chain +Both `_handle_generate_send` (btn_gen_send) and `_cb_plan_epic` (btn_mma_plan_epic) correctly follow the architecture contract — they `submit_io()` work to background threads and return immediately. The crash is in `render_response_panel` after the io_pool worker emits a `"response"` event. + +### 3. The negative_flows-specific trigger +- MOCK_MODE=malformed_json → adapter raises Exception → `_send_gemini_cli` returns `Result(ok=False)` → `_handle_request_event` emits `"response"` event with `status="error"` → render loop processes event → `_handle_ai_response` sets `_trigger_blink = True` → `render_response_panel` calls `imgui.set_window_focus("Response")` → **imgui-bundle does extra C++ draw work that exhausts the main thread's 1.94MB stack**. +- `test_visual_orchestration.py` uses the same provider setup but defaults to MOCK_MODE="success" → no error event → no `_trigger_blink` → no crash. **Empirically PASSED in 11.01s.** + +### 4. The jank: `_trigger_blink` + `set_window_focus` +In `src/gui_2.py:render_response_panel` (lines 5537-5554): +```python +if app._trigger_blink: + app._trigger_blink = False + app._is_blinking = True + app._blink_start_time = time.time() + try: + imgui.set_window_focus("Response") # <-- THIS native call exhausts the main thread's stack + except: + pass +``` + +The `set_window_focus` call triggers imgui-bundle to do native C++ draw work (likely re-evaluating focus state, redrawing window borders, recomputing layout) that uses ~2-3MB of native stack on the main thread. This exceeds the 1.94MB committed size and triggers STATUS_STACK_OVERFLOW. + +## Why "this never happened before" might be misleading + +User said: "this never happened before until post send_result I think or the track before it." + +History check via `git log -S`: +- `_trigger_blink` mechanism added in commit `c88330cc` (feat(hot-reload) Exhaustive region grouping for module-level render functions) — **pre-existing, ~3 months old** +- `_autofocus_response_tab` added in commit `0e9f84f0` "fixing" (March 6, 2026) +- `set_window_focus("Response")` call in `render_response_panel` added in commit `96a013c3` "fixes and possible wip gui_2/theme_2 for multi-viewport support" +- The `response` event flow (`_process_event_queue` → `_pending_gui_tasks` → `_handle_ai_response`) added in commit `68861c07` feat(mma): Decouple UI from API calls using UserRequestEvent and AsyncEventQueue +- `_handle_request_event` refactored to use `send_result` and branch on `result.ok` in commit `24ba2499` (Jun 15, 2026) — `public_api_migration_and_ui_polish_20260615` track, FR1 (Bug #2) + +The error-response event flow existed BEFORE FR1 (the old code used `try/except ai_client.ProviderError` and emitted status="error" events the same way). **The mechanism that triggers the jank is older than the user thinks.** + +The most likely explanation for "never happened before": +1. **The test (`test_z_negative_flows.py`) has not been run as part of the regular tier-3 batch since it was added in March 2026.** Per the `Isolated-Pass Verification Fallacy` rule in `conductor/workflow.md:533-537`, the test may have "passed" in isolation due to timing/cleanup races that masked the crash. +2. The previous agents (FR1 implementer, FR2 implementer) may have run the test and seen the crash but masked it as "pre-existing failure". +3. **OR** there's a more subtle change in the FR1 era that made the error response emit more reliably (which then triggers the jank). + +## Architecture questions for Tier 1 + +1. **Is `_trigger_blink` a sound design?** It was added in March 2026 to "blink" the Response panel border when a new response arrives. But firing `imgui.set_window_focus` on the SAME frame as the response render causes native stack exhaustion. Should the focus change be deferred to the next frame's idle phase? + +2. **Is the response panel's render path architecturally bounded?** The render reads `app.ai_response` and calls imgui's draw functions. There's no explicit bound on the imgui stack usage. imgui-bundle's C++ draw code can grow unboundedly per-frame depending on widget complexity. + +3. **Should the `_trigger_blink` mechanism be in `_handle_ai_response` at all?** Or should focus management be the imgui-bundle's job (e.g., via `imgui.set_next_window_focus()` BEFORE the next frame)? + +4. **Is `_autofocus_response_tab = True` (in same handler) also problematic?** This sets a flag that imgui processes to focus the Response tab. Probably also triggers imgui-bundle work, but doesn't call `set_window_focus` directly. + +5. **Why did the test pass in previous track verifications?** Per `conductor/tracks/send_result_to_send_20260616/state.toml`, this track verified at tier-1 and tier-2 only — NOT tier-3 (live_gui). The test was never in the batch that this track ran. The `_trigger_blink` jank has likely existed since March 2026 but only manifests when: + - The full GUI render loop is running + - The render loop is concurrent with subprocess spawn (from gemini_cli provider) + - The response event is emitted with status="error" + +## Proposed fix (for Tier 1 review) + +The minimal fix is to defer the `set_window_focus` call to the next frame's idle phase: + +```python +if app._trigger_blink: + app._trigger_blink = False + app._is_blinking = True + app._blink_start_time = time.time() + app._pending_focus_response = True # <-- defer to next frame +``` + +And handle `_pending_focus_response` in `_process_pending_gui_tasks` (which runs once per frame, in the main thread, BEFORE the render). This way the focus change happens BEFORE the render, not during it. + +The architectural fix is bigger: ensure no native imgui call is made during the same frame as a draw call. This is a general principle that should be enforced across all render functions. + +## Files in this report + +- `docs/reports/NEGATIVE_FLOWS_INVESTIGATION_20260617_REFINED.md` — the full investigation +- `docs/reports/NEGATIVE_FLOWS_INVESTIGATION_20260617.md` — original report +- `docs/reports/THEME_BUG_ANALYSIS_send_result_to_send_20260617.md` — the theme fix that started this +- `scripts/tier2/artifacts/send_result_to_send_20260616/WHATS_SPECIAL.md` — what's unique about this test +- `scripts/tier2/artifacts/send_result_to_send_20260616/ARCHITECTURE_CHECK.md` — click chain isolation verification +- `scripts/tier2/artifacts/send_result_to_send_20260616/diag_*.py` — all diagnostic scripts (preserved for Tier 1 review) +- `logs/sloppy_*.log` — diagnostic logs + +## Recommendation + +**Defer the focus change to next frame's idle phase.** This is the smallest architectural fix. The full architectural question (whether imgui-bundle's per-frame stack usage is bounded) should be investigated separately — possibly by adding a stack-depth guard before each imgui draw frame, or by measuring imgui-bundle's actual C stack usage in test.