fix: make the refs-check broadcast worker-independent [IFC-3246]

Merged
OG
1mo ago
+994-220

Why

The read-only refs check decided whether to tell the worker pool about a moved ref by comparing the remote head against the checking worker's own origin/<ref>. The claim and due keys that choose that worker are shared, and copies diverge in normal operation because pull_read_only fetches on a single worker. A worker that was already current found nothing to announce, set the due key, and left the rest of the pool behind for the whole interval. SC-009 rests on the FR-017 convergence broadcast, and this is exactly the case in which it did not fire. Raised by @gmazoyer in review on #10669.

Tracing it surfaced a second gap on the same path, already present on stable: workers fetched without force, so a tag that had moved upstream was rejected ("would clobber existing tag") by receiving workers and by an import, and silently resolved to its old commit when pull_read_only looked it up.

Goal: the decision to broadcast no longer depends on which worker runs the check, and every worker can follow a moved tag. This must land on the epic branch before the epic merges to develop, since the commit view (T092) and the branch-drift rows read remote_head from whichever worker answers.

Non-goals: no change to the T092 read path, which still resolves the head without fetching; no multi-worker fixture, since SC-009 stays verified through the broadcast.

Closes IFC-3246. Also closes INBOX-336 (a broadcast lost after a successful fetch).

What changed

Behavioural changes:

  • A check now announces a move that the last check did not list, even when the checking worker's own copy is already current.
  • A worker behind a move that was already listed fetches for itself without broadcasting it again.
  • A broadcast that fails is retried by the next check instead of being lost.
  • A ref with no recorded listing, on the first check after deployment or after the cache loses its data, is broadcast once and not reported as moved.
  • A read-only repository's fetch is stopped after 900 seconds, together with every process git started for it, so a stalled remote cannot hold the repository lock indefinitely on a worker receiving a broadcast.
  • The refs check's log records (movements, failures, skipped claims) now appear in the flow run's logs.
  • Read-only repositories follow a tag that moved upstream on every worker: the receiving workers, the import, and the commit lookup pull_read_only relies on.

Implementation notes:

  • The check asks two separate questions. Whether this worker fetches is decided against its own copy, as before. Whether the pool is told is decided per tracked ref against git:refs_check:listed:<id>:<ref>, the remote head the last check listed.

  • Only the check writes that key, and it writes it while holding the per-repository claim, so no two writers race. It writes every listed head once every broadcast of that check has gone out, unchanged heads included. A failed broadcast therefore leaves the earlier value for the next check to retry, and the value lapses only for a ref no check has listed for 30 days, such as one of a deleted repository.

  • There is no fallback to the imported commit. The read-only import flows do not write the key.

  • The write is best effort: a failed write costs one repeated broadcast rather than the result of the check.

  • The read of the last listing is one get_values call, inside the same timeout as the remote listing.

  • "Import latest commit" sends no broadcast of its own. Its commit update goes through the CoreReadOnlyRepositoryUpdate mutation, which submits pull_read_only pinned to the new commit, and that pull broadcasts it. A new component test pins this chain and checks that the import flow itself sends nothing.

  • InfrahubRepositoryBase.fetch_from_origin is the one fetch used by fetch(), the read-only commit lookup and the checker's gateway. For a read-only repository it forces and runs with the low-speed transport settings. Forcing is safe because each imported commit keeps a worktree of its own. A read-write repository's fetch is unchanged and still has no time limit, although it holds the same lock.

  • The read-only fetch runs through run_git_with_deadline (backend/infrahub/git/bounded_command.py) instead of GitPython's Remote.fetch(kill_after_timeout=...), which never stops a fetch whose remote sends nothing (GitPython up to 3.2.0, the latest release): on the deadline it closes the stderr pipe before it signals git, and that close blocks on the thread reading the pipe. run_git_with_deadline starts git in its own process group:

    • at READ_ONLY_FETCH_TIMEOUT_SECONDS (900) the whole group gets SIGTERM, so git removes its lock and temporary files;
    • if the group is still running READ_ONLY_FETCH_STOP_GRACE_SECONDS (10) later, it gets SIGKILL;
    • the group includes the helpers git starts, such as git-remote-https and ssh;
    • git runs with the C locale, as GitPython runs it, because git errors are classified by their English wording.

    A plain SIGKILL was not used because it can leave a .lock file that blocks every later fetch of that repository until someone removes it by hand.

  • The refs check logs plain text through infrahub.log.get_run_logger(), the infrahub.tasks logger the worker forwards to Prefect, instead of the structlog application logger, which the worker does not forward by default. Values that used to be structlog fields are part of the message text.

