Skip to content

direct: store a dashboard contents digest in state instead of the contents - #6105

Open
Sankalp-Mittal wants to merge 4 commits into
sankalp-mittal/dashboards-serialize-earlyfrom
sankalp-mittal/dashboards-sha-state
Open

direct: store a dashboard contents digest in state instead of the contents#6105
Sankalp-Mittal wants to merge 4 commits into
sankalp-mittal/dashboards-serialize-earlyfrom
sankalp-mittal/dashboards-sha-state

Conversation

@Sankalp-Mittal

@Sankalp-Mittal Sankalp-Mittal commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Changes

Add a hashed_in_state lifecycle rule to the direct engine. A field declared under it is persisted to state as a sha256_hashed_in_state:<hex> digest of its contents instead of the contents themselves. dashboards.serialized_dashboard is the first (and only) field to declare it.

CompactState applies the rule, and it runs on every value entering the state diff — the saved state, the local config and the remapped remote — as well as on the state being persisted. Once the saved value is a digest, all three sides must be digests or the comparisons would be hash-vs-content nonsense. The full contents stay in the plan's new_state and are sent to the API on every create and update, so deploys are unaffected.

Because CompactState runs on every diff side and before every save, it is routinely handed state that is already compact. Hashing is therefore idempotent: a value that is already a sha256_hashed_in_state:<hex> placeholder is returned unchanged rather than hashed again. If it were not, re-compacting an already-compact state would produce hash(placeholder) != placeholder on some sides of the diff and report a permanent phantom serialized_dashboard change on every deploy.

Hashing is skipped when it would not pay for itself: a value whose JSON encoding is no longer than the 87-byte placeholder is persisted raw. The verdict depends only on the value, so all call sites agree on it and the diff sides stay comparable — including when a field is small for one resource and large for another, or grows past the threshold between deploys.

Declaring a field is restricted to top-level paths and rejected otherwise: CompactState shallow-copies the state so the caller's value (reused for the deploy API call) is untouched, which only isolates depth-1 fields.

Why

serialized_dashboard holds inlined dashboard JSON, frequently multi-MB, and state only ever compares it for equality — nothing reads the stored copy back out. A digest answers the same question, so resources.json no longer carries a copy of every dashboard on every deploy.

No state version bump: legacy state holding the full contents is hashed on read for comparison and rewritten compactly on the next save, so existing bundles do not report a dashboard update on upgrade. bundle plan still shows the real content diff.

The downgrade direction is intentionally left ungated. An older CLI that predates hashed_in_state reads the stored sha256_hashed_in_state:<hex> as if it were the contents; since serialized_dashboard is ignore_remote_changes but not ignore_local_changes, the resulting Old(hash) != New(contents) mismatch is read as a local change and republishes the dashboard once. That is non-destructive and self-correcting — the old CLI rewrites the full contents, and a later upgrade re-hashes on read — so a single spurious republish per dashboard
on downgrade is accepted rather than blocked with a state-version gate.

hashed_in_state is orthogonal to ignore_remote_changes. serialized_dashboard happens to need both, for unrelated reasons: it is hashed because the JSON is large, and separately ignored remotely because the server normalizes it so its remote digest never equals the config digest — drift is detected via etag.

Tests

resources/dashboard-state-sha asserts state holds only the digest while the API receives the full contents, across create, a no-op re-plan, an edit and the resulting update, for READPLAN in ["", "1"]. Its fixture is sized above the threshold on purpose.

Unit tests cover digest stability and idempotency, the top-level-path restriction, the legacy-full-content upgrade path, and both sides of the size threshold. The idempotency test is the only one that feeds a placeholder back through hashStateValue, guarding the re-compaction path above against a double-hashing regression. The fixtures are guarded by requireLargeEnoughToHash / requireTooSmallToHash, so resizing one fails with an actionable
message rather than silently inverting what the test proves.

Dashboard, bind and migrate goldens regenerated; the digest is masked as [HASH] by a single rule in acceptance/bundle/test.toml, anchored on the prefix.

This pull request and its description were written by Isaac.

@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: d53c524

Run: 32246573922

Env 🟨​KNOWN 🔄​flaky 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 292 1160 7:38
💚​ aws windows 4 4 294 1158 9:43
💚​ azure linux 4 4 291 1160 8:35
💚​ azure windows 4 4 293 1158 9:33
🟨​ gcp linux 1 1 4 292 1160 10:24
🔄​ gcp windows 1 3 4 294 1158 11:49
8 interesting tests: 4 SKIP, 3 RECOVERED, 1 KNOWN
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🟨​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 🟨​K 🔄​f
💚​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R 💚​R
Top 9 slowest tests (at least 2 minutes):
duration env testname
7:32 azure windows TestAccept
7:08 aws windows TestAccept
7:07 gcp windows TestAccept
3:48 gcp linux TestAccept
3:42 aws linux TestAccept
3:38 azure linux TestAccept
2:25 aws windows TestAccept/bundle/resources/dashboards/unpublish-out-of-band/DATABRICKS_BUNDLE_ENGINE=terraform
2:17 gcp windows TestAccept/bundle/resources/dashboards/unpublish-out-of-band/DATABRICKS_BUNDLE_ENGINE=terraform
2:03 aws windows TestAccept/bundle/resources/dashboards/detect-change/DATABRICKS_BUNDLE_ENGINE=terraform

