From c9208de05d8583b677117fe43c9d3567e89eb2ce Mon Sep 17 00:00:00 2001 From: blasty Date: Fri, 7 Aug 2026 12:39:54 +0200 Subject: Rebase MISTER EXO's ida-codemode port onto the current tree Mechanical part of the port: the 27-file patch was cut against a base ~148 commits behind us, so it did not apply. Resolved 11 conflicts (all of them diff drift, not semantic clashes) and the three file deletions: - app.py: the patch re-inserted _do_rename/_do_name_addr/_seek_split etc. as "theirs" because our tree moved them to edit_ctl.py/trace_ctl.py. Kept ours and applied the real intent (WorkerClient->CodeModeClient, .call->.invoke, _open_worker_client->_open_database_client) at their current homes. - domain.py: kept Head as a NamedTuple -- the patch reverted it to a frozen dataclass, which the perf work measured at 2.9us vs 1.9us per row on a quarter-million-row walk. Dropped _fetch_output (no download_url under Code Mode) and its now-dead urllib/json imports. - pane.py: the patch's deletion swallowed our zellij support along with the worker-reaping block it meant to remove. Kept zellij, removed the reaping. - test_scenarios.py: the idb_save->save_database teardown hunk belongs to tests/_fixtures.py in our tree; applied it there and kept our pc_num_format scenario that the drift landed on. Three defects in the patch itself, fixed here: - It made "import idatui" hard-require ida_codemode, so every offline suite died at import -- including the pure ones (graph/index/trace) that are the house rule for "tests/run.py --fast". The import is now deferred and gated on the binding, which is also what lets the port's own contract tests inject a fake DatabaseHandle. - project.stage() inlined an ida_codemode.registry import and treated "library not installed" as "someone owns this database", which broke IDA-free project staging. Ownership lookup moved to codemode_client.database_owner(). - tests/test_codemode_client.py had no NEEDS_IDA marker, which tests/run.py rejects outright. Offline suite: 301 passed, 0 failed. Against master's 344 the whole delta is accounted for: -40 worker_client (module deleted), -18 launch sweep checks (behaviour deliberately removed) +3 guarding that it stays removed, +2 pool (GUI-save semantics), +13 new codemode_client contract tests. NOT yet done, and the port is not functional without it: the adapter is missing five operations our tree grew since the patch's base (flowchart, op_format, pc_nums, pc_num_format, survey_binary) and its "heads" predates back-walking and digest/expect. --- docs/CODEMODE_PORT.md | 175 ++++++++++++++++++++++++++++++++++++++++++++++++ docs/PAGING_FINDINGS.md | 54 +++++++-------- docs/PROJECTS.md | 41 ++++++------ docs/SPLIT_VIEW.md | 12 ++-- 4 files changed, 226 insertions(+), 56 deletions(-) create mode 100644 docs/CODEMODE_PORT.md (limited to 'docs') diff --git a/docs/CODEMODE_PORT.md b/docs/CODEMODE_PORT.md new file mode 100644 index 0000000..4b4bc45 --- /dev/null +++ b/docs/CODEMODE_PORT.md @@ -0,0 +1,175 @@ +# ida-tui → IDA Code Mode port + +This port is an experiment: can ida-tui be implemented as an ordinary client of +`ida_codemode`, sharing GUI databases and managed idalib workers instead of +owning a private worker and depending on ida-pro-mcp tool functions? + +## Result + +Yes for the database lifecycle and the complete current TUI feature set, with a +small number of operations implemented using IDAPython inside Code Mode's +`execute_python` sandbox because ida-domain does not yet expose the required +behavior. + +The old components are gone: + +- `idatui/worker.py` (private pickle/socket idalib process) +- `idatui/worker_client.py` +- `server/patch_server.py` (ida-pro-mcp tool injection) + +The replacement is `idatui/codemode_client.py`. + +## Lifecycle mapping + +`CodeModeClient.connect()` calls `ida_codemode.client.DatabaseHandle.open()`. +Resolution is therefore Code Mode's resolution, not ida-tui's: + +1. Match a registered GUI by executable path. +2. Otherwise match the owner of the expected IDB. +3. Otherwise serialize creation and start a managed `ida-codemode-worker`. +4. Establish an authenticated SSE lease. +5. Wait through the public autoanalysis route. + +The handle's registry entry supplies the backend, PID, executable path, IDB path, +and record ID used by the status/pool layers. + +Closing ida-tui closes only its lease. It never closes a GUI or kills an idalib +process. A managed worker saves and exits under Code Mode's own policy after its +last lease disappears. A second agent or TUI can keep using the same instance. + +This also changes project pooling semantics. `DatabasePool` is an LRU pool of +leases, not process ownership. Managed-IDB save-on-evict remains; budget eviction +does not implicitly save a GUI. Eviction cannot force a shared worker to exit, +and GUI process memory is only advisory. + +## ida-domain coverage + +The remote snippets receive Code Mode's preloaded `db` (`ida_domain.Database`). +The following TUI needs map to public ida-domain entities: + +| TUI need | ida-domain surface | +|---|---| +| Function paging, lookup, names, sizes | `db.functions` | +| Segments and names | `db.segments` | +| Instructions and plain disassembly | `db.instructions`, `db.functions.get_instructions()` | +| Heads and item classification | `db.heads`, `db.bytes` | +| Bytes and strings | `db.bytes`, `db.strings` | +| Symbol resolution and rename | `db.names`, `db.functions` | +| Comments | `db.comments` | +| Imports and exports | `db.imports`, `db.entries` | +| Xrefs and fine type predicates | `db.xrefs` / `XrefInfo` | +| Named types, members, parse/apply | `db.types` | +| Function prototypes and local variables | `db.pseudocode`, `PseudocodeFunction.local_variables` | +| Decompilation text and object references | `db.pseudocode` | + +All values are reduced to JSON primitives inside the database process. No SWIG +or ida-domain object crosses the Code Mode boundary. + +## Remaining IDAPython gaps + +Code Mode intentionally allows regular Python imports, so these features still +work, but they identify useful additions to ida-domain: + +1. **Rich continuous listing** + - ida-domain enumerates defined heads and renders plain disassembly. + - ida-tui also needs coalesced undefined runs, IDA colour-tag spans, function + banners, code-label rows, file-region offsets, and expanded struct members. + - The `heads` operation uses `ida_bytes`, `ida_lines`, and related modules for + this presentation model. + +2. **Instruction/function carving** + - Creating an instruction and walking a speculative decode run requires + `ida_ua.create_insn` and processor flow/return checks. + - Function creation exists in ida-domain; the explicit-end fallback still + needs lower-level item boundaries. + +3. **ARM/Thumb state** + - T-register ranges and segment addressing use `ida_segregs`, `ida_idp`, and + `ida_segment`. There is no equivalent ida-domain operation. + +4. **Detailed decompiler diagnostics and line maps** + - Pseudocode text, ctree objects, and the address map are available through + ida-domain. + - Reproducing IDA's per-rendered-line coverage uses + `cfunc.get_line_item`; obtaining the exact Hex-Rays failure description + uses `hexrays_failure_t`. + +5. **A few type/item primitives** + - Deleting a named local type and some exact item-undefinition/data-creation + behavior still use `ida_typeinf`/`ida_bytes` directly. + +These uses are isolated in `idatui/codemode_client.py`; the paging and Textual +layers do not import IDAPython. + +## API limitations exposed by the port + +### No rollback or close-without-save + +A Code Mode lease has no rollback operation. Closing a GUI handle leaves the GUI +state as-is. A managed idalib worker currently saves when its final lease closes. +Consequently ida-tui's old “discard & quit” guarantee cannot be implemented. +The UI now labels this choice “leave as-is & quit” and does not explicitly save, +but managed-worker policy may still persist the changes. + +A true discard action would need a Code Mode/database API for transaction-like +rollback, a close policy on a newly-owned worker, or a TUI-managed disposable DB +copy. + +### Typed loader options only + +`DatabaseHandle.open()` supports processor, natural loading address, file type, +output database, and fresh-database selection. It does not support ida-tui's +arbitrary `ida_args` escape hatch. The adapter rejects unsupported switches +rather than silently loading at the wrong architecture/base. + +### No database-change notification stream + +The lease reports liveness, not mutations. If a GUI user or another Code Mode +client renames/retypes content while ida-tui is open, already-materialized TUI +caches are not invalidated automatically. TUI-originated edits invalidate their +own caches correctly. A database revision counter or change feed would make +shared interactive editing robust. + +### Discovery requires a path for ambiguity + +`ida-tui` with no path attaches automatically when exactly one database is +registered. With several registrations it lists them and requires an explicit +executable/IDB path. There is not yet a pre-connection database picker in the +Textual UI. + +### `DatabaseHandle` import stability + +The usable library primitive currently lives at +`ida_codemode.client.DatabaseHandle`; `ida_codemode.__init__` exports nothing. +The port therefore depends on a submodule path. Exporting the handle and public +client exceptions from the package root would make the supported library API +clearer. + +## Safety differences + +ida-tui no longer removes `.id0/.id1/.id2/.nam/.til` files before opening. That +was only defensible when the TUI exclusively owned a private process; it is +unsafe when a GUI or another client may own the database. Code Mode registry +locks, health probes, and IDA itself now arbitrate ownership. + +The old pane “reap private workers” behavior is obsolete. A TUI crash closes its +lease at the socket/kernel boundary; Code Mode decides whether a managed worker +still has clients and when it should stop. + +## Verification surfaces + +The non-IDA suite verifies project staging, LRU lease behavior, load-option +translation, and adapter response/error normalization. The existing live suites +remain the end-to-end contract: + +```sh +uv run python tests/test_codemode_client.py +uv run python tests/test_pool.py +uv run python tests/test_project.py +uv run python tests/test_scenarios.py /path/to/binary +``` + +For GUI reuse, open the same binary in an IDA with the Code Mode plugin, confirm +it appears in `ida_codemode.registry.discover_instances()`, then launch +`ida-tui /path/to/binary`. The TUI status/`CodeModeClient.backend` should report +`gui`, and closing the TUI must leave IDA open. diff --git a/docs/PAGING_FINDINGS.md b/docs/PAGING_FINDINGS.md index bcde583..bd6c38f 100644 --- a/docs/PAGING_FINDINGS.md +++ b/docs/PAGING_FINDINGS.md @@ -2,10 +2,10 @@ Measured against a real target: `libcrypto.so.3` (5.7 MB, **10,092 functions**, biggest function **52,120 instructions**). These constraints drive the domain / -paging layer. They describe the ida-pro-mcp *tool functions* (`list_funcs`, -`disasm`, `decompile`, `xref_query`, …) which the idalib worker now calls -in-process (`idatui/worker.py`) — the shapes and caps below are the tools' -behaviour and are unchanged by dropping the HTTP transport. +paging layer. The measurements below came from the former ida-pro-mcp tool +backend. The Code Mode port preserves the adapter response shapes and conservative +page sizes, but executes enumeration through ida-domain; old server caps and RTT +numbers are historical rather than Code Mode constraints. ## Response shape (list_* / *_query tools) @@ -93,32 +93,26 @@ disasm totals are **top-level** fields, not under `asm`: (correct). The pseudocode view must handle "decompilation failed" gracefully — fall back to the disassembly view or show an error panel. -Normal decompile bodies are server-truncated with a `[N chars total]` marker -(still to be solved for full-body display — see Phase 2). - -## Worker lifecycle (idatui's own idalib worker) - -idatui no longer uses ida-pro-mcp's shared HTTP supervisor. `idatui/worker.py` -opens exactly **one** database with `idapro.open_database(...)` in its own process -and serves tool calls over a unix socket (`WorkerClient`). Consequences vs the -old supervisor model, which several design choices here were built around: - -* **No `max_workers` cap, no cross-session contention.** Each TUI owns its - worker; there is no "Maximum idalib worker count reached" and no shared license - slot to free. -* **No idle self-exit / keepalive dance.** The old per-worker `WorkerLifecycle` - watchdog (`idle_ttl_sec`, default 600s) and the `KeepAlive` heartbeat that - fought it are gone with the supervisor. The worker lives as long as the TUI - holds the socket and dies with it. `WorkerClient.keepalive()` is a no-op kept - for API parity, and `--ttl` is passed through but the single owned worker does - not self-reap. -* **A crashed worker drops the socket**, surfacing as `IDAConnectionError`; the - app's `_reconnect` respawns a fresh worker (re-opening + re-analyzing the - binary). The hard-kill lock recovery below still applies. +Code Mode returns the complete execution result directly; ida-tui no longer +needs MCP structured-content/download-URL recovery for large pseudocode bodies. + +## Code Mode lifecycle + +`CodeModeClient` owns an authenticated SSE lease on a registered database: + +* A matching GUI is preferred and remains open when the TUI exits. +* Otherwise Code Mode reuses or starts a shared managed idalib worker. +* Releasing one lease never terminates another client's session. A managed + worker saves and exits after its final lease under Code Mode's grace policy. +* Lease loss surfaces as `IDAConnectionError`; reconnect performs discovery + again and may bind a newly-created instance. It does not silently swap the + handle underneath an operation. +* `--ttl` and the old keepalive flag are compatibility no-ops; the lease itself + carries heartbeats. ## Writable path requirement (operational) -`idb_open` writes the `.i64` next to the input binary, so the path must be -**writable**. Opening from read-only dirs (e.g. `/usr/lib`) fails with -`"Failed to open database"`. Copy targets into a writable dir first -(`targets/` in this repo). +Attaching to a registered GUI does not require ida-tui to write beside the input. +Creating a managed database does require a writable output path. Multi-binary +projects provide one in their sidecar. ida-tui does not sweep IDA scratch files, +because another registered session may own them. diff --git a/docs/PROJECTS.md b/docs/PROJECTS.md index fe6c2a8..0efee31 100644 --- a/docs/PROJECTS.md +++ b/docs/PROJECTS.md @@ -7,9 +7,11 @@ search across all of them, and (later) follow calls from one into another. ## The constraint that shapes everything -`idatui/worker.py` is `serve(sock, binpath)` — **one worker process holds exactly -one database** (idalib is main-thread-only and single-DB). So N binaries = N -worker processes, each with the analyzed DB resident. +IDA still exposes one active database per GUI/idalib process. Code Mode makes +those instances discoverable and shareable: each project entry retains one +`DatabaseHandle` lease, which may target a registered GUI or a managed idalib +worker. N resident project databases can therefore mean up to N processes, but +ida-tui no longer owns or terminates them. Measured cost (this box, `targets/`): @@ -30,13 +32,13 @@ crypto library. Two capabilities that feel like one, but aren't: -1. **Switching** to a binary needs a *live worker*. +1. **Switching** to a binary needs a *live Code Mode lease*. 2. **Searching across** binaries does *not* — if a per-binary index (functions, strings, imports/exports) is cached on disk. That split is the unlock: project-wide search stays instant across every binary, including ones never opened this session, and only *jumping* to a hit costs a -worker spawn. +Code Mode attach/open. ## Layout @@ -83,17 +85,16 @@ basename and must be unique (it names the staged file). ## Runtime -- **`WorkerPool`** — one `WorkerClient` per binary, spawned lazily on first - switch, kept resident until the memory budget is exceeded, then LRU-evicted. - Eviction **saves the DB first**, so returning to a binary is a DB load, not a - re-analysis. Binaries can be pinned to stay resident. +- **`DatabasePool`** — one `CodeModeClient` lease per resident binary, attached + lazily on first switch and LRU-released when the advisory memory budget is + exceeded. Eviction explicitly saves managed IDBs but never implicitly saves a + GUI. Closing a lease never kills a GUI or another client's managed worker; + Code Mode owns final worker shutdown. - **`BinaryState`** — per binary: `client, program, nav, cur, func_index, pref/active/split, filter`. Switching snapshots the current state and restores - the target's. `_after_reconnect` already does exactly this swap (client + - program, reload the index, re-open the entry) — switching reuses that seam. -- **Clean shutdown** — the worker currently does `close_database(save=False)` and - is hard-killed on exit, which is why wedge files accumulate. Projects need - save-on-evict and an orderly close anyway, so that gets fixed here. + the target's. `_after_reconnect` provides the client/program swap seam. +- **Clean shutdown** — release all leases. Managed idalib workers save/close on + their own main thread after the final lease; GUI sessions remain open. ## UI @@ -109,8 +110,8 @@ basename and must be unique (it names the staged file). ## Phases **Phase 1 — project model + switching. DONE.** Project file + staging -(`idatui/project.py`), `WorkerPool` with budget eviction / save-on-evict / -clean shutdown (`idatui/pool.py`), `BinaryState` snapshot+restore and the switch +(`idatui/project.py`), `DatabasePool` with budgeted lease release and +save-on-evict (`idatui/pool.py`), `BinaryState` snapshot+restore and the switch itself, the `Ctrl+O` switcher palette, the active binary in the status line, and `--project` (which creates the project when given binaries). One active binary; no cross-binary search yet. @@ -118,9 +119,9 @@ no cross-binary search yet. Project mode is **additive**: with no `--project` the app is byte-for-byte the single-binary tool it was, which is what keeps the 167-check pilot honest. Switching reuses the `_after_reconnect` shape — swap client+program, rebuild the -index, reopen the entry. A binary whose worker is still resident restores -instantly (its `Program` and index are still in memory); an evicted one comes -back with a fresh worker but keeps its nav history, since that is just addresses. +index, reopen the entry. A binary whose lease is still resident restores +instantly (its `Program` and index are still in memory); an evicted one attaches +again but keeps its nav history, since that is just addresses. **Phase 2 — index cache + project-wide search. (symbols done)** `idatui/index.py` keeps one **SQLite FTS5 trigram** index at @@ -222,7 +223,7 @@ records nothing — that's not navigation. *Pre-warm follows the linkage graph, not list order.* When a binary finishes indexing, `_prewarm_provider` warms the binary that provides the most of its imports — where a follow is most likely to take you, so its startup is paid -before you ask. `WorkerPool.prewarm()` refuses rather than evicting: spending a +before you ask. `DatabasePool.prewarm()` refuses rather than evicting: spending a binary you visited on one you haven't is a straight downgrade, and it would throw away that binary's caches too. At a tight budget pre-warm simply does nothing. It estimates the cost of a not-yet-spawned worker from the largest resident one, diff --git a/docs/SPLIT_VIEW.md b/docs/SPLIT_VIEW.md index de37dc1..c421656 100644 --- a/docs/SPLIT_VIEW.md +++ b/docs/SPLIT_VIEW.md @@ -29,11 +29,11 @@ Ghidra highlights **all** instructions a C line owns. We have one ea per line (the marker), not the set. Getting the set is the only real work, and it's a known technique: -ida-pro-mcp derives the per-line marker via +The old ida-pro-mcp backend derived the per-line marker via `cfunc.get_line_item(line, col=0, …).get_ea()`. To get the **full set**, sweep every column of the line (`get_line_item(line, x, …).get_ea()` for `x` in -`0..len`) and collect distinct non-`BADADDR` EAs. Same proven API, swept across -the line. A custom `decomp_map(ea)` tool in `server/patch_server.py` returns +`0..len`) and collect distinct non-`BADADDR` EAs. The Code Mode adapter's +`decomp_map(ea)` operation returns `[{line, primary_ea, eas:[…]}, …]`; invert for `ea → line`. ## State model @@ -78,14 +78,14 @@ decomp→listing uses `ListingModel.ensure_ea`. Tab re-links from the new driver Still single-ea per line (one instruction highlighted); the region comes in phase 3. -**Phase 3 — rich highlight. DONE.** `decomp_map` custom tool -(`server/patch_server.py`) sweeps `cfunc.get_line_item` across every column of +**Phase 3 — rich highlight. DONE.** The Code Mode `decomp_map` operation +(`idatui/codemode_client.py`) sweeps `cfunc.get_line_item` across every column of each pseudocode line and collects the EAs from each item's `dstr()` (`'EA: desc'` — the same source as the `/*ea*/` marker, so it aligns). `Program.decomp_map(ea)` returns the per-line ea lists (cached by name-gen); the app loads it async into `_split_eamap` / `_split_ea2line` and `_sync_split` bands the **whole** instruction region of a C line (and uses the exact ea→line inverse for the reverse). Falls -back to the single marker until the map lands. Verified on the pilot's real worker +back to the single marker until the map lands. Verified on a real Code Mode database (alignment + multi-instruction region band). **Phase 4 — polish. DONE.** -- cgit v1.3.1-sl0p From 1ee6f15d4940138153e439bd97ea33f5b1fb56ba Mon Sep 17 00:00:00 2001 From: blasty Date: Fri, 7 Aug 2026 15:09:40 +0200 Subject: docs: upstream findings for the ida-codemode maintainers Nine items found while porting a latency-sensitive client, each with the measurement behind it and a suggested fix, ordered by what it cost us. The two that matter: timeout_trace returns itself, which turns on line tracing in every frame (52x on ida_bytes.get_flags, 10x on a listing page), and to_jsonable walking large results (114x what json.dumps of the same data costs). Together they were the difference between 35x slower than the worker we replaced and within 2x of it. Both live in the runtime, so every client pays them until they are fixed centrally -- we only found them because we had an in-process implementation to compare against. Also records the things we had to reach into internals for (waiting for a lease to release), the failure modes that are silent or fatal rather than typed (loader switches on an existing IDB, an IDB replaced under a live lease), and the one feature we could not port at all (discard-and-quit, since a managed worker saves when its final lease closes). --- docs/CODEMODE_UPSTREAM.md | 269 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 docs/CODEMODE_UPSTREAM.md (limited to 'docs') diff --git a/docs/CODEMODE_UPSTREAM.md b/docs/CODEMODE_UPSTREAM.md new file mode 100644 index 0000000..f903599 --- /dev/null +++ b/docs/CODEMODE_UPSTREAM.md @@ -0,0 +1,269 @@ +# Findings from porting a real client to IDA Code Mode + +Notes for the `ida-codemode` maintainers, gathered while porting **ida-tui** (a +Textual TUI frontend for IDA) from a private idalib worker to +`ida_codemode.client.DatabaseHandle`. + +Everything below is measured, not inferred. Where we worked around something, the +workaround is named so you can judge whether the library should make it +unnecessary. + +**Environment:** ida-codemode 0.3.1, IDA 9.4 (idalib), Linux, single managed +worker backend, quiet box. Target for timings: `targets/echo` unless stated. + +**What the client does**, for scale: it renders a continuous disassembly listing, +pseudocode, a CFG graph view and a hex view, paging over the database as the user +scrolls. It is latency-sensitive in a way an agent-driven MCP client is not — a +keypress must repaint. It issues ~1–8 operations per user action. + +--- + +## 1. `timeout_trace` enables line tracing in every frame — 52x on IDA calls + +**Highest-impact item by a wide margin.** + +`runtime.py` wraps every `execute_python` in `sys.settrace(timeout_trace)` to +enforce the deadline. `timeout_trace` ends with `return timeout_trace`, and +returning a trace function from a `'call'` event asks CPython to trace **every +line of that frame**. So every line of every function the snippet touches pays a +Python-level callback, and the specialising interpreter is disabled throughout. + +Measured inside the worker, same process, same database: + +| | traced (stock) | untraced | native idalib | +|---|---|---|---| +| `ida_bytes.get_flags(ea)` | 5.49 µs | 0.106 µs | 0.119 µs | +| our 200-row listing page | 20.2 ms | 2.0 ms | — | + +Untraced matches a plain idalib process, so the trace hook accounts for +essentially all of it. For us this was the single largest cost in the port — +larger than HTTP, serialisation and IDA itself combined. + +Reproduce inside any `execute_python`: + +```python +import sys, time, ida_bytes +def bench(): + t = time.perf_counter() + for _ in range(20000): ida_bytes.get_flags(0x1000) + return (time.perf_counter() - t) / 20000 * 1e6 +traced = bench() +old = sys.gettrace(); sys.settrace(None) +try: untraced = bench() +finally: sys.settrace(old) +result = {"traced_us": traced, "untraced_us": untraced} +``` + +**Suggested fixes, cheapest first** + +1. `return None` from `timeout_trace` instead of itself. You keep `'call'`-event + deadline checks — which is enough to interrupt anything that calls a function + — and drop per-line tracing entirely. +2. On 3.12+, use `sys.monitoring` with only the events you need; it is designed + for exactly this and is far cheaper than `settrace`. +3. Or drop the trace and rely on the `threading.Timer` → + `ida_kernwin.set_cancelled()` path you already have, accepting that a + pure-Python loop with no calls in it cannot be interrupted. + +**Our workaround** (we would rather not ship it): the snippet detaches the trace +and restores it in a `finally`. That gives up deadline enforcement for +pure-Python loops inside our own code; your native cancel timer is unaffected and +still fires. Every client that does real work per call will eventually find this +and do the same, which is an argument for fixing it in the runtime. + +--- + +## 2. `to_jsonable` dominates any large result + +`execute_python` runs `to_jsonable()` over whatever the snippet returns. Our +answers are already JSON-safe and they are big — a 200-row listing page is +roughly 10k small objects. + +| | cost | +|---|---| +| `to_jsonable(page)` | 66.2 ms | +| `json.dumps(page, separators=(",",":"))` — same data | 0.58 ms | +| serialised size | 34.9 KB | + +That is 114x, and it was 72% of the page's total cost before we changed it. + +**Suggested fixes** + +- Fast-path values that are already JSON-safe (a cheap recursive type check that + bails to the original object beats rebuilding it), or +- let a snippet opt out by returning an already-serialised payload — a documented + envelope such as `{"__json__": "<...>"}`, or simply passing `str`/`bytes` + through untouched. + +**Our workaround:** snippets `json.dumps` inside the database process and return +one string, which the client parses. `to_jsonable` then walks a single scalar. +Cost went 66.2 ms → ~0.6 ms. It works, but every client with a large result set +has to discover and re-implement it. + +--- + +## 3. The per-operation floor is `execute_sync`, not HTTP + +Same worker, same connection, 200 iterations: + +| | cost | +|---|---| +| `GET /health` (no `execute_sync`) | **0.165 ms** | +| `execute_python("result = 1")` | **2.025 ms** | + +HTTP framing is ~7% of the floor; marshalling the operation onto IDA's main +thread is the other ~93%. The worker runs IDA's own `kernwin.serve()`, so this is +plausibly IDA's dispatch latency rather than anything you control — but it is +worth **documenting**, because it sets a hard 2 ms per-operation budget that +shapes how a client must be designed. + +It did not hurt us (our call volume is 1–8 per user action; 4 calls to build a +1060-block graph), but a client that makes one call per row or per symbol will be +20–100x slower than an in-process one and the authors will not know why. + +**Suggested fixes:** document the floor; and consider a batch endpoint — accept +`[{op, args}, ...]` and dispatch them within a single `execute_sync` — which +would let chatty clients amortise it without redesigning around it. + +--- + +## 4. Loader switches on an existing database are a FATAL, not an error + +Opening a target that already has an `.i64`, while passing spawn-only options, +kills the worker: + +``` +FATAL ERROR: @0:636[] +Switch '-b400' can be used only when loading a new file +``` + +The client sees only: + +``` +IDAConnectionError: idalib worker launcher exited with status 1 +``` + +This is easy to hit and hard to diagnose: it is the natural second run of +anything that opens a raw blob (`processor=`/`image_base=`/`file_type=` are +recorded in the database the first run produced). Our test suite hit it as a +crash five minutes into a run. + +**Suggested fixes** + +- In `DatabaseHandle.open()`, when the resolved IDB already exists and + `new_database` is not set, either ignore the spawn-only options or raise a + typed error naming them — before handing them to IDA. +- Propagate the worker's fatal text into the client exception. The message + already exists on the worker's stderr; losing it turns a one-line fix into a + bisect. + +**Our workaround:** the client checks whether the expected IDB exists and drops +`processor`/`image_base`/`file_type` when it does. + +--- + +## 5. Deleting or replacing an IDB under a live lease fails silently + +A suite that did "delete the `.i64`, reopen the same path" (safe when it owned a +private worker) now races the previous worker's lease grace. The reopen produced +a handle that never became usable, with no error — just a database with no +listing, and every wait timing out. + +**Suggested fixes** + +- Detect that the IDB backing a registered instance has been removed or replaced + and fail loudly (the registry already holds `idb_key`). +- Expose a **public** "wait until this database is released" primitive. We needed + one and ended up reaching into `registry.REGISTRY_DIR` and `FileLock` to build + it, which is not an API we should be depending on. +- Document the lease-grace window as part of the lifecycle contract. + +--- + +## 6. No close-without-save, and no rollback + +A managed worker saves when its final lease closes. A GUI handle leaves GUI state +as-is. Neither gives a client a way to say "discard what I did". + +ida-tui had a "discard & quit" that we could not port; it is now "leave as-is & +quit", and we cannot honestly promise the user their edits are not persisted. + +**Suggested fixes:** a close policy on a lease the client created +(`close(save=False)`), or a transaction/rollback API, or a documented +disposable-copy pattern that clients can follow. + +--- + +## 7. No change notification for shared databases + +The lease reports liveness, not mutations. If a GUI user or another Code Mode +client renames or retypes while we are attached, our materialised caches (name +generation, decompilation, listing pages) are silently stale. Our own edits +invalidate correctly; someone else's cannot. + +**Suggested fix — cheap and sufficient:** a monotonic database revision counter, +bumped on any mutating operation and exposed on `/health` (and ideally on the +lease event stream). Clients can then invalidate by comparing one integer. A full +change feed would be better but is much more work; the counter alone would make +shared editing safe for every caching client. + +--- + +## 8. Package exports and API surface stability + +`ida_codemode/__init__.py` exports nothing, so a library consumer must import +from submodules: + +```python +from ida_codemode.client import DatabaseHandle, ClientError, RemoteError, InstanceDisconnectedError +from ida_codemode.registry import REGISTRY_DIR, FileLock, RegistryEntry, canonical_path, idb_key, scan_instances +from ida_codemode.resolver import IdbBusy, expected_idb_path +``` + +Some of those are clearly internals (`FileLock`, `REGISTRY_DIR`) that we only +touch because no public equivalent exists (see §5). + +**Suggested fix:** export `DatabaseHandle` and the public exception types from the +package root, and mark the intended-public registry helpers explicitly. It also +makes "what is API and what is internal" answerable, which right now it is not. + +--- + +## 9. A testing note: `DatabaseHandle.open()`'s 30 keyword-only options + +The port we started from called `open(..., loading_address=...)`. The real +parameter is `image_base`. Every `connect()` would have raised `TypeError` on the +first call, and its contract tests passed anyway, because a hand-written fake +handle accepts `**kwargs`. + +Not a library bug — but with 30 keyword-only options it is a very easy mistake, +and it is invisible to exactly the offline tests people write. + +**Suggested fix:** ship `py.typed` and/or a `Protocol` for the handle, so a fake +can be checked against the real signature and a typo is caught statically. (We +added a test asserting our kwargs are a subset of +`inspect.signature(DatabaseHandle.open).parameters`, which is a poor substitute.) + +--- + +## Priority, from a client author's view + +| # | item | impact | fixable by you? | +|---|---|---|---| +| 1 | `timeout_trace` line tracing | 52x on IDA calls, 10x on real operations | yes, one line | +| 2 | `to_jsonable` on large results | 114x on serialisation | yes | +| 7 | no change/revision counter | correctness for shared editing | yes, cheap | +| 4 | loader switches fatal on reopen | crashes, hard to diagnose | yes | +| 5 | replaced/deleted IDB under lease | silent hang | yes | +| 6 | no close-without-save | a feature we had to drop | design question | +| 8 | package exports | forces internal imports | yes, trivial | +| 3 | 2 ms `execute_sync` floor | shapes client design | document; maybe batch | +| 9 | typed handle for fakes | catches a whole bug class | yes | + +Items 1 and 2 together were the difference between "the port is 35x slower than +the private worker it replaced" and "the port is within 2x, and faster on several +operations". Both are in the runtime, not in client code — which is why they are +worth fixing centrally rather than leaving each client to rediscover. + +Happy to supply the benchmark harness (it is backend-agnostic and runs against +both our old worker and Code Mode), or to test a patch. -- cgit v1.3.1-sl0p