fix(backend): warm up a repository without resetting it [IFC-3345]

Merged
OG
1mo ago
+438-141

Why

When a worker reads a repository's commit log or branch drift and has no local copy, it starts a "warm-up" that clones the repository and asks every other worker to clone it too. The warm-up added in #10836 had three problems, raised in this review comment and described in IFC-3345:

  • It reset its new copy to the commit Infrahub has imported. When that commit was no longer on the remote (for example after a force push), the reset failed. The warm-up then stopped before telling other workers to clone, and set the repository's operational status to error. A read-only repository has no periodic sync to repair this, so other workers kept answering NOT_CLONED.
  • It could roll back a sync, because the reset moved the local branch back to the imported commit.
  • It could hang every git operation on the repository. It held the repository lock while it waited for a clone that another task on the same worker had started, and that clone was waiting for the same lock.

Goal: the warm-up clones the repository and asks every other worker to clone it, without moving any local branch, and nothing after the clone can stop that request.

Non-goals:

  • Fixing T107: for a read-only repository, "import last commit" can record a commit it did not import when the local copy is already at the remote head. This change leaves more copies at the remote head, so T107 can happen more often. Its owner is not decided yet (this epic or IFC-2998).
  • Making other flows that read a local branch (the staging-to-active merge, a proposed change's repository tests) converge. IFC-3210 owns that.
  • No new GraphQL field, no graph write and no migration.

Part of the IFC-3101 commit-visibility feature: task T110 in tasks.md. It must merge into this epic branch before the epic merges into develop.

Jira: IFC-3345

What changed

Behavioral changes

  • The warm-up no longer resets any copy. Its own copy, and every copy other workers create from its request, stays at the remote head, the same as every first clone today.
  • The warm-up now sends RefreshGitClone for both repository kinds. It no longer sends a RefreshGitFetch pinned to the imported commit.
  • The warm-up sends RefreshGitClone right after its clone. Only then does it fetch, and a failed fetch is logged as a warning instead of failing the warm-up.
  • The warm-up no longer holds the repository lock while it clones. The clone takes that lock itself.
  • When a worker receives RefreshGitClone and its copy has no fetch time, it now fetches the copy, so the commit log and branch drift show a fetch time on every worker. A failed fetch is logged and does not fail the request.
  • A copy has no fetch time when it has no FETCH_HEAD (a clone writes none) or an empty one. Git writes an empty FETCH_HEAD when a fetch fails, so the commit log and branch drift now report fetchedAt as null after a failed fetch, instead of the time of the failed attempt. The next clone request fetches that copy again. (Second commit, from Guillaume's review comment.)
  • A read-write repository with nothing imported is still not warmed up. Its sync creates the copy.

Implementation notes

  • The warm-up and the RefreshGitClone handler share one helper, fetch_if_no_fetch_time in backend/infrahub/git/state/warm_up.py. It fetches only a copy for which read_fetched_at (the commit-log reader's own check, in log_reader.py) returns no time, checks again once it holds the lock, and catches only GitCommandError.
  • GitRepositoryWarmUp loses its location field. Only the pinned fetch used it. The warm-up has not been released, so nothing outside this repository depends on it.
  • A read-only warm-up no longer reads the imported commit through the API, because it does not use it.
  • The branch-drift query still starts the warm-up for the first branch with something imported. Only the code comment changed: the reason is now the read-write check, not the commit to pin.

What stayed the same

  • The shape of the commit-log and branch-drift responses. Only the description of fetchedAt changed, to say it is null after a failed fetch (schema/schema.graphql and the frontend's generated types are regenerated).
  • The RefreshGitFetch handler, which this branch does not touch.

How to review

  1. backend/infrahub/git/state/warm_up.py: the new order (clone, broadcast, fetch) and the fetch helper.
  2. backend/infrahub/message_bus/operations/git/repository.py: the RefreshGitClone handler now calls the helper.
  3. backend/infrahub/git/state/log_reader.py: read_fetched_at, which returns no time for an empty FETCH_HEAD.
  4. backend/tests/integration/git/test_repository_warm_up.py: the tests listed below.
  5. dev/specs/ifc-3101-repo-commit-visibility/: T110 is new, T042 is marked as partly replaced, and contracts/message_bus.md, data-model.md, research.md, plan.md and quickstart.md describe the new warm-up.

Where extra scrutiny helps

  • The deadlock test (test_warm_up_shares_a_clone_another_handler_started_on_this_worker) uses a test client that pauses before its first GraphQL call, to start a clone from another task at the right moment. With the old code it times out after 60 seconds. It never asserts on elapsed time.
  • After a failed fetch, fetchedAt is null even when an earlier fetch succeeded. Git empties FETCH_HEAD before it contacts the remote, so the earlier time cannot be recovered from it. Keeping the last successful fetch time would need Infrahub to record it itself after each fetch; that is not part of this PR.

Declined review findings (from a local cubic review run with the checklists in #10715)

  • "Move the try/finally in the deadlock test into a yield fixture": the finally waits for a task that is part of the scenario under test, bounded by asyncio.wait_for. It restores no global state, which is the case the testing checklist asks a fixture for.
  • "Check for a missing FETCH_HEAD instead of reinitialized" was first declined, then adopted after a later review showed that reinitialized misses a copy that another call on the same worker cloned.
  • "NEVER_CLONED_LOCATION is a leftover": the component tests still use it to build the request.
  • "A successful fetch from a remote with no branches also leaves FETCH_HEAD empty": a copy of a repository with no commits fails the local-copy check, so the commit log reports it as not cloned and never reads its fetch time.

How to test

INFRAHUB_USE_TEST_CONTAINERS=true uv run pytest -n 0 \
  backend/tests/integration/git/test_repository_warm_up.py \
  backend/tests/component/message_bus/operations/git \
  backend/tests/unit/git

Locally: 629 passed. These tests fail without the change:

  • test_warm_up_clones_at_the_remote_head_and_broadcasts_a_clone: the old warm-up sent a pinned RefreshGitFetch.
  • test_warm_up_of_a_read_only_repository_whose_imported_commit_left_the_remote: the old warm-up raised RepositoryError.
  • test_warm_up_shares_a_clone_another_handler_started_on_this_worker: the old warm-up deadlocked and the test timed out.
  • test_warm_up_broadcasts_even_when_the_fetch_fails and test_the_clone_request_fetches_an_existing_copy_never_fetched: fail against the code before the fetch helper.
  • test_the_clone_request_succeeds_when_the_fetch_fails: fails when the except is removed.
  • test_the_clone_request_fetches_a_copy_whose_last_fetch_failed and test_a_clone_whose_last_fetch_failed_reports_no_fetch_time (in test_commit_log.py): fail against the first commit, which treated an empty FETCH_HEAD as a fetch.

Impact & rollout

  • Backward compatibility: GitRepositoryWarmUp drops location. The warm-up flow has not been released.
  • Performance: each warm-up and each worker that receives its request runs at most one fetch, and only for a copy that has no fetch time. A read-only warm-up makes one API call fewer.
  • Config/env changes: none.
  • Deployment notes: none. This merges into the epic branch, not into develop.

Checklist

  • Tests added/updated
  • Changelog entry added (uv run towncrier create ...): not needed, because the warm-up has not been released
  • External docs updated (if user-facing or ops-facing change): not needed
  • Internal .md docs updated (internal knowledge and AI code tools knowledge)
  • I have reviewed AI generated content