direct: persist state before long-running waits in DoCreate/DoUpdate - #5391
Draft
denik wants to merge 32 commits into
Draft
direct: persist state before long-running waits in DoCreate/DoUpdate#5391denik wants to merge 32 commits into
denik wants to merge 32 commits into
Conversation
Collaborator
Integration test reportCommit: 060ad91
14 interesting tests: 4 SKIP, 4 flaky, 3 RECOVERED, 2 FAIL, 1 KNOWN
Top 37 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/wait-method-removal
branch
from
June 1, 2026 13:05
b00a6f3 to
786f578
Compare
denik
force-pushed
the
denik/wait-method-removal
branch
from
June 2, 2026 19:10
f0e65f7 to
9f7fc23
Compare
denik
force-pushed
the
denik/wait-method-removal
branch
from
June 3, 2026 15:15
bb3c2e3 to
cdd95c8
Compare
denik
force-pushed
the
denik/wait-method-removal
branch
from
June 4, 2026 09:41
df19a64 to
bf602f1
Compare
…nternally Resource implementations cannot recover from a state-persistence failure — the resource already exists on the server and aborting would not undo its creation. Log the error via logdiag instead of propagating it, and drop the error return so call sites are a single statement. Co-authored-by: Denis Bilenko
All six resources with a gap between resource creation and a subsequent long-running wait now call engine.SaveState immediately after the create API returns, using waiter.Name() (available before Wait()) for the postgres resources and createResp.DashboardId for the dashboard. This ensures an interrupted deployment leaves a tracked resource rather than an orphan the next plan must rediscover from remote state. Co-authored-by: Denis Bilenko
dashboard.DoCreate now calls engine.SaveState immediately after the dashboard is created (with etag persisted), before publishDashboard. A failed publish leaves the draft tracked in state rather than orphaned; the next deploy finds it via DoRead and re-publishes via DoUpdate without recreating the dashboard. The old trash-on-publish-failure cleanup is removed — it was a fragile workaround for the lack of state persistence and is now unnecessary. Acceptance tests: - publish-failure-cleans-up-dashboard: updated to reflect the new behavior per engine (direct: draft persists with URL; terraform: existing behavior, cleaned up). Output files split to per-engine variants (out.summary.*.txt, out.dashboardrequests.*.txt). - publish-failure-retry (new, direct only): verifies end-to-end that a transient publish failure leaves the draft in state (summary shows URL, plan detects diff), and the subsequent deploy re-publishes without issuing a CREATE call. Co-authored-by: Denis Bilenko
…cements The local testserver uses ?o= and cloud uses ?w= for the workspace/org ID in dashboard published URLs. Add a parent-level [[Repls]] rule that maps both to ?[WSPARAM]= so the output files are environment-independent. Co-authored-by: Denis Bilenko
Use per-test [[Repls]] rules (matching raw digits) in the two publish-failure tests so the URL parameter is normalized to ?[WSPARAM]=[NUMID] regardless of whether the testserver or cloud environment is used. Revert the over-broad parent-level rule that broke detect-change's existing ?[ow]=... cleanup. Co-authored-by: Denis Bilenko
macOS ships bash 3.2 which does not support &>> (bash 4+). Replace with >> file 2>&1 which works on all bash versions. Co-authored-by: Denis Bilenko
MSYS_NO_PATHCONV=1 (set in the parent test.toml) prevents MSYS2 from converting POSIX paths to Windows paths, causing Python to receive a broken path (/c/a/... instead of C:\a\...) when invoking fault.py. Unset it before the fault.py call, matching the pattern used by other dashboard scripts before their bin helper invocations. Co-authored-by: Denis Bilenko
…d=false
Engine.SaveState:
- Accepts resourceKey for logging: "SaveState: resources.X id=Y N bytes: {...}"
- Skips WAL write if state is unchanged (structdiff.IsEqual), logging a skip message.
- Records the last saved value in e.lastSaved for subsequent comparisons.
Dashboard DoCreate:
- Saves intermediate state with Published=false (the actual draft state) instead of
the user's Published=true. This ensures the planner sees a real diff (false→true)
on the next deploy if publish is interrupted, rather than treating the resource as
up-to-date and silently skipping the publish.
publish-failure-retry acceptance test:
- Adds `bundle plan -o json` and extracts the published change to verify
old=false, new=true in the plan after a failed publish.
README.md: update stale SetID+SaveState reference to current SaveState(ctx, id, state) API.
Co-authored-by: Denis Bilenko
…reate/DoUpdate Mergiraf reintroduced the standalone WaitAfterCreate and WaitAfterUpdate methods during the rebase against main (sql_warehouse.go had a merge conflict). Inline their wait logic directly into DoCreate and DoUpdate and remove the standalone methods, consistent with the Engine callback pattern used by other resources. Co-authored-by: Denis Bilenko
…ublish-failure-retry - Replace fixed hex regex [[Repls]] with replace_ids.py called after the first deploy; the real dashboard ID is captured from state as [DASHBOARD1_ID]. - Inline the plan -o json | jq output directly into output.txt instead of saving to out.plan_published_change.json and trace cat-ing it. Co-authored-by: Denis Bilenko
…dashboard DoUpdate Two more cases where state could be persisted earlier to avoid orphaning or stale-etag conflicts: sql_warehouse DoCreate: the warehouse is created, then DoCreate polls for RUNNING (and may Stop it). If interrupted during the wait, the warehouse was orphaned. Now SaveState is called right after Create returns the id, before the wait. dashboard DoUpdate: Update() bumps the server-side etag, then publishDashboard() can fail. Mirror the DoCreate fix — save the new etag with Published=false before publishing. This keeps the etag in sync (a stale etag makes the next Update fail with a conflict) and records published=false so the planner re-publishes next deploy. Resolves the pre-existing TODO. Add publish-failure-retry-on-update acceptance test: deploy, then trigger an update with an injected publish failure, and verify the update issued a PATCH (no CREATE) and the plan records published old=false. Co-authored-by: Denis Bilenko
DoUpdate applies up to four sequential mutating calls (tags, AI gateway, config, notifications) and then waits up to 35 minutes for the endpoint to finish updating. If that wait was interrupted, none of the applied changes were persisted to state, forcing them to be re-applied on the next deploy. Save the new config right after the mutating calls and before the wait, mirroring DoCreate which already saves before waitForEndpointReady. The id (endpoint name) matches the one DoCreate persists, so there is no id mismatch. Co-authored-by: Denis Bilenko
…, not resource id) The previous attempt to save state before the async wait in postgres DoCreate used engine.SaveState(ctx, waiter.Name(), config). waiter.Name() returns the LRO operation name (e.g. .../branches/foo/operations/UUID), not the resource name (…/branches/foo) that DoCreate ultimately returns. Using the operation name as the id creates a dead WAL entry and does not help with orphan recovery. Replace with a TODO comment explaining the two options for a proper fix: 1. Derive the resource name from request inputs (Parent + resource-type + Id). 2. Use waiter.Metadata() if it surfaces the resource name before Wait completes. No panic in production: the framework saves state via db.SaveState (not through the Engine) after DoCreate returns, so the mismatch between operation-name and real-id never hits the Engine's id-mismatch check. The test panic was a test artifact where the same Engine was reused across Create→Update. Co-authored-by: Denis Bilenko
Previously apply.go called db.SaveState directly after DoCreate/DoUpdate returned, bypassing the engine. This meant a DoCreate that called engine.SaveState with a wrong id (e.g. an LRO operation name instead of the real resource name) would go undetected — the final save would silently use the correct id. Route both final saves through the engine instead: - If DoCreate/DoUpdate called engine.SaveState with a wrong id, the engine's id-mismatch check panics, catching the bug immediately in tests. - If DoCreate/DoUpdate already saved identical state (e.g. the resource saved before a long wait), the engine's dedup skips the redundant write. - State-save I/O errors are logged internally by the engine and no longer abort the deployment (the resource was already created/updated successfully; aborting would confuse the user). UpdateWithID is left unchanged: DoUpdateWithID takes no Engine parameter. Co-authored-by: Denis Bilenko
The type does exactly one thing: save state. StateSaver names that directly. NewNopEngine -> NewNopStateSaver, engine.go -> state_saver.go. Co-authored-by: Denis Bilenko
…aver - SaveStateWith[F any]: type-safe helper that temporarily sets a field to an intermediate value before saving, then restores it. Used to save Published=false before publishDashboard and Lifecycle=nil before warehouse/cluster lifecycle management, so the planner sees a real diff if deployment is interrupted mid-way. - Change saveFunc signature to func(id string, b json.RawMessage) error and add DeploymentState.SaveStateJSON to accept pre-marshaled bytes. StateSaver already marshals x to JSON for dedup; passing those bytes directly avoids a redundant marshal on every save. - Fix lastSaved aliasing: store []byte (JSON snapshot) instead of any pointer. SaveStateWith modifies the config, saves, then restores; a pointer-based lastSaved would point at the restored value, making the subsequent final save look like a no-op and leaving Published=false in the WAL. Co-authored-by: Isaac
Co-authored-by: Isaac
If DoCreate is interrupted after the app is created but before manageLifecycle completes (during waitForApp or the deploy step), the app can reach ACTIVE on its own with no active deployment. Previously, engine.SaveState saved lifecycle.started=true (the desired value). On the next plan the planner would see no localDiff (state == desired) and no remoteDiff for lifecycle (remote is already ACTIVE), while source_code_path drift is silently skipped by OverrideChangeDesc when the remote has no active deployment. Result: the planner marks the resource as Skip, and the app stays permanently un-deployed. SaveStateWith(lifecycle=nil) records that the app exists but lifecycle has not been applied yet. This creates a localDiff (nil→desired) that is not remote-skippable, forcing DoUpdate → manageLifecycle → Deploy on the next run. Co-authored-by: Isaac
…per-test
The global ETAG replacement in acceptance/bundle/resources/dashboards/test.toml
replaced all 8+ digit numbers in all dashboard test outputs. This prevents
local tests from using add_repl.py to record specific etag values (e.g.
the bumped etag after a PATCH), which is necessary for tests that verify
etag-tracking behavior across SaveState calls.
Move the replacement to the three tests that actually need it for cloud
compatibility (non-deterministic real etags):
- detect-change: shows etag in bundle summary output
- change-name: etag appears in Terraform PATCH request body
- change-embed-credentials: etag appears in Terraform PATCH request body
Local-only tests now see the etag replaced by the global [NUMID] rule
(acceptance/test.toml, \d{8,}) instead of [ETAG]. The out.plan.direct.json
files update accordingly: [ETAG] (unquoted, invalid JSON) → "[NUMID]"
(quoted, valid JSON string).
Co-authored-by: Isaac
genie_space was added to main after this branch was cut, using the old DoCreate/DoUpdate signatures that lack the *StateSaver parameter added by this PR to the IResource interface. Co-authored-by: Isaac
…nt test SaveStateWith(Published=false) in DoUpdate makes the stale-published-content bug permanently unrecoverable: after a PATCH+failed-POST, state has the new etag + Published=false. On the next plan, the planner sees remote.Published=true == desired=true and skips (remote_already_set), so neither a plain re-deploy nor --force can fix the stale content. Without SaveStateWith, state retains the pre-PATCH etag. The next plan detects the etag mismatch as "modified remotely" and blocks — but --force recovers it by forcing a full PATCH+POST cycle. Also cherry-picks the publish-failure-stale-content acceptance test from denik/dashboard-published-bug, which documents this pre-existing bug and confirms --force recovers it. Includes the testserver change that bumps the dashboard etag on every PATCH (matching cloud behavior), and test improvements from that branch (explicit ETAG_1/ETAG_2 labels, add_repl.py calls, etc.). Co-authored-by: Isaac
…le DoCreate/DoUpdate Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
…ignature instance_pool and job_run were added on main after this branch was cut, so their DoCreate/DoUpdate still had the pre-StateSaver signature and failed the IResource interface check. Add the *StateSaver parameter (unused). Also fix the app retry tests and dashboard publish-failure tests for the post-rebase testserver: the apps testserver now keeps DELETING apps visible (cloud-realistic), and the parent dashboards test.toml injects eventual- consistency staleness on direct; opt the publish-failure/retry tests out of that injection since they drive an explicit read-back. Co-authored-by: Isaac
The Engine->StateSaver rename missed the async-APIs section of the dresources README, which still called the second DoCreate/DoUpdate argument a *Engine. Co-authored-by: Isaac
Revert the redaction added during the rebase conflict resolution: it depended on structwalk.RedactSensitiveFields, which is being removed. State is marshaled with plain json.Marshal in both SaveState and StateSaver.SaveState, as before. Co-authored-by: Isaac
The scripts already capture each etag by value into ACC_REPLS (ETAG_1, ETAG_2,
ETAG), so the catch-all "\"[-0-9]{8,}\"" / "\"[0-9]{8,}\"" [[Repls]] were
redundant and risked masking unrelated quoted long integers. Remove them;
change-name and change-embed-credentials test.toml held nothing else, so drop
those files entirely.
Co-authored-by: Isaac
Now that DoRead derives published from the publish/update timestamps (#6119), the reason DoUpdate avoided saving state before the publish is gone: a stale publish reports remote published=false, so the remote_already_set skip that would have stranded it can no longer happen. Save the post-update etag and published=false before publishing. A failed publish is now recoverable with a plain deploy: the next plan sees desired published=true against a remote reported as false and republishes. Saving the post-update etag also keeps state in sync with remote, so CheckDashboardsModifiedRemotely no longer misreports this as an out-of-band edit and --force is no longer required. Drops the Badness marker from publish-failure-stale-content and updates it to assert recovery on a plain re-deploy. Co-authored-by: Isaac
Dropping RedactSensitiveFields from the state save path also fixes the values persisted for protobuf Duration fields. RedactSensitiveFields deep-clones the struct field-by-field via reflection, which loses durationpb.Duration's internal state, so its custom marshaler emitted the zero value: state recorded history_retention_duration and suspend_timeout_duration as "0s" instead of the real "604800s"/"300s". Plain json.Marshal preserves them. Only the saved "old" values in these plan goldens change; there is no behavior change beyond state now matching what the server returned. Co-authored-by: Isaac
denik
force-pushed
the
denik/wait-method-removal
branch
from
August 5, 2026 13:03
b3faedb to
060ad91
Compare
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
2 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.
Changes
Replace the
WaitAfterCreate/WaitAfterUpdateresource methods with inlinewaits inside
DoCreate/DoUpdate, and give both methods a*StateSaverargument so a resource can persist intermediate state before a long-running
wait. This prevents orphaning a created resource if the deploy is interrupted
mid-wait: the state is already recorded, so the next deploy reconciles it
instead of leaking it.
StateSaverdeduplicates writes, logs I/O failures without aborting thedeploy, and routes the final Create/Update save so an id mismatch is caught.
SaveStateWithtemporarily overrides a field (e.g.published=false,started=true) so the planner sees a real diff if the wait is interrupted.Tests
Unit and acceptance tests, including new dashboard publish-failure/retry
scenarios that exercise the save-before-wait behavior.