Skip to content

Fix fact caching with ansible-core 2.19+ and stop fact updates leaking across inventories - #1126

Open
cigamit wants to merge 7 commits into
mainfrom
fix-fact-cache-schema-prefix
Open

cigamit wants to merge 7 commits into
mainfrom
fix-fact-cache-schema-prefix

Conversation

@cigamit

@cigamit cigamit commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fact caching does not work with any execution environment running ansible-core 2.19 or newer. Facts gathered by a job never reach the database, and cached facts are not visible to the playbook. Our default EE ships ansible-core 2.16, so it isn't affected yet. Any newer EE is.

This PR makes fact caching work with both the older and the 2.19+ cache formats. It also ports three upstream AWX fixes, which stop facts leaking between inventories and stop concurrent jobs wiping each other's facts. Review rounds then closed several more gaps: smart inventories, constructed inventories, vaulted values, host-name collisions and memory use.

Related upstream issue: ansible/awx#13863. Its latest comment reports facts being written to s1_<hostname> files; this PR explains and fixes that.

Root cause

Starting in 2.19, ansible-core wraps every persistent cache plugin. Each host's facts are stored in a file named s1_<host> containing {"__payload__": "<json string>"}. Ascender only reads and writes plain <host> files, so:

  • Facts gathered by a job are lost: ansible writes them to s1_<host>, which Ascender never reads.
  • Cached facts are not seen by the playbook: Ascender writes them to <host>, which ansible no longer reads.

The payload is not plain fact data. Values carry ansible's internal type information, for example:

{"value": "run-7", "tags": [{"__ansible_type": "Origin", ...}], "__ansible_type": "_AnsibleTaggedStr"}

So simply copying the payload into the database (the workaround suggested in the upstream issue) would corrupt facts.

Changes

Support for both cache formats (ascender/main/tasks/facts.py)

We can't know which ansible-core an EE has before the job runs (jobs don't record it), so both formats are handled:

  • Before the run: start_fact_cache writes both <host> and s1_<host>. The summary file now records exactly which files were written (written_files), so a file that failed to write (e.g. a host name too long once s1_ is added) is never mistaken for a deliberate clear.
  • After the run: finish_fact_cache reads whichever file ansible updated and converts the 2.19+ format back to plain data: type tags are dropped and dates become ISO strings.
  • Clearing facts: a fact file written for a host that is now missing means meta: clear_facts ran, on either version.

Vaulted values

  • 2.19+ caches a decrypted vaulted value as plaintext tagged with its ciphertext. That plaintext is never stored: the ciphertext is turned back into the {"__ansible_vault": ...} marker the stock 2.16 EE uses. This also covers inline !vault values.
  • Facts that already carry that marker are passed to 2.19+ unchanged. The 2.19+ cache decoder can only rebuild a vaulted value from plaintext, so the marker reaches a later 2.19+ job as a dict rather than a usable secret. On the stock 2.16 EE it is decrypted with a vault credential, exactly as before.

Data 2.19+ can't represent safely

  • Reserved key: facts containing a dict with the reserved __ansible_type key get only the plain file. ansible-core 2.19+ has no way to escape that key: an unknown value aborts the whole playbook, and a known one is silently converted.
  • Literal __payload__ facts: a plain file holding a fact literally named __payload__ is still read as facts. Only the shared file described below needs its format decided before reading.
  • Malformed files: a malformed 2.19+ file is skipped like invalid JSON, so it can no longer abort fact saving for the rest of the job's hosts.

Host-name collisions (x and s1_x in the same inventory)

The file s1_x is both host s1_x's older-format file and host x's 2.19+ file.

  • Before the run: it is written for s1_x, so the stock 2.16 EE behaves exactly as before. Host x gets no cached facts on 2.19+; ansible ignores a file that isn't in its format.
  • After the run: which version ran is worked out from files only one version could have touched (updated or deleted). That decides who owns the shared file. With no clear evidence, the shared file is skipped with a warning instead of guessed.

Upstream ports: ansible/awx#16318, #16361, #16403

  • finish_fact_cache now takes the job's host queryset. Previously it looked up hosts by name across all inventories, so same-named hosts in unrelated inventories received the job's facts. This was reproduced live.
  • Constructed inventories store facts on their source hosts (new Job.get_source_hosts_for_constructed_inventory()).
  • Fact clearing is skipped when another job saved newer facts after this job was created.
  • Hosts that never had a fact file written are no longer "cleared".
  • Reference files are backdated by 2 seconds so they can't be mistaken for files ansible updated.
  • Hosts are matched against a single directory listing.

Fact clearing (finish_fact_cache)

  • Concurrent saves: the host rows are locked in id order and timestamps rechecked inside a transaction, so another job's facts saved between the check and the clear can't be wiped.
  • Smart inventories: the rows are now locked through a fresh query by id. Previously the clear raised a database error that was only logged, so the job showed "successful" while facts were silently kept.
  • Memory: only the columns needed to clear are loaded, not every cleared host's facts.

Constructed-inventory syncs (ascender/main/tasks/jobs.py)

  • Input inventories share one cache directory, so their host names are now checked for collisions together.
  • Only the host names are collected up front. Hosts and their facts are then streamed through a single pass, so peak memory stays proportional to one batch rather than all inputs combined.

Testing

Automated

  • Ported upstream's test_fact_cache.py (inventory scoping, concurrent-job protection, constructed inventories) and updated unit/models/test_jobs.py.
  • New tests cover:
    • both formats, using real ansible-core 2.21 output
    • vaulted values
    • reserved keys and literal __payload__ facts
    • malformed files and failed writes
    • host-name collisions on both versions
    • smart-inventory clearing
    • constructed-inventory collision and streaming
    • a SQL-capture check that clearing doesn't load facts
    • a two-thread race that holds a row lock while the clear runs (10/10 stable)
  • Each new test fails without its fix.
  • The invalid-JSON unit test previously used a mock that returned no hosts, so it passed even with broken handling. It now reads every file.
  • 981 passed across the fact, job, inventory, injector and task suites. ruff check and format are clean.

Live, on the dev stack (an EE built with ansible-core 2.21.5, plus the stock 2.16 EE)

Scenario Before After
2.21 job: playbook sees previous facts ❌ ✅
2.21 job: gathered facts saved to DB ❌ ✅
2.21 sliced job (3 slices) ❌ ✅
meta: clear_facts on 2.21 / 2.16 ❌ / ✅ ✅ / ✅
2.16 job reads facts written by a 2.21 job n/a ✅
Same-named host in another inventory ❌ overwritten ✅ untouched
Smart-inventory job clearing facts ❌ silently not cleared ✅
Strict constructed sync on 2.16 ✅ ✅
Vaulted fact on 2.21 then reused on 2.16 ❌ plaintext / marker lost ✅ encrypted in DB, decrypted on 2.16
Colliding fa / s1_fa: update and clear on both versions ❌ facts swapped ✅

Known limitations

  • Vaulted facts on 2.19+: a vaulted cached fact reaches a later 2.19+ job as the __ansible_vault dict, not a decryptable value. ansible-core's cache can only rebuild a vaulted value from plaintext, which we deliberately never store.
  • Colliding host names: if hosts x and s1_x both exist, x gets no cached facts before a 2.19+ run. In the much rarer case where s1_x also has a fact literally named __payload__, 2.19+ could misread the shared file. Both judged acceptable given how unlikely the pairing is.
  • Constructed-inventory syncs run on the control-plane EE (currently 2.16) and will rely on the 2.19+ handling once that image moves.
  • Future formats: a future ansible-core format change (e.g. an s2_ prefix) will need another update.
  • Upstream syncs: upstream AWX has no s1_ handling yet, so keep this change on future upstream syncs.

@cigamit cigamit self-assigned this Oct 15, 2026
Copilot AI balanced review requested due to automatic review settings October 15, 2026 17:45
@cigamit cigamit added the bug Something isn't working label Oct 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Filename collisions, failed reference writes, and non-atomic clearing can retain stale facts or erase valid facts.

4 open findings
What changed in this PR

Adds ansible-core 2.19+ fact-cache compatibility while retaining legacy support and restricting updates to the intended inventory hosts.

Changes:

  • Reads and writes both cache formats, converting tagged values.
  • Scopes updates to inventory/source hosts and adds fact-clearing safeguards.
  • Adds functional regressions and updates unit tests.
