Skip to content

AAP-90692 Nested POST /api/v2/inventories/{id}/hosts/ times out - #16657

Open
lallen92 wants to merge 5 commits into
ansible:develfrom
lallen92:AAP-90692-nested-host-post-timeout
Open

lallen92 wants to merge 5 commits into
ansible:develfrom
lallen92:AAP-90692-nested-host-post-timeout

Conversation

@lallen92

@lallen92 lallen92 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor
SUMMARY

Fixes a hang/timeout on POST /api/v2/inventories/{id}/hosts/ for inventories with thousands of hosts.

SubListCreateAttachDetachAPIView.attach()'s membership check used if sub not in relationship.all(). Django's QuerySet has no__contains__, so it falls back to iterating the entire related queryset for a nested host create, that means fetching and JSON-decoding every host in the inventory (including each one's variables field) just to confirm one host is already attached. Cost scales with total host count, not with the request itself.

Swapped it for Django's built-in QuerySet.contains() (available since Django 5.0, we're on 5.2.8), which compiles to a single indexed, LIMIT 1-bounded existence check for the specific host's pk instead of materializing the whole collection. Same semantics, no behavior change for callers, this is also the shared pattern behind every other SubListCreateAttachDetachAPIView/SubListAttachDetachAPIView subclass (HostGroupsList, RoleUsersList, RoleTeamsList, InventoryInstanceGroupsList, etc.), so the fix benefits all of them, not just inventory hosts.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API
STEPS TO REPRODUCE AND EXTRA INFO
  1. Create an inventory with several thousand hosts (reproduced at 6,251).
  2. POST /api/v2/inventories/{id}/hosts/ with a new unique hostname.
  3. Before the fix: request hangs, eventually hits the uWSGI graceful timeout (~10s) with variables JSON-decode frames in the traceback.

Verified against a real 6,251-host inventory on a running instance:

Before After
Membership-check query full table scan, all columns, no LIMIT WHERE inventory_id = ? AND id = ? LIMIT 1
Membership-check time 144.6ms 0.6ms (~240x faster
End-to-end POST times out (~10s) 201 Created in 0.586s

Added a regression test asserting the membership-check SQL is id-filtered and LIMIT-bounded, a plain query-count assertion doesn't catch this, since both the broken and fixed code issue exactly one query; the defect is in that query's cost, not its count.

Summary by CodeRabbit

  • Performance

    • Improved inventory host attachment checks with a targeted, bounded membership lookup, reducing unnecessary work when inventories contain many hosts.
    • Host association checks now scale more efficiently during inventory updates.
  • Bug Fixes

    • Preserved existing behavior that adds a host only when it is not already associated with the inventory.
    • Improved consistency when attaching hosts to inventories.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d9a47953-39de-4b8c-a53d-d835c202a966

📥 Commits

Reviewing files that changed from the base of the PR and between 7a2bb0f and afaa3f2.

📒 Files selected for processing (3)
  • awx/api/generics.py
  • awx/main/tests/functional/api/test_inventory.py
  • awx/main/tests/unit/api/test_generics.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The attach method now uses relationship.contains(sub) instead of loading all related objects. Tests configure the new mock method and verify that inventory host membership queries filter by primary key and use LIMIT.

Changes

Relationship membership

Layer / File(s) Summary
Bounded membership check
awx/api/generics.py, awx/main/tests/unit/api/test_generics.py, awx/main/tests/functional/api/test_inventory.py
The attach path uses relationship.contains(sub) before adding the sub-object. Unit mocks provide the new method. Functional coverage verifies primary-key filtering and a LIMIT clause in the membership query.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: djulich

Merge Risk: ⚪ Minimal · up to afaa3

The change preserves relationship scoping while replacing full materialization with bounded membership queries; no merge-blocking risk is evidenced.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. 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 identifies the affected nested API request and the timeout issue addressed by the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@aap-pde-ci-bot

Copy link
Copy Markdown

❌ Test Results - FAILED

Summary

Metric Count
Total Tests 612
✅ Passed 559
❌ Failed 2
⚠️ Errors 1
⏭️ Skipped 50
⏱️ Duration 3847.89s

Pass Rate: 91.3%

❌ Failed Tests

Test Class
test_constructed_inventory_basic_aggregation opt.test-suite.tests.inventories.test_constructed_control_plane.TestConstructedInventoryControlPlane
test_constructed_inventory_basic_aggregation opt.test-suite.tests.inventories.test_constructed_control_plane.TestConstructedInventoryControlPlane
test_deleted_uj_logged_as_deleted_user[system_job] opt.test-suite.tests.test_activity_stream.TestActivityStream

@aap-pde-ci-bot

Copy link
Copy Markdown

❌ Test Results - FAILED

Summary

Metric Count
Total Tests 612
✅ Passed 560
❌ Failed 1
⚠️ Errors 1
⏭️ Skipped 50
⏱️ Duration 3884.73s

Pass Rate: 91.5%

❌ Failed Tests

Test Class
test_constructed_inventory_basic_aggregation opt.test-suite.tests.inventories.test_constructed_control_plane.TestConstructedInventoryControlPlane
test_constructed_inventory_basic_aggregation opt.test-suite.tests.inventories.test_constructed_control_plane.TestConstructedInventoryControlPlane

@sonarqubecloud

Copy link
Copy Markdown

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.

2 participants