Repository navigation
Conversation
bajtos
changed the title
c29d5fd to
2a405da
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The panic recovery logic treats panic(nil) as “no panic” (since recover() returns nil for both), which can silently swallow a real panic and incorrectly mark the handler as successful.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR makes handler panics non-fatal to the request flow by recovering panics at the dispatcher boundary, converting them into short-lived HandlerExecutionError receipts, and logging the panic + stack via slog. It also threads a configurable logger from server.NewHTTP into the dispatcher so dependents can control where panic diagnostics go.
Changes:
- Add panic recovery around handler execution in
execution/dispatcher, log panic metadata + stack, and return an error receipt instead of letting the panic escape. - Introduce
dispatcher.WithLoggerandserver.WithLoggeroptions (defaulting toslog.Default()at the dispatcher) to control panic logging. - Add tests to assert receipt content/TTL, logging behavior, isolation from other invocations, and nil-logger panics.
| File | Description |
|---|---|
execution/dispatcher/dispatcher.go |
Wrap handler execution with panic recovery + logging and convert panics into handler execution error receipts. |
execution/dispatcher/options.go |
Add WithLogger(*slog.Logger) option and store it in dispatcher config. |
execution/dispatcher/panic_test.go |
New tests for panic-to-receipt behavior, TTL, logging fields/stack, and isolation. |
server/options.go |
Add server.WithLogger(*slog.Logger) to configure panic logging via the server. |
server/http.go |
Pass configured server logger through to dispatcher options. |
server/http_test.go |
Add server-level test ensuring configured logger receives the panic log line. |
notes.md |
Document the behavioral change: handler panics now yield error receipts and get logged. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a405da5a6
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| slog.Any("panic", value), | ||
| slog.String("stack", string(debug.Stack())), | ||
| ) | ||
| err = fmt.Errorf("handler panicked: %v", value) |
There was a problem hiding this comment.
When a handler panics with a multikey.Signer, its concrete value is a byte slice containing the private key, so slog.Any logs those bytes and the %v formatting embeds them in the error receipt returned to the caller. Detect signer values and report only a safe identifier such as the DID rather than exposing the panic value verbatim.
AGENTS.md reference: AGENTS.md:L147-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think it's highly unlikely for a handler to panic with multikey.Signer.
@alanshaw do you agree?
bajtos
changed the title
A panic message can carry anything the handler was holding, such as a payload it failed to parse or a path on the server, and none of it was written for the client. The receipt now says only that the handler panicked; the task CID in it is the key to the operator's log. Reporting the panic is the consumer's decision, so WithLogger on the dispatcher and the server is replaced by WithPanicLogger, a callback that receives the request and the recovered value inside the deferred recover, where the stack is still available. The default prints the panic and stack through the standard log package and runtime.Stack, as net/http does for the panics it recovers, so the module links neither log/slog nor runtime/debug into a consumer binary. Addresses review comments on #64. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
|
@codex review |
A panicking handler used to escape the dispatcher and rely on whatever ran above it: net/http logs the panic and drops the connection, so the client got no receipt at all. Once a request's invocations execute on their own goroutines (FIL-1261), the same panic would take down the whole process, and nothing outside ucantone can wrap those goroutines. Recover the panic around the handler call only, so validator and response-construction panics still propagate as bugs. The recovered panic follows the returned-error path: a HandlerExecutionError receipt with the short TTL and the message "handler panicked: <value>", no stack on the wire. The stack goes to a logger instead. Add dispatcher.WithLogger and a server.WithLogger passthrough, taking a *slog.Logger. The default is slog.Default(), which writes through the standard log package, so out of the box the output matches what net/http printed for the panics it recovered. This also changes the single-invocation path: a panic now yields a receipt instead of a dropped connection. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
recover() returns nil both when the handler returned and when it called panic(nil) under GODEBUG=panicnil=1, so the nil check treated that panic as success and handed back an empty response; on the batch path the nil receipt then crashed batch.NewResponse. A flag set after the handler returns is the panic indicator now. The wire format caps an error message at 8192 bytes, and a handler can panic with text of any size. The receipt carries the first MaxPanicMessageLength bytes of the panic value; the full value stays in the log. Addresses review comments on #64. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
A panic message can carry anything the handler was holding, such as a payload it failed to parse or a path on the server, and none of it was written for the client. The receipt now says only that the handler panicked; the task CID in it is the key to the operator's log. Reporting the panic is the consumer's decision, so WithLogger on the dispatcher and the server is replaced by WithPanicLogger, a callback that receives the request and the recovered value inside the deferred recover, where the stack is still available. The default prints the panic and stack through the standard log package and runtime.Stack, as net/http does for the panics it recovers, so the module links neither log/slog nor runtime/debug into a consumer binary. Addresses review comments on #64. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The default panic logger currently logs the recovered panic value with %v, which can leak sensitive data (e.g., private key bytes if a signer is panicked/logged).
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
82d938b to
d0c951d
Compare
Co-authored-by: ash <alan138@gmail.com>
Written by Claude.
A panicking handler used to escape the dispatcher and rely on whatever ran above it. Under net/http that meant a logged panic and a dropped connection, so the client got no receipt. Once a request's invocations execute on their own goroutines (the next PR in this stack), the same panic would take down the whole process, and nothing outside ucantone can wrap those goroutines.
The dispatcher now recovers a panic around the handler call only, so validator and response-construction panics still propagate as bugs. A recovered panic follows the returned-error path: a
HandlerExecutionErrorreceipt with the short TTL and the fixed messagehandler panicked. The panic value never reaches the wire, since it was not written for the client and can carry internals. The task CID in the receipt is the key to the operator's log.New option:
dispatcher.WithPanicLoggerand aserver.WithPanicLoggerpassthrough, taking aPanicLoggercallback that receives the request and the recovered value. The callback runs inside the deferred recover, soruntime.Stackordebug.Stackcalled from it returns the panicking goroutine's stack. The default prints the command, task, panic value and stack through the standardlogpackage andruntime.Stack, the way net/http reports the panics it recovers, so the module links neitherlog/slognorruntime/debuginto a consumer binary.Behaviour change for dependents: on the single-invocation path a panic now yields a receipt instead of a dropped connection.
Second PR in the stack for FIL-1261, on top of #63.
Tests: receipt name, message and TTL for string, error and
panic(nil)values, the callback's request and value, isolation from other invocations, nil-callback panics on both options, and the server passthrough. Run with the race detector.make cipasses.Stack created with GitHub Stacks CLI • Give Feedback 💬