fix: recover panics from validation code by bajtos · Pull Request #67 · fil-forge/ucantone

bajtos · 2026-09-22T14:51:52Z

Written by Claude.

The dispatcher recovered panics only around the handler call. Validation runs application code as well: a DID resolver from WithDIDResolver, a verifier factory, a nonstandard-signature verifier. A panic there escaped Execute. Under net/http that dropped the connection; once the server runs each invocation of a request on its own goroutine (#66), it would crash the process.

Execute now wraps its whole body in a recovery boundary, which also covers the single-invocation server.Execute path. The recover around the handler call from #64 is gone: the handler runs inside that boundary, so it was redundant. Every panic fails its task with a new ExecutionFailure receipt, with the same short TTL as a returned handler error, and the panic value goes to the PanicLogger. The receipt does not say a panic happened, let alone where. A client cannot act on that distinction, and debugging goes through the log line anyway. The new error name is additive on the wire.

Fourth PR in the stack for FIL-1261, between #64 and #66. Found by Codex's review of #66.

Tests: the handler-panic tests from #64 now assert the ExecutionFailure receipt, which proves the outer boundary covers them. A dispatcher test with a panicking DID resolver checks the receipt, its TTL and the logger call. A server test runs a batch where validation panics for one issuer and the other still gets its receipt. make ci passed except the examples package, which cannot bind a TCP port in the sandbox.

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

The dispatcher recovered panics only around the handler call. The audience check, handler lookup and validation ran outside that boundary, and validation runs application code as well: a DID resolver from WithDIDResolver, a verifier factory, a nonstandard signature verifier. A panic there escaped Execute. Under net/http that dropped the connection; once the server runs each invocation of a request on its own goroutine, it would crash the process. Execute now wraps its whole body in a recovery boundary. The handler boundary stays where it was, so a handler panic keeps its HandlerExecutionError receipt. A panic anywhere else fails the task with a new ExecutionPanicError receipt, with the same short TTL and reported through the same PanicLogger. The name differs because clients switch on it and "handler" would be wrong for a resolver. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

The guard handler captured the parent test's t inside subtests, and t.Fatal from a subtest panics with a confusing message instead of reporting the failure. Pass the subtest's t and use t.Error. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

chatgpt-codex-connector · 2026-09-22T14:52:06Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-22T14:56:29.117193Z	`847ec3a`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot

Copilot review overview

🟢 Approval recommended

The change is narrowly scoped, preserves the handler-panic behavior, and adds targeted tests that cover the newly recovered validation-panic cases end-to-end.

Review effort: Lite
Findings: None

What changed in this PR

This PR hardens ucantone’s execution pipeline by ensuring panics that occur outside handlers (notably in validator extension points like DID resolvers and verifier factories) are recovered into short-lived failure receipts, preventing dropped HTTP connections and avoiding process crashes once per-request invocations run concurrently.

Changes:

Wraps the full Dispatcher.Execute body in a recovery boundary; non-handler panics now yield an ExecutionPanicError receipt (short TTL) and are reported via the existing PanicLogger.
Introduces execution.ExecutionPanicErrorName + NewExecutionPanicError for the new receipt failure name/message.
Adds regression tests covering panics during validation both in dispatcher-only execution and via the HTTP server batch path.

File	Description
server/options.go	Updates `WithPanicLogger` docs to reflect that the logger now receives panics from validation as well as handlers.
server/http_test.go	Adds an integration test ensuring a validation panic fails only the affected task and other batch invocations still complete.
notes.md	Documents the broader recovery boundary and the distinction between handler vs non-handler panic receipts.
execution/errors.go	Adds `ExecutionPanicErrorName` and `NewExecutionPanicError` for panics outside handler execution.
execution/dispatcher/validation_panic_test.go	New unit test verifying receipt shape, TTL behavior, and panic logger invocation for validation panics.
execution/dispatcher/options.go	Updates `PanicLogger`/`WithPanicLogger` documentation to cover validation panics and the new error type.
execution/dispatcher/dispatcher.go	Implements the full `Execute` recovery boundary and refactors short-lived failure receipt construction into `transientFailure`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

bajtos · 2026-09-22T15:08:22Z

+// decided the outcome by then: a handler panic fails the task with the receipt
+// [execution.NewHandlerExecutionError] builds, whose message says only that the
+// handler panicked, and any other panic with the one
+// [execution.NewExecutionPanicError] builds. The logger runs on the panicking


I am not sure if there is much value in distinguishing between panics in the handler vs elsewhere. Maybe we should keep only ExecutionPanicError caught at the outer boundary. WDYT?

I don't think it matters much to the caller. Neither for a developer, since debugging is going to involve finding the log line since there's no details in the error response.

IDK if we should even give away that this was a panic. This could just be surfaced as a generic ExecutionFailure.

Yeah, I was thinking the same - we should not reveal that this was a panic, just respond with a generic ExecutionFailure.

Done. There is now a single recovery boundary in Execute, which handles both handler and validation panics. Any panic is turned into the same ExecutionFailure receipt, and it no longer mentions that a panic happened.

alanshaw

Happy for this to merge but please consider the feedback.

alanshaw · 2026-09-23T10:30:51Z

 	}
 }

