1 files changed, 269 insertions, 0 deletions
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 <pid> 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.