audit of core deferral code - #39311
Merged
Merged
Conversation
Action expansion was being recorded as action deferral, instance of action expansion deferral. We don't actually have "action deferrals" right now though, so remove that too while we're at it.
These were never wired into module expansion
jbardin
force-pushed
the
jbardin/deferral-auditing
branch
from
September 30, 2026 12:18
7984b5d to
f9aa50c
Compare
jbardin
commented
Sep 30, 2026
| diags = diags.Append(n.reportPlan(ctx, deferred, planDeferred, importing, change, instanceRefreshState, instancePlanState, repData)) | ||
|
|
||
| } else { | ||
| if n.skipPlanChanges { |
Member
Author
There was a problem hiding this comment.
I hate unnecessary else blocks, especially in long sections where you can't easily see which condition's else you're under, or when the primary path is the else. This makes the refresh-only path the exception, and the normal planing path the main line.
jbardin
marked this pull request as ready for review
September 30, 2026 12:34
Remove dead code in GetDeferredPartialExpandedResource Make "deferrals off" behavior of Deferred structure consistent. The Report* methods still record even when deferrals are off, so the duplicate-report panics still catch bugs outside stacks Create a shared helper of the "insert once, panic on duplicate" logic Remove duplicate code in nodeApplyableDeferredInstance
LoadPlannedDeferrals decides between a full and a partial deferral from the wildcard instance key in the address. It no longer relies on DeferredReasonInstanceCountUnknown. PartialExpandedResources(addrs.ConfigResource) returns the partial expansions for a resource. decodeDeferredResources is the reverse of the plan's deferredResources. A missing schema or a decode failure is now a normal error diagnostic that stops apply before the walk begins. Before, it was an error inside a graph node checkForPartialExpansion asks ctx.Deferrals() for partial expansions instead of having them pushed in by the transformer We can remove the special transformer and extra deferral-related nodes from the graph entirely.
unify where NodeAbstractResourceInstance defers the instance from multiple call sites. Fix where deferred destroys could lose actions. The actions are now deferred. Fix panic from "checking whether X should be deferred when it was already deferred". Now we also check that a resource can't be deferred during apply if it was not planned as such.
There was some confusing logic and repeated calls around resource deferrals and actions, because each can defer the other. planActionTriggers now returns (invocations, deferred, diags). It no longer adds invocations to the plan or removes the resource's change, so all bookkeeping is in the caller. recordActionInvocations adds the invocations to the plan once nothing needs to be undone. finalizePlan saves the working-state object before writing the change and planned object, then plans actions. If an action is deferred, it restores that object as well as removing the change. Before, the planned object was left in the working state. The evaluator always prefers the deferred value, so nothing could see that leftover object. Actions are planned right after the change is written, so on deferral only the change needs undoing. Before, the orphan node had already removed the object from the working state and never put it back.
jbardin
force-pushed
the
jbardin/deferral-auditing
branch
from
October 1, 2026 13:25
f9aa50c to
5a06dda
Compare
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
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.
Stepping through the core deferral handling uncovered some more loose ends and repetition. Cleanup the code some more, and try to make it more understandable.
action expansion deferral. We don't actually have "action deferrals" right now though, so remove that too while we're at it.
ReportModuleExpansionDeferredwhich were never wired into module expansionGetDeferredPartialExpandedResourceDeferredstructure consistent. TheReport*methods still record even when deferrals are off, so the duplicate-report panics still catch bugs outside stacksputUniquefor the "insert once, panic on duplicate" logic in DeferringLoadPlannedDeferralsdecides between a full and a partial deferral from the wildcard instance key in the address. It no longer relies onDeferredReasonInstanceCountUnknown.PartialExpandedResourcesreturns the partial expansions for a resource, so resource nodes can access them during apply.decodeDeferredResourcesis the inverse of the plan'sdeferredResources. A missing schema or a decode failure is now a normal error diagnostic that stops apply before the walk begins, instead of an error inside a graph node.checkForPartialExpansionasksctx.Deferrals()for partial expansions instead of having them pushed in by the transformer. We can remove the special transformer and extra deferral-related nodes from the graph entirely.NodeAbstractResourceInstancedefers the instance from multiple call sites.