fix(reconciler): fail closed when the default branch is unknown - #586
Draft
leoparente wants to merge 5 commits into
Draft
fix(reconciler): fail closed when the default branch is unknown#586leoparente wants to merge 5 commits into
leoparente wants to merge 5 commits into
Conversation
leoparente
requested review from
davidlanouette,
grant-nbl,
jajeffries,
manrodrigues,
mfiedorowicz,
paulstuart and
samiura
as code owners
August 31, 2026 15:57
Vulnerability Scan: Passed — diode-authImage:
Commit: dc55643 |
Vulnerability Scan: Passed — diode-reconcilerImage: No vulnerabilities found. Commit: dc55643 |
|
Go test coverage
Total coverage: 56.7% |
Vulnerability Scan: Passed — diode-ingesterImage: No vulnerabilities found. Commit: dc55643 |
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
At boot the default-branch fetch races OAuth client bootstrap. When it
loses, NetBox answers 403 Invalid token, the refresher logs a warning and
leaves the cache cold, and both processors carry on:
branchID := ""
if branch, err := p.ops.DefaultBranch(ctx); err == nil && branch != nil {
branchID = branch.ID
}
results := p.ops.BulkPlanApply(ctx, batch, branchID)
A cold cache returns (nil, nil), so err is nil and branch is nil, and the
empty branchID is indistinguishable from "no default branch is configured".
Work is planned and applied against main, bypassing the branch's approval
boundary, until the next refresh up to DefaultBranchRefreshInterval (5m)
later. There was no retry: one attempt, then a full interval.
HasBranchLoaded() already existed to tell those two states apart, but it
was only ever read for a log field and was not on the interface the
processors consume, so no caller could act on it.
Put it on IngestionProcessorOps and gate both poll loops on it, before
claiming rather than after, so deferred work stays queued instead of
stranded in a claimed state. Retry the initial fetch with backoff (1s
doubling to 30s) so the boot race resolves in seconds rather than keeping
processors idle for five minutes.
Deliberately gated on HasBranchLoaded() rather than on branch presence:
"no default branch configured" must stay a fast path, so single-tenant
deployments are unaffected.
Tests: both processors are asserted to claim nothing while the cache is
cold, verified to fail when the gate is removed; the refresher is asserted
to recover from an initial 403 well inside the refresh interval. Existing
processor tests stub the new method. Full suite and golangci-lint pass.
Fixes #553
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leoparente
force-pushed
the
fix/reconciler-branch-boot-race
branch
from
August 31, 2026 16:04
14d723a to
97102bb
Compare
leoparente
marked this pull request as draft
August 31, 2026 16:04
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14d723adea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The gate is re-evaluated on every poll iteration by every worker, so the warning fired once per idle interval per worker for as long as the cache stayed cold. The backoff bounds that for a boot race, but misconfigured credentials keep the cache cold indefinitely, and the interval scales with INGESTION_LOG_PROCESSOR_CONCURRENCY / AUTO_APPLY_PROCESSOR_CONCURRENCY. Track the cold state in an atomic.Bool and log on the transition into it, leaving per-attempt diagnostics to the branch refresher. Also log on the way out, so operators get both ends of the window rather than a warning with no matching recovery. Caught in review by Codex on #586. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #553.
The bug
At boot the default-branch fetch races OAuth client bootstrap. When it loses, NetBox answers
403 Invalid token, the refresher logs a warning and leaves the cache cold — and both processors carry on regardless:A cold cache returns
(nil, nil).erris nil,branchis nil, sobranchIDstays""— indistinguishable from "no default branch is configured". Work is planned and applied against main, bypassing the branch's approval boundary.Two things made it worse than a momentary blip:
fetchAndStoreBranchran once at boot, then waited a fullDefaultBranchRefreshInterval(5 minutes) before trying again. So one lost race meant up to five minutes of branchless applies.HasBranchLoaded()already existed to distinguish "known absent" from "not yet known" — but it was only ever read to populate a log field, and wasn't on theIngestionProcessorOpsinterface the processors consume. No caller could act on it. The distinction was built and never wired up.The reporter's workaround is telling: restart the reconciler twice, ~45s apart, and grep the logs for the warning. I hit this same warning in a local stack while working on something else and put it down to a stale token — it's easy to misread as a config problem.
The fix
HasBranchLoaded()onIngestionProcessorOpsand gate both poll loops on it.processBatch. Both loops claim from the DB then process, so deferring after the claim would strand rows in a claimed state. Deferring before it leaves them queued.Gated on
HasBranchLoaded()rather than on branch presence deliberately: no default branch configured has to stay a fast path, so single-tenant deployments are unaffected.Tests
Full suite and
golangci-lintclean. While fixing the stubs I found the missing mock expectation wedged the package at Go's 600s test timeout — testify callsFailNowfrom the processor goroutine — so the suite is back to ~5s from that 600s.Worth a reviewer's eye
Deferring blocks both planning and applying while the cache is cold. Planning against the wrong branch produces the wrong diff, so I don't think plan-but-don't-apply is safe — but if you'd rather change-set generation continued during the window, that's the knob to turn.
🤖 Generated with Claude Code