tfbridge: match scalar set elements by value when extracting inputs - #3570
Open
Zamiell wants to merge 1 commit into
Open
tfbridge: match scalar set elements by value when extracting inputs#3570Zamiell wants to merge 1 commit into
Zamiell wants to merge 1 commit into
Conversation
extractInputs matches TypeSet elements by content rather than position, but only when every element is an object. Sets of scalars fell back to positional matching, and since the string case returns the new state, refresh rewrote the recorded inputs into the order the provider returned them in. A set is unordered, so that order generally differs from the order the values appear in the program. The rewritten inputs are therefore permanently different from what the program produces on the next run. Diff() does not see this, so the drift is silent, but the engine compares raw input bags when deciding whether a checkpoint write can be elided. Every affected resource forces a full state serialization and upload on every subsequent update. Match scalar elements by exact value, and fall back to positional matching when the two sides do not hold the same multiset of values so that genuine membership changes stay visible.
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.
Claude Opus 5 found/fixed this bug while investigating a non-deterministic Pulumi plan.
LLM:
Match scalar set elements by value when extracting inputs
Fixes the third instance of a family of
extractInputsbugs wherepulumi refreshsilentlyrewrites the inputs recorded in state, causing permanent drift that
Diff()cannot see.Problem
extractInputsmerges the inputs recorded in state with the state returned by the provider. Forarrays it matches elements by position, which is correct for
TypeListbut wrong forTypeSet.#3384 and #3410 addressed this by adding
matchSetElements, which matches set elements bycontent -- but only when every element is an object:
A set of scalars therefore falls back to positional matching, and the scalar branch of
extractInputsreturnsnewState:The net effect is that refresh replaces the recorded inputs with the provider's ordering.
A set is unordered, so the order the provider returns generally differs from the order the values
appear in the program. Once refresh has rewritten the inputs, they are permanently different from
what the program produces on every subsequent run.
Why this matters
The drift is invisible to
Diff(), so nothing is displayed andpulumi preview --expect-no-changesand
pulumi refresh --expect-no-changesboth pass.It is not harmless, though. The engine compares raw input bags in
sameSnapshotMutation.mustWritewhen deciding whether a checkpoint write can be elided. A resource whose inputs differ from its
state cannot be elided, so every affected resource forces a full state serialization and upload on
every subsequent update, for as long as the difference persists.
We hit this on a stack with roughly 1400 resources and a 19.5MB state file, where the redundant
writes turned a one-line change into a 16 minute
pulumi up. Fixing #3567 dropped the resourcesthat drift after a refresh from 787 to 34. This bug accounts for 17 of the remaining 34, all of
them a
TypeSetof strings; applying this change as well takes those 17 to 0 and brings the sameupdate down to roughly 90 seconds. The other 17 involve unrelated properties and are not addressed
here.
Fix
Match scalar set elements by exact value.
Matching is only applied when both sides hold the same multiset of values. When set membership
genuinely changed,
matchScalarSetElementsreturnsnilso the caller keeps its existingpositional behavior and the change remains visible as drift. Elements that are not plain scalars
(computed, secret, output, nested collections) also fall back to positional matching rather than
being guessed at.
Testing
TestRegressScalarSetOrderRefreshPreservesStateInputsinpkg/testsis an end-to-end test: it runsa real
upfollowed by arefreshand asserts on the exported stack state.Asserting on state is deliberate and necessary.
ExpectNoChangespasses on bothrefreshandpreviewwhile this bug is occurring, becauseDiff()returnsDIFF_NONE. State is the only placethe bug is observable, so a test built on
ExpectNoChangeswould not catch a regression here.TestRefreshExtractInputsScalarTypeSetReorderinpkg/tfbridgecovers the unit-level behavior:set_order_preserved-- a reordered scalar set keeps the recorded ordertypelist_still_positional--TypeListis unaffected and stays positionalchanged_membership_still_visible-- a genuine membership change is not maskedduplicate_values_preserved-- repeated values are matched one-for-oneBoth the end-to-end test and the two order-sensitive unit subtests were verified to fail without the
change in
schema.goand pass with it. The two control subtests pass either way, confirming theexisting behavior they cover is unchanged.
go test ./pkg/tfbridge/passes.go test ./pkg/tests/passes except forTestAccProviderConfig,which fails identically on an unmodified checkout in my environment because it requires
make install_plugins.Note on scope
pkg/pfhas its own input extraction path, which I have not examined. This change is limited to theSDK-based bridge.