From a2c345aa8260fa7e9805474e0b68cdc96bd86a9d Mon Sep 17 00:00:00 2001 From: blasty Date: Fri, 7 Aug 2026 14:54:25 +0200 Subject: Record the test-speed findings in .fastfeedback/SPEED.md (force-added) The suite went from ~9m20s (with thumb_ui crashing and rawimage_rpc failing) to 146s, 788 passed / 0 failed, without deleting a single check. Every second of that came from four mistakes, all of which will be made again by the next person who does not know about them, so they are written down next to the commands: 1. waiting on a signal that can no longer happen (model-identity gates, after edits were changed to keep the listing's walk) -- 30s in blob_ui, 4x60s in thumb_ui, each followed by a check that then passed vacuously; 2. waiting on a signal that is set too early (app._t is assigned before the navigation it starts) -- a race master's millisecond backend won and Code Mode's loses; 3. regenerating a fixture with os.urandom into a temp dir, so the pristine database cache could never apply; 4. deleting a database and reopening the same path, which races the lease of the worker that still owns it. It also records the per-suite timings, which suite owns which behaviour (batch rename lives ONLY in test_rawimage_rpc -- four full pilot runs missed that bug), and two harness traps: `bgrun run` blocks the agent's tool call unless launched with setsid and all three fds redirected, and `pkill -f` matches the agent's own command line and kills the shell issuing it. Force-added over the .gitignore entry: this is the kind of thing that is worth more to the next reader than it costs in repo noise. Drop the file and the ignore rule together if that judgement is wrong. --- .fastfeedback/SPEED.md | 203 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 .fastfeedback/SPEED.md diff --git a/.fastfeedback/SPEED.md b/.fastfeedback/SPEED.md new file mode 100644 index 0000000..8dbe860 --- /dev/null +++ b/.fastfeedback/SPEED.md @@ -0,0 +1,203 @@ +# SPEED — ida-tui-maybe + +_Read this BEFORE running anything slow._ This repo does **not** use pytest for its +suites; every suite is a standalone script with its own runner and its own tally line +(`N passed, M failed` / `N checks, M failed`). `speedscan`'s generic pytest template is +wrong here — these are the real commands. + +--- + +## The two interpreters + +| python | has | use for | +|---|---|---| +| `python3` (system) | stdlib only | every **pure** suite. No IDA, no Code Mode. | +| `~/ida-venv/bin/python` | textual + idapro + ida_codemode | anything marked `ida`, and the TUI itself | + +`tests/run.py --list` prints `pure` / `ida` per file. A `pure` file must keep running +under system `python3` — that is a house rule, and it is why `idatui/codemode_client.py` +defers its `ida_codemode` import instead of doing it at module top. + +--- + +## Escalation ladder for THIS repo + +**Cheap gates (seconds, run these first)** +```bash +python3 -c "import ast;ast.parse(open('idatui/codemode_client.py').read())" # ~0.05s syntax +python3 tests/run.py --fast # every pure suite, 302 checks, 0.6s +python3 tests/run.py graph -x # one suite by substring, fail-fast +``` + +**Narrow test — DEFAULT RUNG.** The pilot suite takes `--only `: +```bash +~/ida-venv/bin/python tests/test_scenarios.py /tmp/scratch_bin --only rename # ~10s +~/ida-venv/bin/python tests/test_scenarios.py --list # scenario names +``` +`--only` matches substrings, so `--only listing` runs listing_view + listing_name_addr + +listing_make_string + listing_struct_expand. Name the scenario exactly to stay narrow. + +**Full gate — BACKGROUND ONLY, once, at the end** (see next section for the launch form): +```bash +~/ida-venv/bin/python tests/run.py # every suite, serial on purpose +``` + +--- + +## Coverage map — which suite catches what + +The pilot suite is big and slow and it is **not** the whole story. Two backend bugs +this port shipped were invisible to `test_scenarios.py` and only fell out of the +smaller RPC/UI suites: + +| suite | ~time | catches | +|---|---|---| +| `test_scenarios.py` | 166s | the TUI end to end: nav, views, listing, graph, opfmt, rename-one | +| `test_rawimage_rpc.py` | ~60s | **`rename_many` (batch rename), define/blob workflow, opfmt over RPC** | +| `test_blob_ui.py` | ~40s | raw-image/blob UI | +| `test_project_ui.py` | ~8s | multi-binary project UI | +| `test_trace_rpc.py` / `test_trace_ui.py` | ~30s | Tenet trace integration | + +**If you touch a backend operation, grep for its callers and run the suite that owns +them — not just the pilot.** `rename` is exercised one-edit-at-a-time by the pilot and +as a *batch* only by `test_rawimage_rpc.py`, which is exactly where the list-vs-dict +bug hid. + +--- + +## Backgrounding in THIS harness — the gotcha that cost a turn + +`bgrun run ... ` **blocks the agent tool call** even though bgrun itself returns in ms. +`bgrun` line 64 is `( "$@" >"$LOG" 2>&1; echo $? >"$EXIT" ) &` — the command's own stdio +goes to the log, but the *backgrounded subshell* still holds the inherited stdout fd, and +the harness's bash waits for EOF on that pipe, not for the parent to exit. + +**Always launch detached with all three fds redirected:** +```bash +SK="$HOME/.the assistant/agent/skills/fast-feedback/scripts" +timeout 20 setsid "$SK/bgrun" run fullgate -- ~/ida-venv/bin/python tests/run.py \ + /tmp/bgrun.out 2>&1 ; cat /tmp/bgrun.out # returns in ~7ms +timeout 20 "$SK/bgrun" check fullgate # non-blocking, safe +``` +`bgrun check` / `bgrun list` do **not** need this treatment; only `run` does. + +**Never `pkill -f ` here.** The agent's own shell command line contains the +pattern you are matching, so `pkill -f "tests/run.py"` kills the very shell issuing it +and the rest of the command silently never runs. Kill by **pid** (`bgrun list`, +`pgrep -af` first), then gate on it with `waitfor pid-gone `. + +**Killing a pilot leaves a Code Mode worker behind** for its `--lease-grace` (20s); it +self-exits, and it is holding only that run's temp `.i64`, so it does not block a new +run. Do not `kill -9` it — a hard-killed IDA wedges its database. + +--- + +## The four ways a test here wastes minutes + +Every slow suite in this repo was slow for one of these, not for doing real work. +Check them before optimising anything else. + +1. **A wait on a signal that can no longer happen.** `wait(lambda: lst.model is not + old, ..., 60)` was the idiom for "the edit landed". The perf work made edits KEEP + the listing's walk and re-render in place, so the model object is never replaced: + every one of those waits sat out its full timeout and the check afterwards passed + **vacuously**. This alone was 30s in blob_ui and 4x60s in thumb_ui. + → Gate on what the check is about (the row is code, the status says Thumb, the + function is in the index), via `settle(app, pred)` from `idatui._sync`. +2. **A wait on a signal that is set too early.** `app._t` is assigned when the key is + handled; the navigation it starts runs in a worker. Waiting on it and then reading + the cursor is a race master won (single-digit ms backend) and Code Mode loses. + → `settle()` gates on quiescence *and* the predicate, which is what you want. +3. **Regenerated fixtures.** `os.urandom` into a fresh `TemporaryDirectory` means new + bytes at a new path every run, so the pristine-database cache can never apply and + full auto-analysis is paid forever. → `_fixtures.synthetic(name, build)` writes + deterministic bytes to a stable path; `staged()` then caches the analysis. + Determinism is also correctness: "this blob has no functions" must not depend on luck. +4. **Deleting a database and reopening the same path.** Safe when the TUI owned a + private worker; under Code Mode the previous phase's worker still holds the lease + for its grace period, so the delete races a live owner and the reopen yields no + listing. → Give each phase its own temp copy (`fresh_copy`). Never `os.remove` an + `.i64` a suite is about to reopen. + +**`settle(app, pred, timeout=...)`** (`idatui/_sync.py`) is the one true gate: it drains +the message pump, waits for workers, and returns the moment `pred` holds. It is what the +live RPC layer uses, so tests and driver agree on what "done" means. A bare +`settle(app)` (no pred) means "the app finished reacting" — the right gate when the edit +may legitimately do nothing (e.g. carving random bytes). + +## Measured timings (2026-08-07, this box, warm) + +| command | time | notes | +|---|---|---| +| `python3 tests/run.py --fast` | **0.6s** | 302 checks, all pure suites | +| `python3 tests/test_graph.py` | ~1s | 86 checks, pure layout engine | +| `python3 tests/test_codemode_client.py` | ~0.1s | 14 checks, fakes the DatabaseHandle | +| `~/ida-venv/bin/python tests/test_rawimage_rpc.py` | ~60s | 21 checks, owns batch rename | +| pilot boot (first scenario) | ~10-25s | opens the DB; seeded from `.pristine.i64` | +| pilot `--only graph` | ~10s | 50 checks | +| **pilot full (`test_scenarios.py`)** | **80.5s** | 56 scenarios, 301 checks (was 166s) | +| `test_trace_ui.py` | 22.0s | 39 checks | +| `test_rawimage_rpc.py` | 13.1s | 21 checks, owns batch rename | +| `test_project_ui.py` | 8.1s | 30 checks | +| `test_thumb_ui.py` | **8.5s** | 20 checks — was 313s AND crashing | +| `test_blob_ui.py` | **4.1s** | 30 checks — was 39.8s | +| `tests/run.py` (everything) | ~2m20s | was ~9m20s. BACKGROUND ONLY | + +IDA-suite total went from ~9m21s to ~2m16s (4.1x) by fixing the four wastes above — +not by removing a single check. + +### Backend performance note (why the pilot is 2.8x slower on codemode) +`heads` is the listing's paging call. Measured on `targets/echo`, 200 rows: + +| | master (worker: pickle over unix socket) | codemode (HTTP + JSON + `to_jsonable`) | +|---|---|---| +| `heads count=200` | 2.6 ms | 92 ms | +| `heads count=500` | 6.3 ms | 214 ms | +| empty round trip | — | 2.0 ms | + +So the transport floor is 2ms/call and the rest is per-row work + `to_jsonable` +(~71ms of a 200-row page). This is architectural, not a bug in the port. The +digest/`expect` path (a page that has not changed is answered with a hash and a count, +no rows) is the main mitigation and is implemented. + +**Corollary for testing:** the codemode backend is slow enough that UI settle races +appear that never appear on master. See KNOWN-FLAKY. + +--- + +## Known-flaky + +- `test_scenarios.py --only listing_view` (**codemode backend only**). Passed 2 runs, + failed the 3rd, on an unchanged tree and a fresh scratch DB. Symptom: `listing shows + data heads (not just code)` reports `{'label','sep','funchdr','code'}` and + `total=1500`, i.e. the model is read before its pages have materialised. Adding *any* + delay (even a single `stderr.write`) makes it pass, which is the signature of a settle + race, not a logic bug. The scenario waits on `c.lst.total > 0`, which becomes true + before the rows exist. **Do not chase this as a regression without reproducing it + 3+ times.** Real fix would be a wait on materialised rows, not on `total`. + +--- + +## Slow traps + +- **Never run the pilot suite against `targets/echo` directly.** It edits the database. + The suite already stages a scratch copy (`tests/_fixtures.py: staged()`), seeded from + `.pristine.i64` which nothing writes back to. Pass a **copy** in `/tmp` when + invoking by hand, or you are testing your own history. +- **`tests/run.py` runs suites serially on purpose.** Running them 4-up took the suite + from 153s to 296s and got three killed mid-analysis (idalib contends hard). Do not + "optimise" it with parallelism. +- **Cold analysis dominates a first run.** `.pristine.i64` turns ~30s of auto-analysis + into a file copy; if it is missing or older than the binary it is rebuilt. A suite that + suddenly takes minutes longer is usually rebuilding that cache. +- **Under load, idalib gets SIGKILLed mid-analysis** and it looks like a hang or empty + output. Check `uptime` before believing a failure. +- **`experiments/*.py` need `PYTHONPATH=$PWD`** under `~/ida-venv/bin/python` (they are + scripts, not a package entry point). + +## Comparing against master (backend A/B) + +The port replaces the whole backend, so "is this a regression?" means running the same +scenario on both. `git stash -u; git checkout master; ; git checkout -; git stash pop` +works, but **use a different scratch binary per branch** (`/tmp/x_master`, `/tmp/x_port`) +so the two runs never share a `.pristine.i64` or a live Code Mode instance. -- cgit v1.3.1-sl0p