direct: fix diffing duration and timestamp fields - #6377
Open
denik wants to merge 15 commits into
Open
Conversation
structdiff walks exported fields, but duration.Duration and common/types/time.Time keep their whole payload in an unexported protobuf pointer, so any two values compared equal and the direct engine planned "0 to change" for an edit like suspend_timeout_duration: 300s -> 600s. Detect such structs by shape rather than by name: no fields for the walk to see, plus a json.Marshaler of their own. That covers the two SDK types without listing them, and leaves genuinely empty proto messages (jobs.ScheduleTriggerState and friends) comparing equal as before. Values are compared through their JSON form. isEmptyStruct had the same blind spot and skipped these changes with reason "empty" even once structdiff reported them, so it needs the same check. Duration fields that were skipped as "empty" now show their real reason, "spec:input_only". Co-authored-by: Isaac
Collaborator
Integration test reportCommit: db436eb
9 interesting tests: 4 SKIP, 4 FAIL, 1 KNOWN
Top 5 slowest tests (at least 2 minutes):
|
Both engines now produce identical plan, deploy and endpoint output, so the per-engine files are redundant: fold them back into output.txt, which then asserts the engines agree. Drop Badness too, the bug it described is fixed. Co-authored-by: Isaac
andrewnester
approved these changes
Aug 25, 2026
Every existing case reaches a duration through a struct field. Comparing two of them directly is the shape the bug was first reported in and had no test. Co-authored-by: Isaac
denik
enabled auto-merge
August 25, 2026 14:45
The direct engine now sends the PATCH, and the real API rejects the mask path it builds: suspend_timeout_duration is a oneof member, so the API only accepts the group name spec.suspension. Separate bug, separate fix (#6374). The testserver ignores update_mask, which is why the local run is unaffected. Co-authored-by: Isaac
The direct engine now sends the PATCH, and the API rejects the mask path it builds: suspend_timeout_duration is a member of the suspension oneof, so only the group name spec.suspension is accepted. Separate bug, separate fix (#6374). Teach the testserver to validate update_mask against the paths the real API accepts, so the failure reproduces locally rather than only on aws. The engines diverge from the second deploy on, so those steps go back to per-engine files, and the deploy requests are recorded per engine: same body, different mask. Cloud stays enabled, with the golden recording what the real workspace does. Co-authored-by: Isaac
denik
changed the title
Co-authored-by: Isaac
postgres_endpoints was not the only resource affected. Scanning every direct engine input type for opaque fields turns up four: postgres_endpoints (suspend_timeout_duration), postgres_projects (history_retention_duration, default_endpoint_settings.suspend_timeout_duration), postgres_branches (ttl, expire_time, source_branch_time) and secrets (expire_time). Add tests for the two that show the fix working end to end. secrets covers the timestamp type and updates with update_mask=*, so nothing else gets in the way. postgres_projects.history_retention_duration is masked under its own name, so its update_mask is accepted too. The remaining fields are members of a oneof and fail the same way postgres_endpoints does. The testserver dropped both duration fields on project update, reporting a change that never took effect, so persist them. Co-authored-by: Isaac
Every opaque field a user can set now has a test. Three record the oneof update_mask failure for follow-up: branch ttl and expire_time are masked under spec.expiration, and a project's default_endpoint_settings.suspend_timeout_duration under default_endpoint_settings.suspension, while the direct engine sends the field name and gets 400. source_branch_time is a different failure mode worth its own test: it is immutable, so a change recreates the branch and never touches update_mask. Before the diffing fix the change was invisible and the branch silently kept its old fork point. Local only, since a literal fork timestamp is not valid against a live workspace. Testserver gaps this surfaced: branch create and update both dropped expire_time, and the project mask allow-list was missing the initial_* paths the Terraform provider sends alongside spec. Co-authored-by: Isaac
The bundle names contain "postgres", so a bare //postgres filter also matched the workspace-files state paths on Windows and the state writes leaked into the request goldens. Use the same exclusion list as the sibling postgres tests. Co-authored-by: Isaac
A real workspace caps how far ahead a branch may expire and rejects the literal timestamps with 400 "expiration time exceeds the maximum expiration time", so the branch is never created. update_ttl covers the same expiration oneof on cloud. Co-authored-by: Isaac
The human-readable plan is identical for both engines, so it moves back into output.txt where it asserts they agree. The per-engine plan files now hold the JSON plan's action and changes, which is where the engines actually differ: direct reports suspend_timeout_duration old/new, terraform reports no changes at all. That exposed non-determinism in the product: collectUpdatePathsWithPrefix built update_mask by iterating a map without sorting, so with two paths the order flipped between runs and the API rejected a different one each time. Sort it, as its leaf-path sibling already does. Add removal coverage, which nothing had: secrets clears expire_time end to end, postgres_projects clears history_retention_duration, and removing a member of the suspension oneof fails the same way updating it does. Testserver: honor the mask on project update so a masked path absent from the body clears the field instead of being ignored, and the same for secrets, which are masked with update_mask=*. Run the secrets test on cloud, as secrets/basic does. Co-authored-by: Isaac
Cloud says the API keeps history_retention_duration when the mask names it and the body omits it, so the fake's original guard was right and honoring the mask was wrong. Reverted, with the verified behavior noted next to the guard. Read the resource back after each removal and plan again. The two removals then tell different stories: the project reports a change, saves it to state and never mentions it again, because the GET does not echo spec and spec:input_only hides the remote value, so config and backend stay silently out of step. The endpoint removal fails on the oneof mask, keeps the old value in state and re-plans the same failing update on every deploy. Co-authored-by: Isaac
Each test now exercises one transition. The removal tests start from a config with the field set rather than inheriting the state a failed update left behind, so what they record is unambiguous. Every test records the requests it sends and the plan's changes per engine, and the removal tests plan once more afterwards to record the drift. Two gaps closed while splitting: update_history_retention lost its json plan when the removal phase moved out, and recreate_source_branch_time never recorded one, so the recreate showed up only as plan text. Co-authored-by: Isaac
bundle plan -o json was missing --var, so it failed, the script aborted there and the deploy, the request recording, the state read and the drift plan never ran. The golden recorded that faithfully, which made the test pass while covering almost nothing. Also stop committing out.requests.txt, which is scratch. Co-authored-by: Isaac
Cloud says the API keeps expire_time even under update_mask=*, so a removal never takes effect. Restore the fake's guard and record what that leaves behind: deploy reports success and every later plan asks for the removal again, which the secret GET makes visible unlike the postgres project case. The fake also hardcoded the secret owner, where a real workspace returns the requester, so use CurrentUser instead. And move Badness above the [[Repls]] header, where TOML does not swallow it as Repls.Badness. 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
3 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.
duration.Durationandcommon/types/time.Timekeep their payload in an unexported protobuf pointer, so structdiff saw any two values as equal andsuspend_timeout_duration: 300s -> 600splanned 0 to change. Such structs are now recognized by shape — no fields for the walk to see, plus ajson.Marshalerof their own — and compared through their JSON form, so there is no type list to maintain.isEmptyStructhad the same blind spot, hence two goldens now showingspec:input_only.Four resources have such fields in their config, and every field a user can set now has a test:
secrets.expire_timeandpostgres_projects.history_retention_durationapply cleanly — the first updates withupdate_mask=*, the second is masked under its own name.postgres_branches.source_branch_timeis immutable, so a change recreates the branch and never touches update_mask. Before this fix it was invisible and the branch silently kept its old fork point.postgres_endpoints.suspend_timeout_duration,postgres_branches.ttl,postgres_branches.expire_timeandpostgres_projects.default_endpoint_settings.suspend_timeout_durationare members of a oneof: the plan is right, but applying it fails because the direct engine masks the field under its own name while the API only accepts the group name (spec.suspension,spec.expiration). That is a separate bug; the tests record the failure for the follow-up fix.The testserver now validates update_mask against the paths the real API accepts, so those failures reproduce locally rather than only on cloud, and it no longer drops
expire_timeon branch create/update or the two duration fields on project update — it was reporting changes that never took effect.Test added in #6375.