sneak/simplelog
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases 2 Wiki Activity

JSONHandler.Handle returns nil even when the write to stdout failed #22

New Issue
Closed
opened 2026-08-10 14:50:43 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-10 14:50:43 +02:00
Collaborator
Copy Link
Copy Source

Found during the adversarial review of
#21
(#21 (comment)). Pre-existing,
not introduced by that PR.

JSONHandler.Handle discards the error from its write to stdout and returns
nil unconditionally. slog.Handler.Handle is declared to return an error
precisely so a failing sink can be reported; returning nil tells log/slog
the record was delivered when it was not.

This surfaced in that review as a lint question — the line is written
_, _ = fmt.Fprintln(...), which a newer golangci-lint flags and the repo's
pinned linter does not. Silencing the linter is not the fix and would bury the
defect. The defect is that a log line can vanish with nothing reporting it.

Definition of done

  • A failed write to the sink is returned from Handle rather than discarded, in
    every handler that writes to a sink, not only JSONHandler.
  • The behaviour is decided deliberately for the multiplex case: one child
    failing must not silently suppress delivery to its siblings, and the caller
    must still learn that something failed.
  • A test drives a handler whose sink returns an error and asserts the error
    reaches the caller. This requires the sink to be injectable; if it is not
    today, make it so rather than skipping the test.
  • Whatever is decided is written down in the README alongside the handler
    descriptions, since "logging can fail and here is how you find out" is
    caller-facing behaviour.
  • The repo's full check is green.

Implementation requirements

  • Do not paper over this by writing to a buffer that cannot fail.
  • Do not reintroduce any path back into the stdlib log package: that is the
    v1.0.0 deadlock in #18, and
    TestJSONHandlerDeadlock guards it.
  • Landing commit title must end with (closes #<this issue>).
Found during the adversarial review of https://git.eeqj.de/sneak/simplelog/pulls/21 (https://git.eeqj.de/sneak/simplelog/pulls/21#issuecomment-53518). Pre-existing, not introduced by that PR. `JSONHandler.Handle` discards the error from its write to stdout and returns `nil` unconditionally. `slog.Handler.Handle` is declared to return an `error` precisely so a failing sink can be reported; returning `nil` tells `log/slog` the record was delivered when it was not. This surfaced in that review as a lint question — the line is written `_, _ = fmt.Fprintln(...)`, which a newer golangci-lint flags and the repo's pinned linter does not. Silencing the linter is not the fix and would bury the defect. The defect is that a log line can vanish with nothing reporting it. ## Definition of done - A failed write to the sink is returned from `Handle` rather than discarded, in every handler that writes to a sink, not only `JSONHandler`. - The behaviour is decided deliberately for the multiplex case: one child failing must not silently suppress delivery to its siblings, and the caller must still learn that something failed. - A test drives a handler whose sink returns an error and asserts the error reaches the caller. This requires the sink to be injectable; if it is not today, make it so rather than skipping the test. - Whatever is decided is written down in the README alongside the handler descriptions, since &quot;logging can fail and here is how you find out&quot; is caller-facing behaviour. - The repo&#39;s full check is green. ## Implementation requirements - Do not paper over this by writing to a buffer that cannot fail. - Do not reintroduce any path back into the stdlib `log` package: that is the `v1.0.0` deadlock in https://git.eeqj.de/sneak/simplelog/issues/18, and `TestJSONHandlerDeadlock` guards it. - Landing commit title must end with ` (closes #<this issue>)`.
clawbot commented 2026-10-03 14:38:20 +02:00
Author
Collaborator
Copy Link
Copy Source

State, 2026-10-03 12:40 UTC: queued, not started. Waits on #16: the handlers changed on main (slog attribute fix) and next does not have that yet, so this unit branches from next after that lands. It touches handler code, tests and README.md only, so it may run in parallel with #20. The issue body is the brief.

Model: opus-5-5

State, 2026-10-03 12:40 UTC: queued, not started. Waits on https://git.eeqj.de/sneak/simplelog/issues/16: the handlers changed on `main` (slog attribute fix) and `next` does not have that yet, so this unit branches from `next` after that lands. It touches handler code, tests and `README.md` only, so it may run in parallel with https://git.eeqj.de/sneak/simplelog/issues/20. The issue body is the brief. Model: opus-5-5
clawbot commented 2026-10-06 01:19:59 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan. Starts from next once #16 has landed, because it needs the handler code from main.

  1. JSONHandler and ConsoleHandler each get an unexported out io.Writer field. The constructors set it to os.Stdout and keep their signatures, and WithAttrs/WithGroup carry it over to the new handler. Handle returns the error from the write (wrapped with %w) instead of throwing it away. Judgement call: the field stays unexported, so tests in the package can supply a failing sink without adding public API. A public way to choose the output would be new design and is not part of this unit.
  2. WebhookHandler.Handle already returns an error when the request fails. It now also returns one when the server answers with a status outside 2xx, since the record was not accepted. Judgement call, in line with this issue: otherwise the line is lost and nothing reports it.
  3. MultiplexHandler.Handle calls every child even when one fails, and returns all the failures joined with errors.Join (nil when none failed). Its doc comment stops saying it stops at the first error.
  4. Tests: a writer that always fails. JSONHandler and ConsoleHandler each return an error that errors.Is matches. A multiplex of a failing child and a recording child still delivers to the second and returns the first one's error. A webhook pointed at an httptest server that answers 500 returns an error.
  5. README.md, next to the handler descriptions: what each handler returns when its output fails, what the multiplex does, and that the slog.Logger methods (slog.Info and the rest) discard a handler's error, so a caller who needs to know calls the handler's Handle directly. That last part is how log/slog works, not a choice made here. The README says it so nobody assumes the error reaches them.
  6. No path back into the stdlib log package. TestJSONHandlerDeadlock stays.

Model: opus-5-5

Plan. Starts from `next` once https://git.eeqj.de/sneak/simplelog/issues/16 has landed, because it needs the handler code from `main`. 1. `JSONHandler` and `ConsoleHandler` each get an unexported `out io.Writer` field. The constructors set it to `os.Stdout` and keep their signatures, and `WithAttrs`/`WithGroup` carry it over to the new handler. `Handle` returns the error from the write (wrapped with `%w`) instead of throwing it away. Judgement call: the field stays unexported, so tests in the package can supply a failing sink without adding public API. A public way to choose the output would be new design and is not part of this unit. 2. `WebhookHandler.Handle` already returns an error when the request fails. It now also returns one when the server answers with a status outside 2xx, since the record was not accepted. Judgement call, in line with this issue: otherwise the line is lost and nothing reports it. 3. `MultiplexHandler.Handle` calls every child even when one fails, and returns all the failures joined with `errors.Join` (nil when none failed). Its doc comment stops saying it stops at the first error. 4. Tests: a writer that always fails. `JSONHandler` and `ConsoleHandler` each return an error that `errors.Is` matches. A multiplex of a failing child and a recording child still delivers to the second and returns the first one's error. A webhook pointed at an `httptest` server that answers 500 returns an error. 5. `README.md`, next to the handler descriptions: what each handler returns when its output fails, what the multiplex does, and that the `slog.Logger` methods (`slog.Info` and the rest) discard a handler's error, so a caller who needs to know calls the handler's `Handle` directly. That last part is how `log/slog` works, not a choice made here. The README says it so nobody assumes the error reaches them. 6. No path back into the stdlib `log` package. `TestJSONHandlerDeadlock` stays. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bot
Agent's turn to work
 critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/simplelog#22