[dotnet-port-fixes] Prevent workflow telemetry serialization panics - #929
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the workflow observability/telemetry path so that JSON serialization failures (including panic from custom MarshalJSON) do not abort workflow execution. It aligns the Go port with the existing fallback marker behavior ([Unserializable: ...]) and adds regression tests covering both the low-level observability helpers and end-to-end in-process workflow runs.
Changes:
- Added a
recover-based fallback in internal telemetry serialization to handlepanicduringjson.Marshal. - Added unit tests to verify fallback behavior for both marshal errors and marshal panics in observability helpers.
- Added an in-process workflow regression test ensuring panicking sensitive-data serialization does not fail workflow execution and telemetry attributes still emit fallback strings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| workflow/internal/observability/observability.go | Wraps telemetry serialization with panic recovery and reuses a shared fallback formatter. |
| workflow/internal/observability/observability_test.go | Adds regression tests for fallback behavior when MarshalJSON panics (and for executor input/output tags). |
| workflow/inproc/observability_test.go | Adds an end-to-end inproc workflow test ensuring panicking payload serialization does not fail workflow execution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Parity Review — PR #929Scope: internal-only (no exported API changes) Changed Go contract: None. All changes are in Upstream evidence reviewed:
Result: ✅ Aligned — the Go change faithfully ports the upstream .NET fix. The upstream broadened exception catching to cover non-
|
Summary
Prevent workflow telemetry serialization from failing execution when sensitive-data attributes hit panic-style JSON serialization failures. The Go port now falls back to the existing
[Unserializable: ...]marker for both ordinary marshal errors and panics, and adds regression coverage for direct observability helpers and in-process workflow execution.Ported .NET PRs
8a0731ad92560d7eb5c181682d9501a82a6b4a70.Breaking Changes
No. This keeps the existing public Go API and only hardens internal telemetry serialization so workflow execution continues when telemetry-only serialization panics.
Tests and Examples
go test ./workflow/internal/observability ./workflow/inprocNotes
This is the remaining narrow parity gap after earlier observability fallback coverage: Go already handled
json.Marshalerror returns, but customMarshalJSONpanics could still escape through telemetry serialization.Closes #909