@Sankalp-Mittal
Sankalp-Mittal marked this pull request as ready for review July 30, 2026 15:55

@denik denik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we automatically migrate old state so that there is no drift when we load state with full serialized dashboard?

It would be nice to have acc test for this case.

"parent_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/resources",
"published": true,
"serialized_dashboard": "{\"pages\":[{\"displayName\":\"Page One\",\"name\":\"02724bf2\"}]}",
"serialized_dashboard": "sha256_hashed_in_state:[HASH]",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"sha256_hashed_in_state:[HASH]"

could just be sha256:[HASH]

Also, do we need a replacement for HASH here, dopes it change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

  • The long name was to avoid potential future conflicts in naming as right now this prefix is used to check if a field has been hashed or not
  • Added the replacement for [HASH] since makes the output much more readable.

Comment thread libs/hash/hash.go Outdated

@shreyas-goenka shreyas-goenka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Looks good to me mostly. Can you see whether we can avoid this showing up in the plan diff? If that's a big lift or not?

         "old": "sha256_hashed_in_state:[HASH][0]",
          "new": "sha256_hashed_in_state:[HASH][0]",
          "remote": "sha256_hashed_in_state:[HASH][1]"

Comment thread bundle/direct/dresources/state_compaction.go Outdated
Comment thread bundle/direct/dresources/state_compaction_test.go Outdated
"old": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\"}]}",
"new": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\"}]}",
"remote": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\",\"pageType\":\"PAGE_TYPE_CANVAS\"}]}"
"old": "sha256_hashed_in_state:[HASH][0]",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we continue to show the same output in plan? Rather than the hash? Two reasons:

  1. The plan can be serialized to a file and be consumed again.
  2. Users should be able to filter out and see the exact diff.

Can you also confirm that after these changes a plan serialized and used with a --plan flag continues to work?

@shreyas-goenka shreyas-goenka Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For me ensuring (1) is more important. (2) is optional and maybe compacting the plan is actually better for readibility.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We won't get the diff if we want to hash since there is no way to retrieve the old dashboard state from the hash anyways

Comment thread acceptance/bundle/migrate/dashboards/out.plan_after_migrate.json Outdated
@denik

denik commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

This is a high risk change for serialized plan, so please convert dashboard tests to do EnvMatrix.READPLAN variant (ideally as a separate PR so we can confirm it all works on main).

@Sankalp-Mittal Sankalp-Mittal changed the title direct: store serialized_dashboard in state as a content hash direct: store a dashboard contents digest in state instead of the contents Aug 5, 2026
janniklasrose pushed a commit to GrantIsEaton/cli that referenced this pull request Aug 5, 2026
## Changes

Add `EnvMatrix.READPLAN = ["", "1"]` to the dashboard acceptance tests
that
deploy, and route their `bundle deploy` calls through the `readplanarg`
helper so
each test runs twice: once with an in-memory plan, once applying a plan
saved to
a file with `--plan`.

Because all `EnvMatrix` variants must produce identical output files,
any
divergence between the two deploy paths now fails the test
automatically.

Tests converted:

| Test | What it exercises |
| --- | --- |
| `resources/dashboards/change-serialized-dashboard` | edits
`serialized_dashboard` → update; asserts the PATCH/POST request bodies |
| `resources/dashboards/nested-folders` | reads `serialized_dashboard`
back from the API after deploy |
| `resources/dashboards/publish-failure-cleans-up-dashboard` | publish
failure path and the cleanup DELETE |
| `deployment/bind/dashboard` (+ `recreation/`) | bind writes state
without going through `DoCreate`/`DoUpdate` |

## Why

`bundle deploy --plan` takes a different code path than a plain deploy:
everything applied comes from the serialized plan file rather than from
a plan
computed in memory. Only one dashboard test exercised that path, so a
regression
in what a saved plan carries for dashboards would have gone uncaught.

