Skip to content

fix(docker): resolve egress DNS off the event loop (#2423) - #2428

Open
greerso wants to merge 1 commit into
unclecode:developfrom
Magnolia-Tech-Services-LLC:fix/egress-dns-off-event-loop
Open

fix(docker): resolve egress DNS off the event loop (#2423)#2428
greerso wants to merge 1 commit into
unclecode:developfrom
Magnolia-Tech-Services-LLC:fix/egress-dns-off-event-loop

Conversation

@greerso

@greerso greerso commented Oct 21, 2026

Copy link
Copy Markdown

Closes #2423.

What it does

PinningProxy._handle_connect and _handle_absolute called resolve_and_pin(), which runs a blocking socket.getaddrinfo, directly on the asyncio event loop for every Chromium connection. An ad-heavy page opens hundreds of connections, so the server's one event loop sat in DNS lookups and every other request waited behind them. _normalize_and_validate_seeds in both crawl handlers resolved on the loop the same way.

The three lookups now run in a worker thread:

pin = await asyncio.to_thread(resolve_and_pin, f"https://{host}:{port_s}")    # _handle_connect
pin = await asyncio.to_thread(resolve_and_pin, f"http://{sp.hostname}:{port}")  # _handle_absolute
urls, refused = await asyncio.to_thread(_normalize_and_validate_seeds, urls)  # both crawl handlers

The egress rule is unchanged: resolve once, reject any non-global answer, pin the IP. resolve_and_pin and the seed check only read module constants and call getaddrinfo, so they are safe to run in a thread.

Tests

  • New: test_slow_dns_does_not_block_the_event_loop (test_security_egress_proxy.py). It sends 2 CONNECTs and 2 absolute-form requests with a 1 s fake resolve_and_pin. The event loop must never stall 0.5 s or more, and the lookups must overlap. On develop it fails with a 4.0 s stall.
  • Changed: test_every_crawl_handler_validates_destination (test_security_ssrf_crawl.py). It reads the handler source for _normalize_and_validate_seeds(. It now also accepts the to_thread(_normalize_and_validate_seeds, form. Each handler must still name the shared helper.
  • Changed: test_chained_plain_http_pinned_absolute_form_no_smuggling.
    • When the proxy has already closed the client connection, the smuggled write can draw a reset or broken pipe instead of a clean EOF. The test now treats that as "nothing came back", like the EOF.
    • The race already exists on develop: the unchanged test failed 1 in 60 local runs on develop and 2 in 60 with this change. With the edit: 0 in 60.
    • The security assertions are unchanged: rebind.evil never reaches upstream, and the upstream sees only the pinned IP and Connection: close.

Run locally (macOS, Python 3.12), on develop and on this branch:

cd deploy/docker
pytest tests/test_security_egress_proxy.py tests/test_security_ssrf_egress.py \
  tests/test_seed_refusal.py tests/test_security_ssrf_seeder.py \
  tests/test_security_ssrf_crawl.py tests/test_security_webhook_pinning.py
  • develop: 85 passed.
  • This branch: 86 passed (85 plus the new test).

Not caused by this change: tests/test_issue_2127_docker_pdf.py and tests/test_docker_pdf_crawler_pairing.py fail 5 tests on develop and on this branch alike. Their stub for _normalize_and_validate_seeds returns a list, not the (allowed, refused) tuple.

Docker testing

There are no Dockerfile or compose changes. We run this patch on top of unclecode/crawl4ai:0.9.5 in production.

A/B on the same image and the same load (60 s warm-up, then 4 min at 16 concurrent /crawl requests to 120 real news and retail pages, with an unauthenticated POST /crawl {} timed every second):

Probe p50 Probe max Probes over 8 s Crawls finished Successful Median crawl
stock 0.9.5 10.9 s 39.1 s 11 of 19 29 0 90.1 s
with the fix 0.02 s 1.14 s 0 of 215 178 134 15.6 s

First 15 minutes in production after the deploy: 443 probes, p50 0.00 s, max 0.5 s, none over 8 s; about 700 completed crawls an hour.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LCwrQkwJvshBArmoS7DQ8C

@greerso @claude
fix(docker): resolve egress DNS off the event loop (unclecode#2423)
33a3285 
PinningProxy._handle_connect and _handle_absolute called resolve_and_pin(),
a blocking socket.getaddrinfo, directly on the asyncio loop for every
Chromium connection. Ad-heavy pages open hundreds of connections, so the
server's one event loop sat in DNS lookups and every other request (even a
401 at the auth gate) waited 10 to 39 s behind them. The seed check in both
crawl handlers resolved on the loop the same way.

The lookups now run in a worker thread (asyncio.to_thread). The egress rule
is unchanged: resolve once, reject any non-global answer, pin the IP.

Tests:
- test_slow_dns_does_not_block_the_event_loop: 2 CONNECTs and 2
  absolute-form requests with a 1 s fake lookup; the loop never stalls
  0.5 s and the lookups overlap. Fails on develop (4.0 s stall).
- test_every_crawl_handler_validates_destination accepts the to_thread
  call form.
- test_chained_plain_http_pinned_absolute_form_no_smuggling: the smuggled
  write can draw a reset or broken pipe when the proxy has already closed
  the connection. That race exists on develop (1 in 60 runs locally) and
  the thread hop makes it a little likelier. Treated as the same "nothing
  came back"; the assertions that rebind.evil never reaches upstream are
  unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCwrQkwJvshBArmoS7DQ8C
@SohamKukreti

SohamKukreti commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Thanks @greerso, this is a clear and well-measured fix. We reproduced #2423 and tested this branch against develop, with live runs and the full suite.

What we verified

  • Fixes the stall. With 500 concurrent CONNECTs to uncached real hosts, the probe max was 30 ms on this branch and 50 s on develop. With 50 slow seeds, the probe max was 5 ms on this branch and 5.7 s on develop.
  • No regressions. deploy/docker/tests plus the related root tests: 398 passed on this branch and 397 on develop. The only difference is the new test. The other failures are the same on both. We ran a real /crawl with Chromium through the proxy, CONNECT, plain HTTP, /crawl/stream, and all-refused and mixed seeds. All of them behave the same as on develop, with the same 400 body.
  • Security is unchanged.
    • 12 internal and blocked targets still get 403: localhost, 169.254.169.254, [::1], [::ffff:127.0.0.1], 10.0.0.1, 0.0.0.0, nip.io/localtest.me, host.docker.internal, and NXDOMAIN hosts.
    • DNS rebinding still gets one lookup, and the proxy dials only the pinned IP. A host with mixed public and private records is still rejected.
    • A resolver crash or a dead executor fails closed, and nothing is dialed.
    • CRAWL4AI_ALLOW_INTERNAL_URLS behaves as before.
  • Tests.
    • The new test is solid. It fails if either half of the fix is reverted (2.0 s stall each, 4.0 s with both). It passed 50/50 runs normally and 20/20 under full CPU load. It still passes with run_in_executor, so it tests behaviour, not implementation.
    • The smuggling-test change is justified. The original failed 60/60 under CPU load with ConnectionResetError. The new version passed 60/60, and it still fails when the proxy really forwards the smuggled request.

Two requests before merge

  1. Use a dedicated executor for egress DNS. asyncio.to_thread uses the loop's default executor, which has min(32, cpu+4) workers, so only 6 on a 2-CPU container. The crawler shares that executor for scraping (content_scraping_strategy.py), LLM extraction, PDF and chardet. Our measurements:

    • When the pool was busy with slow jobs, a Chromium CONNECT waited 19.7 s. On develop it took 0.01 s.
    • Hung DNS delayed unrelated to_thread work by about 70 s.

    This is still far better than the frozen loop on develop. But it couples DNS and scraping, and a client that controls a slow authoritative DNS server can use that to starve scraping for other users. A small bounded pool for these lookups would remove this, for example loop.run_in_executor(_DNS_POOL, resolve_and_pin, ...).

  2. Add a runtime test for the api.py change. If we revert both api.py call sites to the sync call, the whole suite stays green. Only the source-string test sees the change, and that test also passes when the call's result is discarded or the call is commented out. A slow-validate_url_destination test, like the proxy one, would cover it.

Nits

  • The new comment is on _handle_connect only. A short note on _handle_absolute and on the two api.py lines would keep them consistent.
  • In the PR description, 4 of the 5 PDF test failures come from the lambda urls: urls stub. test_default_strategy_still_uses_pool fails for a different reason: an unexpected max_pages kwarg.

Out of scope, for a follow-up (no change needed here)
These calls still do blocking DNS on the event loop:

  • webhook.py _deliver: resolve_and_pin and check_redirect
  • validate_url_destination in handle_llm_qa, process_llm_extraction, handle_markdown_request and create_new_task
  • the URL checks in the server.py endpoints (generate_html, generate_screenshot, generate_pdf, execute_js)
  • job.py

With 20 concurrent /md requests, the loop still stalled about 1.8 s. We can track these in a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@greerso @SohamKukreti