Fixes #38: a webhook server that accepts the connection and never answers no longer stops every log call.
The webhook handler's own client, added in #35, now has a timeout. A request still running after 5 seconds, reading the answer included, fails, and Handle returns a timeout error.
Handle reads the answer to the end before closing it, so the client reuses the connection. If that read fails, Handle returns its error wrapped as error reading webhook answer: ....
Two new tests point the handler at an httptest server that never answers and at one that sends its status and then stalls the answer, and check that Handle returns a timeout error. Each runs Handle in a goroutine and fails if it has not returned within webhookTimeout plus a second.
The README's "When delivery fails" section states the timeout.
Worth knowing: each new test takes the full 5 seconds.
Judgement call: the timeout is 5 seconds, a fixed constant (webhookTimeout), not a setting. A dead server holds up each log call for that long.
Judgement call: the answer is read to the end with no size limit; the timeout bounds how long that can take.
Judgement call: when the answer cannot be read, Handle returns the read error even if the status was outside 2xx.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/simplelog/issues/38: a webhook server that accepts the connection and never answers no longer stops every log call.
- The webhook handler's own client, added in https://git.eeqj.de/sneak/simplelog/pulls/35, now has a timeout. A request still running after 5 seconds, reading the answer included, fails, and `Handle` returns a timeout error.
- `Handle` reads the answer to the end before closing it, so the client reuses the connection. If that read fails, `Handle` returns its error wrapped as `error reading webhook answer: ...`.
- Two new tests point the handler at an `httptest` server that never answers and at one that sends its status and then stalls the answer, and check that `Handle` returns a timeout error. Each runs `Handle` in a goroutine and fails if it has not returned within `webhookTimeout` plus a second.
- The README's "When delivery fails" section states the timeout.
Worth knowing: each new test takes the full 5 seconds.
Judgement call: the timeout is 5 seconds, a fixed constant (`webhookTimeout`), not a setting. A dead server holds up each log call for that long.
Judgement call: the answer is read to the end with no size limit; the timeout bounds how long that can take.
Judgement call: when the answer cannot be read, `Handle` returns the read error even if the status was outside 2xx.
Model: opus-5-5
webhook_handler.go, Handle: when the server answers with a 2xx status and then never finishes the answer body, the read of the body is cut off at the timeout, its error is thrown away, and Handle returns nil after holding the log call for 5 seconds. That misses the issue's definition of done ("Handle returns an error that says so") and contradicts the Handle doc comment, the README line "A request that has not finished after 5 seconds fails with a timeout error", the commit message and the PR body. Acceptable: when reading the answer fails, Handle returns that error, and a test covers a server that sends its status and then stalls the body.
handler_errors_internal_test.go, TestWebhookHandlerTimesOutOnServerThatNeverAnswers: the test calls Handle with no limit of its own. If the timeout is lost, the test does not fail; it hangs the whole test run until the go test default limit of 10 minutes, and a much larger timeout would still pass. The issue asks the test to show Handle returning within the timeout. Acceptable: the pattern TestJSONHandlerDeadlock and the webhook tests in attrs_test.go already use, running Handle in a goroutine and failing if it has not returned within webhookTimeout plus a small margin.
Model: opus-5-5
1. `webhook_handler.go`, `Handle`: when the server answers with a 2xx status and then never finishes the answer body, the read of the body is cut off at the timeout, its error is thrown away, and `Handle` returns nil after holding the log call for 5 seconds. That misses the issue's definition of done ("`Handle` returns an error that says so") and contradicts the `Handle` doc comment, the README line "A request that has not finished after 5 seconds fails with a timeout error", the commit message and the PR body. Acceptable: when reading the answer fails, `Handle` returns that error, and a test covers a server that sends its status and then stalls the body.
2. `handler_errors_internal_test.go`, `TestWebhookHandlerTimesOutOnServerThatNeverAnswers`: the test calls `Handle` with no limit of its own. If the timeout is lost, the test does not fail; it hangs the whole test run until the `go test` default limit of 10 minutes, and a much larger timeout would still pass. The issue asks the test to show `Handle` returning within the timeout. Acceptable: the pattern `TestJSONHandlerDeadlock` and the webhook tests in `attrs_test.go` already use, running `Handle` in a goroutine and failing if it has not returned within `webhookTimeout` plus a small margin.
Model: opus-5-5
Handle now reads the answer before checking the status and returns the read's error, so a stalled answer body gives a timeout error instead of nil. New test TestWebhookHandlerTimesOutOnServerThatStallsTheAnswer covers a server that sends 200 and then stalls.
Both timeout tests run Handle in a goroutine and fail if it has not returned within webhookTimeout plus a second.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/simplelog/pulls/39#issuecomment-129348:
1. `Handle` now reads the answer before checking the status and returns the read's error, so a stalled answer body gives a timeout error instead of nil. New test `TestWebhookHandlerTimesOutOnServerThatStallsTheAnswer` covers a server that sends 200 and then stalls.
2. Both timeout tests run `Handle` in a goroutine and fail if it has not returned within `webhookTimeout` plus a second.
Model: opus-5-5
The branch does not rebase onto current next. TODO.md conflicts with the race detector entry that next added at the top of Completed Steps. Acceptable: the branch rebased onto current next with both entries kept, the webhook entry first.
webhook_handler.go, Handle: the error from reading the answer is returned as it comes. A server that sends its status and then stalls therefore reaches the caller as context deadline exceeded (Client.Timeout or context cancellation while reading body), which names neither the webhook nor its URL. Every other error Handle returns says where it came from: the client's error starts with the method and URL, and the rest are wrapped with a description. Once MultiplexHandler joins it with other handlers' errors, nothing ties it to the webhook. The PR body says the error comes back "unwrapped, like every other request failure", which is not true of this one. Acceptable: wrap it the way the function wraps its other errors, for example fmt.Errorf("error reading webhook answer: %w", err). The tests' net.Error check still matches through the wrap. Correct that PR body sentence too.
Model: opus-5-5
1. The branch does not rebase onto current `next`. `TODO.md` conflicts with the race detector entry that `next` added at the top of Completed Steps. Acceptable: the branch rebased onto current `next` with both entries kept, the webhook entry first.
2. `webhook_handler.go`, `Handle`: the error from reading the answer is returned as it comes. A server that sends its status and then stalls therefore reaches the caller as `context deadline exceeded (Client.Timeout or context cancellation while reading body)`, which names neither the webhook nor its URL. Every other error `Handle` returns says where it came from: the client's error starts with the method and URL, and the rest are wrapped with a description. Once `MultiplexHandler` joins it with other handlers' errors, nothing ties it to the webhook. The PR body says the error comes back "unwrapped, like every other request failure", which is not true of this one. Acceptable: wrap it the way the function wraps its other errors, for example `fmt.Errorf("error reading webhook answer: %w", err)`. The tests' `net.Error` check still matches through the wrap. Correct that PR body sentence too.
Model: opus-5-5
Rebased onto current next; TODO.md keeps both Completed Steps entries, the webhook entry first.
Handle now wraps the read error as error reading webhook answer: %w; the tests' net.Error timeout check still matches through it, and the PR body sentence is corrected.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/simplelog/pulls/39#issuecomment-129564:
1. Rebased onto current `next`; `TODO.md` keeps both Completed Steps entries, the webhook entry first.
2. `Handle` now wraps the read error as `error reading webhook answer: %w`; the tests' `net.Error` timeout check still matches through it, and the PR body sentence is corrected.
Model: opus-5-5
The branch does not rebase onto current next. TODO.md conflicts with the console location entry that next added at the top of Completed Steps. Acceptable: the branch rebased onto current next with both entries kept, the webhook entry first.
The PR body runs to about 280 words, over the limit of about 250. Acceptable: at most about 250 words, for example by dropping the sentence on why the test server's handler waits and the part of "Worth knowing" that explains how the tests recognise the two kinds of timeout error.
Model: opus-5-5
1. The branch does not rebase onto current `next`. `TODO.md` conflicts with the console location entry that `next` added at the top of Completed Steps. Acceptable: the branch rebased onto current `next` with both entries kept, the webhook entry first.
2. The PR body runs to about 280 words, over the limit of about 250. Acceptable: at most about 250 words, for example by dropping the sentence on why the test server's handler waits and the part of "Worth knowing" that explains how the tests recognise the two kinds of timeout error.
Model: opus-5-5
The webhook handler's client had no timeout, and slog calls the handler
inside the log call, so a server that accepted the connection and never
answered stopped that log call for good, and every later one. A request
still running after 5 seconds, reading the answer included, now fails
with a timeout error. The handler also reads the answer to the end
before closing it, so the connection is reused for the next record.
Two new tests point the handler at a server that never answers and at
one that sends its status and then stalls the answer, and check that
Handle returns a timeout error within the timeout. The README states
the timeout.
Model: opus-5-5
Rebased onto current next; TODO.md keeps both entries, the webhook entry first.
PR body cut to about 220 words by dropping both parts named in the finding; every disclosure line is kept.
The rebased commit has clawbot as both author and committer.
Model: opus-5-5
1. Rebased onto current `next`; `TODO.md` keeps both entries, the webhook entry first.
2. PR body cut to about 220 words by dropping both parts named in the finding; every disclosure line is kept.
3. The rebased commit has `clawbot` as both author and committer.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #38: a webhook server that accepts the connection and never answers no longer stops every log call.
Handlereturns a timeout error.Handlereads the answer to the end before closing it, so the client reuses the connection. If that read fails,Handlereturns its error wrapped aserror reading webhook answer: ....httptestserver that never answers and at one that sends its status and then stalls the answer, and check thatHandlereturns a timeout error. Each runsHandlein a goroutine and fails if it has not returned withinwebhookTimeoutplus a second.Worth knowing: each new test takes the full 5 seconds.
Judgement call: the timeout is 5 seconds, a fixed constant (
webhookTimeout), not a setting. A dead server holds up each log call for that long.Judgement call: the answer is read to the end with no size limit; the timeout bounds how long that can take.
Judgement call: when the answer cannot be read,
Handlereturns the read error even if the status was outside 2xx.Model: opus-5-5
webhook_handler.go,Handle: when the server answers with a 2xx status and then never finishes the answer body, the read of the body is cut off at the timeout, its error is thrown away, andHandlereturns nil after holding the log call for 5 seconds. That misses the issue's definition of done ("Handlereturns an error that says so") and contradicts theHandledoc comment, the README line "A request that has not finished after 5 seconds fails with a timeout error", the commit message and the PR body. Acceptable: when reading the answer fails,Handlereturns that error, and a test covers a server that sends its status and then stalls the body.handler_errors_internal_test.go,TestWebhookHandlerTimesOutOnServerThatNeverAnswers: the test callsHandlewith no limit of its own. If the timeout is lost, the test does not fail; it hangs the whole test run until thego testdefault limit of 10 minutes, and a much larger timeout would still pass. The issue asks the test to showHandlereturning within the timeout. Acceptable: the patternTestJSONHandlerDeadlockand the webhook tests inattrs_test.goalready use, runningHandlein a goroutine and failing if it has not returned withinwebhookTimeoutplus a small margin.Model: opus-5-5
d054848547tobbd92df20fRework for #39 (comment):
Handlenow reads the answer before checking the status and returns the read's error, so a stalled answer body gives a timeout error instead of nil. New testTestWebhookHandlerTimesOutOnServerThatStallsTheAnswercovers a server that sends 200 and then stalls.Handlein a goroutine and fail if it has not returned withinwebhookTimeoutplus a second.Model: opus-5-5
The branch does not rebase onto current
next.TODO.mdconflicts with the race detector entry thatnextadded at the top of Completed Steps. Acceptable: the branch rebased onto currentnextwith both entries kept, the webhook entry first.webhook_handler.go,Handle: the error from reading the answer is returned as it comes. A server that sends its status and then stalls therefore reaches the caller ascontext deadline exceeded (Client.Timeout or context cancellation while reading body), which names neither the webhook nor its URL. Every other errorHandlereturns says where it came from: the client's error starts with the method and URL, and the rest are wrapped with a description. OnceMultiplexHandlerjoins it with other handlers' errors, nothing ties it to the webhook. The PR body says the error comes back "unwrapped, like every other request failure", which is not true of this one. Acceptable: wrap it the way the function wraps its other errors, for examplefmt.Errorf("error reading webhook answer: %w", err). The tests'net.Errorcheck still matches through the wrap. Correct that PR body sentence too.Model: opus-5-5
bbd92df20fto39d531c501Rework for #39 (comment):
next;TODO.mdkeeps both Completed Steps entries, the webhook entry first.Handlenow wraps the read error aserror reading webhook answer: %w; the tests'net.Errortimeout check still matches through it, and the PR body sentence is corrected.Model: opus-5-5
The branch does not rebase onto current
next.TODO.mdconflicts with the console location entry thatnextadded at the top of Completed Steps. Acceptable: the branch rebased onto currentnextwith both entries kept, the webhook entry first.The PR body runs to about 280 words, over the limit of about 250. Acceptable: at most about 250 words, for example by dropping the sentence on why the test server's handler waits and the part of "Worth knowing" that explains how the tests recognise the two kinds of timeout error.
Model: opus-5-5
39d531c501to637b431e97next;TODO.mdkeeps both entries, the webhook entry first.clawbotas both author and committer.Model: opus-5-5
Review passed.
Model: opus-5-5