Skip to content

fix: close schedule poller Redis result consumers after dispatch - #44543

Closed
CodeWarriors0 wants to merge 4 commits into
langgenius:mainfrom
CodeWarriors0:fix/schedule-poller-redis-connection-cleanup
Closed

CodeWarriors0 wants to merge 4 commits into
langgenius:mainfrom
CodeWarriors0:fix/schedule-poller-redis-connection-cleanup

Conversation

@CodeWarriors0

@CodeWarriors0 CodeWarriors0 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Production impact: this issue caused more than 10,000 Redis connections to accumulate in my production environment.
This is the production observation that motivated the fix, not a count generated by the tests. Validation of the fix under production load is not claimed here.

Fixes #44542.

Problem

The scheduled-workflow poller dispatches due workflows through a Celery group without waiting for its result. In a long-lived gevent worker, the group's callbacks still start a Redis result drainer. Canceling a subscription does not stop that drainer, so its backend and connection can remain alive and accumulate over repeated dispatch batches.

Issue #44542 contains the affected environment and reproduction steps. Actual dispatches matter; empty polls do not exercise this path. Retained client IDs and increasing idle times are more useful than expecting a strict +1 in the server-wide connection count after each workflow execution.

Fix

Keep group dispatch but give each dispatch an explicitly owned Redis result backend. In finally, stop its gevent drainer with a bounded wait, close PubSub and the private connection pool, clear result references, and restore the previous backend. Do not disconnect the broker or an initialized shared backend.

Commit successful schedule updates before waiting for cleanup, so the cleanup wait does not extend the successful transaction's row-lock lifetime. No dependencies, configuration, or unrelated application code are changed.

CI follow-up

The first revision failed Style Check / Python Style because the no-new-getattr guard rejected two calls. Commit 39fd25cfdd replaces those calls with Celery's existing backend accessor and an explicit gevent drainer type.

The regression tests were also rewritten to meet the repository's testing policies: they instantiate real Celery, Redis, gevent, SQLAlchemy Session, and model objects, and patch boundary methods instead of adding Mock/MagicMock/SimpleNamespace constructors. No guard exemptions, disabled checks, or CI policy changes were added.

Verification

Final local validation on Python 3.13 with upstream's locked dependencies:

  • Focused regression suite: 22 passed.
  • Default coverage tracing for _dispatch_result_backend: 36/36 executable lines and 12/12 branches covered locally, with no coverage-policy changes.
  • Commit 7e92273d5a adds synchronous PubSub/pool-close failure tests so these paths are also verified without gevent context switches.
  • Ruff lint and formatting for both changed files: passed.
  • make type-check-core: passed.
  • All five changed-code guards, run against the PR's upstream baseline: passed (getattr, controller SQLAlchemy, session mocks, base models, test doubles).
  • Repository-wide no-spec-mock guard: passed.
  • Import contracts and response contracts: passed.
  • git diff --check: passed.

The focused test command is:

uv run --project api pytest api/tests/unit_tests/schedule/test_workflow_schedule_task.py -q --no-cov

Tests cover normal and exceptional cleanup, restoration of initialized/uninitialized backends, late AsyncResult destruction, shared/non-Redis bypass, unstarted/running/stopped drainers, and publication/SQLite commit failures. A separate subprocess runs real Celery/gevent groups and real Redis connection-pool/PubSub code over local socket pairs; it verifies SUBSCRIBE bytes, concurrent dispatch isolation, and socket closure. It does not connect to a Redis server.

Limits: the new-head GitHub CI must still complete. These tests are not a full worker/Redis integration, network-fault, or production-load validation. The production impact reported above is separate from the automated evidence.

Checklist

  • Linked the associated issue with Fixes #44542.
  • Explained the production symptom, affected path, and scoped fix.
  • Added and verified focused regression tests.
  • Corrected the initial repository-policy violations without exemptions.
  • New-head GitHub CI has completed successfully.

No user-facing documentation update or screenshots are applicable to this backend resource-lifecycle change. This contribution is AI-assisted; the actual checks and verification limits are disclosed above.

From DeepSeek Harness

@github-actions github-actions Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Oct 22, 2026
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 77.07% 77.08% +0.01%
Strict coverage 76.75% 76.75% +0.01%
Typed symbols 67,175 67,209 +34
Untyped symbols 20,071 20,072 +1
Modules 3934 3935 +1

@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.99%. Comparing base (cc5b0b5) to head (91db6b4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #44543      +/-   ##
==========================================
- Coverage   89.05%   88.99%   -0.06%     
==========================================
  Files        5616     5608       -8     
  Lines      337608   337460     -148     
  Branches    67539    67390     -149     
==========================================
- Hits       300652   300319     -333     
- Misses      31710    31900     +190     
+ Partials     5246     5241       -5     
Flag Coverage Δ
api 89.10% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Oct 22, 2026
@CodeWarriors0

Copy link
Copy Markdown
Contributor Author

Coverage follow-up:

  • The early 68.51% project report for 39fd25cfdd was generated while coverage uploads were still being combined. The final Codecov commit data reports state=complete, 89.05% project coverage, 5,616 files, and 11 sessions. Both API unit shards, controller coverage, and integration coverage were merged. The GitHub project-check notification was still showing an earlier aggregate when checked.
  • The completed patch report then showed two uncovered cleanup-error lines. Existing gevent tests asserted those calls, but the default collector did not record them. Commit 7e92273d5a adds synchronous consumer failure cases using real runtime objects and boundary-method patches.
  • Local results with the repository's default coverage configuration: 22 tests passed; the cleanup helper has 36/36 executable lines and 12/12 branches covered. No thresholds, exclusions, or CI settings were changed. This is a helper-level local result, not a claim that the whole module or new-head CI is at 100%.

The new-head CI run is pending; the production implementation is unchanged by this follow-up.

@wylswz

wylswz commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Hi @CodeWarriors0 , thanks for reporting this issue. We have merged a simpler fix by removing group usage. Will close this one.

@wylswz wylswz closed this Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scheduled workflow polling leaves Redis result connections open (10,000+ observed in production)

2 participants