fix: stabilize managed idalib IDB lifecycle on Windows - #44
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes Windows worker shutdown/persistence and transport behavior in ways that are hard to fully validate without real Windows + IDA runtime verification.
Pull request overview
This PR hardens the Windows managed-idalib worker lifecycle by switching to a more reliable loopback transport, preserving canonical IDB identity when reopening packed databases, and adding an authenticated shutdown path so hidden workers can pack/close databases instead of being force-killed.
Changes:
- Prefer IPv6 loopback (
::1) for managed workers on Windows when bind-tested, with IPv4 fallback, and ensure bracketed IPv6 base URLs (http://[::1]:port). - Add an authenticated shutdown JSON-RPC method (
ida-multi-mcp/shutdown) to let the manager request a clean worker stop, enabling packed-save + close rather than abrupt termination. - Retry/fail-closed behavior around canonical module metadata for IDB inputs to avoid registering an instance under an ambiguous/non-canonical identity; extend tests around all of the above.
File summaries
| File | Description |
|---|---|
| tests/test_idalib_worker_download_url.py | Adds IPv6 URL bracketing, packed-close behavior tests, and shutdown RPC token enforcement tests for the worker. |
| tests/test_idalib_manager.py | Adds tests for IPv6 port binding, Windows loopback host preference, shutdown request success parsing, metadata retry/fail-closed, and graceful termination behavior. |
| tests/test_http_ipv6.py | Adds unit tests for IPv6 Host header parsing and IPv6 HTTP server-class selection. |
| src/ida_multi_mcp/vendor/zeromcp/mcp.py | Adds IPv6-capable HTTP server classes, robust Host parsing via urlparse, and selects IPv6 server classes when hosting on ::1. |
| src/ida_multi_mcp/idalib_worker.py | Implements bracketed origin construction, authenticated shutdown RPC registration, and packed-save-on-close with fallback to save-on-close. |
| src/ida_multi_mcp/idalib_manager.py | Implements Windows IPv6 loopback preference, per-worker shutdown tokens passed via env, shutdown RPC request helper, and IDB canonical-metadata retry/fail-closed logic. |
| src/ida_multi_mcp/ida_mcp/zeromcp/mcp.py | Mirrors the IPv6 server-class + Host parsing improvements for the HTTP transport copy. |
| src/ida_multi_mcp/ida_mcp/http.py | Extends strict Origin/Host validation to accept bracketed IPv6 loopback. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 290cc46eb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| requested = _request_worker_shutdown(host, port, shutdown_token) | ||
| try: | ||
| proc.wait(timeout=_WORKER_SHUTDOWN_TIMEOUT if requested else 5) |
There was a problem hiding this comment.
When idalib_close runs concurrently with a long IDA request, the worker's non-threaded HTTPServer cannot process the shutdown POST until that request completes; meanwhile _request_worker_shutdown times out after 10 seconds, and this false branch waits only 5 more seconds before entering the signal/kill fallback. Since routed calls may run for up to 300 seconds and CTRL_BREAK_EVENT is unreliable for these hidden Windows workers, closing a busy session can still reach TerminateProcess, bypassing the new packed close and risking stale or loose IDB files. The shutdown path needs to wait for the queued RPC for at least the supported in-flight request duration or use an independently serviced shutdown channel.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17a46470f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| module_name = None | ||
| attempts = _IDB_METADATA_QUERY_ATTEMPTS if is_idb_input else 1 | ||
| for attempt in range(attempts): | ||
| metadata = query_binary_metadata(selected_host, port, timeout=5.0) |
There was a problem hiding this comment.
When an .i64/.idb references a large original binary or resides on slow storage, idb_metadata_resource() reads the entire input file and computes multiple hashes before returning its module name (api_resources.py:46-55), so this five-second timeout can expire despite a healthy worker. Because the worker uses the non-threaded HTTP server, each timed-out request continues running while the next retry queues; after four such attempts module_name remains unset and spawn_session terminates an otherwise valid session. Fetch canonical identity through a lightweight resource or let the in-progress metadata request complete instead of retrying it with this short timeout.
Useful? React with 👍 / 👎.
| # Create server with deferred binding | ||
| assert issubclass(request_handler, McpHttpRequestHandler) | ||
| self._http_server = (ThreadingHTTPServer if background else HTTPServer)( | ||
| self._http_server = _http_server_class(host, background)( |
There was a problem hiding this comment.
When the newly supported host="::1" path is used, serve() still prints http://::1:<port>/mcp and http://::1:<port>/sse at lines 413-414. Those are malformed URLs because an IPv6 literal in a URL authority must be bracketed, so anyone using these startup diagnostics receives unusable endpoints even though the server bound successfully; format the displayed host as [::1] just as _http_origin() now does.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35fbdc1102
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **id** — 4-char instance identifier (k7m2, px3a, etc.) | ||
| - **pid** — Process ID of the IDA Pro instance | ||
| - **host** — Always 127.0.0.1 (localhost) | ||
| - **host** — Loopback only: Windows managed workers prefer `::1` after a |
There was a problem hiding this comment.
This commit modifies the architecture documentation without adding an absolute Last updated date, so readers cannot determine when the newly documented Windows transport behavior became current. Add the commit’s update date near the document header as required for every modified document.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
| - **host** — Loopback only: Windows managed workers prefer `::1` after a | ||
| successful bind probe and otherwise use `127.0.0.1`; all other platforms use | ||
| `127.0.0.1` |
There was a problem hiding this comment.
The new Windows IPv6-preference behavior is documented without references to the affected code paths, leaving maintainers unable to trace this architectural statement to _preferred_loopback_host() and the IPv6 ZeroMCP server selection. Add links to the relevant implementation files/functions alongside this behavior.
AGENTS.md reference: AGENTS.md:L14-L16
Useful? React with 👍 / 👎.
| ) | ||
|
|
||
| self._processes[instance_id] = proc | ||
| self._shutdown_tokens[instance_id] = shutdown_token |
There was a problem hiding this comment.
When idalib_status detects a crashed worker, get_status() deletes the process entry and unregisters the instance but never removes the corresponding entry from the newly added _shutdown_tokens mapping. Repeated worker crashes followed by status checks therefore retain an unbounded number of per-process secrets for the lifetime of the router; mirror the cleanup already performed by list_sessions().
Useful? React with 👍 / 👎.
Summary
.i64/.idbfiles and fail closed if metadata cannot be establishedDBFL_KILL | DBFL_COMP, while retaining save-on-close as the data-preserving fallbackidapropackage and retain the underlying detail for other import failuresWhy
On the affected Windows host, fresh IPv4 loopback connections were intermittently reset even for a minimal standard-library HTTP server, while the IPv6 loopback control completed 100/100 requests. Separately,
CREATE_NO_WINDOWworkers could not reliably receiveCTRL_BREAK_EVENT, leaving loose.id0/.id1/.id2/.nam/.tilfiles and a stale packed IDB.The shutdown token is generated per worker, passed only through the child environment, removed before serving, and excluded from request logs.
The same host also exposed a misleading diagnostic: Windows Application Control can block an unsigned
idalib.dllwith WinError 4551 whileidaprois installed correctly. The worker now reports that policy failure instead of advising a pointless package reinstall.Verification
python -m pytest -q: 446 passed, 9 skippedpy_compile: passedgit diff --check: passed::1, analysis settled twice, searches/xrefs completed, and authenticated shutdown exited cleanly.i64; no worker remains@codex Please review this PR for correctness, Windows process lifecycle safety, transport compatibility, diagnostic accuracy, and test coverage.