Skip to content

Run Python SDK handlers concurrently, not inline on the dispatch stream - #146

Merged
keithfz merged 1 commit into
mainfrom
keithzeto/cd-596-python-sdk-concurrent-handlers
Sep 30, 2026
Merged

keithfz merged 1 commit into
mainfrom
keithzeto/cd-596-python-sdk-concurrent-handlers

Conversation

@keithfz

@keithfz keithfz commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The Python SDK ran each handler inline on the dispatch stream loop:

for response in stub.Dispatch(...):
    result = handler_to_invoke(ctx)
    self.agent_stub.ReportInvocation(invoke_info)

The next message was not read until the current handler returned, so one slow handler stalled everything behind it. There is no threading anywhere else in the SDK, so the concurrency limit was exactly one.

Webhooks make this visible, and it is what was reported in CD-596:

  1. The agent accepts a webhook and returns 2xx
  2. It queues the invocation and sends it to the SDK
  3. It starts a timeout timer
  4. The SDK is still busy on the previous invocation
  5. The timer fires — the agent logs invocation error: or context deadline exceeded

The Go SDK does not have this problem — it already runs a goroutine per invocation.

Change

Handlers now run on a bounded ThreadPoolExecutor, eight at a time by default and configurable with max_concurrent_handlers.

A semaphore stops the loop reading once every worker is busy. That is deliberate: the agent then sees its own queue fill, instead of this process queueing without limit.

Reporting moved onto the worker, so a failure there can no longer propagate into the dispatch loop and force a reconnect. It is caught and logged — otherwise the future would swallow it and the invocation would look like it vanished.

Note for handler authors

Two invocations of the same handler can now run at the same time, so handlers must be thread safe. max_concurrent_handlers=1 restores the old behaviour. Documented in the SDK README.

Tests

test_handlers_run_concurrently dispatches four invocations that block on a barrier. It passes only if all four are in flight at once, and fails against the serial version — verified by temporarily reverting the submit.

Two test-infrastructure fixes came with it:

  • The mock gRPC server was not referenced anywhere after the fixture returned, so it could be garbage collected mid-test and stop listening.
  • A MagicMock cannot serve a streaming RPC, so no existing test had ever exercised dispatch — the stream always failed and the client retried. The new test uses a real servicer.

Full suite: 9 passed, ruff clean.

🤖 Generated with Claude Code

aszarama
aszarama previously approved these changes Sep 14, 2026
The dispatch loop called each handler inline:

    for response in stub.Dispatch(...):
        result = handler_to_invoke(ctx)
        self.agent_stub.ReportInvocation(invoke_info)

so the next message was not read until the current handler returned. One
slow handler stalled every invocation behind it. There is no threading
anywhere else in the SDK, so the limit was exactly one at a time.

Webhooks make that visible. The agent accepts a webhook, returns 2xx,
queues it, sends it, and starts a timeout timer. While the SDK is busy on
the previous invocation the queued ones time out, and the agent logs
"invocation error:" or "context deadline exceeded". The sender was told
2xx, so the webhook is simply lost. Paychex hit this on xld_deploy_sync,
which syncs deployments from XL Deploy and is slow enough to fall behind.

Run handlers on a bounded thread pool instead, which is what the Go SDK
already does with a goroutine per invocation. A semaphore stops the loop
reading once every worker is busy, so the agent sees its own queue fill
rather than this process growing without bound.

Reporting now happens on the worker, so a failure there can no longer
propagate into the dispatch loop and force a reconnect. It is caught and
logged, otherwise the future would swallow it and the invocation would
look like it vanished.

Two test fixes come along with it. The mock server was not held anywhere,
so it could be garbage collected mid-test and stop listening, and a
MagicMock cannot serve a streaming RPC at all, which is why no test had
ever exercised dispatch. The new test uses a real servicer and fails
against the serial version.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@keithfz
keithfz force-pushed the keithzeto/cd-596-python-sdk-concurrent-handlers branch from 74098e6 to 44845fe Compare September 30, 2026 14:46
@keithfz
keithfz requested a review from aszarama September 30, 2026 14:46
keithfz added a commit that referenced this pull request Sep 30, 2026
The PR scan on #146 flagged 52 fixable HIGH CVEs. 47 are OS packages:
libssl3t64, openssl and openssl-provider-legacy at 3.5.7-1~deb13u2, and
linux-libc-dev at 6.12.107-1. The cached apt layer is stale again.

A fresh apt layer built today gets openssl 3.5.7-1~deb13u3 and
linux-libc-dev 6.12.111-1, which are the fixed versions.

The other 5 findings (engine.io and brace-expansion) come from the
snyk-broker fork and are fixed in cortexapps/snyk-broker#30.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@keithfz
keithfz merged commit 62ca18a into main Sep 30, 2026
20 of 22 checks passed
@keithfz
keithfz deleted the keithzeto/cd-596-python-sdk-concurrent-handlers branch September 30, 2026 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants