Skip to content

feat(codec): WithoutPanicOnIOError - #166

Merged
hferentschik merged 1 commit into
mainfrom
mgallo/without-panic-on-io-error
Oct 6, 2026
Merged

hferentschik merged 1 commit into
mainfrom
mgallo/without-panic-on-io-error

Conversation

@marchmallow

@marchmallow marchmallow commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Motivation & Context

  • Goal: a caller outside Temporal worker code — an API handler, a standalone client, or a goroutine the Temporal SDK does not manage — can get an LPS IO error back instead of having it panic and crash the process.
  • Motivation: the codec panics on every IO error so that, inside a Temporal workflow, the SDK's WorkflowPanicPolicy can retry the workflow task without risking replay non-determinism (feat: Coded now panics on IO error #97). That same panic fires for calls outside workflow code, where nothing catches it. Reported against a Rapid API wrapping client.GetWorkflow(...).Get(): DataDog/dd-source Slack thread (internal).
  • Why now: builds on codec: add WithReturnErrorOnIOError option for non-worker callers #130 (@NielkSalocin), open since June with no reviews, which already proposed this option under the name WithReturnErrorOnIOError. This PR keeps that commit and adds the review round it was waiting on, renamed to WithoutPanicOnIOError to match the name already in use by the consuming change (ddoghq/dd-source#114498, soon paired with a per-call-context variant on top of it).
  • Follow-up: a dd-source PR wires this into Atlas's Temporal client so it's selected automatically per call (workflow code still panics; everything else returns the error) — opened alongside this one.
Observable behavior Before After
IO error inside workflow code Panic Unchanged: panic
IO error in an API handler, standalone client, or unmanaged goroutine Panic (crashes the process) *IOError returned, with WithoutPanicOnIOError()
Panic value / error message error from fmt.Errorf("large payload codec IO error: %v", cause) *IOError (implements error, Unwrap() error); same message text
Response body on an IO error Never closed (leaked the connection; a panic masked this by killing the process) Closed on every path

Interface Changes

  • New: WithoutPanicOnIOError() codec option (default behavior, panicking, is unchanged).
  • New: exported *IOError type. It is both the panic value and the returned error, so errors.As now reaches the underlying cause (a *url.Error, a status-code mismatch, …) that %v-formatting previously hid.
  • Unchanged: every other exported API, and the panic message text, so existing log-based monitors keep matching.

Validation

  • go build, go vet, and go test pass in codec/.
  • New tests: WithoutPanicOnIOError returns a *IOError (errors.As) on both encode and decode IO errors; the existing panic tests now assert the panic value's concrete type too.
  • New test: the response body is closed on an IO error, for both encode and decode. It uses a fake RoundTripper that returns a canned response with no real network connection underneath — an httptest.Server-backed version of this test passed even with the fix reverted, because Go's net/http.Transport closes idle response bodies of its own accord in the background, independent of application code; the fake transport removes that confound.

Risks

  • *IOError's Unwrap() is new; a caller somewhere could be relying on errors.Is/errors.As not reaching the cause. None are known (dd-source string-matches the panic message, which this preserves), but this is a public module.
  • Response bodies are now closed on every path, including ones that used to panic before reaching the close. No case changes behavior other than returning the connection to the pool instead of leaking it.

@marchmallow
marchmallow force-pushed the mgallo/without-panic-on-io-error branch 2 times, most recently from 0b18eba to 988d45e Compare October 1, 2026 15:23
@marchmallow
marchmallow marked this pull request as ready for review October 1, 2026 15:28
By default the codec panics on IO errors so the Temporal worker's
WorkflowPanicPolicy can handle them, avoiding non-determinism issues
inside workflow functions.

Callers outside a Temporal worker context (API handlers, goroutines not
managed by the SDK, activities that fetch a prior workflow result via
client.GetWorkflow().Get()) have no such safety net — a panic there
crashes the process rather than failing a single task.

WithoutPanicOnIOError() makes the codec return errors instead of
panicking in those contexts, allowing the caller to handle transient
LPS IO failures gracefully.

The existing default (panic) behaviour is unchanged.
@hferentschik
hferentschik force-pushed the mgallo/without-panic-on-io-error branch from 988d45e to 00e9f36 Compare October 6, 2026 09:28
@hferentschik hferentschik changed the title codec: WithoutPanicOnIOError, built on #130 feat(codec): WithoutPanicOnIOError, built on #130 Oct 6, 2026
@hferentschik hferentschik changed the title feat(codec): WithoutPanicOnIOError, built on #130 feat(codec): WithoutPanicOnIOError Oct 6, 2026

@hferentschik hferentschik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think the need for a panic vs error per call is real. In fact I think the better solution is the one here.

@hferentschik
hferentschik merged commit 00e9f36 into main Oct 6, 2026
3 checks passed
@hferentschik
hferentschik deleted the mgallo/without-panic-on-io-error branch October 6, 2026 12:18
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.

3 participants