Move direct engine state lifecycle to top-level callers#4928
Merged
Conversation
- Open state once in process.go after PullResourcesState for direct engine - Open state in destroy.go and configsync/diff.go before CalculatePlan - Finalize state in deployCore/destroyCore after Apply, before PushResourcesState - Remove Open from CalculatePlan, InitForApply, ExportState - Remove Finalize from Apply (moved to callers) - Add modified flag to DeploymentState to skip no-op Finalize - Fix panic syntax (panic takes single arg) Co-authored-by: Isaac
The bundle plan command uses PreDeployChecks without Deploy, so state must also be opened when PreDeployChecks is set. Co-authored-by: Isaac
These commands manage state outside ProcessOptions and need explicit Open before calling statemgmt.Load for direct engine. Co-authored-by: Isaac
Apply no longer calls Finalize internally, so callers that need the state written to disk must call Finalize explicitly. Co-authored-by: Isaac
- Serial numbers decrease because Finalize is now a no-op when state is unmodified (prevents spurious serial increments) - "Updating deployment state..." no longer appears when deploy fails without modifying state (no state file to push) Co-authored-by: Isaac
- Finalize: move `modified = false` after successful save, restore serial on failure so retries work correctly - Open: set `modified = true` after migrateState changes the version, so Finalize persists migrated state Add unit tests for the state lifecycle. Co-authored-by: Isaac
Centralize Open/Finalize pairing: process.go gets deferred Finalize for all flows going through ProcessBundleRet. Exception flows (destroy, run, dashboard, configsync/diff) that don't use process.go also get deferred Finalize after their own Open calls. Explicit Finalize in deployCore/destroyCore is kept because it must run before PushResourcesState. Deferred call is a no-op after that since Finalize is idempotent when state is unmodified. Co-authored-by: Isaac
Per design: migrations happen on the fly each Open() and don't need to be persisted. Only real state updates (SaveState, DeleteState) cause Finalize to write to disk. This avoids spurious serial bumps. Co-authored-by: Isaac
- DeleteState only sets modified when the key actually existed - ValidatePlanAgainstState accepts *DeploymentState instead of re-reading the file Co-authored-by: Isaac
Add PostStateFunc callback to ProcessOptions that runs within the state lifecycle scope (after state open, before deferred Finalize). Add NeedDirectState flag for flows that need state but don't set InitIDs etc. - destroy: uses PostStateFunc to call phases.Destroy within state scope - run: restructured into inline path (SkipInitialize) and normal path (full ProcessBundleRet with PostStateFunc) - config-remote-sync: uses PostStateFunc; DetectChanges reuses b.DeploymentBundle for direct engine instead of creating a new one Co-authored-by: Isaac
Scripts don't need state, so don't use InitIDs/ErrorOnEmptyState in ProcessBundleRet. Instead, load state inside PostStateFunc only for resource runs. Co-authored-by: Isaac
- Revert broken outputs from previous InitIDs attempt - Scripts now go through full ProcessBundleRet (with PullResourcesState) so request logs show additional state-reading calls Co-authored-by: Isaac
Use full ProcessBundleRet with AlwaysPull and NeedDirectState for run, so state lifecycle is managed by process.go. Scripts get unnecessary PullResourcesState but no state loading (ErrorOnEmptyState is only called for resource runs inside PostStateFunc). Fix non-deterministic request ordering in databricks-cli script tests by using print_requests.py --sort. Co-authored-by: Isaac
…pull Scripts no longer go through ProcessBundleRet with AlwaysPull+NeedDirectState. Instead, after Initialize, the run argument is resolved and scripts execute immediately without state operations. Resource runs handle state inline (matching the original main approach) with explicit Open/Finalize. Restores the original test scripts and request assertions from main. Co-authored-by: Isaac
- Revert acceptance/cmd/workspace/apps/run-local-node/output.txt to main (was overwritten by broad -update run, unrelated to state lifecycle) - Add NeedDirectState to shouldReadState so callers don't need to also set AlwaysPull for the state block to execute Co-authored-by: Isaac
Restructure cmd/bundle/run.go so that resource runs use ProcessBundleRet with AlwaysPull, ErrorOnEmptyState, and NeedDirectState instead of hand-rolling PullResourcesState, StateDB.Open, and Finalize. Scripts are handled in PostInitFunc (before state is pulled) via executeScript which replaces the process on success. Resource runs are handled in PostStateFunc with state already opened by process.go. Co-authored-by: Isaac
InitializeURLs makes an extra API call to get the workspace ID. It was previously called for both InitIDs and ErrorOnEmptyState, but only InitIDs callers (summary, open) need URLs. ErrorOnEmptyState callers (run) do not. Co-authored-by: Isaac
CalculatePlan now reads from in-memory StateDB.Data, not from disk. The Finalize() call is still needed to persist state, but the comment incorrectly stated it was needed "so CalculatePlan can read it". Task: 011.md Co-authored-by: Isaac
Return (false, err) instead of diags.Extend() in convertState which returns (bool, error), not diag.Diagnostics. Task: 012.md Co-authored-by: Isaac
…/Finalize moves Co-authored-by: Isaac
Avoid creating a state file when the plan has no operations, which prevents spurious state files from being written on no-ops. Co-authored-by: Isaac
NeedDirectState leaked the direct/terraform engine distinction into the top-level ProcessOptions API. Since every caller that set NeedDirectState also set PostStateFunc, use PostStateFunc != nil as the signal that the state DB should be opened. Co-authored-by: Isaac
denik
requested review from
pietern,
shreyas-goenka and
simonfaltum
as code owners
Approval status: pending
|
Co-authored-by: Isaac
Co-authored-by: Isaac
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
1 participant
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.
Changes
Open/Finalize) from internal methods to top-level command callers, so state is opened exactly once per command. Double-open panics.bundle runno longer pulls remote state it doesn't need./api/2.0/preview/scim/v2/Mecall duringbundle runwhen onlyErrorOnEmptyStateis set.Why
The previous pattern hid
Open/Finalizeinside low-level methods, making it hard to reason about when state was opened, whether it would be written, and whether double-opens could occur. Lifting the lifecycle to callers makes the invariant obvious and enforceable, and avoids unnecessary state I/O and API calls in read-only flows.Preparation for WAL & metadata service.
Tests
DeploymentState: open/save/finalize round-trip, panic on double open, delete tracking.bundle run, and config-remote-sync flows.