Skip to content

Cleanup in appropriate order when running parallel execution#38247

Open
dsa0x wants to merge 5 commits into
mainfrom
sams/tf-test-parallel-deps-cleanup
Open

Cleanup in appropriate order when running parallel execution#38247
dsa0x wants to merge 5 commits into
mainfrom
sams/tf-test-parallel-deps-cleanup

Conversation

@dsa0x
Copy link
Copy Markdown
Member

@dsa0x dsa0x commented Mar 7, 2026

This PR started with me tracking a bug where in parallel mode, we still had some dependency ordering based on the sequence of runs.

For example,

test {
  parallel = true
}

run "test" {
  state_key = "main"
  variables {
    id = "test"
    unused = "unused"
  }
}

run "test_two" {
  state_key = "state2"
  variables {
    // This dependency is a later run, but that should be fine because we are in parallel mode.
    id = run.test_three.id

    // The output state data for this dependency will also be left behind, but the actual
    // resource will have been destroyed by the cleanup step of test_three.
    unused = run.test.unused
  }
}

run "test_three" {
  state_key = "state3"
  variables {
    id = "test_three"
    unused = run.test.unused
  }
}

run "test_four" {
  state_key = "main"
  variables {
    id = "test"
    unused = "unused"
  }
}

In the test above, run.test_three is destroyed before run.test_two, but that is wrong. test_two depends on test_three, so test_two should be destroyed first, i.e reversing the order of creation.

Having fixed that in f6f234f, I realized that the original implementation was unnecessarily complex. Instead, what I have done now is only connect references between the last runs of each state key. These last runs represent the last applied state.

In the above, the state_key main will contain the state applied after evaluating run.test_four. Therefore, any reference to run.test (which also used the state_key main) do not need to be taken into account when connecting cleanup nodes, as run.test_four is the "owner" of the cleanup process.

Target Release

1.15.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dsa0x dsa0x marked this pull request as ready for review March 9, 2026 08:04
@dsa0x dsa0x requested a review from a team as a code owner March 9, 2026 08:04
@dsa0x dsa0x requested a review from Copilot May 21, 2026 06:54
@dsa0x dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label May 21, 2026
@dsa0x dsa0x removed the no-changelog-needed Add this to your PR if the change does not require a changelog entry label May 21, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes teardown (state cleanup) ordering for terraform test when runs execute in parallel, ensuring cleanup respects run dependencies rather than incidental run sequence. It also simplifies the cleanup dependency modeling by connecting teardown nodes based only on the “owning” (last) run for each state_key.

Changes:

  • Reworked teardown graph construction to derive cleanup dependencies from run-node dependencies and to connect only the last run per state_key as the cleanup “owner”.
  • Updated/added tests and fixtures to validate correct teardown ordering in parallel dependency scenarios.
  • Added a v1.15 bugfix changelog entry for the parallel cleanup dependency ordering fix.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/moduletest/graph/transform_state_cleanup.go Refactors teardown subgraph dependency collection and simplifies cleanup-node edge creation based on owning runs per state_key.
internal/moduletest/graph/test_graph_builder.go Updates TeardownSubgraph wiring to pass the run graph via the renamed field.
internal/command/testdata/test/parallel_deps/main.tftest.hcl Adds a new test fixture exercising parallel teardown dependency ordering.
internal/command/testdata/test/parallel_deps/main.tf Adds supporting Terraform config for the new parallel dependency fixture.
internal/command/testdata/test/cleanup/main.tftest.hcl Minor formatting adjustment in existing cleanup fixture.
internal/command/test_test.go Adds a JSON-output-based regression test to assert teardown ordering for parallel dependencies.
.changes/v1.15/BUG FIXES-20260309-084347.yaml Adds changelog entry for the bugfix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +6004 to +6008
actual := output.All()

var teardownOrder []string
lines, err := parseJSONLines(t, actual)
if err != nil {
@dsa0x dsa0x force-pushed the sams/tf-test-parallel-deps-cleanup branch from a7bf179 to f0416b9 Compare May 21, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants