Skip to content

fix: return codes.Internal on recovered panic instead of nil error - #46

Open
janwytze wants to merge 1 commit into
johnbellone:mainfrom
janwytze:fix/recover-panic-as-internal-error
Open

fix: return codes.Internal on recovered panic instead of nil error#46
janwytze wants to merge 1 commit into
johnbellone:mainfrom
janwytze:fix/recover-panic-as-internal-error

Conversation

@janwytze

Copy link
Copy Markdown
Contributor

Problem

When Repanic is disabled (the default), recoverWithSentry recovers a handler panic and reports it to Sentry, but the server interceptors use unnamed return values, so after recovery the function falls through to its zero value — a nil error. gRPC then treats the RPC as successful:

  • Streaming handlers return OK with a missing/partial response (silent data loss; metrics count it as success).
  • Unary handlers return a nil response that fails to marshal, surfacing as an opaque Internal error that masks the real panic.

In all cases nothing distinguishes a panicked RPC from a healthy one at the gRPC layer.

Fix

  • Give UnaryServerInterceptor and StreamServerInterceptor named returns.
  • Have recoverWithSentry translate a recovered panic into status.Errorf(codes.Internal, "%v", err) (and mark the transaction SpanStatusInternalError) on the non-repanic path.
  • Repanic: true behaviour is unchanged; normal handler errors pass through unchanged.

Tests

Adds tests that actually invoke the interceptors (the existing tests only cover construction):

  • panic → recovered as codes.Internal (unary + stream)
  • WithRepanicOption(true) → panic propagates (unary + stream)
  • normal error passes through unchanged (unary + stream)

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

When Repanic is disabled (the default), recoverWithSentry recovered the
panic and reported it to Sentry, but the interceptor's unnamed return
values fell through to their zero value — a nil error. gRPC then treated
the RPC as successful: streaming handlers returned OK with a missing or
partial response, and unary handlers returned a nil response that fails
to marshal with an opaque "Internal" error, masking the real panic.

Give both server interceptors named returns and have recoverWithSentry
translate a recovered panic into a status.Errorf(codes.Internal, ...)
error (and mark the transaction as failed) on the non-repanic path.
Repanic=true behaviour is unchanged.

Add tests covering panic recovery, the repanic option, and normal-error
pass-through for both unary and stream interceptors.
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.

1 participant