fix(mcp): close the exit stack when an HTTP MCP connect fails (100% CPU spin) - #5531
Closed
MahmutDehhan wants to merge 1907 commits into
Closed
fix(mcp): close the exit stack when an HTTP MCP connect fails (100% CPU spin)#5531MahmutDehhan wants to merge 1907 commits into
MahmutDehhan wants to merge 1907 commits into
Conversation
Recognize api.cerebras.ai as a Cerebras cloud provider so llama.cpp/LM Studio cache-affinity fields are not attached even when endpoint_kind is misconfigured as local. Add regression coverage for provider detection, self-hosted classification, and payload field exclusion.
* fix(security): prevent ReDoS in LLM-output tool/think parsers The regexes that parse untrusted model output in text_helpers.py and tool_parsing.py are delimiter-bounded with a lazy [\s\S]*? (or an ambiguous (\s+[^>]*)?). Applied with re.sub/re.finditer over a whole response, they degrade to O(n^2) when the closing delimiter is absent: the engine rescans to end-of-string from every opener. Model output is untrusted, so a prompt-injected or malicious model can stall the agent loop with many unclosed openers (measured ~25s on a 60KB <thought flood). - text_helpers.py: replace ambiguous <thought(\s+[^>]*)?> with <thought([^>]*)> (identical capture, no \s+/[^>]* overlap); skip the Gemma <|channel>...<channel|> subs when no <channel|> closer is present. - tool_parsing.py: gate _TOOL_CALL_RE, _XML_TOOL_CALL_RE and _TOOL_CODE_RE (in parse_tool_blocks and strip_tool_blocks) on a cheap presence check for their closing delimiter. With no closer the regex cannot match, so skipping is equivalent; only the wasted O(n^2) rescan is removed. Resolves CodeQL py/polynomial-redos #230, #231, #232, #233, #235, #236, #524. The _XML_OPEN_TOOL_CALL_RE alerts (#234, #477) are false positives (its greedy [\s\S]*\Z is linear) and left untouched. * fix(security): close ReDoS gaps in tool/think parsers from review Addresses two review findings on the closer-guard approach: - Whole-string "closer exists?" checks were bypassable: a stale closer before an opener flood, or a closer with no reachable inner `}`, kept the guard true while every opener still rescanned to end-of-string (O(n^2)). Replace the substring guards with `_iter_delimited`, a forward-only scan that pairs each opener with a *later* closer and stops once none is reachable (O(n)). `parse_tool_blocks` and `strip_tool_blocks` (via `_strip_delimited`) both use it for the [TOOL_CALL], <tool_call>/<function_call>, and <tool_code> formats. Verified equivalent to the original regexes on well-formed inputs. - `<thought([^>]*)>` dropped the tag-name boundary and corrupted unrelated tags (`<thoughtful>` -> `<thinkful>`). Use `<thought(\s[^>]*)?>`: the single fixed `\s` keeps the pattern linear (no `\s+`/`[^>]*` overlap) while restoring the boundary; capture is byte-for-byte identical for real `<thought ...>` openers. Adds regressions for stale-closer-before-opener, closer-present-without- inner-brace, and the <thoughtful>/<thoughts> passthrough. * fix(security): close Gemma channel ReDoS guard flagged in review vdmkenny noted the same bypassable whole-string guard remained in text_helpers.py: `if "<channel|>" in out.lower()` gating the Gemma thought/response channel subs. A stale `<channel|>` before a `<|channel>thought` opener flood keeps the guard true while every opener still rescans to end-of-string (measured ~7.3s at 4k openers). Replace it with `_sub_delimited`, the same forward-only scan used for the tool-call parsers: pair each opener with a later closer, stop when none is reachable (O(n)). Verified output-equivalent to the original capture regexes on well-formed multi-channel inputs; the stale-closer case now runs in <2ms. Adds a regression for stale-closer-before-opener on the Gemma path. * fix(security): harden strip_think() think-tag ReDoS flagged in review The earlier fixes hardened normalize_thinking_markup and the delimiter scanners, but the production entrypoint strip_think() still ran _THINK_CLOSED_RE / _THINK_ATTR_RE / _THINK_OPEN_RE (and the stray-tag _THINK_TAG_RE) over untrusted model output. Those kept the same ReDoS shapes: the lazy `<open>[\s\S]*?</close>` rescanned to end-of-string from every opener, and `(?:\s+[^>]*)?` / `[^>]*` attribute scans ran to end-of-string from every opener on a "many openers, no closer" flood. On the prior head, malformed `<think` / `<thinking` / `<thought` floods took 6-14s through strip_think(). The shipped `<thought>` normalization had the same residual: the single-opener case was linear but an opener flood was still O(n^2) (~4.4s). - Replace the lazy multi-pass _THINK_CLOSED_RE loop with the existing forward-only _sub_delimited scan (pair each opener with the first reachable closer, stop when none is reachable). One pass collapses sequential and nested blocks as before. - Bound every opener/stray-tag attribute scan at `<` (`[^<>]` not `[^>]`) so a no-`>` opener flood can't drive a single match attempt to end-of-string. Identical capture for well-formed think/thought tags. - email_helpers._strip_think: compute had_think from the single linear _THINK_TAG_RE instead of the lazy closed/open `.search()` calls, which had the same O(n^2) on the email reply/summary/extraction paths. All flood variants now finish in <10ms (were 6-14s). Output verified byte-for-byte identical to the prior implementation over a 34-case corpus (nested, mismatched, attr, uppercase, Gemma, prose, prompt-echo). Adds strip_think() timing regressions for malformed openers, opener floods (all three tag names), the closed-opener flood, and the malformed-closer flood. * docs: trim verbose comments in think-tag ReDoS fix
Accept calendar datetime phrases such as "3pm tomorrow" by adding a time-first natural-language parser branch mirroring the reminder parser. Add regression coverage proving time-first forms match their existing day-first equivalents.
Use the upload handler's tolerant index loader when reading upload metadata so corrupt uploads.json degrades to missing metadata instead of a 500. Return 400 for malformed vision JSON request bodies and add regression coverage for both paths.
* fix(search): use generated query for chat mode web search #4547 * style(search): tidy query generation call --------- Co-authored-by: Alexandre Teixeira <alexandremagteixeira@gmail.com>
Guard the agent_max_tool_calls settings read so hand-edited or agent-written non-numeric settings.json values fall back to 0 instead of crashing agent-mode chat stream initialization. Add regression coverage for guarded coercion.
* fix: improve uploaded document retrieval and deep research reuse * test: add coverage for upload manifest and document pagination * chore: rerun CI * fix: restore _insert_before_latest_user helper * fix(agent_loop): restore missing upload context helper
… call fails to convert (#1917) * fix: tool results misthreaded when a native call fails to convert * Unpack the third converted_calls return from _resolve_tool_blocks in the fenced-example tests
An account configured with SMTP only (no imap_host) has no inbox, but the
inbox list path still called _imap_connect, which handed an empty host to
imaplib. imaplib.IMAP4("", 993) silently dials localhost:993 and fails with
"[Errno 111] Connection refused", so the email panel's poll logged a
"Failed to list emails" ERROR every ~60s and surfaced a scary error in the UI.
_imap_connect now fails fast with a typed EmailNotConfiguredError (subclass of
RuntimeError, so existing broad handlers keep working) when no imap_host is set,
and the inbox list returns an empty result for that case instead of an error.
SMTP send is unaffected.
Keep an unhealthy MemoryVectorStore instance available for health reporting instead of discarding it as disabled. This lets health checks report a degraded/down vector-store state while preserving focused regression coverage for initializer behavior.
… (#2732) _store_email_flag and _move_email_message (used by the archive / delete / move / mark-read endpoints) had an else branch that, when _uid_exists returned False, ran conn.store(uid, ...) / conn.copy(uid, ...) followed by a folder-wide conn.expunge(). But imaplib's plain store()/copy() take a message SEQUENCE NUMBER, not a UID, so the op landed on whichever message occupied sequence position == the UID value, and the expunge then permanently removed it. A stale cached UID (or a server whose UID probe misbehaves) therefore deleted an unrelated email instead of reporting 'not found'. There is no valid case where treating a UID as a sequence number is correct, so drop the fallback: when the UID isn't present, return False — callers already surface 'Email not found'. Only the UID command path remains. Sibling of #1874 (which fixes the auto-spam poller's _imap_move in email_helpers.py); this covers the user-facing endpoints in email_routes.py. Part of #2124.
_scheduled_poll_once selected rows WHERE status='pending' and only wrote
status='sent'/'failed' after the SMTP send and IMAP append completed -
no atomic claim in between. Two overlapping callers (the in-process 30s
poller and an externally cron/systemd-driven 'odysseus-mail
poll-scheduled', or the CLI run manually) can both SELECT the same
pending row before either UPDATEs it, and both send it. _start_poller's
own docstring already names this exact risk ('avoid two copies of
_scheduled_poll_once racing on the same SQLite') but nothing in the code
enforced it - it was advisory only.
Add an atomic per-row claim: UPDATE ... SET status='sending' WHERE
id=? AND status='pending', proceeding only when rowcount == 1. The
loser of the race sees rowcount == 0 and skips the row instead of
sending a duplicate.
Adds a regression test that drives two real threads through the real
_scheduled_poll_once against a shared SQLite file, synchronized with a
barrier and a widened send-path window, and asserts exactly one send
fires. Reverting the fix makes the test fail reliably (5/5 runs); with
the fix it passes reliably (5/5 runs).
Fixes #5109
…107)
_load() returned whatever json.loads() produced without checking it was a
dict; _update() did the same before assigning data[key] = value. If the
oauth_tokens column ever held a JSON array or primitive (DB corruption,
manual edit, migration drift), _load()'s callers crashed with
AttributeError on .get(), and _update() crashed with TypeError trying to
item-assign into a list/string/int.
Validate the parsed value is a dict in both methods, falling back to {}
otherwise - same recovery behavior already used elsewhere in the codebase
for this exact JSON-blob-is-not-a-dict shape (_parse_tool_args,
_is_sensitive_path's siblings).
Adds 3 regression tests for _load, get_tokens, and _update against a
non-dict oauth_tokens value.
Fixes #5082
_sync_blocking (src/caldav_sync.py) and _writeback_blocking (src/caldav_writeback.py) each open their own caldav.DAVClient via _build_dav_client, but never close it. The client owns an HTTP session with a pooled connection; without a close() that connection is held until process exit. Previously the fix added explicit client.close() calls before each early return and at the end of the DB finally block. This still leaked the client when SessionLocal() raised before the DB try/finally was entered. Now _sync_blocking wraps the entire post-construction path in an outer try/finally that calls client.close() unconditionally, covering: - AuthorizationError / NotFoundError early return - URL-fallback failure early return - no-calendars early return - normal return after sync - SessionLocal() construction failure (new regression coverage) _writeback_blocking already used a try/finally (unchanged). - src/caldav_sync.py: replace scattered client.close() calls with a single outer try/finally block around the discovery + DB sync path - tests/test_caldav_client_cleanup.py: add CalendarDeletedEvent to the database stub; add regression test for SessionLocal() failure path Closes #4593
…796) * fix(calendar): trust operator CA bundle in CalDAV test_connection The pre-flight test used httpx with trust_env=False, which ignored SSL_CERT_FILE/REQUESTS_CA_BUNDLE. Self-signed CalDAV servers that the real sync accepts (via caldav lib → requests → honors bundle) were rejected by the test with CERTIFICATE_VERIFY_FAILED. Build an explicit SSL context that loads the operator's CA bundle and clears VERIFY_X509_STRICT (which rejects certs without a keyUsage extension — common in self-signed setups). SSRF guards (follow_redirects=False, trust_env=False) are preserved. Fixes #4795 Fixes #4779 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(calendar): add regression tests and edge case handling for SSL context Per review: add route-level regression tests covering SSL_CERT_FILE precedence, VERIFY_X509_STRICT clearing, missing bundle graceful fallback, and empty env var handling. Also log a warning when the configured CA bundle path doesn't exist instead of silently falling back to system CAs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(calendar): rewrite SSL tests to exercise route handler directly Addresses review feedback: tests now use FastAPI TestClient to hit the actual test_connection route, capturing the verify= kwarg passed to httpx.AsyncClient. This ensures the route's SSL context construction is covered, not a test-side duplicate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ci: retrigger CI (redirect hardening test is a CI-env flake, passes locally) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(tests): remove module-level sys.modules stubs that leaked into other tests The collection-time MagicMock stub of `caldav` replaced the real library for every later test in the same process — test_caldav_redirect_hardening's DAVClient became a mock that never sent the PROPFIND, failing its must-reach-the-public-server assertion in CI. conftest already pre-imports the real sqlalchemy/core.database, and the route's lazy imports are patched per-request, so the stub block was both harmful and unnecessary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(calendar): verify exact CA bundle precedence --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Alexandre Teixeira <alexandremagteixeira@gmail.com>
…-call (#5106) stream_agent_loop's per-tool drain loop had no cleanup path for early generator close. Starlette throws GeneratorExit into the generator at whatever await point it's suspended on when the SSE client disconnects (aclose()) - here that's 'await _progress_q.get()' inside the drain loop, before the final 'await _tool_task' line ever runs. The task, which wraps execute_tool_block, was left running unawaited and uncancelled. For bash/python tools this orphans the underlying subprocess: subprocess_tools.py already has correct CancelledError handling that kills the child process, but only runs if the task is actually cancelled. A client disconnecting mid long-running command left that subprocess running server-side for its full duration with nothing left to reap it. Wrap the drain loop in try/finally: on early exit, cancel _tool_task (if not already done) and await it so the existing subprocess-kill path runs. Adds a regression test that drives the real stream_agent_loop with a fake tool handler, closes the generator mid tool-call (mirroring what Starlette does on disconnect), and asserts the handler observed cancellation immediately - not merely via asyncio.run()'s own end-of-run task cleanup, which would mask the bug. Fixes #5105
* fix(chat): Expand user chat bubble edit textbox width - Update user chat bubble width from `fit-content` to `85%` to ensure consistency with the AI chat bubble edit textbox width. * style(chat): Refine user message bubble width logic - Change general bubble width to `fit-content` - Set width to 85% specifically for user messages containing a `textarea`
…hes (#5149)
conn.search() / conn.fetch() operate on volatile positional sequence
numbers that shift whenever messages are deleted or expunged. Three call
sites in the sig-learner (_pull_headers, _fetch_bodies) and morning-brief
email section were storing these as "uid" and reusing them in subsequent
fetches — causing wrong-message returns or NO responses if another client
modified the mailbox concurrently.
Replaced with conn.uid("SEARCH", ...) / conn.uid("FETCH", ...), which use
persistent RFC 3501 UIDs. _scan_one (urgency action) already did this
correctly; these were the remaining callers.
The reproduction window is narrow (requires concurrent deletion between
search and fetch), so the fix is verified by regression tests rather than
manual end-to-end: _SpyImap raises AssertionError if conn.search() or
conn.fetch() are called instead of conn.uid().
…ls (#5420) * fix: harden stabilization attachment and agent guards * fix(uploads): preserve durable references during cleanup * fix(uploads): close cleanup and compaction races
…(#4411) (#5160) * docs: update static/js/MODULE_SUMMARY.md to reflect current ES6 frontend Rewrite the stale module summary to match the current no-build, ES6-module frontend architecture. Adds coverage of app.js orchestration, the chat/SSE pipeline (chat.js, chatStream.js, chatRenderer.js, streamingRenderer.js), new subsystems (research/, compare/, document streaming, cookbook*, skills.js), and removes the obsolete <script> load order assumptions. * cleanup: remove dead MEMORY_DOC / memory_doc paths (closes #4411) Removes the unused MEMORY_DOC constant and the matching DataConfig memory_doc field / set_data_paths entry. No runtime code imports or references these paths, so this is a no-behavior-change dead-code cleanup under the storage-architecture tracker #4377.
* fix(db): restrict data/app.db to 0600
app.db holds bearer-token hashes, bcrypt password hashes, and encrypted
provider keys but was created under the default umask (0644 -> world-readable),
unlike .app_key/vault/integrations which are already 0600 via safe_chmod.
init_db() now chmods the SQLite file to 0600 right after create_all (POSIX
only; no-op on Windows, skipped for Postgres / in-memory). Unconditional and
idempotent, so it also re-locks already-deployed 0644 installs on next
startup. The transient rollback journal inherits 0600 from the parent file at
creation - no sidecar handling needed; -wal/-shm don't exist until WAL is
enabled (#4409 C4) and inherit the same mode then.
Satisfies Rule B, unblocking #4413 and the vault/integration secret moves.
Mirrors src/secret_storage.py:43-45.
Verified: security + DB-permission suites pass; 6 pre-existing visual_report
failures (missing markdown/nh3 deps) are unrelated.
Closes #4407
* fix(db): harden SQLite path parsing and re-lock sidecars
Address review feedback on #4420.
P2: derive the file to chmod from engine.url (SQLAlchemy's parsed URL)
via _sqlite_db_path(), instead of DATABASE_URL.replace("sqlite:///", "").
A driver-qualified URL (sqlite+pysqlite://) or one carrying query args
(?cache=shared) previously slipped past the prefix check / string slice
and left the DB world-readable; the parsed path resolves correctly and
drops the query.
P3: re-lock stale -wal/-shm/-journal sidecars to 0o600 at startup. The
main file is chmod'd first, so any sidecar SQLite creates afterward
inherits 0o600, but a -wal/-shm left world-readable by an older 0o644
install (once WAL was enabled) could still expose DB pages. Absent
sidecars are the normal case, not an error.
Tests: unit-test _sqlite_db_path across driver/query/memory/postgres URL
forms, and a subprocess test asserting stale 0o644 -wal/-shm are
re-locked on startup.
* fix(db): handle sqlite file URI app db permissions
* fix(db): close remaining SQLite permission bypasses
---------
Co-authored-by: Ethan <23321960+0xLeathery@users.noreply.github.com>
Co-authored-by: Alexandre Teixeira <alexandremagteixeira@gmail.com>
…dle registration cleanup
…mporter (#5261) * Harden skill importer against SSRF: block private targets + revalidate redirects per hop The skill importer validated only the initial URL with the lenient SSRF guard (block_private=False) and then fetched with follow_redirects=True, so a 3xx to an internal/metadata address (169.254.169.254, 127.0.0.1, RFC-1918) was still connected to — inconsistent with the hardened services/search/content.py :_get_public_url path. Add a _get_checked() helper that follows redirects manually and re-runs the SSRF guard with block_private=True on every hop, and route all three fetch sites (skills.sh unwrap, _fetch_bytes, _list_github_dir) through it. GitHub's own redirects and the final-host _assert_github_url checks are preserved. Adds hermetic regression tests (IP-literal hosts, faked HTTP layer) and updates the existing mock signature for the new block_private kwarg. Defense-in-depth: the endpoint is admin-gated (require_admin) and admins are trusted per THREAT_MODEL.md, so this is not a cross-boundary vulnerability. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: enforce follow_redirects=False invariant in mock client Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
_connect_stdio and _connect_sse guard their AsyncExitStack with
`finally: if not registered: await stack.aclose()`. _connect_http does not, and a failed
connect there leaks the stack -- along with the anyio task group entered inside
streamablehttp_client.
The abandoned stack is eventually finalized by the garbage collector, from a DIFFERENT
task than the one that entered it. anyio's CancelScope.__exit__ raises
RuntimeError: Attempted to exit cancel scope in a different task than it was entered in
BEFORE reaching `self._tasks.remove(self._host_task)`. The completed host task is never
removed, `_active` stays True, and `_deliver_cancellation` re-arms
`get_running_loop().call_soon(...)` on every iteration of the event loop, forever.
`loop._ready` therefore never empties, so `_run_once` computes a zero timeout and calls
`selector.select(0)`. The process spins:
epoll_wait ~149,000 calls/sec
timeout 0x00000000 on 441,269 / 441,269 calls (100%)
sys_exit 0x0 -- zero fds ready, every single call
hot DSO 68% libpython3.14.so, _PyEval_EvalFrameDefault
On our deployment this pinned a full CPU core continuously for over seven days -- across a
container recreate and a host reboot -- while the container's healthcheck reported healthy
the entire time. It re-fires on every startup: our odysseus and its MCP server run in
different compose projects, so nothing orders them, the MCP URL does not resolve yet, and
`session.initialize()` raises `httpcore.ConnectError [Errno -2]`.
Note the fix must be `finally`, not `except Exception`. The failure surfaces as
CancelledError, which derives from BaseException, so the existing
`except Exception as e: logger.error("Failed to connect HTTP MCP server ...")` handler
below never fires -- confirmed in production, where that line does not appear once in the
entire log history despite the connect failing at every boot.
Reproduced in the running image against an NXDOMAIN host:
no close / except Exception -> RuntimeError -> CPU 99% (spinning)
finally + aclose -> clean teardown -> CPU 4% (idle)
After deploying: 101% -> 0.15% CPU sustained; voluntary/nonvoluntary context switches went
from 5,719/326,576 (never yielding) to 1,803/57 (a normally sleeping event loop); the
cancel-scope RuntimeError disappeared from the logs (37 occurrences in the prior history,
0 since); and the previously-unreachable error handler now logs the connect failure
properly.
|
The following required sections are missing or incomplete. Please update the PR description to address them:
This comment is deleted automatically once all sections are complete. |
github-actions
Bot
added
the
needs work
pewdiepie-archdaemon
force-pushed
the
main
branch
from
July 23, 2026 16:22
45760d6 to
cf4e240
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_connect_stdioand_connect_sseguard theirAsyncExitStackwithfinally: if not registered: await stack.aclose()._connect_httpdoes not — and a failed connect there leaks the stack, along with the anyio task group entered insidestreamablehttp_client.The failure chain
The abandoned stack is eventually finalized by the garbage collector, from a different task than the one that entered it. anyio's
CancelScope.__exit__raises:…before reaching
self._tasks.remove(self._host_task). The completed host task is never removed,_activestaysTrue, and_deliver_cancellationre-armsget_running_loop().call_soon(...)on every iteration of the event loop, forever.loop._readytherefore never empties, so_run_oncecomputes a zero timeout and callsselector.select(0). The process spins:On our deployment this pinned a full CPU core continuously for over seven days — across a container recreate and a host reboot — while the container healthcheck reported
healthythe entire time.It re-fires on every startup: our odysseus and its MCP server run in different compose projects, so nothing orders them, the MCP URL does not resolve yet, and
session.initialize()raiseshttpcore.ConnectError [Errno -2].Why
finally, notexcept ExceptionThe failure surfaces as
CancelledError, which derives fromBaseException— so the existing handler below:never fires. Confirmed in production: that line does not appear once in the entire log history, despite the connect failing at every single boot. Using
except Exceptionhere would ship the bug.Reproduced in the running image against an NXDOMAIN host:
After deploying the fix
The change follows the exact
registered+finallyidiom the other two transports already use.