Clean up direct-engine state entries for remotely deleted resources removed from config#5496
Merged
Merged
Conversation
… from config When a resource was deleted remotely and also removed from the bundle config, CalculatePlan dropped the Delete entry from the plan, so the stale state entry was never removed and reappeared on every deploy. Keep the Delete entry instead; Delete() already tolerates resources that are gone and removes the entry from state. Also add the missing location guard in metadata.Compute for pipelines and dashboards (jobs got it in #3710), which made the same scenario hard-fail the deploy. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
Integration test reportCommit: 2b34822
23 interesting tests: 13 SKIP, 7 KNOWN, 3 RECOVERED
Top 4 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Co-authored-by: Isaac
Member
Author
|
The direct engine now keeps the delete entry for a remotely deleted resource so its stale state entry gets cleaned up during destroy, which makes the destroy output of acceptance/bundle/resources/dashboards/delete-trashed-out-of-band legitimately diverge per engine. Split that section into out.destroy.terraform.txt and out.destroy.direct.txt per the repo convention, same as the existing jobs/remote_delete/destroy test. |
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Jun 10, 2026
Per review feedback: planning now sets gone on a Delete entry when the remote read returns 404. Applying such an entry removes it from state without calling the delete API, and deploy/destroy approval prompts no longer list it as a deletion or ask for approval, since nothing is actually deleted. With that the destroy output for remotely deleted resources matches the terraform engine again, so the per-engine out.destroy split is reverted for delete-trashed-out-of-band and the removed_from_config test now also asserts that no delete API calls are made for gone resources. Also reword the state-only resource comments in metadata.Compute to explain the statemgmt.Load injection that makes the guards necessary. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Member
Author
|
Addressed the review feedback in a01eb5d:
Validation: |
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
4 tasks
…t-stale-state # Conflicts: # bundle/direct/bundle_apply.go # bundle/direct/bundle_plan.go
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
approved these changes
Jul 2, 2026
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
4 participants
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.
Why
Found during a full-repo review of the CLI. With the direct engine, a resource that was deleted remotely and then removed from the bundle config could never leave the deployment state. The plan dropped its Delete entry, so the state cleanup never ran and the stale entry came back on every deploy. For pipelines and dashboards the same scenario was worse: the deploy hard-failed in metadata computation with "failed to compute relative path", and since the state never got cleaned, every following deploy failed the same way.
Changes
Before, deploying after a resource was deleted remotely and removed from config either left a stale state entry forever (jobs) or failed the deploy outright (pipelines, dashboards); now the deploy plans a delete for the stale entry, cleans it from state, and completes.
bundle/direct/bundle_plan.go: when planning a Delete and the remote read reports the resource is gone, keep the Delete entry instead of removing it from the plan.Delete()already tolerates missing resources and removes the entry from state when applied.bundle/deployplan/plan.go: removeRemoveEntry, whose only caller was the code path above.bundle/deploy/metadata/compute.go: add the missingl.File == ""guard to the pipelines and dashboards loops, matching the guard the jobs loop received in direct: handle remotely deleted resources #3710. State-only resources have no config location, andfilepath.Relerrors on an empty path.A side effect visible in test output:
bundle destroywith the direct engine now lists a remotely deleted resource under "The following resources will be deleted", since its state entry is now explicitly cleaned instead of silently dropped.Test plan
acceptance/bundle/resources/jobs/remote_delete/removed_from_configcovering deploy, remote delete of a job and a pipeline, config removal, deploy twice, and summary, for both engines. With the direct engine the first deploy cleans the stale entries and the second deploy and summary are clean. The terraform variant also exercises the new metadata guard, since terraform keeps the gone resources in its state.TestComputeMetadataMutatorStateOnlyResourcesverifying state-only jobs, pipelines, and dashboards are skipped without error.bundle/direct,bundle/deployplan, andbundle/deploy/metadata.jobs/remote_delete,volumes/remote-delete,volumes/remote-change-name,vector_search_indexes/drift,permissions,deployment/bind,deployment/unbind,bundle/destroy,bundle/generate,bundle/invariant,clusters,quality_monitors,registered_models, synced tables). Onlyjobs/remote_delete/destroychanged output (direct destroy now lists the gone job) and was regenerated with-update../task fmt-q,./task lint-q,./task checks.This pull request and its description were written by Isaac.