fix: prevent git task workers from diverging during sync fan-out (#9349) by polmichel · Pull Request #9360 · opsmill/infrahub

polmichel · 2026-05-26T22:53:49Z

Why

In a multi-worker task pool, the git sync flow fans work out across workers. When a new upstream commit landed between the orchestrator resolving HEAD and the other workers running their own git pull, workers ended up on different commits for the same repo.

Goal: make every fan-out worker converge on the exact commit the orchestrator pinned, regardless of what lands upstream during fan-out.

This PR also carries a second, independent git fix (see below).

Closes #9349

What changed

Behavioral

The sync orchestrator now records the commit SHA it just synced and broadcasts it in RefreshGitFetch. Receiving workers check out that exact SHA instead of fast-forwarding to whatever upstream HEAD currently is.
Separate fix: pulling a repository that had to create a missing local branch crashed when recording its commit value (an unbound infrahub_branch). The branch is now bound before the commit is written.

How to review

A commit by commit approach would be relevant since there is:

One commit for the initial bug fix 93e86eb and one for the related test 7e2f139
One commit for the collateral bug and related test 308110c
One commit for the refactor 419ef91

Following commits are unitary fix or similar issue using the new pinned commit, or documentation/lint/format matter.
Extra scrutiny welcome on the pin source per flow.

How to test

# Divergence reproduction (fails before the fix, passes after)
uv run pytest backend/tests/component/message_bus/operations/git/test_refresh_git_fetch.py -x -v

# Missing-branch pull regression
uv run pytest backend/tests/component/git/test_git_repository.py -x -v

Known follow-ups (intentionally out of scope)

That read-only flow broadcasts repository_kind=REPOSITORY for a read-only repo. Worth confirming independently.

Checklist

Tests added/updated
Changelog entries added (9349.fixed.md, +pull-new-branch-unbound-commit.fixed.md)
External docs updated (no user-facing/ops-facing surface change)
Internal .md docs updated (no knowledge-doc change needed)
I have reviewed AI generated content

Summary by cubic

Prevents git task workers from diverging during sync fan-out by pinning the orchestrator commit and hard-resetting to that SHA under the repo lock. Also fixes a crash when pulling a new branch, records commit values, broadcasts the correct kind for read-only adds, and improves errors when a pinned commit is unreachable; addresses IFC-2642.

Bug Fixes
- Added optional commit to RefreshGitFetch; workers fetch then hard-reset via reset_to_commit under the repo lock, falling back to pull when the pin is missing or can’t be resolved.
- Applied pinning across periodic sync, repository add (read/write and read-only), branch create, merge, and read-only pull; ref-only read-only flows resolve a concrete SHA once and broadcast it.
- Read-only add now broadcasts repository_kind=READONLYREPOSITORY so workers materialize the correct repo type.
- Fixed missing-branch pull crash by binding the branch before writing its commit; commit value updates when a new local branch is created.
- Clearer error when a pinned commit isn’t reachable after fetch; raises a descriptive RepositoryError and leaves the worktree unchanged; narrowed pin-resolution exceptions; added fan-out convergence and sad-path tests; updated RPC mocks and message-bus docs for the new commit field.
Refactors
- Removed unused _update_commit_value_if_requested helper.

^{Written for commit 472ad02. Summary will update on new commits.}

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

get_commit_value only raises ValueError (branch absent) or InvalidGitRepositoryError (clone missing/corrupt). Catching the GitError base class also swallowed GitCommandError, HookExecutionError, and other unexpected git failures, masking them behind a silent fall-back to pull(). Narrow both pin-resolution sites to the exceptions actually expected. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

After merging into the destination branch, resolve the resulting commit and broadcast it in RefreshGitFetch so fan-out workers check out the merge commit instead of pulling the destination branch to whatever upstream HEAD is at that moment, keeping the pool converged if upstream advances during fan-out. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Resolve the new branch's commit and broadcast it in RefreshGitFetch so fan-out workers check out that exact SHA rather than pulling the branch to whatever upstream HEAD is at fetch time. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Sign in to view

            await repo.sync_from_remote()

            # Notify other workers they need to clone the repository
            notification = messages.RefreshGitFetch(


cubic-dev-ai

1 issue found across 8 files

Confidence score: 3/5

There is a concrete regression risk in backend/infrahub/git/tasks.py: pull_read_only broadcasts commit=model.commit rather than the commit actually synced, which can mislead downstream workers.
The most severe impact is user-facing sync divergence: when model.commit is None, fan-out workers may fall back to pull() and reintroduce drift instead of converging on the intended revision.
Given the issue’s relatively high severity/confidence (7/10, 8/10), this is a meaningful but likely targeted fix rather than a broad systemic failure, so merge risk is moderate.
Pay close attention to backend/infrahub/git/tasks.py - ensure the broadcasted commit matches the synced commit to prevent fallback pulls and divergence.

_{Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic}

codspeed-hq · 2026-05-26T23:00:22Z

Merging this PR will not alter performance

✅ 12 untouched benchmarks

_{Comparing pmi-IFC-2642-task-workers-diverge (472ad02) with stable (bf77496)}

Add the generated row for the new commit field and tighten its description to a single line so the message-bus events reference matches generation. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

A ref-only pull (commit unset) resolved a concrete SHA during sync but broadcast the unset commit, leaving fan-out workers to re-resolve the ref independently and diverge. Resolve the ref once and use that SHA for both the sync and the broadcast. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Resolve the ref to a concrete SHA once and use it for both the initial sync and the RefreshGitFetch broadcast, so fan-out workers cloning the new read-only repository converge on that commit instead of re-resolving the ref. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

polmichel · 2026-05-26T23:13:00Z

@@ -145,6 +157,7 @@ async def add_git_repository_read_only(model: GitRepositoryAddReadOnly) -> None:
                repository_kind=InfrahubKind.REPOSITORY,


Is this an issue that this is tagged as REPOSITORY and not a read-only repository type?

it certainly looks wrong

i'll push a commit changing that, but will also ask a review from @ogenstad or @gmazoyer tomorrow before merging

cubic-dev-ai

1 issue found across 3 files (changes from recent commits).

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="docs/docs/reference/message-bus-events.mdx">

<violation number="1" location="docs/docs/reference/message-bus-events.mdx:82">
P2: The Type column for the `commit` field is set to "N/A", but a commit SHA is a string value. This is inconsistent with the other fields in the same table (all typed as "string").</violation>
</file>

_{Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic}

cubic-dev-ai · 2026-05-26T23:14:05Z

 | **repository_kind** | The type of repository | string | None |
 | **infrahub_branch_name** | Infrahub branch on which to sync the remote repository | string | None |
 | **infrahub_branch_id** | Id of the Infrahub branch on which to sync the remote repository | string | None |
+| **commit** | Commit SHA to check out, pinned by the sync orchestrator instead of pulling the latest upstream HEAD | N/A | None |


P2: The Type column for the commit field is set to "N/A", but a commit SHA is a string value. This is inconsistent with the other fields in the same table (all typed as "string").

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At docs/docs/reference/message-bus-events.mdx, line 82: <comment>The Type column for the `commit` field is set to "N/A", but a commit SHA is a string value. This is inconsistent with the other fields in the same table (all typed as "string").</comment> <file context> @@ -79,6 +79,7 @@ For more detailed explanations on how these events are used within Infrahub, see | **repository_kind** | The type of repository | string | None | | **infrahub_branch_name** | Infrahub branch on which to sync the remote repository | string | None | | **infrahub_branch_id** | Id of the Infrahub branch on which to sync the remote repository | string | None | +| **commit** | Commit SHA to check out, pinned by the sync orchestrator instead of pulling the latest upstream HEAD | N/A | None |   </file context>

Suggested change

| **commit** | Commit SHA to check out, pinned by the sync orchestrator instead of pulling the latest upstream HEAD | N/A | None |

| **commit** | Commit SHA to check out, pinned by the sync orchestrator instead of pulling the latest upstream HEAD | string | None |

This documentation is auto-generated. This is surely due to the fact that the field as a default value at None, but I am not totally sure.

Thanks for the context—understood that this table is auto-generated.

The add and read-only-add flows now resolve a commit and broadcast it, so the mocked repos must return a SHA from get_commit_value (and expose ref for the read-only flow), and the read-only sync assertion expects the pinned commit. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic across multiple workers, introducing commit pinning and a new reset_to_commit method to prevent divergence; such changes affect a critical path for repository data integrity and require human review due to potential impact on fan-out behavior...

Re-trigger cubic}

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic to prevent worker divergence, introducing a new reset_to_commit method, altering pull and message broadcast behavior, and changing concurrency handling under locks; while well-tested, the blast radius and risk to data integrity across all...

Re-trigger cubic}