This lands the coverage on its own, ahead of the `serialized_dashboard`
content-hash change (databricks#6105), so that any later failure is unambiguously
attributable to that change rather than to a pre-existing gap.

## Result

No request or state golden changed. The only golden edits are removed
`>>> [CLI] bundle deploy` trace lines — the deploys are no longer traced
because
the command line itself differs between variants (`--plan` vs none).

That the shared goldens are byte-identical across both variants is the
point:
`out.patch.requests.direct.txt`, `out.post.requests.txt` and
`out.state_after_bind.*.json` confirm both paths issue the same API
requests and
persist the same state on `main` today.

## Not converted

`migrate/dashboards` is deliberately left alone. It asserts on GET
requests to
`//dashboards` to confirm the direct engine ran, but a saved-plan deploy
skips
the read phase, so no GETs are recorded and that assertion has nothing
to match.
Converting it would mean weakening a real assertion. Migration therefore
remains
uncovered for the saved-plan path — worth noting, since like bind it
writes state
without going through `DoCreate`/`DoUpdate`.

## Tests

Acceptance-only; no production code changes. Verified green on
unmodified
`origin/main` before converting, so failures were attributable. `task
fmt`,
`task ws` and `task lint-q` are clean.

This pull request and its description were written by Isaac.
@Sankalp-Mittal
Sankalp-Mittal force-pushed the sankalp-mittal/dashboards-sha-state branch from 7b5f9c7 to e3dd4f3 Compare August 6, 2026 12:26
Comment thread bundle/direct/dresources/state_compaction_test.go
Comment thread bundle/direct/bind.go Outdated
Comment thread bundle/migrate/build_state.go Outdated
"parent_path": "/Workspace/Users/[USERNAME]",
"published": true,
"serialized_dashboard": "{\"pages\":[{\"name\":\"02724bf2\",\"displayName\":\"Dashboard test bundle-deploy-dashboard\"}]}\n",
"serialized_dashboard": "sha256_hashed_in_state:[HASH]",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a bit too long, "sha256:HASH" should be enough?

Also, why use repl for this hash, is it unstable?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

  • I'll change it
  • It's not unstable but some parts of the hash were being captured by [NUMID] rule so I used this repl as it also makes the output more readable and easy to see the different hashes

Comment thread bundle/direct/dresources/README.md Outdated
Comment thread bundle/direct/dresources/README.md Outdated
Comment thread bundle/direct/dresources/README.md Outdated
Comment thread bundle/direct/dresources/config.go Outdated
Comment thread bundle/direct/dresources/state_compaction_test.go Outdated
Comment thread bundle/direct/dresources/state_compaction_test.go Outdated
Comment thread bundle/direct/dresources/state_compaction_test.go Outdated
Comment thread bundle/direct/apply.go
Comment thread bundle/direct/bundle_plan.go Outdated
// hashed_in_state); hashing it on read lines the sides up so an unchanged resource
// shows no change. This only affects the in-memory copy used for the diff; the
// on-disk entry keeps its full contents until the resource is next saved.
savedState, err = dresources.CompactState(adapter.ResourceConfig(), savedState)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should avoid using the same name here, savedState -> compactedSavedState

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

However, I do wonder if we need a fully copy of savedState, localState and remoteState here.

Perhaps we can go ahead with the diff on unchanged state and the filter out localDiff entries that match hashed_fields?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done the name change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand this completely, but from what I understand this is what you are suggesting

  • take the diff of the raw states
  • for the changes states, filter out the hashed_fields
  • For the hashed fields do another check after hashing the fields

I don't see how this is better that what we do now?

Sankalp-Mittal and others added 3 commits September 1, 2026 19:09
Recreated on top of the early-serialization middle PR. Adds hashed_fields for dashboards.serialized_dashboard so state persists a sha256: content hash; the change-serialized-inline plan golden now shows the field hashed on the direct engine.

Co-authored-by: Isaac <no-reply@databricks.com>
Now that the early-serialization mutator guarantees serialized_dashboard is a string before the deploy engine runs, hashStateValue takes a string and hashes it directly instead of json.Marshal-ing an arbitrary value, and the size threshold compares the raw string length (what state actually stores). CompactState type-asserts the field to a string and panics on anything else: a non-string is a broken invariant (a programming error), not a user error. nil (an unset field) passes through. Addresses review comments to assume a string, drop the marshal, and assert/panic otherwise.

Co-authored-by: Isaac <no-reply@databricks.com>
Introduce a distinct compactedSavedState variable in CalculatePlan instead of overwriting savedState with its compacted form, so it is clear which is the raw state and which is the hashed copy used for the diff (review comment).

Co-authored-by: Isaac <no-reply@databricks.com>
@Sankalp-Mittal
Sankalp-Mittal force-pushed the sankalp-mittal/dashboards-sha-state branch from b026842 to 511f586 Compare September 1, 2026 19:34
@Sankalp-Mittal
Sankalp-Mittal changed the base branch from main to sankalp-mittal/dashboards-serialize-early September 1, 2026 19:34
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.

5 participants