fix(backend): branch registry updates - #10396
Conversation
github-actions
Bot
added
the
group/backend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 4 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 branch registry cache sync after BranchUpdate and migrate_branch, and makes refresh_branches log-and-continue; tests pin each corrected behavior, so no human-owned tradeoff remains.
Re-trigger cubic
| # The branch it gave up on has to be reported, with its traceback: absorbed is not silent | ||
| failures = [ | ||
| record.msg | ||
| for record in caplog.records | ||
| if isinstance(record.msg, dict) | ||
| and record.msg.get("event") == f"Failed to refresh branch '{broken_branch.name}' in the registry" | ||
| ] |
There was a problem hiding this comment.
Could this reuse find_logged_event from component/core/merge/conftest.py? It does the same record.msg dict scan. Maybe worth moving it somewhere shared.
There was a problem hiding this comment.
yes good idea. this is done
| if active_branch.name in registry.branch: | ||
| await update_branch_registry(db=db, branch=active_branch) | ||
| else: | ||
| await create_branch_registry(db=db, branch=active_branch) |
There was a problem hiding this comment.
wow the fact that these two functions have the same number of characters and that only the first words differ confused me. I had to read it 5 times to finally see they are 2 different functions 🫨
no action to take though, it's just come from my broken eyes
| await obj.save(db=db, user_id=graphql_context.active_account_session.account_id) | ||
|
|
||
| # update registry after txn commit, so it cannot diverge from db on failure | ||
| registry.branch[obj.name] = obj |
There was a problem hiding this comment.
Is this safe the branch is not already in this worker's registry? Branch.get_by_name never loads the schema, and registry.get_branch() and create_branch_registry() both do when they insert.
If the entry lands schema-less, does the next refresh_branches ever fix it?
Is this where the BranchSaver you mention in the description would come in handy?
There was a problem hiding this comment.
very good point. updated all the registry update sites to only update an existing branch in the registry and not add a new one. I think if everything is working correctly, then this would not be an issue that we could encounter b/c the branches and their schemas will always be up-to-date, but it is better to be safe
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Confidence score: 5/5
- In
backend/tests/component/graphql/mutations/test_branch.py, the docstring and inline comments reference concrete functions and “the sweep,” which violates the repository’s code-doc-style rule and may become stale; rephrase them without direct code references.
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/graphql/mutations/test_branch.py">
<violation number="1" location="backend/tests/component/graphql/mutations/test_branch.py:607">
P3: This docstring and the inline comments name concrete functions (`refresh_branches`, `Branch.get_by_name`, `create_branch`) and 'the sweep', which the repo's code-doc-style rule forbids: references to other code rot silently. Describe the behavior without naming the call sites.</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
| async def test_branch_update_leaves_an_unknown_branch_out_of_the_registry( | ||
| db: InfrahubDatabase, base_dataset_02: dict, session_admin: AccountSession, local_services: InfrahubServices | ||
| ) -> None: | ||
| """A branch this worker has never seen must stay out of the cache, so refresh_branches still creates it. |
There was a problem hiding this comment.
P3: This docstring and the inline comments name concrete functions (refresh_branches, Branch.get_by_name, create_branch) and 'the sweep', which the repo's code-doc-style rule forbids: references to other code rot silently. Describe the behavior without naming the call sites.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/component/graphql/mutations/test_branch.py, line 607:
<comment>This docstring and the inline comments name concrete functions (`refresh_branches`, `Branch.get_by_name`, `create_branch`) and 'the sweep', which the repo's code-doc-style rule forbids: references to other code rot silently. Describe the behavior without naming the call sites.</comment>
<file context>
@@ -601,6 +601,51 @@ async def test_branch_update_description(
+async def test_branch_update_leaves_an_unknown_branch_out_of_the_registry(
+ db: InfrahubDatabase, base_dataset_02: dict, session_admin: AccountSession, local_services: InfrahubServices
+) -> None:
+ """A branch this worker has never seen must stay out of the cache, so refresh_branches still creates it.
+
+ `Branch.get_by_name` does not load the schema, so caching it here would leave an entry that the sweep
</file context>
| log.info(f"No migrations detected for branch '{obj.name}'") | ||
| obj.graph_version = GRAPH_VERSION | ||
| await obj.save(db=db) | ||
| registry.refresh_cached_branch(obj) |
There was a problem hiding this comment.
I'm not completely sure what problem it is we are solving here. I'm not saying anything is wrong, however the way I read this is that we'd be solving a problem for the current worker but we don't have anything to indicate to other workers that they need to take action. I.e. to trigger the other workers to refresh their view. It might be that this happens elsewhere. Just asking as a sanity check if we need anything else to happen here as well (and in the other places where we refresh this. Another consideration would be if we need to update the hashes within the components to indicate that a branch gets out of sync until things converge. I'm not sure that the graph_version or status are currently included in that hash. I think it might only be the actual schema that gets included.
So this is mainly a sanity check.
3 participants
two or three small fixes
migrate_branchtask when branch state is saved to the databaserefresh_branchestask, log the exception and continue, so that other branches can still be refreshedthe first two show the need for a
BranchSavercomponent that would handle making the change to the database and then updating the registry so that they cannot get out of sync