fix(chat): enforce ConcurrencyConfig.max_concurrent + release 0.4.26.1#60
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnforces ConcurrencyConfig.max_concurrent via an asyncio.Semaphore for the "concurrent" strategy; validates construction-time values; bumps package to 0.4.26.1; updates changelog, contributing guidance, and UPSTREAM_SYNC; adds tests for concurrency limits and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Chat as Chat
participant Sem as Semaphore
participant Handler as Handler
Client->>Chat: submit message / dispatch
Chat->>Sem: acquire() (if max_concurrent set)
Sem-->>Chat: acquired
Chat->>Handler: dispatch handler (run)
Handler-->>Chat: completes / errors
Chat->>Sem: release() (if acquired)
Sem-->>Chat: released
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/chat.py`:
- Around line 306-310: The current initialization of self._concurrent_semaphore
silently treats _concurrency_max_concurrent <= 0 as None which re-enables
unbounded concurrency; instead validate _concurrency_max_concurrent (the
constructor/initializer that sets _concurrency_max_concurrent) and raise a clear
ValueError for zero or negative values, only allowing None for "no limit" via an
explicit None input and creating
asyncio.Semaphore(self._concurrency_max_concurrent) when a positive integer is
provided; update any docs/comments and tests referencing
_concurrency_max_concurrent or _concurrent_semaphore accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 337e95e4-7062-4f02-ae19-03841c69d287
📒 Files selected for processing (5)
CHANGELOG.mdCONTRIBUTING.mdpyproject.tomlsrc/chat_sdk/chat.pytests/test_chat_faithful.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request bumps the version to 0.4.26.1 and introduces several fixes and features, most notably the enforcement of max_concurrent for the concurrent strategy using an asyncio.Semaphore. Feedback suggests that this limit should be applied globally across all event types and strategies to ensure consistent resource bounding. Additionally, it is recommended to consider lazy initialization for the semaphore to better support multi-event loop environments.
patrick-chinchill
added a commit
that referenced
this pull request
Apr 23, 2026
- Raise `ValueError` on `max_concurrent <= 0` instead of silently falling back to unbounded (CodeRabbit MAJOR). A user setting `max_concurrent=0` likely means "throttle hard" and would be surprised by unlimited dispatch otherwise. - Remove redundant local `import asyncio` in 2 test methods — module is already imported at the top of the file (github-code-quality). - Soften early `assert max_observed == 2` to `<= 2`. The early check is about "bound not exceeded during gate-closed phase"; the final `== 2` after drain is what asserts the peak was actually reached (github-code-quality "redundant comparison"). - New test: `test_max_concurrent_zero_or_negative_raises` covers 0, -1, -100.
patrick-chinchill
added a commit
that referenced
this pull request
Apr 23, 2026
- **Reject `max_concurrent` with non-concurrent strategy**: if a user
sets `ConcurrencyConfig(strategy="queue", max_concurrent=5)`, they
have a misconfig — the field is only honored under `"concurrent"`.
Upstream accepts silently but we fail-fast so it's never invisible.
New test `test_max_concurrent_with_non_concurrent_strategy_raises`.
- **Tighten error message**: raw input via `!r` repr instead of private
`self._concurrency_max_concurrent` attribute; message now also tells
the user how to fix ("pass None for unbounded").
- **Test robustness**: replace `for _ in range(20): await asyncio.sleep(0)`
yield-counting loops with `asyncio.wait_for(_reach_cap(), timeout=1.0)`
polling until the condition holds. Fixed counts can be flaky on slow
CI when handlers have multiple internal await points.
- **Document divergence in UPSTREAM_SYNC.md**: added row to Known
Non-Parity table with rationale (upstream accepts the config field
but never enforces it; we do).
- **Inline breadcrumb comment** `# Divergence from upstream — see
docs/UPSTREAM_SYNC.md` at the `_concurrent_semaphore` construction
site, matching the divergence discipline in CLAUDE.md.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_chat_faithful.py`:
- Around line 2362-2404: Update the test so it explicitly covers the case where
the concurrency config is constructed with max_concurrent=None (not just the
unset/default string). Instead of only calling _init_chat(...,
concurrency="concurrent"), parameterize or add a second invocation that passes a
ConcurrencyConfig(max_concurrent=None) (or the equivalent constructor used in
the codebase) to _init_chat and run the same in-flight assertion; keep the rest
of test_max_concurrent_none_allows_unbounded logic identical and reference the
existing test name and the _init_chat helper and ConcurrencyConfig symbol to
locate where to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eadd3007-1120-4f09-a52c-7f4c116f6ada
📒 Files selected for processing (1)
tests/test_chat_faithful.py
Collaborator
Author
patrick-chinchill
left a comment
patrick-chinchill
left a comment
There was a problem hiding this comment.
Re-reviewed current head 97cc314. This looks good to merge. Focused verification passed: uv run pytest tests/test_chat_faithful.py -q -k max_concurrent (5 passed). I couldn't submit an approval because GitHub does not allow approving your own pull request.
Fixes #51. Upstream TS accepts the config field but never enforces it (3 writes, 0 reads). Python now uses an asyncio.Semaphore so a consumer setting `concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=N)` actually gets an N-way cap on in-flight handlers. Divergence from upstream — Python leads here. The upstream behavior is documented ("Default: Infinity") but not implemented; our enforcement matches the documented contract. Changes: - `Chat.__init__` constructs `asyncio.Semaphore(max_concurrent)` when the value is set and positive, leaves it `None` otherwise (unbounded). - `_handle_concurrent` acquires the semaphore around `_dispatch_to_handlers` when present; falls through to direct dispatch when `None`. - 2 new tests in `TestConcurrencyConcurrent`: - `test_max_concurrent_bounds_in_flight_handlers`: 5 messages with `max_concurrent=2`; verifies `max_observed == 2` at any time. - `test_max_concurrent_none_allows_unbounded`: 5 concurrent handlers without a bound all run in parallel (regression guard for the default). Release prep for 0.4.26.1: - Version bump `0.4.26` → `0.4.26.1` in pyproject.toml - `UPSTREAM_PARITY` stays `4.26.0` (still synced to 4.26.0 upstream; no new upstream sync in this release) - CHANGELOG entry with Fixes / Python-specific / New public APIs / Internals / Known gaps sections - CONTRIBUTING.md version-scheme wording: `.patch` bumps during alpha can be additive (fixes + features). Tightens to "fixes-only" once we hit 1.0. Test plan: - `uv run ruff check src/ tests/ scripts/` — clean - `uv run ruff format --check src/ tests/ scripts/` — clean - `uv run python scripts/audit_test_quality.py` — 0 hard failures - `uv run pytest tests/ --tb=short -q` — 3471 passed, 2 skipped
- Raise `ValueError` on `max_concurrent <= 0` instead of silently falling back to unbounded (CodeRabbit MAJOR). A user setting `max_concurrent=0` likely means "throttle hard" and would be surprised by unlimited dispatch otherwise. - Remove redundant local `import asyncio` in 2 test methods — module is already imported at the top of the file (github-code-quality). - Soften early `assert max_observed == 2` to `<= 2`. The early check is about "bound not exceeded during gate-closed phase"; the final `== 2` after drain is what asserts the peak was actually reached (github-code-quality "redundant comparison"). - New test: `test_max_concurrent_zero_or_negative_raises` covers 0, -1, -100.
- **Reject `max_concurrent` with non-concurrent strategy**: if a user
sets `ConcurrencyConfig(strategy="queue", max_concurrent=5)`, they
have a misconfig — the field is only honored under `"concurrent"`.
Upstream accepts silently but we fail-fast so it's never invisible.
New test `test_max_concurrent_with_non_concurrent_strategy_raises`.
- **Tighten error message**: raw input via `!r` repr instead of private
`self._concurrency_max_concurrent` attribute; message now also tells
the user how to fix ("pass None for unbounded").
- **Test robustness**: replace `for _ in range(20): await asyncio.sleep(0)`
yield-counting loops with `asyncio.wait_for(_reach_cap(), timeout=1.0)`
polling until the condition holds. Fixed counts can be flaky on slow
CI when handlers have multiple internal await points.
- **Document divergence in UPSTREAM_SYNC.md**: added row to Known
Non-Parity table with rationale (upstream accepts the config field
but never enforces it; we do).
- **Inline breadcrumb comment** `# Divergence from upstream — see
docs/UPSTREAM_SYNC.md` at the `_concurrent_semaphore` construction
site, matching the divergence discipline in CLAUDE.md.
Review caught: between `_reach_cap` returning and the `assert in_flight == 2` line, a third task could briefly reach the semaphore-gated handler on a fast multi-core runner. The 10-yield loop that followed was also vulnerable. `max_observed` is captured inside the handler under the cooperative scheduler — it's the atomic signal for 'did the bound leak'. The final `assert max_observed == 2` after all tasks drain is the authoritative check. Removing the point-in-time assertions eliminates the race without losing coverage.
…t max_concurrent=None
CodeRabbit minor: the test only verified the string form ('concurrent');
adding the ConcurrencyConfig(max_concurrent=None) case proves the
explicit null path also stays unbounded. The two take different code
paths in Chat.__init__ (string branch → defaults all fields; dataclass
branch → reads the field), so both need coverage.
Self-review caught my over-correction. asyncio uses a single-threaded cooperative scheduler — tasks 3-5 are parked on semaphore.acquire() and can't interleave between '_reach_cap' returning and the next synchronous assertion. 'in_flight == 2' IS stable there. Keeping max_observed == 2 as the primary signal + in_flight == 2 as a snapshot check while the gate is closed gives stronger coverage than either alone. Also: added ids=['string', 'config_none'] to the unbounded parametrize. Without it pytest falls back to 'concurrency_value1' for the ConcurrencyConfig case, which is opaque in CI logs.
patrick-chinchill
force-pushed
the
fix/max-concurrent-enforcement
branch
from
97cc314 to
ff10803
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_chat_faithful.py (1)
2269-2417: Add an exception-path regression for semaphore release.These tests cover the steady-state cap and constructor validation, but not the "release on error" half of the feature. If
_handle_concurrent()is ever rewritten away fromasync with, a leaked permit after one failing handler would slip past this suite.🧪 Suggested test
+ async def test_max_concurrent_releases_permit_on_handler_error(self): + state = create_mock_state() + adapter = create_mock_adapter("slack") + + chat, _, _ = await _init_chat( + adapter=adapter, + state=state, + concurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=1), + ) + + first_started = asyncio.Event() + second_started = asyncio.Event() + seen: list[str] = [] + + `@chat.on_mention` + async def handler(thread, message, context=None): + seen.append(message.id) + if message.id == "msg-1": + first_started.set() + raise RuntimeError("boom") + second_started.set() + + first = asyncio.create_task( + chat.handle_incoming_message( + adapter, + "slack:C123:1", + create_test_message("msg-1", "Hey `@slack-bot`"), + ) + ) + await asyncio.wait_for(first_started.wait(), timeout=1.0) + await asyncio.gather(first, return_exceptions=True) + + await chat.handle_incoming_message( + adapter, + "slack:C123:2", + create_test_message("msg-2", "Hey `@slack-bot`"), + ) + await asyncio.wait_for(second_started.wait(), timeout=1.0) + assert seen == ["msg-1", "msg-2"]As per coding guidelines, "Every test must fail when the code is wrong."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chat_faithful.py` around lines 2269 - 2417, The suite is missing a regression test that a semaphore permit is released when a handler raises in _handle_concurrent(); add a test that configures ConcurrencyConfig(strategy="concurrent", max_concurrent=1), registers an `@chat.on_mention` handler that acquires the gate then raises an exception, dispatches two messages via chat.handle_incoming_message such that the first acquires the semaphore and fails, then verify the second is able to acquire the semaphore and complete (e.g., use Events/gate to control timing and assert finished count and max_observed to ensure no permit leak). Ensure the test fails if permits are not released (i.e., second task blocks forever or max_observed stays 1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/UPSTREAM_SYNC.md`:
- Line 460: Update the docs row to state that only non-None values for
max_concurrent are rejected when strategy != "concurrent" (i.e.,
max_concurrent=None is allowed for "queue"/"debounce"/"drop"); mention that when
strategy=="concurrent" a non-None max_concurrent is enforced via
asyncio.Semaphore to bound in-flight handlers and that the field is a no-op if
left as None. Reference the config field name max_concurrent and the strategy
string "concurrent" and the check in src/chat_sdk/chat.py that raises only when
max_concurrent is not None and strategy != "concurrent".
In `@src/chat_sdk/chat.py`:
- Around line 318-333: The code currently only checks
self._concurrency_max_concurrent <= 0 but does not reject non-integer types
(floats, bools, strings) which can lead to incorrect Semaphore behavior; add an
explicit type validation for self._concurrency_max_concurrent before the <= 0
check: require it to be an actual int (reject bool since bool is subclass of
int) by using either type(self._concurrency_max_concurrent) is int or
(isinstance(self._concurrency_max_concurrent, int) and not
isinstance(self._concurrency_max_concurrent, bool)); if the check fails raise a
ValueError that includes the offending value (raw_max) and a clear message; keep
the subsequent <= 0 check and the Semaphore creation
(asyncio.Semaphore(self._concurrency_max_concurrent)) unchanged otherwise.
---
Nitpick comments:
In `@tests/test_chat_faithful.py`:
- Around line 2269-2417: The suite is missing a regression test that a semaphore
permit is released when a handler raises in _handle_concurrent(); add a test
that configures ConcurrencyConfig(strategy="concurrent", max_concurrent=1),
registers an `@chat.on_mention` handler that acquires the gate then raises an
exception, dispatches two messages via chat.handle_incoming_message such that
the first acquires the semaphore and fails, then verify the second is able to
acquire the semaphore and complete (e.g., use Events/gate to control timing and
assert finished count and max_observed to ensure no permit leak). Ensure the
test fails if permits are not released (i.e., second task blocks forever or
max_observed stays 1).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c765cd6-3d4e-4891-b6d6-7defb34f71f3
📒 Files selected for processing (6)
CHANGELOG.mdCONTRIBUTING.mddocs/UPSTREAM_SYNC.mdpyproject.tomlsrc/chat_sdk/chat.pytests/test_chat_faithful.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- CONTRIBUTING.md
- CHANGELOG.md
- Reject non-integer `max_concurrent` (float, bool, str) at construction instead of deferring to `asyncio.Semaphore` behavior — `1.5` silently drives the counter negative; `True` allocates a 1-way bound from a bool; `"2"` raises the wrong exception. CodeRabbit flagged. - Clarify the `ConcurrencyConfig.max_concurrent` divergence row in UPSTREAM_SYNC: rejection only fires when the field is non-`None`; `max_concurrent=None` stays compatible with every strategy. - Added `test_max_concurrent_non_integer_raises` covering six bad-type cases (float, True, False, str, 0.0, list). - Refresh stale test count (3544 → 3545). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 21, 2026
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
1 participant
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.
Fixes #51. Release prep for
0.4.26.1.What
ConcurrencyConfig.max_concurrentwas accepted atChat.__init__and never read anywhere (3 writes, 0 reads). Upstream TS has the same gap — the field is documented as"Default: Infinity"but never enforced. Consumers settingconcurrency=ConcurrencyConfig(strategy="concurrent", max_concurrent=5)got unlimited concurrency silently.This PR:
asyncio.Semaphore(max_concurrent)inChat.__init__when the value is set and positive_handle_concurrentaround_dispatch_to_handlersNone/ unset keeps unbounded behavior (matches upstream TS default ofInfinity)Divergence note: upstream also doesn't enforce, so Python is leading here. Upstream can mirror the pattern if they want.
Release 0.4.26.1
Bundles this fix with the other landed work for the Python-only follow-up on
0.4.26:await(#44), Teams zero-height divider (#45),max_concurrentenforced (this PR, #51)request.text()duck-type handling (Flask sync support),inspect.isawaitableon sync-or-async handlersChat.thread()factory (#46), Slackcurrent_token/current_client(#47)Version scheme clarified in CONTRIBUTING.md:
.patchbumps during alpha can be additive (fixes + features) since the package is markedDevelopment Status :: 3 - Alphaand pre-1.0 per semver convention. Once we hit1.0,.patchtightens to fixes-only.Test plan
TestConcurrencyConcurrent:test_max_concurrent_bounds_in_flight_handlers— 5 dispatched, cap 2, observes cap holdstest_max_concurrent_none_allows_unbounded— no cap, all 5 run concurrentlyuv run ruff check src/ tests/ scripts/— cleanuv run ruff format --check src/ tests/ scripts/— cleanuv run python scripts/audit_test_quality.py— 0 hard failuresuv run pytest tests/ --tb=short -q— 3471 passed, 2 skippedRelease sequencing
Depends on PRs #48 (bug fix bundle) and #49 (pyrefly zero-errors) landing first. Once all three are on
main:pyproject.tomlshows0.4.26.1v0.4.26.1gh release create v0.4.26.1 --target main --title "v0.4.26.1 — Python-only follow-up on 4.26"→ triggers PyPI publishCloses #51.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores