fix(http): plugins use the pooled client, and a ratchet watches the raw sessions outside the backend (#12979) by mrveiss · Pull Request #17973 · mrveiss/AutoBot-AI

mrveiss · 2026-10-04T15:24:25Z

Thinking Path

The issue records 134/127 per-request constructions. That number is stale, and the first job was to replace it rather than quote it.

Measured with an AST walker against origin/main at 0a4cffced, over tracked *.py:

instrument	files	occurrences
`git grep -c 'aiohttp\.ClientSession('`	70	115
AST walk (`ast.Call` whose func is `*.ClientSession` or bare `ClientSession`)	61	99
AST walk, non-test files only	36	59

The 16-occurrence gap between grep and AST is prose — docstrings, an emitted log string and a regex replacement template. That gap is the whole reason this guard parses rather than matches.

Of the 59 non-test constructions, 12 are inside autobot-backend/, which autobot-backend/tests/test_raw_client_session_ceiling_12992.py already ratchets at 13. Reading that file changed the plan: #12979's batches 4–9 drained the backend's convertible tail, and every one of the 12 is already a documented carve-out — knowledge/connectors/oauth_flow.py:188, services/npu_client.py:_get_session, services/npu_pipeline/dispatcher.py and npu_client.py, and skills/sync/mcp_transport.py each carry an in-code #12979 note; api/provider_auth.py (2), api/marketplace_sources.py, content_reach/_url_guard.py and agent_loop/search/config_declared_provider.py pass an SSRF-pinned connector=; services/slm_client.py and orchestration/dag_executor.py pass a custom TLS context. None of those can go through the pool: HTTPClientManager owns one shared TCPConnector and session.request() takes no per-request connector override, so converting a pinned site silently drops the pin and reopens #12278.

That left the real finding: 47 sites, four fifths of the population, outside autobot-backend/ and under no ratchet at all.

So this PR does two things rather than sweeping: it converts the one coherent cluster that is genuinely per-request, and it puts the remainder under a guard that can name what is left.

What Changed

Converted — 9 sites, 2 files, the core plugins. Every one was async with aiohttp.ClientSession() as session: against a module-constant public host, with no connector, no TLS context and no lifetime past the statement:

Timeouts (review fix). The pooled session default is ClientTimeout(total=30, connect=5, sock_read=10); the raw sessions used aiohttp's 300s default. All 9 converted calls now pass an explicit per-call timeout= from autobot_shared/generation_http_timeouts.py: synchronous Stability generation gets 300s total and read (AUTOBOT_GENERATION_REQUEST_TIMEOUT_S); Flux submit/poll and the 6 video submit/poll calls (enqueue-only or status) get 30s (AUTOBOT_GENERATION_POLL_TIMEOUT_S); connect is 10s (AUTOBOT_GENERATION_CONNECT_TIMEOUT_S). Env-backed, registered in env_registry_ai.py, ENV_VARS.md regenerated.

plugins/core-plugins/video-generation-plugin/tools/providers.py — 6 (Runway, Sora, Kling × submit/poll)
plugins/core-plugins/image-generation-plugin/tools/generate_image.py — 3 (Flux submit, Flux poll, Stability)

All now use get_http_client().tracked_request(...), which owns the active-request counter so it cannot be forgotten (#12981). Redirect behaviour is unchanged: guard_egress is deliberately not passed, because these hosts are module constants rather than config- or user-supplied, which is the condition Rule 8 names — and guard_egress forces allow_redirects=False, which would be a behaviour change dressed as a cleanup. The lazy import aiohttp / except ImportError probes went with them; the pooled client carries the dependency.

providers_test.py now doubles HTTPClientManager instead of stubbing the aiohttp module in sys.modules. The queued post= / get= shape is unchanged, so all 13 test bodies read exactly as before.

Left, with the reason stated — recorded in the guard's INVENTORY, 35 sites in 21 files:

category	files	why
`entrypoint`	17	operator-run scripts with a `__main__` guard — infrastructure deploy/monitor/diagnose tools and the SLM agent, which run outside the app process and have no pooled client to share
`foreign-runtime`	2	`autobot-npu-worker/resources/windows-npu-worker/` — a separately packaged Windows deployable without `autobot_shared` on its path
`long-lived`	1	`autobot_shared/paperclip_client.py` — the session is stored on the instance across `open()`/`close()`, the same shape as the backend's documented carve-outs
`docs-example`	1	`docs/examples/mcp_agent_workflows/base.py`

The inventory does not claim all 35 are deliberate, and says so in its own docstring. The 23 sites in the infrastructure operator scripts are most likely unconverted work. I did not establish that they are carve-outs, so I did not write a justification claiming they are. #12979 stays open; this is Refs, not Closes.

The guard — repo_tests/_raw_aiohttp_session.py (detector) + raw_aiohttp_session_inventory_12979_test.py (8 tests) + raw_aiohttp_session_contrast_test.py (11 fixtures):

AST, never text. Measured above: a text guard would be set 16 too high, and — the sharper half — is satisfied by a comment carrying the same string. test_prose_only_occurrences_are_not_counted asserts a fixture where aiohttp.ClientSession( appears only in a docstring, a reviewer comment, a template constant, an f-string and a log call.
Sets, not totals (RATCHET_BASELINES.md rule 4) — two populations can agree on a count and differ in membership.
The guards(ratchet): "no ceiling may be raised" is not enforced — moving both anchors passes, and that is the documented practice #17970 hole is closed. A bare count ratchet is satisfied by editing the baseline and the code in one commit. Here every inventory entry must also satisfy a predicate read from the code: entrypoint needs a real top-level __main__ guard, long-lived needs every site to be stored rather than consumed inline. A per-request session in an ordinary library module matches no category, so no number written in the inventory admits it — demonstrated as mutation M2 below. The honest limit, stated in the docstring: someone could bolt an unused __main__ block onto a library module. That is a visible and absurd diff.
The boundary is declared and pinned (rule 1 + rule 2). test_exclusion_rules_mirror_the_backend_sibling loads the backend guard by path and asserts its exclusion rules; test_the_two_scopes_partition_the_tracked_tree asserts the four buckets are disjoint, non-empty and sum to the tracked population, so a rename of autobot-backend/ or a widened exclusion goes red rather than opening a silent gap. Phrasing it as "nothing is uncovered" would have been a tautology — the leftover set is computed from the same predicate the scope uses.
Known positive before any count is read, plus a population floor (961 scoped files measured; floor 800) that distinguishes nothing found from did not look.
The second derivation shares no enumeration with the sibling: git ls-files here, rglob there.

One known gap, written down rather than left as a blind spot. from aiohttp import ClientSession as Session is invisible to the detector — it matches the spelling at the call site. git grep -c 'ClientSession as ' -- '*.py' returns rc=1 with no output, so the gap costs nothing today. test_a_renamed_direct_import_is_counted asserts the gap explicitly and tells the next author to tighten it if that spelling ever appears.

Verification

Measurement, both instruments, on origin/main at 0a4cffced:

git grep -c 'aiohttp\.ClientSession(' origin/main -- '*.py'   ->  70 files, 115 occurrences
AST walk (git show per file, origin/main)                     ->  61 files,  99 constructions
AST walk, non-test                                            ->  36 files,  59 constructions
  of which inside autobot-backend/                            ->  11 files,  12 constructions

Tests:

repo_tests/raw_aiohttp_session_inventory_12979_test.py  ....  8 passed
repo_tests/raw_aiohttp_session_contrast_test.py ............ 11 passed
plugins/.../video-generation-plugin/tools/providers_test.py . 13 passed
autobot-backend/tests/test_raw_client_session_ceiling_12992.py (sibling, untouched)  2 passed
repo_tests/python_filter_covers_its_guards_test.py
repo_tests/marker_suite_root_coverage_test.py ............... 29 passed
flake8 (6 changed files) .................................... rc=0

Mutation results — the guard was broken on purpose five ways:

#	mutation	expected	observed
M1	per-request `async with aiohttp.ClientSession() as s:` appended to `autobot_shared/network_utils.py` (a scoped library module)	RED	`FAILED test_no_raw_session_outside_the_recorded_inventory`
M2	M1 plus an `INVENTORY` entry for it, inheriting category `entrypoint` — the #17970 two-record hole	RED	`FAILED test_every_inventory_entry_satisfies_its_declared_category`
M3	both restored	GREEN	19 passed
M4	prose-only occurrence (comment + string constant) appended to the same live file	GREEN	19 passed — the text-matching trap does not fire
M5	a session re-introduced into the converted `providers.py`	RED	`FAILED ... new files: ['plugins/core-plugins/video-generation-plugin/tools/providers.py']`

Pre-push (PATH=/home/martins/.venv-python-suite/bin:$PATH, no --no-verify, no core.hooksPath override):

[pre-push OK] open-PR cap: 14/40 open PRs -- below cap
[pre-push OK] pytest: all relevant tests pass

Two repo hooks caught real defects in this branch before it left the machine, both recorded here rather than quietly fixed: the print() guard fired on prose in a docstring, and git-toplevel-env-scrubbed fired because the walker called git ls-files without scrubbed_git_env() — an inherited GIT_DIR outranks cwd= and would have enumerated another checkout's index without erroring. Both fixed in the commit.

Head history: 3fd9ffa5fe (original push) → b052cc66f2 (merge of main) → c56d1a1172 (timeout fix) → df7e4ebc0a (rebased onto 72273222) → a488c3713e (rebased onto 8b7de912) → e32394f869 (exact timeout assertions) → bdef94fe3b (rebased onto the bot merge of main, which includes the image mirror workflow; this is the pushed head). → 47c9c82354 (CI-red and review fixes: positive-finite timeout overrides, plugin tests wired into pytest.ini and the four workflows, inventory test via tracked_paths, main-guard Eq check; local until pushed).

Timeout fix verified by generate_image_test.py (Stability and Flux calls carry a ClientTimeout) and providers_test.py::test_every_request_passes_a_timeout (all 6 video calls, sock_read >= 30).

Collision check. git diff --name-only origin/main...origin/<b> was run for all 14 open PR branches. One of my 39 text-grep candidates appeared: autobot-infrastructure/shared/scripts/phase_validation_system.py, owned by #17952. I did not edit it; its diff on that branch changes no ClientSession construction, so the inventory entry (count 2) remains valid. No other file in this PR appears in any open PR.

What I could not establish. Whether the 23 infrastructure-script sites are deliberate. They have the structural shape of operator entrypoints, which is what the entrypoint category asserts and all it asserts. I did not read each script's lifecycle closely enough to say any of them should stay raw, and I did not write a carve-out comment claiming so.

Refs #12979, #12992, #13625, #17970.

Single-issue rationale

This closes nothing, so there is no closing issue to batch with. It is deliberately partial: 9 of 47 sites converted, with the other 35 recorded rather than swept, because #12979's full population is 36 files and no reviewer can honestly pass on that in one diff. The two files it touches outside repo_tests/ are the two core plugins, which no open PR touches. Appending it to any in-flight PR would mix a behaviour change in the outbound-HTTP path with that PR's scope and put a new repo-wide ratchet behind an unrelated review.

Model Used

Claude Opus 5 (1M context)

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Image and video generation requests now use configurable HTTP timeouts. Synchronous image generation requests default to 300 seconds, while status checks and queued submissions default to 30 seconds. Connection attempts default to 10 seconds.
- These timeout values can be adjusted through environment variables.
Documentation
- Added details about the generation timeout settings and their defaults to the environment-variable documentation.

…aw sessions outside the backend (#12979) Measured on origin/main at 0a4cffc with an AST walker: 59 aiohttp.ClientSession constructions in 36 non-test files, not the 134/127 the issue records. 12 of those are inside autobot-backend/, where #12979's batches 4-9 already drained the convertible tail and test_raw_client_session_ceiling_12992.py ratchets what is left. The other 47 were under no guard at all. Converted 9 of the 47, in the two core plugins. Every one was a plain `async with aiohttp.ClientSession() as session:` against a module-constant public host, with no connector, no TLS context and no lifetime past the statement, so routing them through get_http_client().tracked_request() changes nothing but the pooling and the active-request accounting. The video provider tests now double HTTPClientManager instead of the aiohttp module; the queued post=/get= shape is unchanged, so the 13 test bodies read as before. Left the rest, with reasons. Every remaining autobot-backend/ site is already a documented carve-out: an SSRF-pinned connector the pooled client cannot accept per request, a custom TLS context, or a session held open across calls. autobot_shared/paperclip_client.py is the same long-lived shape. The 23 sites in the infrastructure operator scripts are unconverted work rather than carve-outs, and the inventory says so rather than inventing a justification for them. The guard is an inventory, not a count. 35 sites in 21 files, compared as a set so two populations cannot agree on a total and differ in membership. #17970's hole -- a baseline and its measurement edited together passing every check -- is closed by requiring each entry to satisfy a structural predicate read from the code: a per-request session in an ordinary library module matches no category, so no number written in the inventory admits it. Detection is AST, never text, because the string appears in docstrings and templates here and a text guard is satisfied by a comment carrying it; the contrast file asserts that directly.

coderabbitai · 2026-10-04T15:24:33Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c3a2b9d7-3e68-45ab-b437-faa0f2efa163

📥 Commits

Reviewing files that changed from the base of the PR and between bdef94f and 47c9c82.

📒 Files selected for processing (13)

.github/workflows/ci.yml
.github/workflows/coverage.yml
.github/workflows/marker-tests.yml
.github/workflows/test-durations.yml
autobot_shared/env_utils.py
autobot_shared/env_utils_test.py
autobot_shared/generation_http_timeouts.py
autobot_shared/generation_http_timeouts_test.py
plugins/core-plugins/image-generation-plugin/tools/generate_image_test.py
plugins/core-plugins/video-generation-plugin/tools/providers_test.py
pytest.ini
repo_tests/collection_coverage_test.py
repo_tests/raw_aiohttp_session_inventory_12979_test.py

📝 Walkthrough

Walkthrough

Generation providers now use the shared HTTP client with configurable request timeouts. New repository tests detect raw aiohttp.ClientSession calls and validate a scoped inventory of those calls.

Changes

Generation HTTP timeouts

Layer / File(s)	Summary
Timeout settings and factories `autobot_shared/env_registry_ai.py`, `autobot_shared/generation_http_timeouts.py`, `docs/developer/ENV_VARS.md`	Registers and documents configurable 300-second generation, 30-second polling, and 10-second connection timeouts. Timeout factories apply the configured total, socket-read, and connection values.
Image generation requests `plugins/core-plugins/image-generation-plugin/tools/generate_image.py`, `plugins/core-plugins/image-generation-plugin/tools/generate_image_test.py`	Flux and Stable Diffusion requests use the shared HTTP client with their respective timeout factories. Tests check timeout values and request counts.
Video provider requests `plugins/core-plugins/video-generation-plugin/tools/providers.py`, `plugins/core-plugins/video-generation-plugin/tools/providers_test.py`	Runway, Sora, and Kling submit and poll requests use the shared HTTP client with polling timeouts. Tests use a shared-client stub and check the request timeouts.

Raw aiohttp session inventory

Layer / File(s)	Summary
Session detection and parser tests `repo_tests/_raw_aiohttp_session.py`, `repo_tests/raw_aiohttp_session_contrast_test.py`	Adds AST-based detection and path scanning for `ClientSession` calls. Tests cover call forms, recorded details, ordering, and syntax errors.
Tracked-file inventory guard `repo_tests/raw_aiohttp_session_inventory_12979_test.py`	Adds a scoped tracked-file inventory with per-file counts and structural categories. Tests also check exclusions, scope partitions, and the sibling guard.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to bdef9

Invalid timeout settings can leave generation requests without their intended deadlines. Validate those settings before merging; also fix the configuration-sensitive test and inventory check.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 46.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 9 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies both main changes: routing plugin requests through the pooled client and adding a guard for raw sessions outside the backend. It is specific and relevant to the pull reque…

Full details: Docstring Coverage

Explanation

Docstring coverage is 46.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 9 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

… of the pooled 10s read cap (#12979)

…stants (#12979)

github-actions · 2026-10-10T09:04:14Z

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

coderabbitai

Actionable comments posted: 3

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 55a82ef0-87e8-4bc8-807f-bbf28b3ab0e2

📥 Commits

Reviewing files that changed from the base of the PR and between 1c472bd and bdef94f.

📒 Files selected for processing (10)

autobot_shared/env_registry_ai.py
autobot_shared/generation_http_timeouts.py
docs/developer/ENV_VARS.md
plugins/core-plugins/image-generation-plugin/tools/generate_image.py
plugins/core-plugins/image-generation-plugin/tools/generate_image_test.py
plugins/core-plugins/video-generation-plugin/tools/providers.py
plugins/core-plugins/video-generation-plugin/tools/providers_test.py
repo_tests/_raw_aiohttp_session.py
repo_tests/raw_aiohttp_session_contrast_test.py
repo_tests/raw_aiohttp_session_inventory_12979_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

… tests, route inventory through tracked_paths (#12979)

mrveiss mentioned this pull request Oct 4, 2026

perf(http): 134 raw aiohttp.ClientSession constructions remain (127 per-request) despite the shared pool at 151 sites #12979

Open

mrveiss and others added 6 commits October 5, 2026 19:11

Merge branch 'main' into issue-12979-aiohttp-session

205756c

Merge branch 'main' into issue-12979-aiohttp-session

d063624

Merge branch 'main' into issue-12979-aiohttp-session

b052cc6

Merge branch 'main' into issue-12979-aiohttp-session

981f58e

fix(plugins): generation calls carry a provider-sized timeout instead…

822b470

… of the pooled 10s read cap (#12979)

test(plugins): pin the Flux and Stability timeouts to their exact con…

bdef94f

…stants (#12979)

mrveiss marked this pull request as ready for review October 10, 2026 09:02

coderabbitai Bot reviewed Oct 10, 2026

View reviewed changes

Comment thread autobot_shared/generation_http_timeouts.py Outdated

Comment thread plugins/core-plugins/video-generation-plugin/tools/providers_test.py Outdated

Comment thread repo_tests/raw_aiohttp_session_inventory_12979_test.py

fix(plugins): reject non-positive generation timeouts, collect plugin…

47c9c82

… tests, route inventory through tracked_paths (#12979)

mrveiss merged commit 7139a85 into main Oct 10, 2026
86 checks passed

mrveiss deleted the issue-12979-aiohttp-session branch October 10, 2026 10:10

mrveiss mentioned this pull request Oct 10, 2026

fix(ci): service containers pull from the ghcr mirror with the workflow token (#18160) #18197

Merged

Navigation Menu

Repository navigation

fix(http): plugins use the pooled client, and a ratchet watches the raw sessions outside the backend (#12979) - #17973

fix(http): plugins use the pooled client, and a ratchet watches the raw sessions outside the backend (#12979)#17973
mrveiss merged 8 commits into
mainfrom
issue-12979-aiohttp-session

mrveiss commented Oct 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 4, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mrveiss commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

What Changed

Verification

Single-issue rationale

Model Used

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Oct 10, 2026

✅ SSOT Configuration Compliance: Passing

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mrveiss commented Oct 4, 2026 •

edited

Loading

coderabbitai Bot commented Oct 4, 2026 •

edited

Loading