Skip to content

fix(http): raw aiohttp.ClientSession ceiling regressed 12→13 (undocumented offenders, #12992) #13041

Closed
Closed
fix(http): raw aiohttp.ClientSession ceiling regressed 12→13 (undocumented offenders, #12992)#13041
Labels
backendbugSomething isn't workingdocsfrontendinfrastructuretesting

Description

@mrveiss
mrveiss
opened on Jul 30, 2026

Context

autobot-backend/tests/test_raw_client_session_ceiling_12992.py::test_raw_client_sessions_do_not_exceed_ceiling is currently failing on origin/Dev_new_gui (verified locally against a2f788889):

AssertionError: 13 raw aiohttp.ClientSession(...) constructions found, exceeding the recorded ceiling of 12 (#12992).
Current offenders:
  autobot-backend/agent_loop/search/config_declared_provider.py: 1
  autobot-backend/api/marketplace_sources.py: 1
  autobot-backend/api/provider_auth.py: 2
  autobot-backend/content_reach/_url_guard.py: 1
  autobot-backend/knowledge/connectors/oauth_flow.py: 1
  autobot-backend/orchestration/dag_executor.py: 1
  autobot-backend/services/npu_client.py: 1
  autobot-backend/services/npu_pipeline/dispatcher.py: 1
  autobot-backend/services/npu_pipeline/npu_client.py: 1
  autobot-backend/services/slm_client.py: 1
  autobot-backend/skills/external_importer.py: 1
  autobot-backend/skills/sync/mcp_transport.py: 1

The test's own docstring (as of the #12992 ratchet) documents 12 total carve-outs across: api/provider_auth.py (2), api/marketplace_sources.py, content_reach/_url_guard.py, knowledge/connectors/oauth_flow.py, skills/external_importer.py (SSRF-pinned, 6 sites), services/npu_client.py, services/npu_pipeline/dispatcher.py, services/npu_pipeline/npu_client.py, skills/sync/mcp_transport.py (long-lived, 4 sites), plus custom-TLS carve-outs — 12 total.

Three offenders are NOT in that documented list, i.e. they are new raw aiohttp.ClientSession(...) constructions introduced by PRs that merged into Dev_new_gui after the #12992 ceiling was last verified:

  • autobot-backend/agent_loop/search/config_declared_provider.py
  • autobot-backend/orchestration/dag_executor.py
  • autobot-backend/services/slm_client.py

This is exactly the backlog-refill failure mode #12992's own docstring warns about ("the backlog cannot finish draining while unrelated PRs keep introducing new raw sessions while sweep batches are in flight").

Scope note

Discovered incidentally while auditing #10538 (Slack/Confluence/Jira KB connectors) — unrelated to that issue, filed separately per repo convention.

Proposed fix

For each of the 3 new offenders: either convert to autobot_shared.http_client.get_http_client() (tracked_request()/get_json()/post_json()), or — if the session is genuinely a documented carve-out (long-lived, SSRF-pinned with a connector=, or custom TLS) — add it to the docstring's "Remaining raw sites" list with a same reasoning, and adjust MAX_RAW_CLIENT_SESSIONS accordingly. Re-measure immediately before push per the existing ratchet convention.

Acceptance Criteria

  • test_raw_client_sessions_do_not_exceed_ceiling passes again
  • Each of the 3 new sites is either converted to the pooled client or documented as an intentional carve-out with in-code + docstring justification
  • Ceiling constant reflects the true, re-measured count immediately before push

Activity

  1. mrveiss
    added
    bugSomething isn't working
    on Jul 30, 2026
  2. github-actions
    added
    backend
    docs
    frontend
    infrastructure
    testing
    on Jul 30, 2026

  3. mrveiss commented on Jul 30, 2026

    @mrveiss
    mrveiss
    on Jul 30, 2026
    OwnerAuthor

    Investigated and fixed in #13046 (branch issue-13041).

    Re-measured, confirmed 13 — same offender list this issue reported.

    Correction to this issue's premise: of the three offenders listed as "new and undocumented," only agent_loop/search/config_declared_provider.py actually is. orchestration/dag_executor.py and services/slm_client.py were already listed in the test's module docstring under the "custom non-default TLS/SSL context" carve-out category since batch 8 (#13006) landed the 12-ceiling — checked via git log on the test file. They did not regress; they were already accounted for.

    config_declared_provider.py is a legitimate carve-out, not a violation. Added by #13016 (#12625), its raw session pins a TCPConnector via autobot_shared.security.ssrf_guard.pinned_connector() fresh on every call (config_declared_provider.py:183-187,235) — same SSRF-pinned-connector shape as oauth_flow.py/external_importer.py. Pooling it would silently drop the pin and reopen the #12278 DNS-rebinding hole. #13016 should have bumped the ceiling and documented the carve-out at the time; it didn't, which is the actual defect here.

    #13046 raises MAX_RAW_CLIENT_SESSIONS to 13, documents the new carve-out in the module docstring, and — since this guard has never been collected by any CI workflow (that gap is #10691, evidence already posted there) — adds it as a step in the existing required startup-import-smoke job so this specific regression can't recur silently.

  4. mrveiss
    added a commit that references this issue on Jul 30, 2026

    fix(tests): raise raw-ClientSession ceiling to 13 for SSRF-pinned con…

    dbd1cce

  5. mrveiss commented on Jul 30, 2026

    @mrveiss
    mrveiss
    on Jul 30, 2026
    OwnerAuthor

    Fixed by PR #13046, merged to Dev_new_gui as dbd1cce48.

    Two distinct problems, both addressed.

    1. The ceiling was stale, and the 13th site is legitimate. Re-measured with the guard's own AST walker against origin/Dev_new_gui @ a2f788889: 13. MAX_RAW_CLIENT_SESSIONS raised 12 → 13, negative case confirmed tripping at 14.

    agent_loop/search/config_declared_provider.py is a correct carve-out, not a violation. It resolves and pins a TCPConnector via ssrf_guard.pinned_connector() fresh on every search() call (config_declared_provider.py:183-187,235) before opening its raw session. Pooling it would silently drop the pin and reopen the DNS-rebinding hole closed by #12278. It was introduced by PR #13016 (#12625), which correctly kept it raw but failed to record it — the defect was the missing bookkeeping, not the session.

    This issue's own claim was 1/3 accurate, and is corrected on the record. It named three new undocumented sites. orchestration/dag_executor.py and services/slm_client.py have been in the module docstring's custom-TLS carve-out bullet since batch 8 (#13006) established the 12-ceiling — verified via git log on the test file. Only config_declared_provider.py was genuinely new and genuinely undocumented.

    Carve-out inventory in the module docstring now lists all 13 by category: SSRF-pinned connector — api/provider_auth.py (2), api/marketplace_sources.py, content_reach/_url_guard.py, knowledge/connectors/oauth_flow.py, skills/external_importer.py, agent_loop/search/config_declared_provider.py; long-lived — services/npu_client.py, services/npu_pipeline/dispatcher.py, services/npu_pipeline/npu_client.py, skills/sync/mcp_transport.py; custom TLS — services/slm_client.py, orchestration/dag_executor.py.

    2. The deeper cause: this guard had never run in CI. That is why PR #13016 could add a session, turn the guard red, and merge fully green. No workflow collected it — autobot-backend/tests/** appears only in migration-gate.yml (migrations subdir) and startup-import-smoke.yml (test_startup_imports.py), and the required check named "Unit & Integration Tests" is defined in frontend-test.yml and is entirely frontend.

    The guard is stdlib-only (ast/pathlib, no backend imports), so it is now a second step in startup-import-smoke.yml — a required check whose job already installs the backend dependency set. Verified by building a Python 3.14 venv and running the job's own install steps (requirements-ci.txt + pytest + editable autobot_shared), then running the guard both alongside test_startup_imports.py and standalone: 2 passed each time. The negative case was reproduced by temporarily reverting the ceiling to 12 and observing the exact failure this issue reports.

    A guard that does not run is indistinguishable from a guard that passes. This one now runs.

    The broader gap — that the backend test suite as a whole does not execute in CI — is #10691, where I have posted the full evidence and blast radius. It is deliberately untouched here; wiring one stdlib-only guard into an existing required job is a different scale of change from enabling the whole suite.

  6. mrveiss
    closed this as completedon Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    backendbugSomething isn't workingdocsfrontendinfrastructuretesting

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions