Skip to content

Add regression test for task.duration on skipped tasks - #67943

Open
myps6415 wants to merge 1 commit into
apache:mainfrom
myps6415:fix-task-duration-skipped-61849
Open

Add regression test for task.duration on skipped tasks#67943
myps6415 wants to merge 1 commit into
apache:mainfrom
myps6415:fix-task-duration-skipped-61849

Conversation

@myps6415

@myps6415 myps6415 commented Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

task.duration was not emitted for skipped tasks: finalize() only emits it when ti.end_date is set, and the skip paths set end_date on the outbound TaskState message but not on the local task instance. Reproducible with ShortCircuitOperator, BranchPythonOperator, and BashOperator with skip_on_exit_code (narrowed in #61849).

The code change originally proposed here landed separately in #75685, which asserts on ti.end_date and listeners but not on the metric. This PR now adds only a regression test for the metric itself, so a future change to the finalize() gate cannot drop task.duration for a terminal state unnoticed.

Test plan

  • New parametrized test_task_duration_metric_emitted_for_terminal_states covers success / skipped / failed, checking task.duration with its tags and the legacy dotted name.
  • Reverting the skip-path ti.end_date assignment from Let listeners see the end date of a skipped task instance #75685 makes [skipped] fail; [success] and [failed] still pass.
  • TestTaskInstanceMetrics passes (14 tests); ruff format / ruff check and the commit's prek hooks pass.

closes: #61849

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5.5)

Generated-by: Claude Code (Opus 5.5) following the guidelines

@ashb ashb changed the title Fix missing task.duration metric for skipped tasks Fix missing task.duration metric for tasks that get skipped after starting. Jun 3, 2026
@potiuk potiuk added the ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green. label Jun 3, 2026
@myps6415

myps6415 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@ashb Gentle ping! You looked at this briefly earlier (thanks for the title fix). Would appreciate a review when you have a moment.

@potiuk potiuk removed the ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green. label Aug 13, 2026
@potiuk
potiuk marked this pull request as draft August 13, 2026 12:56
@myps6415
myps6415 marked this pull request as ready for review August 14, 2026 04:27
@myps6415
myps6415 force-pushed the fix-task-duration-skipped-61849 branch from f1a6834 to 5a5c67f Compare September 11, 2026 05:59
@myps6415

myps6415 commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (applied without conflicts). The fix is still needed — finalize() on main still gates task.duration on ti.end_date, which the skip path never set.

This has not had a full CI run yet: the workflows have been at action_required since the Aug 14 push. Could a maintainer approve them? Same area and code owners as #67592.

Both items in the Aug 13 triage note are resolved: the task-sdk failures were this PR's own new test missing the run_type tag (fixed in the Aug 14 push), and the provider/compat failures were the flit 4 --no-setup-py breakage fixed on main by #71118, which this rebase includes.

@myps6415
myps6415 force-pushed the fix-task-duration-skipped-61849 branch from 5a5c67f to 032d2ea Compare October 13, 2026 03:11
@myps6415

myps6415 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main to fix the conflict. Could a maintainer approve the CI run?

@myps6415
Add regression test for task.duration on skipped tasks
10b139d 
The task.duration metric went missing for skipped tasks because finalize()
only emits it when ti.end_date is set, and the skip path left it unset.
apache#75685 fixed the code path for listeners but did not assert on the metric,
so nothing guards against the gate being broken again.
@myps6415
myps6415 force-pushed the fix-task-duration-skipped-61849 branch from 032d2ea to 10b139d Compare October 28, 2026 03:09
@myps6415 myps6415 changed the title Fix missing task.duration metric for tasks that get skipped after starting. Add regression test for task.duration on skipped tasks Oct 28, 2026
@myps6415

myps6415 commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

#75685 landed the same ti.end_date change this PR proposed in June, so I've reduced this to the task.duration metric regression test it doesn't cover, plus closes #61849.

@myps6415

myps6415 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

@ashb Since you merged #75685, this is now a small follow-up: one regression test for the task.duration metric that #75685 doesn't assert, and it closes #61849. CI is green.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashb ashb Awaiting requested review from ashb ashb is a code owner
@kaxil kaxil Awaiting requested review from kaxil
@amoghrajesh amoghrajesh Awaiting requested review from amoghrajesh amoghrajesh is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@myps6415 myps6415

Labels

area:task-sdk

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Metric dag.<dag_id>.<task_id>.duration is missing

2 participants

@myps6415 @potiuk