+const ExecutionPanicErrorName = "ExecutionPanicError"


We don't usually suffix the error name with "Error" (there are exceptions).

Suggested change

const ExecutionPanicErrorName = "ExecutionPanicError"

const ExecutionPanicErrorName = "ExecutionPanic"

Let's use ExecutionFailure per #67 (comment)

Renamed to ExecutionFailure. The Go identifiers keep the …ErrorName / New…Error shape the file already uses for InvalidAudience.

alanshaw · 2026-09-23T10:39:19Z

+// decided the outcome by then: a handler panic fails the task with the receipt
+// [execution.NewHandlerExecutionError] builds, whose message says only that the
+// handler panicked, and any other panic with the one
+// [execution.NewExecutionPanicError] builds. The logger runs on the panicking


I don't think it matters much to the caller. Neither for a developer, since debugging is going to involve finding the log line since there's no details in the error response.

IDK if we should even give away that this was a panic. This could just be surfaced as a generic ExecutionFailure.

Co-authored-by: ash <alan138@gmail.com>

Review of #67 questioned the value of telling a handler panic apart from one in validation code. A client cannot act on the distinction, and a developer debugging it needs the server log line either way, so the receipt should not even say that a panic happened. - Rename `ExecutionPanicError` to `ExecutionFailure`, matching the newer error names that carry no "Error" suffix, and drop "panicked" from its message. - Remove the recover boundary around the handler call. The handler runs on the calling goroutine inside `Execute`, so the boundary there already catches every panic the inner one did. A handler panic now gets the same `ExecutionFailure` receipt as any other. - Merge the two notes.md entries into one that records the decision. `HandlerExecutionError` still reports errors a handler returns. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

bajtos added 2 commits September 22, 2026 16:30

Copilot AI lite review requested due to automatic review settings September 22, 2026 14:51

bajtos added this pull request to stack #68 September 22, 2026 14:52

Copilot started reviewing on behalf of bajtos September 22, 2026 14:53 View session

bajtos mentioned this pull request Sep 22, 2026

feat: execute a request's invocations concurrently #66

Merged

Copilot AI reviewed Sep 22, 2026

View reviewed changes

bajtos commented Sep 22, 2026

View reviewed changes

bajtos requested a review from alanshaw September 22, 2026 15:08

alanshaw approved these changes Sep 23, 2026

View reviewed changes

bajtos and others added 2 commits September 23, 2026 15:15

Update execution/dispatcher/options.go

9cd74ef

Co-authored-by: ash <alan138@gmail.com>

bajtos merged commit 7eea01e into main Sep 23, 2026
6 checks passed

bajtos deleted the miroslav/fil-1261-recover-validation-panics-v2 branch September 23, 2026 13:43

Navigation Menu

Repository navigation

fix: recover panics from validation code - #67

fix: recover panics from validation code#67
bajtos merged 4 commits into
miroslav/fil-1261-recover-handler-panicsfrom
miroslav/fil-1261-recover-validation-panics-v2

bajtos commented Sep 22, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

bajtos Sep 22, 2026

Uh oh!

alanshaw Sep 23, 2026

Uh oh!

bajtos Sep 23, 2026

Uh oh!

bajtos Sep 23, 2026

Uh oh!

alanshaw left a comment

Uh oh!

Uh oh!

alanshaw Sep 23, 2026

Uh oh!

bajtos Sep 23, 2026

Uh oh!

bajtos Sep 23, 2026

Uh oh!

alanshaw Sep 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const ExecutionPanicErrorName = "ExecutionPanicError"
	const ExecutionPanicErrorName = "ExecutionPanic"

Conversation

bajtos commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanshaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bajtos commented Sep 22, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 22, 2026 •

edited

Loading