Retry install's conditional SSA apply when its own controllers win the race - #15
Merged
Merged
Conversation
…e race
applyPreparedObject Creates an object, then ssaApply server-side-applies
it conditional on the Create-time (or Prepare-time) resourceVersion so
that disappearance or replacement fails closed. But the object's own
controller watches these kinds and routinely writes to the fresh object
— a status condition, a finalizer — in the window between that
observation and the apply. Losing that race surfaced as:
install: apply MCPServer/widget-mcp: ssa-apply MCPServer/widget-mcp:
Operation cannot be fulfilled on ... the object has been modified
and aborted the whole install. TestOapExportPackInstall_WithSpiceDB hit
this three times across recent CI runs, each time on whichever resource
the operator reconciled first (MCPServer once, AgentIdentity twice).
A bare resourceVersion conflict is not by itself foul play, so treat it
as the recheck it is: re-read the object, and if the UID still matches
the one this run approved, retry the apply on the current
resourceVersion (retry.RetryOnConflict, bounded). A different UID — a
same-name replacement mid-install — or a failed re-read still fails
closed, which is the property the conditional apply exists for.
Three fake-client tests pin the behavior: a single lost race retries to
success, a replaced UID aborts without a second apply attempt, and a
conflict that never resolves surfaces after a bounded number of tries.
mage test:unit, test:integration, and test:e2e all pass locally.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully deployed
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.
The flake
TestOapExportPackInstall_WithSpiceDBfailed "first Install" three times across recent CI runs:— each time on whichever resource the operator reconciled first (
MCPServeronce,AgentIdentitytwice). The varying resource is the tell: a race, not a deterministic break.Root cause
applyPreparedObjectCreates an object, thenssaApplyapplies it conditional on the Create-time (or Prepare-time) resourceVersion — deliberately, so disappearance or replacement fails closed. But the object's own controller watches these kinds and routinely writes to the fresh object (a status condition, a finalizer) in the window between that observation and the apply. When the controller's write lands first, the conditional apply conflicts and the whole install aborts, with no retry.The fix
Treat a resourceVersion conflict as the recheck it is rather than as proof of foul play: re-read the object, and if the UID still matches the one this run approved, retry the apply on the current resourceVersion (
retry.RetryOnConflict, bounded). A different UID — a same-name replacement mid-install — or a failed re-read still fails closed, which is the property the conditional apply exists for. Thewizardrunapplier shares the error string but stamps no resourceVersion, so it cannot hit this class and is untouched.Verification
apply_conflict_retry_test.go): a single lost race retries to success, a replaced UID aborts with no second apply attempt, a never-resolving conflict surfaces after a bounded number of tries. All three watched failing before the fix — the first reproduces the CI error byte-for-byte.mage test:unit,mage test:integration,mage test:e2e: all exit 0 locally.go test -tags=e2e -count=2 -run '^TestOapExportPackInstall_WithSpiceDB$' ./test/e2e/scenarios/install/: pass.