states: fix RootOutputValuesEqual comparing s2 to itself

[veeceey]

Description

RootOutputValuesEqual has a bug where it iterates over s2.RootOutputValues instead of s.RootOutputValues, so it ends up comparing s2 against itself rather than comparing s (the receiver) against s2 (the argument).

// Before (broken): iterates s2 and looks up in s2
for k, v1 := range s2.RootOutputValues {
    v2, ok := s2.RootOutputValues[k]

// After (fixed): iterates s over and looks up in s2
for k, v1 := range s.RootOutputValues {
    v2, ok := s2.RootOutputValues[k]

This means the function always returns true when both states have the same number of output values, even if the values themselves are completely different. This was introduced in #37886.

Impact

This affects refresh-only plan mode (terraform plan -refresh-only), where RootOutputValuesEqual is used to determine whether root output values changed during refresh. With this bug, output value changes during refresh would never cause the plan to be considered applyable, since the comparison always reports equality.

Test plan

• Added TestStateRootOutputValuesEqual covering identical outputs, differing values, differing sensitivity, differing keys with same count, and differing counts
• The "different values same key" and "different keys same count" cases specifically catch this bug -- they would have incorrectly returned true before this fix
• All existing internal/states tests continue to pass

[hashicorp-cla-app]

All committers have signed the CLA.

[crw]

Hi @veeceey, is there an issue open for this bug? Thanks.

[veeceey]

Hey @crw, I don't think there's an existing issue for this — I found it while reading through the changes in #37886. The bug is pretty straightforward: the loop iterates over s2.RootOutputValues and then also looks up from s2.RootOutputValues, so it's comparing s2 to itself instead of comparing s to s2. Easy to miss in review since the variable names are so similar.

Happy to open an issue first if that's preferred!

[SarahFrench]

H! @veeceey , thanks for spotting this!

I don't think you need to open an issue, but it'd be good to establish whether this is a bug that could change user-facing behaviour. If the bug wasn't user facing then we wouldn't include a change log entry.

From the original PR it looks like it could be user facing but only in some specific conditions:

These comparison are not used in any critical codepaths, but do show up in unexpected places, like backend migrations which don't appear to be needed, or incorrect exit codes when there are no changes planned.

I've proposed a change to the change log entry that frames things from a user's perspective. If you're happy to go ahead with that I'm happy to get this merged.

[SarahFrench]

Suggested change

	body: 'states: fix `RootOutputValuesEqual` comparing `s2` to itself instead of comparing `s` to `s2`, which caused refresh-only plans to never detect output value changes'
	body: 'states: fixed a bug that caused Terraform to be unable to identify when two states had different output values. This may have caused issues in specific circumstances like backend migrations.

[veeceey]

Thanks @SarahFrench — that wording is much better! I've pushed the update. Happy to merge whenever you're ready.

[SarahFrench]

Thanks again!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.