Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.29%. Comparing base (1a39f8c) to head (f20a7be).
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main   #10245      +/-   ##
==========================================
+ Coverage   88.27%   88.29%   +0.01%     
==========================================
  Files         153      153              
  Lines       19663    19694      +31     
  Branches     2266     2277      +11     
==========================================
+ Hits        17358    17388      +30     
- Misses       2005     2006       +1     
  Partials      300      300              

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

@auvipy Cross-checked both Copilot concerns. The second one was valid — good catch by Copilot.

Root cause: first_task_args_applied tracked the first task in the reverse iteration (i.e., the chain's last task), not the first-in-order task. This would misapply partial args for chain(group(), add.s(x), add.s(y))(z) — z would go to add.s(y) instead of add.s(x).

Fix (commit 08e5adc): Instead of the first_task_args_applied flag, we now pre-strip leading empty groups from the deque before the reverse-iteration loop begins. This restores the original is_first_task = not steps logic, which correctly identifies the chain's first-in-order task. Empty groups in middle/trailing positions are still handled by the existing inline continue.

6 regression tests added covering:

• Leading empty group with partial args passthrough
• Multiple leading empty groups
• Empty group in middle of chain
• Trailing empty group
• Verification that args reach correct task (first real, not last)
• Chain of only empty groups

All 205 tests pass.

Thanks for the thorough review @auvipy! I've pushed a new commit addressing each concern. Here's the breakdown:

Concern 1: Args/kwargs forwarding when leading empty groups are skipped

This is handled correctly. The pre-strip in prepare_steps removes leading empty groups from the deque before the reverse iteration begins, so is_first_task (not steps) fires for the correct task — the first real task after the empty groups. Tests test_chain_empty_group_first_passes_args, test_chain_empty_group_first_args_go_to_first_real_task, and test_chain_empty_group_args_reach_correct_task all verify this.

Concern 2: should_apply_first_args flag logic

Verified — the reverse walk applies args correctly. is_first_task = not steps is True for the last item popped from the deque, which is the first logical task. The pre-strip ensures the deque starts with a real task. Multiple regression tests confirm args go to the first real task.

Concern 3: _chain.freeze() returning None

freeze() returns results[0] if results else None. This only returns None for a chain consisting entirely of empty groups (no real work). This is the correct semantic — there is no AsyncResult to return when there are no tasks. test_chain_only_empty_groups covers this path.

Concern 4: Nested chain splicing with leading empty groups ✅ Valid concern, fixed!

When _chain(_chain(group(), add.s()), ...) is processed, the inner chain gets spliced into the deque. The inner empty group() becomes a regular step. Added a post-loop fallback with an applied_first_args tracking flag — if the loop finishes without applying args to any task, it applies them to the logically first task. Added test_nested_chain_with_leading_empty_group to cover this case.

Concern 5: Test docstring references

Fixed — all test docstrings now reference "issue #9772" instead of "PR #10245".

Also fixed the flake8 E303 lint error. All 207 tests pass (206 existing + 1 new).

Cross-check of Copilot's review concerns

I've gone through each concern carefully against the actual code. Here's the analysis:

Concern 1: Empty group() entries cause is_first_task misfire, partial args not applied

Verdict: Already handled — not a real issue.

The code has a two-layer defense:

1. Pre-pass (lines 1175–1186): Strips all leading empty groups from steps before the main loop, so is_first_task = not steps fires correctly for the first real task.
2. Fallback (lines 1320–1325): The applied_first_args flag tracks whether args were applied. If not (e.g., due to chain-splicing interactions), the fallback applies them to tasks[-1] which IS the first logical task (list is in reverse order).

Tests test_chain_empty_group_first_passes_args, test_chain_empty_group_first_args_go_to_first_real_task, and test_chain_empty_group_args_reach_correct_task all verify this works correctly.

Concern 2: applied_first_args applies args to wrong task

Verdict: Not valid — Copilot misread the data structure.

The fallback applies args to tasks[-1]. Since tasks is built in reverse order (the loop pops from a deque and appends), tasks[-1] is the first logical task, not the last. This is correct. The existing test test_chain_empty_group_args_reach_correct_task explicitly verifies that args go to the first task and NOT the last.

Concern 3: _chain.freeze() returns None when all steps are empty

Verdict: Valid — fixed in this push.

When prepare_steps() yields no results (chain of only empty groups), freeze() was returning None, breaking the AsyncResult return type contract. Callers that access .id or .parent would get AttributeError.

Fix: freeze() now returns self.app.AsyncResult(_id or uuid()) for the empty case, preserving the return type contract.

Bonus: The existing test_apply_contains_chords_containing_empty_chain was marked @pytest.mark.xfail because it used to raise IndexError on empty chains. Our fix resolves this, so I've removed the xfail marker — it now passes cleanly.

Added test: test_chain_only_empty_groups_freeze_returns_result

Concern 4: Nested _chain with leading empty groups inside won't be stripped by pre-pass

Verdict: Technically true but already handled — not a bug.

Copilot is correct that the pre-pass only strips top-level leading empty groups. A nested _chain(_chain(group(), add.s()), ...) won't have the inner group() stripped by the pre-pass because the outer head is a _chain, not a group.

However, the main loop handles this correctly:

1. The outer _chain gets spliced (unrolled) via steps_extend(task.tasks)
2. The inner group() is then encountered as a regular step and skipped by the isinstance(task, group) and not task.tasks check
3. If is_first_task wasn't True for the right task, the applied_first_args fallback catches it

The existing test test_nested_chain_with_leading_empty_group validates this exact scenario and confirms args are applied correctly.

Concern 5: Test docstrings reference "PR #10245" instead of issue #9772

Verdict: Not valid — already correct.

All test docstrings already reference "issue #9772", not "PR #10245". No change needed.

Summary

All 208 canvas tests pass, plus the 1 previously-xfailed test now passes too (209 total).

Round 3 cross-check — all 3 Copilot concerns addressed

Concern 1: freeze() returning None — Added a docstring to _chain.freeze() that explicitly documents the empty-chain behavior: when all steps are empty groups (no-ops), freeze() returns a minimal AsyncResult so callers can safely access .id and .parent. The underlying fix was already in place (commit d92e487); this adds the documentation Copilot requested.

Concern 2: Nested chain splicing with leading empty groups — Added 3 new regression tests covering post-splice empty group scenarios:

• test_nested_chain_multiple_leading_empty_groups_after_splice: inner chain with 2 leading empty groups
• test_nested_chain_only_empty_groups_spliced: inner chain that is only empty groups
• test_nested_chain_trailing_empty_group_after_splice: trailing empty group in spliced chain

All pass. The existing code already handles these correctly: when _chain is spliced via steps_extend(task.tasks), the spliced tasks re-enter the main while steps loop where the empty-group continue check (line 1224–1227) skips them, and the applied_first_args fallback (lines 1321–1328) ensures partial args reach the correct first real task. No additional post-splice stripping is needed.

Concern 3: Test docstring references — Already addressed in commit 02f8996. All test docstrings reference "issue #9772" (the underlying issue), not "PR #10245".

All canvas tests pass (211 passed, 1 xfailed).

Addressing Copilot review concerns

Concern 1: PR number references in test docstrings

Verified — all test docstrings already reference issue #9772, not PR #10245. No changes needed. (You can confirm by searching the test file: grep -n "10245\|PR #" t/unit/tasks/test_canvas.py returns nothing.)

Concern 2: freeze() fabricated AsyncResult for empty chains in group/chord headers

I traced the full code path for chord((chain(tuple()),), body):

1. chord.run() checks if header.tasks: — the header group has 1 element (chain(tuple())), so it enters the publishing branch, not the empty-header shortcut (body.delay([]))
2. group._apply_tasks() iterates over the header tasks. For the empty chain:
   • chain.freeze() returns a fabricated AsyncResult (the new code)
   • _chord._descend(sig) returns 0 for empty chains (line 2244–2252) — so chord_size is correctly set to 0
   • chain.apply_async() → chain.run() returns None without publishing any tasks
3. set_chord_size(group_id, 0) is called — chord behavior with size 0 is backend-dependent

This is a pre-existing edge case, not introduced by this PR. The analogous scenario with empty groups (not chains) already has a pre-existing @pytest.mark.xfail test at line 1385 (test_apply_contains_chords_containing_empty_group), with the comment "this is an invalid setup... probably being left incomplete forever".

The prepare_steps stripping handles the common user-facing case: chain(group(), group(), add.s(1,1)) correctly strips the empty groups before they can become chord headers. The fabricated AsyncResult in freeze() is a safety net for edge cases where an empty chain is embedded directly as a chord header element — and in that case, the behavior is no worse than before (previously it would crash).

Concern 3: Removed xfail from test_apply_contains_chords_containing_empty_chain

The removed @xfail from this test (line 1328) is correct. The previous xfail reason was "Invalid canvas setup with bad exception" — the test was marked xfail because it crashed with IndexError, not because the chord-never-unlocking behavior was being tested.

The test now just asserts no exception is raised (group_sig.apply_async() completes without error). It does not assert the chord completes or that the body fires. This is the right scope: verifying crash prevention. The chord-unlocking semantics for degenerate empty headers is a separate concern that already exists (see the still-xfail test_apply_contains_chords_containing_empty_group test).

If we want to address the chord-never-unlocking case for empty chain headers in the future, that would be a separate enhancement — e.g., teaching chord.run() to detect when all header tasks are empty and fall through to the body.delay([]) shortcut. But that's out of scope for this crash-fix PR.

@ChickenBenny would you mind trying and reviewing this?

Round 5: Copilot Concern Analysis & Fix

@auvipy — I've analyzed each Copilot concern and pushed a fix for the one that turned out to be a real bug. Here's the breakdown:

Concern 1 (CRITICAL — canvas.py:1117): Fabricated AsyncResult can cause permanently pending group/chord — REAL BUG, FIXED

This one is valid. Here's the exact code path:

1. When a chord header contains chain(tuple()) (an empty chain), group._freeze_unroll() (line 1978) processes it as a non-group task — it calls task.freeze() on the chain, which hits the fabricated AsyncResult path at line 1117.
2. The fabricated result gets included in the chord's frozen results.
3. group._apply_tasks() then calls sig.apply_async() on the empty chain. chain.run() returns None (line 1077: if results_from_prepare: is falsy, so it falls through without publishing any tasks).
4. But _apply_tasks already incremented p.size += 1 on the barrier at line 1863.
5. Result: the barrier has a slot that no worker will ever fulfill — chord hangs permanently.

The same issue exists in group._prepared() (line 1793-1802), which yields (task, result, group_id) tuples for empty chains that will never complete.

Fix (commit 22cf5dd): Skip empty chains in both _prepared() and _freeze_unroll(), matching the existing empty-group-skip logic in prepare_steps():

# In _prepared() and _freeze_unroll():
elif isinstance(task, _chain) and not task.tasks:
    # Skip empty chains -- they are no-ops.  (Issue #9772)
    continue

This means when a chord header contains only empty chains, header.tasks becomes empty after freezing, and chord.run() takes the else branch at line 2322 (body.delay([])) — executing the body immediately instead of hanging.

Concern 2 (CRITICAL — test_canvas.py:1335): Test doesn't verify chord completes — ADDRESSED BY CONCERN 1 FIX

With the Concern 1 fix, the empty chains are now stripped from the chord header during _freeze_unroll(). The chord header becomes effectively empty, so the chord body is executed immediately via body.delay([]). Updated the test docstring to explain the expected behavior. The test itself (calling apply_async() without raising) is still correct — it now works for the right reason (empty header = immediate body execution) rather than the wrong reason (fabricated result attached to barrier).

Concern 3 (MEDIUM — canvas.py:1239): Nested chain splice misses leading empty groups — NOT A REAL ISSUE

The concern says the "strip leading empty groups" pre-pass (lines 1192-1198) won't catch empty groups inside nested chains after splicing. This is correct — the pre-pass only runs once at the top of prepare_steps(). However, this doesn't matter because:

• When steps_extend(task.tasks) splices a nested chain at line 1238, the nested chain's tasks are added to the steps deque.
• Each task is then processed in subsequent iterations of the while steps: loop.
• The empty group check at lines 1231-1234 (if isinstance(task, group) and not task.tasks: continue) runs every iteration, not just for leading groups.
• So empty groups from nested chains ARE caught — just by the in-loop check rather than the pre-pass.

The existing tests test_nested_chain_with_leading_empty_group and test_nested_chain_multiple_leading_empty_groups_after_splice already cover this case and pass.

Concern 4 (LOW — test_canvas.py:883): Docstrings reference PR instead of issue — FALSE ALARM

All docstrings consistently reference "issue #9772", not "PR #10245". Checked every occurrence — line 882, 896, 909, 922, 934, 946, 964, 975, 988, 1008, 1025, 1042, 1334 — all say issue #9772. No fix needed.

Concern 5 (from earlier round): freeze() return type — ALREADY ADDRESSED

_chain.freeze() (lines 1112-1117) always returns an AsyncResult: either from results[0] when there are real steps, or a fabricated one via self.app.AsyncResult(_id or uuid()) for all-empty chains. It never returns None. The return type is AsyncResult, not Optional[AsyncResult].

Summary

All 211 canvas tests pass (1 xfail expected).