What stayed the same: no schema, GraphQL or configuration changes; nothing writes the tracked commit or triggers an import from the check; the repository lock still never covers the remote listing; read-write repositories fetch exactly as before.

How to review

  • backend/infrahub/git/refs_check/checker.py carries the core change: _plan_convergence, _read_listed, _record_listed and _converge.
  • backend/infrahub/git/base.py holds the shared read-only fetch, and backend/infrahub/git/bounded_command.py runs it with the deadline; the constants are in backend/infrahub/git/constants.py.
  • Log output changes shape: refs-check records on stdout JSON no longer carry repository, branch, ref and the heads as separate keys, only in the message. A log pipeline filtering on those keys would stop matching.
  • Spec updates: T065h in tasks.md records the change, with matching notes in plan.md, research.md, data-model.md and the FR-017 verification text in spec.md.

How to test

uv run invoke backend.test-unit
INFRAHUB_USE_TEST_CONTAINERS=true uv run pytest -n 0 backend/tests/component/git backend/tests/component/message_bus/operations/git/test_refresh_git_fetch.py backend/tests/component/graphql/mutations/test_repository.py
INFRAHUB_USE_TEST_CONTAINERS=true uv run pytest -n 0 backend/tests/integration/git/test_readonly_refs_check.py

Locally: the backend unit suite passed (the 9 tests that need local services passed when rerun with testcontainers); the affected component tests passed; the Gogs refs-check integration suite passed. Each of these guards was run against code without its fix and fails there:

  • the failed-broadcast test and the write-order test, with the listing written before the broadcast
  • the import cascade test, with the pull_read_only submission removed from the mutation
  • the recipient test ("would clobber existing tag") and the get_commit_value test (resolves the tag's old commit), without the forced fetch
  • the http:// stalled-remote test and the ignored-SIGTERM test in test_bounded_command.py, with only the git process signalled instead of its whole group: the git-remote-http helper and the sleep survived

Before the push the branch went through four rounds of a local cubic review with the backend and testing checklists from #10715; the fourth came back clean.

Impact & rollout

  • Backward compatibility: no breaking API changes. The new cache key starts empty.
  • One-time broadcast after deployment: the first check of each read-only repository after this ships finds no recorded listing, so it sends one RefreshGitFetch for every Infrahub branch tracking each of its refs, even though nothing moved. The same happens after the cache loses its data, for example a restart without persistence. Every receiving worker then takes each repository's lock, fetches and resets to the commit it already holds, in the same cycle. REFS_CHECK_CONCURRENCY limits only the worker running the checks, not the receiving workers. Later checks broadcast only when a ref moves.
  • Performance: one cache read per repository per check, and no graph reads before the lock.
  • Deployment notes: safe to deploy; targets the epic integration branch.

Checklist

  • Tests added/updated
  • Changelog entry added (uv run towncrier create ...)
  • External docs updated (if user-facing or ops-facing change)
  • Internal .md docs updated (internal knowledge and AI code tools knowledge)
  • I have reviewed AI generated content

Assisted-by: creating-changelog-entries unversioned
Assisted-by: opsmill-dev-commit 0.1.0
Assisted-by: opsmill-dev-pr 0.2.0