File Description
ascender/​main/​tests/​unit/​models/​test_jobs.py Updates cache-clearing and malformed-data tests.
ascender/​main/​tests/​functional/​test_fact_cache.py Adds compatibility, scoping, and clearing coverage.
ascender/​main/​tasks/​jobs.py Passes scoped hosts and job creation time to cache finalization.
ascender/​main/​tasks/​facts.py Implements dual-format caching and revised update/clear handling.
ascender/​main/​models/​jobs.py Extracts constructed-inventory source-host resolution.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ascender/main/tasks/facts.py Outdated
Comment thread ascender/main/tasks/facts.py Outdated
Comment thread ascender/main/tasks/facts.py Outdated
Comment thread ascender/main/tests/unit/models/test_jobs.py
Copilot AI balanced review requested due to automatic review settings October 15, 2026 20:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread ascender/main/tasks/facts.py Outdated
Comment thread ascender/main/tasks/facts.py Outdated
Comment thread ascender/main/tasks/facts.py Outdated
Copilot AI balanced review requested due to automatic review settings October 15, 2026 22:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new reader silently rejects valid legacy facts containing only a string-valued __payload__ key.

1 open finding
2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid misclassifying legacy __payload__ facts as wrappers

ascender/​main/​tasks/​facts.py:85

An older EE can legitimately cache a single fact named __payload__ using cacheable set_fact with gather_facts: false. Its plain file, for example {"__payload__": "run-7"}, matches _is_payload_wrapper, so this check rejects it when schema_qualified=False and silently drops the database update—even when the filename is unambiguous. Accept arbitrary dictionaries in unambiguous legacy files, keep wrapper-based ownership checks separate for shared filenames, and add a regression test for this legacy fact shape.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 16, 2026 19:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved issues can expose vaulted plaintext, assign facts to the wrong host and increase memory usage during bulk clearing.

3 open findings
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid materializing cached facts during clear_facts

ascender/​main/​tasks/​facts.py:320

This materializes every host being cleared, including all cached fact dictionaries, before releasing any of them. A large meta: clear_facts run can therefore exhaust task-worker memory, unlike the previous batched iterator. Clearing only needs host IDs, names, inventory names and modification timestamps. Defer the unused fact data when loading the locked rows.

🧠 Review effort: Balanced

Comment thread ascender/main/tasks/facts.py
Comment thread ascender/main/tasks/facts.py Outdated
Copilot AI balanced review requested due to automatic review settings October 16, 2026 22:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Shared-file data isolation, malformed-payload handling, and constructed-inventory memory usage remain unresolved.

3 open issues
1 previously missed issue

In code that hasn't changed since last review

Medium severity Avoid retaining all inventory facts simultaneously during preparation

ascender/​main/​tasks/​jobs.py:1662

This retains every input inventory's complete ansible_facts payloads simultaneously until cache preparation finishes. Previously each input's host collection could be released before loading the next, so peak fact memory now grows with the sum of all inputs rather than the largest input, risking worker exhaustion on large constructed inventories. Gather names separately for the shared collision check, then stream or batch the fact-bearing hosts.

2 resolved since last review

Review effort: Balanced

Comment thread ascender/main/tasks/facts.py
Comment thread ascender/main/tasks/facts.py Outdated
Copilot AI balanced review requested due to automatic review settings October 17, 2026 03:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Vaulted facts cannot round-trip correctly, and ambiguous cache filenames can expose another host’s facts.

2 open issues
1 previously missed issue

In code that hasn't changed since last review

Medium severity Prevent cross-host schema cache collisions and misrouting

ascender/​main/​tasks/​facts.py:153

Skipping the schema copy still publishes s1_web1's plain facts at the path newer Ansible reads for web1. If those facts are {"__payload__": "{\"x\": 1}"}, the file is a valid schema wrapper and web1 receives the other host's data; a non-JSON payload can instead cause a decoding error. Finish-time ownership checks cannot prevent this during the playbook. Avoid publishing ambiguous shared files or separate the format namespaces, and add a regression that consumes the prepared cache through the newer Ansible reader.

2 resolved since last review

Review effort: Balanced

Comment thread ascender/main/tasks/facts.py
Copilot AI balanced review requested due to automatic review settings October 17, 2026 03:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-version serialization and concurrent fact-clearing behavior require final human validation.

1 open issue
1 resolved since last review

Review effort: Balanced

@cigamit
cigamit requested a review from TheWitness October 17, 2026 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

2 participants