fix(memgw): cancel timed-out MCP calls + strict loopback host parsing — Codex PR #30 review - #31
Conversation
… — Codex PR #30 review Two chatgpt-codex-connector[bot] review comments on PR #30 that still applied to current main (the other 13 were already addressed in the merged PR): 1. client.py #13 — Cancel timed-out MCP calls: _run_sync now cancels the concurrent.futures.Future on timeout before re-raising, so a stalled MCP endpoint doesn't leave a pending HTTP session running on the shared background loop after the caller has given up / opened the breaker. 2. __init__.py #15 — Parse the host before allowing keyless local mode: is_available() now urlparse's the URL and requires an exact loopback host (localhost/127.0.0.1/::1) instead of a substring match, so a URL like 'https://localhost@example.com/mcp' (host=example.com) is no longer trusted as local keyless mode. Adds regression tests: test_memgw_client_timeout.py (cancel-on-timeout fails against pre-fix client) + TestKeylessLocalModeHostParsing (3/4 fail pre-fix). Deferred (already fixed in merged PR #30): MCP dep import check, MCP dep pin, loop lock, isError handling, stale-prefetch generation, sync/delegation thread tracking + shutdown join, session-switch prefetch invalidation, no join on turn path, default provider unset. Deferred (complex, needs interface change): #3/#4 per-turn user_id refresh for shared gateway sessions — sync_turn/prefetch only receive session_id, not user_id, so threading identity through requires a MemoryProvider interface change; #1 in-tree placement is an architectural call (AGENTS.md says existing in-tree providers stay).
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tests/plugins/memory/test_memgw_client_timeout.py:11: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues: none
Unchanged: 4569 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
![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: 3debe63c97
ℹ️ 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".
| # (Codex PR #30 review). Parse the URL and require an exact loopback host. | ||
| from urllib.parse import urlparse | ||
|
|
||
| host = (urlparse(url).hostname or '').lower() |
There was a problem hiding this comment.
When memgw runs keyless and the configured MEMGW_API_URL/memgw.json URL is malformed, such as a typoed IPv6 URL (http://[::1:8081/mcp) or an invalid NFKC hostname, accessing .hostname raises ValueError. Some status/setup paths call p.is_available() directly, so a bad config can abort the command instead of marking memgw unavailable; catch parse errors here and return False.
Useful? React with 👍 / 👎.
1 participant
Two
chatgpt-codex-connector[bot]review comments on PR #30 that still applied to current main (the other 13 were already addressed in the merged PR):_run_syncnow cancels theconcurrent.futures.Futureon timeout before re-raising, so a stalled MCP endpoint doesn't leave a pending HTTP session running on the shared background loop after the caller has given up / opened the breaker.is_available()nowurlparse's the URL and requires an exact loopback host (localhost/127.0.0.1/::1) instead of a substring match, so a URL likehttps://localhost@example.com/mcp(host=example.com) is no longer trusted as local keyless mode.Regression tests:
test_memgw_client_timeout.py(cancel-on-timeout fails against pre-fix client) +TestKeylessLocalModeHostParsing(3/4 fail pre-fix). Full memgw suite: 184 passed; ruff clean.Deferred (already fixed in merged PR #30): MCP dep import check, MCP dep pin, loop lock, isError handling, stale-prefetch generation, sync/delegation thread tracking + shutdown join, session-switch prefetch invalidation, no join on turn path, default provider unset.
Deferred (complex, needs interface change): #3/#4 per-turn
user_idrefresh for shared gateway sessions (sync_turn/prefetchonly receivesession_id, notuser_id); #1 in-tree placement is an architectural call (AGENTS.md says existing in-tree providers stay).🤖 Generated by the
auto-fix-codex-pr-review-recommendationsautomation. Not auto-merged.