UN-3452 [FEAT] Characterise the seams: dispatch + chord call sites#1950
Open
muhammad-ali-e wants to merge 2 commits intomainfrom
Open
UN-3452 [FEAT] Characterise the seams: dispatch + chord call sites#1950muhammad-ali-e wants to merge 2 commits intomainfrom
muhammad-ali-e wants to merge 2 commits intomainfrom
Conversation
Sub-task A under #1.2 — characterisation suite for the seams that upcoming spine PRs will refactor. Two new test files, zero production changes. Dispatch seam (unblocks PR #8 — @shared_task -> @worker_task migration): - workers/tests/test_dispatch_sites_characterisation.py (276 lines, 11 tests) - Locks contract on the two raw current_app.send_task call sites: - shared/patterns/notification/helper.py:76 (webhook dispatch) - scheduler/tasks.py:157 (scheduled workflow async dispatch) - Tests pin: task name, positional args layout, kwargs layout, target queue, return-value semantics, error-path behaviour - Inventory canary: fails if a third raw current_app.send_task site appears anywhere in workers/ source Chord seam (unblocks PR #13 — chord -> Barrier lift): - workers/tests/test_chord_sites_characterisation.py (316 lines, 9 tests) - Locks contract on the chord pattern via: - WorkflowOrchestrationUtils.create_chord_execution (centralised helper) - WorkflowOrchestrationMixin.create_chord (mixin wrapper) - Tests pin: empty-batch short-circuit (existing defense against silent task drops at scale — Pain Point #2 in the PG Queue decision doc), callback-signature construction, return-value semantics, error propagation, mixin's app extraction + RuntimeError on missing app - Inventory canaries: fail if a third chord(...) call site OR a third `from celery import chord` import appears anywhere in workers/ source - api-deployment/tasks.py:673 inline chord covered only by inventory (direct unit-testing requires heavy mocking of the 273-line _run_workflow_api enclosing function — out of scope here, the canary still catches it for PR #13) Total: 20 tests, ~2s runtime, 0 production changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Summary by CodeRabbit
WalkthroughAdds two pytest characterization modules under ChangesCelery Chord Invocation Contract
Task Dispatch Contract
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| workers/tests/test_chord_sites_characterisation.py | New characterisation test file for two chord call sites; minor gap in import-canary specificity and over-broad skip_dirs logic |
| workers/tests/test_dispatch_sites_characterisation.py | New characterisation test file for two send_task dispatch sites; missing error-path characterisation for the scheduler dispatch failure branch |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
workers/tests/test_chord_sites_characterisation.py:263-282
**Import-canary missing file-identity assertions**
`test_chord_import_only_in_two_files` asserts the *count* of `from celery import chord` lines equals 2, but never checks *which* files they are. The parallel call-site canary (`test_only_two_known_chord_call_sites_in_workers`) correctly follows up with `assert "orchestration_utils.py" in joined` / `assert "api-deployment/tasks.py" in joined`. Without those same assertions here, the canary would silently pass if the two imports moved to entirely different files while the count remained 2 — exactly the scenario you'd want to catch during the Barrier migration.
### Issue 2 of 3
workers/tests/test_dispatch_sites_characterisation.py:172-247
**Missing send_task failure-path characterisation for the scheduler site**
`TestNotificationDispatchSite` has `test_dispatch_returns_false_on_send_task_failure` that pins the error branch when `send_task` raises. The scheduler site has no equivalent. In `scheduler/tasks.py` lines 185–192 there is a distinct error branch: when `current_app.send_task` raises, the exception is caught, logged, and the function returns a `SchedulerExecutionResult.error(...)` object rather than propagating. Without a characterisation test for this path, the upcoming `dispatch()` migration could silently change the error semantics (e.g. re-raise instead of returning an error result) without detection.
### Issue 3 of 3
workers/tests/test_chord_sites_characterisation.py:193-220
**`skip_dirs` check excludes any path component named `tests`, not just the top-level directory**
The guard `any(part in skip_dirs for part in py.parts)` operates on every component of the absolute path, so a hypothetical `workers/shared/tests_helpers/chord_stub.py` with a component matching `tests` would be erroneously excluded. Anchoring the skip to the path relative to `workers_root` (e.g. checking only `py.relative_to(workers_root).parts[0]`) would make the exclusion intent more precise.
Reviews (1): Last reviewed commit: "UN-3452 [FEAT] Characterise the seams: d..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/tests/test_dispatch_sites_characterisation.py`:
- Around line 249-262: The test is incorrectly checking absolute path components
via py.parts which can include ancestors outside the repo; change the skip check
to use only the path relative to workers_root so only in-repo segments are
considered: replace the condition that uses py.parts with one using
py.relative_to(workers_root).parts (i.e., if any(part in skip_dirs for part in
py.relative_to(workers_root).parts):) keeping the rest of the loop
(workers_root, skip_dirs, pattern, hits) unchanged.
- Around line 166-233: The tests patch the wrong target; change every
patch("celery.current_app") in these test methods to
patch("scheduler.tasks.current_app") so the local name imported in
scheduler.tasks (from celery import current_app) is mocked; update all
occurrences in test_dispatch_task_name, test_dispatch_routes_to_general_queue,
test_dispatch_positional_args_layout, test_dispatch_kwargs_layout, and
test_no_dispatch_when_execution_creation_fails to use
"scheduler.tasks.current_app" so _execute_scheduled_workflow sees the mock when
it calls send_task.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4eae378d-ba66-4292-9d62-a87ccaa2cc95
📒 Files selected for processing (2)
workers/tests/test_chord_sites_characterisation.pyworkers/tests/test_dispatch_sites_characterisation.py
Three P2 findings from Greptile, all fixed: 1. test_chord_import_only_in_two_files: add file-identity assertions matching the sibling call-site canary. Without these, the canary would silently pass if the two imports moved to entirely different files while count remained 2 — exactly the silent-miss scenario the Barrier migration could trigger. 2. TestSchedulerDispatchSite: add test_dispatch_returns_error_result_ when_send_task_raises. The scheduler site has a real error branch in scheduler/tasks.py:185-192 that catches send_task exceptions and returns SchedulerExecutionResult.error(...) — without a characterisation test the upcoming dispatch() migration could silently change error semantics (re-raise instead of returning an error result, or swallow silently). Mirrors the equivalent notification-site test_dispatch_returns_false_on_send_task_failure. 3. skip_dirs check anchored to top-level dir relative to workers_root in all three inventory tests. The previous `any(part in skip_dirs for part in py.parts)` check would have erroneously excluded any path with a component named `tests` (e.g. workers/shared/ tests_helpers/foo.py). 21 tests now (was 20), runtime ~3s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
workers/tests/test_dispatch_sites_characterisation.py (1)
166-257:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved:
patch("celery.current_app")does not interceptscheduler.tasks.current_app
scheduler/tasks.pybindscurrent_applocally viafrom celery import current_app. Withpatch()it matters that you patch objects in the namespace where they are looked up. Patching"celery.current_app"replaces the attribute on thecelerymodule, butscheduler.tasks.current_appalready holds a reference to the original proxy and is unaffected — the function under test never sees the mock.All six test methods (
test_dispatch_task_name,test_dispatch_routes_to_general_queue,test_dispatch_positional_args_layout,test_dispatch_kwargs_layout,test_no_dispatch_when_execution_creation_fails,test_dispatch_returns_error_result_when_send_task_raises) need the target changed to"scheduler.tasks.current_app".🐛 Proposed fix
- with patch("celery.current_app") as mock_app: + with patch("scheduler.tasks.current_app") as mock_app:Apply this change to every
patch("celery.current_app")occurrence inTestSchedulerDispatchSite(lines 169, 179, 188, 209, 227, 247).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/tests/test_dispatch_sites_characterisation.py` around lines 166 - 257, The tests patch the wrong target: they patch "celery.current_app" but scheduler.tasks imported current_app locally, so update each patch call in the TestSchedulerDispatchSite tests (test_dispatch_task_name, test_dispatch_routes_to_general_queue, test_dispatch_positional_args_layout, test_dispatch_kwargs_layout, test_no_dispatch_when_execution_creation_fails, test_dispatch_returns_error_result_when_send_task_raises) to patch "scheduler.tasks.current_app" so _execute_scheduled_workflow will see the mock; keep the rest of the assertions and mocked behaviors for mock_app.send_task unchanged.
🧹 Nitpick comments (1)
workers/tests/test_chord_sites_characterisation.py (1)
157-177: 💤 Low valueDocstring claims logging is verified but the test only checks re-raise
"""If chord() raises, the helper logs and re-raises (not swallowed)."""The test body asserts only the re-raise; the logging side-effect is not asserted. Either drop "logs" from the docstring, or add a
patchon the module's logger to assert it was called.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/tests/test_dispatch_sites_characterisation.py`:
- Around line 255-257: The assertion using a disjunction lets two outcomes pass;
run the characterisation test to observe the actual value of result.execution_id
produced by the current implementation, then replace the line "assert
result.execution_id == 'exec-123' or result.execution_id is None" with a single
equality asserting the observed concrete value (e.g. assert result.execution_id
== "<observed_value>") so the test pins the behavior of result.execution_id
precisely.
---
Duplicate comments:
In `@workers/tests/test_dispatch_sites_characterisation.py`:
- Around line 166-257: The tests patch the wrong target: they patch
"celery.current_app" but scheduler.tasks imported current_app locally, so update
each patch call in the TestSchedulerDispatchSite tests (test_dispatch_task_name,
test_dispatch_routes_to_general_queue, test_dispatch_positional_args_layout,
test_dispatch_kwargs_layout, test_no_dispatch_when_execution_creation_fails,
test_dispatch_returns_error_result_when_send_task_raises) to patch
"scheduler.tasks.current_app" so _execute_scheduled_workflow will see the mock;
keep the rest of the assertions and mocked behaviors for mock_app.send_task
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e5bbafd-46a7-457d-848b-f7771979480a
📒 Files selected for processing (2)
workers/tests/test_chord_sites_characterisation.pyworkers/tests/test_dispatch_sites_characterisation.py
Contributor
Author
|
Thanks for the careful review. All three P2 findings addressed in commit
21 tests now (was 20), runtime ~3s. Re-trigger Greptile when ready. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
pk-zipstack
approved these changes
May 8, 2026
athul-rs
approved these changes
May 8, 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
3 participants
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.
What
Two new test files characterising the refactor seams that upcoming spine PRs will touch. Zero production code changes.
workers/tests/test_dispatch_sites_characterisation.pycurrent_app.send_tasksitesworkers/tests/test_chord_sites_characterisation.pychord(...)sites + mixin wrapperLocal run:
20 passed in ~2s.Why
Lock down the current behaviour of the dispatch and chord seams before the upcoming refactors touch them. Per Michael Feathers' Working Effectively with Legacy Code approach: characterisation tests first, then refactor.
Two future spine PRs need this safety net:
@shared_task→@worker_taskdispatch migration — replaces both rawcurrent_app.send_task(...)call sites with a unifieddispatch()helper.chord(...)invocations with a transport-agnosticBarrierabstraction matching the labs target architecture'sDECR remaining:{exec_id}pattern.Chord is the highest-risk Celery construct noted in the workers/ rearchitecture decision (silent task drops at ~130K-task scale). Characterising before refactor is critical.
Reference: UN-3452.
How
Dispatch seam
Sites covered:
shared/patterns/notification/helper.py:76— webhook notification dispatchscheduler/tasks.py:157— scheduled workflow async dispatchFor each site, tests pin:
Chord seam
Sites covered:
shared/workflow/execution/orchestration_utils.py:67—WorkflowOrchestrationUtils.create_chord_execution(centralised helper) + itsWorkflowOrchestrationMixin.create_chordwrapperapi-deployment/tasks.py:673— inline chord inside_run_workflow_api(inventory-only — see note below)Tests pin:
self.appextraction +RuntimeErrorwhen no app boundInventory canaries
Three canary tests scan
workers/source and assert exactly the known number of call sites exist:current_app.send_task(...)invocation siteschord(...)invocation sitesfrom celery import chordimportsIf any future code adds a fourth site outside the new abstraction, the canary fails — forcing the developer to either route through the new dispatcher/Barrier or update the test deliberately.
What's NOT directly tested
The inline
chord(...)atapi-deployment/tasks.py:673lives inside a 273-line function (_run_workflow_api) that requires extensive setup mocking to reach. Direct unit testing would balloon this PR by ~150 lines of mocking infrastructure for marginal value. The inventory canary still catches it: the chord-to-Barrier migration must touch this file or the test fails.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
No. Adds two new test files under
workers/tests/. No production code modified. Tests useunittest.mockto mockcurrent_app.send_taskandchord— they don't invoke any real broker, database, or worker.Database Migrations
None.
Env Config
None — tests rely only on env vars already set by
workers/conftest.py.Related Issues or PRs
Dependencies Versions
None changed.
Notes on Testing
cd workers PYTHONPATH=.:../unstract .venv/bin/pytest \ tests/test_dispatch_sites_characterisation.py \ tests/test_chord_sites_characterisation.py \ -v --no-covExpected:
20 passed in ~2s.Existing tests unaffected — these are purely additive in
workers/tests/.Screenshots
N/A — test files only.
Checklist
🤖 Generated with Claude Code