ajtmccarty · 2026-05-27T15:24:16Z

+        await self.create_branch_in_git(branch_name=branch_name, branch_id=branch_id)
+        return self.get_git_repo_worktree(identifier=branch_name)
+
+    async def _update_commit_value_if_requested(self, branch_name: str, commit: str, update_commit_value: bool) -> None:


this one feels like it should be a conditional at the caller-level.
if update_commit_value: await self._update_commit_value(...)
maybe it needs a different name to distinguish it from self.update_commit_value

This method only exists to encapsulate the following shared code block:

if update_commit_value: infrahub_branch = self._get_mapped_target_branch(branch_name=branch_name) await self.update_commit_value(branch_name=infrahub_branch, commit=commit)

which appears in both infrahub.git.base.InfrahubRepositoryBase.pull and infrahub.git.base.InfrahubRepositoryBase.reset_to_commit. Having said that, this is true that encapsulating this condition might actually hurt readability, and wrapping just two lines isn't really worth it. I'll remove it.

I think this is dead now

:shame:
Thank you.

ajtmccarty · 2026-05-27T15:27:27Z

+        branch_id: str | None = None,
+        create_if_missing: bool = False,
+        update_commit_value: bool = True,
+    ) -> str:


does this really need to return anything if it only ever returns the commit passed in?

You are right, good catch. I'll type the return as None. The return value is not even used.

ajtmccarty · 2026-05-27T15:27:49Z

+
+        """
+        if not self.has_origin:
+            return commit


is this check necessary if this method doesn't contact the remote?

Yes, you're right. At some point the method was called pull_to_commit or something like that, and it called a fetch method that took a specific commit. But Claude warned me that some git servers don't allow fetching a specific commit directly, since there's no guarantee that commit exists in the remote's history. So I renamed it to reset_to_commit and... forgot this one. I'll remove it.

ajtmccarty · 2026-05-27T15:38:20Z

+    if message.commit:
+        # reset_to_commit hard-resets the worktree, which would silently discard a concurrent
+        # local change. Serialize it against the merges and syncs holding the same lock.
+        async with lock.registry.get(name=message.repository_name, namespace="repository"):


is this counting on the lock to ensure there are no local changes that will be discarded by the reset? it seems like it would still be possible to clobber local changes even after the lock is released, but I might not entirely understand this workflow

No, this lock is to make sure that there is no parallel operation that impacts the repository while we are resetting the local worktree on each worker.

This lock cannot protect against local changes, and I have not found a way to prevent local changes to be overwritten by this reset.

The comment is confusing, I'll change it.

Thinking about it more, this lock may be too broad, I think a worker-internal lock would be better. As it stands, all workers will end up locking the repository from any operation one after another.

But this would require to verify that operations related to worker-internal state are all using this scope of lock I guess.

cubic-dev-ai

0 issues found across 3 files (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic across multiple workers, adding commit pinning and hard-reset behavior to prevent divergence, which carries significant risk to production infrastructure and data integrity if incorrect, and requires human review to verify correctness and safe...

Re-trigger cubic}

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic with concurrency fixes and a new reset method, which carries moderate risk and requires domain-specific review.

Re-trigger cubic}

ajtmccarty

looks good to me. but maybe someone more familiar with our git stuff wants to review it too (@ogenstad or @gmazoyer)

ajtmccarty · 2026-05-28T18:14:07Z

+        await self.create_branch_in_git(branch_name=branch_name, branch_id=branch_id)
+        return self.get_git_repo_worktree(identifier=branch_name)
+
+    async def _update_commit_value_if_requested(self, branch_name: str, commit: str, update_commit_value: bool) -> None:


I think this is dead now

ajtmccarty · 2026-05-28T18:20:32Z

@@ -145,6 +157,7 @@ async def add_git_repository_read_only(model: GitRepositoryAddReadOnly) -> None:
                repository_kind=InfrahubKind.REPOSITORY,


it certainly looks wrong

The helper was introduced but never wired into any call site — the three existing producers (create_locally, pull, reset_to_commit) already inline the `if update_commit_value:` conditional the helper was meant to encapsulate. Remove the dead code. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git sync logic including a new reset_to_commit method and changes the RefreshGitFetch message flow, which could impact repository data integrity and concurrency across workers; such critical-path changes require human review even if no issues were detected by the AI.

Re-trigger cubic}

add_git_repository_read_only operates on InfrahubReadOnlyRepository but was broadcasting repository_kind=InfrahubKind.REPOSITORY, so peer workers consuming the RefreshGitFetch notification re-instantiated the repo as InfrahubRepository and ran the read-write pull path. Send the matching READONLYREPOSITORY kind so consumers materialize the same type the producer just created. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic—adding a new reset_to_commit method, altering the pull method, changing how workers converge during fan-out, and introducing a lock in the fetch flow—which impacts data consistency across the task pool and requires a human reviewer to ensure...

Re-trigger cubic}

gmazoyer

Looks good to me.

You could add a couple of tests:

one for the read-only repository_kind correction (REPOSITORY → READONLYREPOSITORY). A short assertion on the broadcast repository_kind from add_git_repository_read_only would lock that in. Maybe an existing test could be slightly changed to integrate this assertion.
one for the failure case where the pinned SHA isn't reachable after fetch (e.g., broadcasted SHA that was force-pushed away). Worth a sad-path test confirming the worker raises rather than silently fast-forwards.

polmichel · 2026-05-29T12:32:01Z

Looks good to me.

You could add a couple of tests:

one for the read-only repository_kind correction (REPOSITORY → READONLYREPOSITORY). A short assertion on the broadcast repository_kind from add_git_repository_read_only would lock that in. Maybe an existing test could be slightly changed to integrate this assertion.

one for the failure case where the pinned SHA isn't reachable after fetch (e.g., broadcasted SHA that was force-pushed away). Worth a sad-path test confirming the worker raises rather than silently fast-forwards.

Thanks for the feedbacks @gmazoyer. I've added the two test. I also added some logic to generate a clean error message when the reset --hard operation fails. This is tested through the second test related to the sad path.

cubic-dev-ai

0 issues found across 3 files (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic, introduces a new commit-pinning mechanism, and touches concurrency control and message bus definitions, all of which have a high blast radius and could lead to data divergence or infrastructure issues if a bug slips through, so human review is needed.

Re-trigger cubic}

… fix Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. This PR modifies core git synchronization logic, introduces a new commit pinning mechanism, refactors git pull and worktree handling, adds locking, and changes message bus messages—all of which are high-impact changes to critical infrastructure that require human review to ensure data integrity and

Re-trigger cubic}

polmichel and others added 9 commits May 27, 2026 00:21

test: add failing test for #9349

7e2f139

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

fix: prevent git sync task workers from diverging (#9349)

93e86eb

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

fix: bind infrahub_branch when pull creates a missing branch

308110c

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

refactor: share worktree resolution between pull and pull_to_commit

419ef91

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

fix: pin commit SHA when pulling a read-only git repository

74e4292

fix: pin commit SHA when adding a git repository

9b0f419

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label May 26, 2026

polmichel commented May 26, 2026

View reviewed changes

Comment thread backend/infrahub/git/tasks.py

await repo.sync_from_remote()

# Notify other workers they need to clone the repository

notification = messages.RefreshGitFetch(

This comment was marked as outdated.

Sign in to view

cubic-dev-ai Bot reviewed May 26, 2026

View reviewed changes

Comment thread backend/infrahub/git/tasks.py Outdated

polmichel marked this pull request as ready for review May 26, 2026 22:59

polmichel requested a review from a team as a code owner May 26, 2026 22:59

polmichel and others added 3 commits May 27, 2026 01:02

docs: regenerate message-bus events for RefreshGitFetch commit field

780d06a

Add the generated row for the new commit field and tighten its description to a single line so the message-bus events reference matches generation. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

polmichel requested a review from a team as a code owner May 26, 2026 23:10

github-actions Bot added the type/documentation Improvements or additions to documentation label May 26, 2026

polmichel commented May 26, 2026

View reviewed changes

cubic-dev-ai Bot reviewed May 26, 2026

View reviewed changes

polmichel marked this pull request as draft May 27, 2026 07:09

polmichel and others added 2 commits May 27, 2026 09:17

duplicated line

789ff41

cubic-dev-ai Bot reviewed May 27, 2026

View reviewed changes

format

23e08ed

polmichel marked this pull request as ready for review May 27, 2026 07:43

cubic-dev-ai Bot reviewed May 27, 2026

View reviewed changes

ajtmccarty reviewed May 27, 2026

View reviewed changes

addressing feedbacks

1238d40

cubic-dev-ai Bot reviewed May 28, 2026

View reviewed changes

get the lock upper in the chain to also encapsulate the pull

170e1ef

cubic-dev-ai Bot reviewed May 28, 2026

View reviewed changes

polmichel requested a review from ajtmccarty May 28, 2026 15:21

ajtmccarty approved these changes May 28, 2026

View reviewed changes

cubic-dev-ai Bot reviewed May 28, 2026

View reviewed changes

gmazoyer approved these changes May 29, 2026

View reviewed changes

polmichel added 3 commits May 29, 2026 14:30

better error message when the reset --hard fails

5accf5e

testing that the repository generated is a read-only repository

96d906c

testing the sad path of hard reset related to git fetch operations

3dba807

cubic-dev-ai Bot reviewed May 29, 2026

View reviewed changes

docs(changelog): add fragment for read-only repository kind broadcast…

472ad02

… fix Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

cubic-dev-ai Bot reviewed May 29, 2026

View reviewed changes

polmichel merged commit e3cc9a1 into stable May 29, 2026
135 of 137 checks passed

polmichel deleted the pmi-IFC-2642-task-workers-diverge branch May 29, 2026 18:33

infrahub-github-bot-app Bot mentioned this pull request May 29, 2026

Merge stable into develop #9395

Closed

		@@ -145,6 +157,7 @@ async def add_git_repository_read_only(model: GitRepositoryAddReadOnly) -> None:
		repository_kind=InfrahubKind.REPOSITORY,

	\| commit \| Commit SHA to check out, pinned by the sync orchestrator instead of pulling the latest upstream HEAD \| N/A \| None \|
	\| commit \| Commit SHA to check out, pinned by the sync orchestrator instead of pulling the latest upstream HEAD \| string \| None \|

Uh oh!

Conversation

polmichel commented May 26, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

How to review

How to test

Known follow-ups (intentionally out of scope)

Checklist

Summary by cubic

Uh oh!

This comment was marked as outdated.

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codspeed-hq Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot May 26, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ajtmccarty left a comment

Choose a reason for hiding this comment

polmichel commented May 26, 2026 •

edited by cubic-dev-ai Bot

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

codspeed-hq Bot commented May 26, 2026 •

edited

Loading

polmichel May 28, 2026 •

edited

Loading

gmazoyer left a comment •

edited

Loading