Skip to content

app/log: fix slog handler panic on named types - #4640

Merged
KaloyanTanev merged 3 commits into
mainfrom
kalo/fix-slog-panic
Aug 11, 2026
Merged

app/log: fix slog handler panic on named types#4640
KaloyanTanev merged 3 commits into
mainfrom
kalo/fix-slog-panic

Conversation

@KaloyanTanev

Copy link
Copy Markdown
Collaborator

Stringify all slog values via fmt.Sprint instead of using zapcore.ReflectType, which panics in the logfmt encoder on named types like protocol.ID.
Add a recover guard in Handle so future encoding panics drop the log line instead of crashing the process.

category: bug
ticket: none

Stringify all slog values via fmt.Sprint instead of using
zapcore.ReflectType, which panics in the logfmt encoder on
named types like protocol.ID. Add a recover guard in Handle
so future encoding panics drop the log line instead of
crashing the process.
@KaloyanTanev
KaloyanTanev requested review from pinebit and a lite review from Copilot August 11, 2026 09:16
@KaloyanTanev KaloyanTanev self-assigned this Aug 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a crash-path in the app/log slog→zap bridge used for go-libp2p logs by avoiding zapcore.ReflectType (which can panic under the logfmt encoder for named types) and adding a defensive recover() to prevent logging panics from crashing the process.

Changes:

  • Convert slog.Attr values to zap string fields to avoid logfmt panics on reflected named types.
  • Add a recover() guard in slogHandler.Handle to drop the log line instead of terminating the process.
  • Add a regression test covering named-string slices and primitive values under logfmt output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
app/log/slog.go Adds panic recovery in Handle and changes attribute conversion to avoid ReflectType panics.
app/log/slog_internal_test.go Adds regression test ensuring named types and primitive values don’t panic under logfmt encoding.

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

Comment thread app/log/slog.go
Comment thread app/log/slog.go
Comment thread app/log/slog_internal_test.go Outdated
Comment thread app/log/slog_internal_test.go
Route the recover output through Error() instead of raw
stderr so it appears in Loki and structured log output.
Also fix test comment accuracy and add bool assertion.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

app/log/slog.go:3

  • The package-level //nolint comment is an incomplete sentence (ends with "while it is"), which makes it hard to understand why these linters are being disabled. Please finish the rationale so future readers know what’s intended.
//nolint:revive,nolintlint // somehow the nolintlint linter catches revive as unnecessary, while it is

Comment thread app/log/slog.go
Wrap the Error() call in the recover handler with its own
defer/recover so that if the structured logger itself panics
we fall back to stderr instead of crashing.
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

app/log/slog.go:3

  • The file-level nolint comment is incomplete (“while it is”) and doesn’t explain the suppression clearly. Please either complete the sentence or reword it so future readers understand why these linters are disabled here.
//nolint:revive,nolintlint // somehow the nolintlint linter catches revive as unnecessary, while it is

app/log/slog.go:63

  • The panic guard currently ignores the incoming context (uses context.Background), which drops trace/span context, and the nested recovery prints the original panic value instead of the secondary panic (r2), making diagnosis harder. Use the handler’s ctx for Error(...) and include both panic values in the stderr fallback.
func (h *slogHandler) Handle(_ context.Context, rec slog.Record) error {
	// Never let a logging panic crash the process.
	defer func() {
		if r := recover(); r != nil {
			defer func() {

@KaloyanTanev
KaloyanTanev enabled auto-merge (squash) August 11, 2026 09:50
@KaloyanTanev
KaloyanTanev merged commit caefc2e into main Aug 11, 2026
11 checks passed
@KaloyanTanev
KaloyanTanev deleted the kalo/fix-slog-panic branch August 11, 2026 09:53
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.19%. Comparing base (aea5631) to head (456ce87).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
app/log/slog.go 25.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4640      +/-   ##
==========================================
+ Coverage   58.16%   58.19%   +0.03%     
==========================================
  Files         247      247              
  Lines       34056    34063       +7     
==========================================
+ Hits        19807    19823      +16     
+ Misses      11779    11767      -12     
- Partials     2470     2473       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

KaloyanTanev added a commit that referenced this pull request Aug 11, 2026
* app/log: fix slog handler panic on named types

Stringify all slog values via fmt.Sprint instead of using
zapcore.ReflectType, which panics in the logfmt encoder on
named types like protocol.ID. Add a recover guard in Handle
so future encoding panics drop the log line instead of
crashing the process.

* app/log: log slog handler panics through charon logger

Route the recover output through Error() instead of raw
stderr so it appears in Loki and structured log output.
Also fix test comment accuracy and add bool assertion.

* app/log: add nested recover for slog panic logging

Wrap the Error() call in the recover handler with its own
defer/recover so that if the structured logger itself panics
we fall back to stderr instead of crashing.
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