Skip to content

Preserve delayed PortableValue raw JSON on checkpoint re-serialization to avoid large-int/type-fidelity loss - #696

Open
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:preserve-delayed-portablevalue-raw-json
Open

Preserve delayed PortableValue raw JSON on checkpoint re-serialization to avoid large-int/type-fidelity loss#696
PratikDhanave wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:preserve-delayed-portablevalue-raw-json

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

PortableValue.MarshalJSON called Any() before checking whether the value was still in its delayed json.RawMessage form. For a delayed value whose TypeID is unregistered (or whose content is any-typed/map-shaped), decodeKnownPortableType returns false and Any() falls back to a generic json.Unmarshal into any, yielding map[string]any + float64. That decoded value is never a json.RawMessage, so the existing raw-preserving branch was dead and MarshalJSON re-encoded lossily.

This makes a durable-JSON restore -> re-checkpoint cycle non-idempotent: an untouched delayed value is re-emitted with different bytes, and any int64/uint64 above 2^53 is truncated through float64.

The fix short-circuits on the retained json.RawMessage (when no deserialized cache exists) before calling Any(), re-emitting the original wire bytes verbatim.

Why (parity)

This matches .NET, which retains the incoming JsonElement and re-emits it verbatim on re-serialization rather than round-tripping through a decoded numeric/object representation. The Go delayed-deserialization path is meant to have the same "carry the bytes until someone reads them" semantics; before this change the marshal step broke that.

Tests

Added TestPortableValueMarshal_PreservesUntouchedDelayedRawJSON in workflow/portable_test.go: it builds wire JSON for a delayed value with an unregistered TypeID and a large integer (9007219254740993, > 2^53), unmarshals into a PortableValue (asserting Delayed()), re-marshals it without any typed access, and asserts the re-emitted Value bytes are byte-identical to the original. Pre-fix it truncates to 9007219254740992; post-fix it is byte-identical.

go build ./..., go vet ./workflow/..., and go test ./workflow/... all pass.

Copilot AI review requested due to automatic review settings July 24, 2026 02:10
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 24, 2026 02:10

Copilot AI 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.

Pull request overview

This PR fixes workflow.PortableValue JSON re-serialization for delayed values by preserving the original json.RawMessage bytes when the value was never read/deserialized, preventing precision/type-fidelity loss (notably for integers > 2^53) during a restore → re-checkpoint cycle.

Changes:

  • Short-circuit PortableValue.MarshalJSON to re-emit the retained json.RawMessage verbatim when v.any is raw JSON and v.cache is still unset.
  • Add a regression test covering an unregistered TypeID with a large integer to ensure re-marshaling preserves the original numeric bytes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
workflow/portable.go Preserves untouched delayed raw JSON during MarshalJSON to avoid lossy generic decoding.
workflow/portable_test.go Adds a regression test ensuring large-integer fidelity is preserved for delayed/unregistered values.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PratikDhanave
PratikDhanave force-pushed the preserve-delayed-portablevalue-raw-json branch from 039df30 to ebb9e55 Compare July 24, 2026 02:14
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 24, 2026
MarshalJSON called Any() before checking for the delayed json.RawMessage
form, so Any() generically decoded the raw bytes into map[string]any /
float64 and the raw-preserving branch was dead. Re-marshaling an untouched
delayed value therefore lost type/large-integer fidelity, making a
durable-JSON restore -> re-checkpoint cycle non-idempotent (int64/uint64
above 2^53 truncated through float64).

Short-circuit on the retained json.RawMessage before decoding so the
original wire bytes are re-emitted verbatim, matching .NET which re-emits
the retained JsonElement.
@PratikDhanave
PratikDhanave force-pushed the preserve-delayed-portablevalue-raw-json branch from ebb9e55 to 068298e Compare July 24, 2026 09:34
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review: ✅ Approved

Scope: workflow/portable.goPortableValue.MarshalJSON behavioral fix + portable_test.go new test.

Changed exported surface: None. This PR fixes the existing MarshalJSON serialization path; no new exported types, functions, or methods are introduced.

Cross-repo parity: The fix explicitly aligns Go with .NET behavior. The .NET implementation retains the incoming JsonElement and re-emits it verbatim on re-serialization rather than round-tripping through a decoded representation. This PR brings Go to the same "carry the bytes until someone reads them" semantic for delayed PortableValues.

No upstream Python equivalent was found for PortableValue specifically (it is a Go/.NET workflow-checkpointing construct), so no Python parity gap applies.

Label action: No public-api-change label needed — no exported Go API surface changed. The parity-approved label already present remains correct.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 41.5 AIC · ⌖ 5.55 AIC · ⊞ 5.9K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants