fix: remove generated template and profile schemas with their node (closes #10049) - #10051
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r node Deleting a node or generic from a SchemaBranch now cascades to its generated Template/Profile kinds, and the template weight pass skips orphaned templates so schema processing reaches the step that cleans them up. Previously, a worker reloading a branch schema after a generate_template node was removed kept the orphaned template entry and failed with SchemaNotFoundError on every refresh, permanently desyncing the branch schema across workers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
AGENT_REVIEW_VERDICT: FIX_APPROVED Overall verdict: APPROVED WITH SUGGESTIONSThe fix correctly addresses the root cause from #10049. A branch that removes a A. Correctness — PASS
B. Code quality — PASS
C. Documentation alignment — PASS
D. Test quality — APPROVED, one gap noted
Recommended next steps
AGENT_REVIEW_ITERATION: fix-1
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the test-coverage gap from the review (section D): added |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Focused bug fix for orphaned template/profile schemas when a node is removed. The change adds a cascade in delete and a safety skip in template generation; tests cover the fix. Bounded and clearly beneficial.
Re-trigger cubic
Kind migrations delete a node and immediately set it back under the same or a new kind without reprocessing, so cascading in delete() removed generated profiles that nothing regenerated. The orphaned-template skip in the weight pass is sufficient on its own: processing now survives to the template and profile management steps, which remove orphaned generated kinds from the maps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI failure analysis: |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. This is a targeted bug fix (5 lines) that prevents a schema reload crash by skipping orphaned templates, letting cleanup proceed. Bounded and clearly beneficial, with focused tests.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes a bug where removing a node with generate_template:true left orphaned template/profile schemas, causing worker crashes. The 5-line skip in _generate_weight_templates and the cascade deletion in delete() clean up generated kinds, with tests covering both paths.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 4/5
- In
backend/tests/component/core/schema/schema_branch/test_process_idempotency.py, there’s a meaningful uncertainty aroundSchemaBranch.get_hash()potentially incorporating DB-assigned in-memory IDs afterload_schema_to_db, which could break idempotency checks across save/load cycles and cause false change detection—add or tighten a round-trip assertion that hash input excludes transient database identifiers.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/component/core/schema/schema_branch/test_process_idempotency.py">
<violation number="1" location="backend/tests/component/core/schema/schema_branch/test_process_idempotency.py:279">
P2: Confirm that SchemaBranch.get_hash() is idempotent after a DB save/load cycle. If load_schema_to_db sets in-memory DB id attributes (e.g., node/relationship database identifiers) that feed into the hash computation, then a fresh SchemaBranch(cache={}, ...) loaded via load_schema_from_db will get different internal ids and produce a different hash than the 'saved_schema' that was mutated in-place by load_schema_to_db. If the hash is pure schema-model content (name, namespace, attributes, etc.), this is fine; if not, test_reload_matches_saved_schema may be flaky or perma-fail.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| schema = registry.schema.register_schema(schema=schema_root, branch=default_branch_scope_class.name) | ||
| # load_schema_to_db mutates the schema in place to assign DB ids, so the saved state is | ||
| # only final once it returns | ||
| await registry.schema.load_schema_to_db(schema=schema, db=db, branch=default_branch_scope_class) |
There was a problem hiding this comment.
P2: Confirm that SchemaBranch.get_hash() is idempotent after a DB save/load cycle. If load_schema_to_db sets in-memory DB id attributes (e.g., node/relationship database identifiers) that feed into the hash computation, then a fresh SchemaBranch(cache={}, ...) loaded via load_schema_from_db will get different internal ids and produce a different hash than the 'saved_schema' that was mutated in-place by load_schema_to_db. If the hash is pure schema-model content (name, namespace, attributes, etc.), this is fine; if not, test_reload_matches_saved_schema may be flaky or perma-fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/component/core/schema/schema_branch/test_process_idempotency.py, line 279:
<comment>Confirm that SchemaBranch.get_hash() is idempotent after a DB save/load cycle. If load_schema_to_db sets in-memory DB id attributes (e.g., node/relationship database identifiers) that feed into the hash computation, then a fresh SchemaBranch(cache={}, ...) loaded via load_schema_from_db will get different internal ids and produce a different hash than the 'saved_schema' that was mutated in-place by load_schema_to_db. If the hash is pure schema-model content (name, namespace, attributes, etc.), this is fine; if not, test_reload_matches_saved_schema may be flaky or perma-fail.</comment>
<file context>
@@ -222,34 +253,128 @@ def test_process_idempotency(register_core_models_schema: SchemaBranch, idempote
+ schema = registry.schema.register_schema(schema=schema_root, branch=default_branch_scope_class.name)
+ # load_schema_to_db mutates the schema in place to assign DB ids, so the saved state is
+ # only final once it returns
+ await registry.schema.load_schema_to_db(schema=schema, db=db, branch=default_branch_scope_class)
+ return schema
+
</file context>
There was a problem hiding this comment.
the IDs are persisted to the database and loaded from the database. so this is not an issue
|
pushed some changes and updated the description
|
Why
Loading a schema change onto a branch that removes a node with
generate_template: true(viastate: absent) leaves the branch permanently unable to converge: workers reloading the branch schema keep the orphanedTemplate<Kind>entry, crash withSchemaNotFoundErrorduringprocess(), and never recover — the UI shows "Loading schemas…" forever, GraphQL on non-initiating workers misses fields from that and later loads, and every subsequent load on the branch re-triggers the failure. Reproduced on 1.10.1, 1.10.5, and currentstable.Goal: removing a node with generated kinds converges on every worker, and the branch stays usable.
Non-goals: the wider no-snapshot/no-rollback design of
SchemaManager.load_schema(a mid-process()failure still leaves the in-memory registry half-updated) is a separate robustness issue and is not addressed here.Closes #10049
What changed
Behavioral changes:
Profile<Kind>andTemplate<Kind>generated from it at the same time, so a worker refreshing the branch rebuilds a consistent schema instead of failing on every refresh.Template<Kind>in the generics map permanently — and in the branch hash.Implementation notes:
The production change is 20 lines in
backend/infrahub/core/schema/schema_branch.py, in two layers:SchemaBranch.delete()cascades to the generated kinds (_delete_generated_kinds). This enforces the invariant at the source: a kind leaving the schema map takes its generated kinds with it, so the orphaned state never exists. The template lookup checksself.templatesand falls back toself.generics, becausegenerate_object_template_from_nodereturns aGenericSchemafor a generic andset()routes it into the generics map._generate_weight_templatesskips a template whose backing node is missing.generate_weight()runs beforemanage_object_template_schemas()inprocess_pre_validation, so this keepsprocess()total for a registry that already holds an orphan. It mirrors the guard already present in_generate_generics_templates_weight.An earlier revision of this branch shipped layer 2 alone, after layer 1 was reverted for breaking
test_exhaustive_handles_same_uuid_vertices_after_kind_migrationin CI. That failure was a test-harness artifact, not a production signal, so layer 1 is back.Keeping both layers is deliberate. Layer 1 prevents the orphan; layer 2 means a reader that meets one anyway does not crash. Making each reader tolerant instead does not scale —
_generate_generics_templates_weightalready needed the same guard.What stayed the same: no schema-format or API changes,
process_pre_validation()ordering untouched, profiles and templates remain excluded from the branch hash.Suggested review order
backend/infrahub/core/schema/schema_branch.py— the whole production change.backend/tests/unit/core/schema/test_schema_branch.py— the cheapest expression of both invariants.backend/tests/component/core/schema/schema_branch/test_process_idempotency.py— the save/reload behaviour.backend/tests/component/graph_traversal/test_path_traversal_query.py— two lines, per above.How to review
The tests are the part worth scrutinising. Each layer of the fix has exactly one dedicated guard, verified by removing each layer and re-running:
TestDeleteRemovesGeneratedKindscascade tests (2 failed, 9 passed); the round-trip tests still pass, sinceprocess()prunes the orphanTestProcessWithOrphanedTemplate(1 failed, 10 passed)TestSchemaBranchDbRoundtrip::test_removing_node_drops_its_generated_kinds—SchemaNotFoundError: Unable to find the schema 'TestGadget'atmanager.py:1123TestSchemaBranchDbRoundtripsaves the base schema to the database once for the class; each test then applies only its own change, on its own branch, writing a single kind. Two disjoint standalone nodes (TestGadgetremoved,TestDoodadflipped) keep the tests independent of execution order.A functional test added earlier on this branch (
backend/tests/functional/schemas/test_load_remove_template_node.py) was removed. Functional tests use a single shared registry (and SchemaManager), so this would not be capable for reproducing the bug. Several component tests that compareSchemaBranchs before and after updates have been added instead.Unable to reproduce the
generate_templateflip causing a schema update crashThe issue states that flipping
generate_template: true → falsewithout deleting the node triggers the same failure, and concludes the trigger is "removal of the generated template, not node deletion per se". I was unable to reproduce this either in tests or against a local Infrahub app. Three independent checks:_generate_weight_templatesresolvestemplate.name, which is the node kind. A flag flip leaves the node in place, so the lookup succeeds andmanage_object_template_schemasdrops the template normally.state: absentfailed on that same instance. An earlier flip run on a contaminated instance did time out, but zero error lines named the flip branch and all 24 named the earlierstate: absentbranch — the sync check is global, so one wedged branch keeps it red. The issue's own steps run the removal first, which is the likely origin of the observation.infrahub-v1.10.1,_generate_weight_templates,delete(), and the removal handling inmanager.pyare materially identical tostable, so the flip could not have failed that way there either.test_disabling_generate_template_drops_templatecovers the flip anyway, since the issue lists it under Expected Behavior — but it passes with both layers removed, so it is a convergence guard, not a regression test for this bug.Testing
Verified end to end on a local deployment with 1 API worker and 2 task workers, same schemas and steps against both versions:
stable@c918af76egenerate_templatenode (state: absent) on a branchSchemaNotFoundError: Unable to find the schema 'DemoDoohickey'generate_template: true → falseon a branchSchemaNotFoundErrorin task-worker logsImpact & rollout
try/exceptper template during weight generation. The component test consolidation cut that file's database work from three full save + reset + core-model-registration cycles to one (70.8s for the file).Checklist
changelog/10049.fixed.md)🤖 Generated with Claude Code