Skip to content

fix: handle deleted workflow job in ancestor_job property#16331

Open
benthomasson wants to merge 2 commits intoansible:develfrom
benthomasson:fix/ancestor-job-null-check
Open

fix: handle deleted workflow job in ancestor_job property#16331
benthomasson wants to merge 2 commits intoansible:develfrom
benthomasson:fix/ancestor-job-null-check

Conversation

@benthomasson
Copy link
Contributor

@benthomasson benthomasson commented Mar 6, 2026

Summary

  • Fix AttributeError 500 error when a workflow job is deleted while its child jobs still exist
  • The ancestor_job property now checks for None from get_workflow_job() before recursing, falling back to self when the parent workflow is gone
  • One-line logical change in awx/main/models/unified_jobs.py

Fixes #16250

Test plan

  • Delete a workflow job that has child jobs, then view a child job via the API — should no longer 500
  • Verify launched_by and summary_fields serialize correctly when ancestor workflow is deleted
  • Verify normal workflow job ancestry still resolves correctly when no deletions have occurred

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Prevented an error when a workflow-spawned job has no corresponding workflow reference, improving stability and avoiding unexpected failures.
  • Tests

    • Added unit tests covering ancestor resolution for normal jobs, missing workflow references, and traversal to parent workflow jobs.

When a workflow job is deleted while its child jobs still exist,
`get_workflow_job()` returns None. The `ancestor_job` property
previously called `.ancestor_job` on the None return value, causing
an AttributeError 500 in the API (e.g. when serializing summary_fields).

Add a null check so that jobs whose parent workflow has been deleted
gracefully fall back to treating themselves as the ancestor.

Fixes ansible#16250

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

📝 Walkthrough

Walkthrough

Modified the ancestor_job property in UnifiedJob to check for a missing workflow job before accessing its ancestor_job. If get_workflow_job() returns None for a workflow-spawned job, the property now returns self instead of raising an AttributeError.

Changes

Cohort / File(s) Summary
UnifiedJob model
awx/main/models/unified_jobs.py
Added a null-check in ancestor_job: when spawned_by_workflow is true, call get_workflow_job() and return its ancestor_job only if the workflow job is not None; otherwise return self.
Unit tests
awx/main/tests/unit/models/test_unified_job_unit.py
Added three tests covering ancestor_job behavior: non-workflow launch returns self, missing/deleted workflow job returns self, and traversal to parent workflow job returns that workflow job.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main fix: handling deleted workflow jobs in the ancestor_job property, which matches the core change in the PR.
Linked Issues check ✅ Passed The PR successfully addresses issue #16250 by adding null-check logic to ancestor_job property and including regression tests covering the crash scenario when get_workflow_job() returns None.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the ancestor_job null-handling bug: modifications to the property logic in unified_jobs.py and corresponding unit tests in test_unified_job_unit.py are both in-scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Covers the crash from ansible#16250 where ancestor_job called .ancestor_job
on None when get_workflow_job() returned None for a deleted workflow.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
awx/main/tests/unit/models/test_unified_job_unit.py (1)

40-46: Strengthen this to cover nested workflow ancestry.

This only proves a single-hop parent lookup. The reported failure comes from recursive ancestry (wf1 -> wf2 -> job), so adding a case where WorkflowJob itself has an ancestor would better protect the intended behavior.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@awx/main/tests/unit/models/test_unified_job_unit.py` around lines 40 - 46,
Extend the test_ancestor_job_traverses_workflow to assert recursive ancestry by
creating a second WorkflowJob (e.g., wf_parent) that is the ancestor of the
first WorkflowJob (wj), then attach wj to wf_parent (so wf_parent -> wj ->
child) and verify child.ancestor_job resolves to wf_parent; use the same classes
referenced in the diff (WorkflowJob, UnifiedJob, WorkflowJobNode) and the
existing mock.patch on django.db.ConnectionRouter.db_for_write to avoid DB
access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@awx/main/tests/unit/models/test_unified_job_unit.py`:
- Around line 40-46: Extend the test_ancestor_job_traverses_workflow to assert
recursive ancestry by creating a second WorkflowJob (e.g., wf_parent) that is
the ancestor of the first WorkflowJob (wj), then attach wj to wf_parent (so
wf_parent -> wj -> child) and verify child.ancestor_job resolves to wf_parent;
use the same classes referenced in the diff (WorkflowJob, UnifiedJob,
WorkflowJobNode) and the existing mock.patch on
django.db.ConnectionRouter.db_for_write to avoid DB access.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e7c9e345-b3f6-4d90-ab40-3a624fa5678d

📥 Commits

Reviewing files that changed from the base of the PR and between ed4d527 and 4d64a71.

📒 Files selected for processing (1)
  • awx/main/tests/unit/models/test_unified_job_unit.py

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 6, 2026

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Internal server error on list Job Template/Jobs when source workflow deleted

1 participant