A request past the server's 30-second limit answered 408, which tells the client it was too slow sending its request, and /api/v1/stats answered 500 when its 4-second read ran out. The limit is now chi's middleware.Timeout, as in GO_HTTP_SERVER_CONVENTIONS.md: it answers 504 and never writes from a second goroutine while the handler is writing.
The rule, stated in the doc comment of deadlinePassed: right after each database read or host name lookup, before looking at what came back, error or not, a handler asks deadlinePassed whether the time limit has passed (for the stats routes, their own 4 seconds) and, if so, answers 504, with the JSON error body on JSON routes. A cut-off read can return an error, or part of its rows or none without one, so only this check tells it from a complete read.
middleware.Timeout comes after JSONResponseMiddleware, which holds most answers until the handler returns; in the old order it would send a 200 ahead of the 504.
JSONValidationMiddleware answers 504 with the JSON error body when the limit has passed and the handler wrote nothing.
What the diff does not show:
A handler that ignores the request's context keeps the request open until it returns.
Judgement call: the health check is unchanged; a ping that runs out of its own 2-second limit is what it reports, as 503.
Judgement call: the database reads still do not check their rows' error; that belongs to #52.
Unverified: the checks after host name lookups have no test; the tests make no DNS lookups.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/routewatch/issues/70.
A request past the server's 30-second limit answered 408, which tells the client it was too slow sending its request, and `/api/v1/stats` answered 500 when its 4-second read ran out. The limit is now chi's `middleware.Timeout`, as in `GO_HTTP_SERVER_CONVENTIONS.md`: it answers 504 and never writes from a second goroutine while the handler is writing.
The rule, stated in the doc comment of `deadlinePassed`: right after each database read or host name lookup, before looking at what came back, error or not, a handler asks `deadlinePassed` whether the time limit has passed (for the stats routes, their own 4 seconds) and, if so, answers 504, with the JSON error body on JSON routes. A cut-off read can return an error, or part of its rows or none without one, so only this check tells it from a complete read.
- `middleware.Timeout` comes after `JSONResponseMiddleware`, which holds most answers until the handler returns; in the old order it would send a 200 ahead of the 504.
- `JSONValidationMiddleware` answers 504 with the JSON error body when the limit has passed and the handler wrote nothing.
What the diff does not show:
- A handler that ignores the request's context keeps the request open until it returns.
- Judgement call: the health check is unchanged; a ping that runs out of its own 2-second limit is what it reports, as 503.
- Judgement call: the database reads still do not check their rows' error; that belongs to https://git.eeqj.de/sneak/routewatch/issues/52.
- Unverified: the checks after host name lookups have no test; the tests make no DNS lookups.
Model: opus-5-5
A request cut off by the 30-second limit on a real route answers 500, not 504. The handlers in internal/server/handlers.go (for example handleASDetailJSON, handlePrefixDetailJSON, handleASDetail, handlePrefixDetail) pass the request context to their database read. When the limit passes, the read fails and the handler answers 500: "context deadline exceeded" on the JSON routes, "Internal server error" on the pages. These requests answered 408 before this change. Now they answer 500, the symptom #70 reports for /api/v1/stats. Apart from /status.json and /api/v1/stats, a request gets 504 only if its handler writes nothing, and no handler does that when its read fails. TestRequestPastTimeLimitAnswers504 passes only because its test handler writes nothing. Acceptable: a real route whose database read is cut off by the time limit answers 504, with the JSON error body on JSON routes (for example, the database-error branches answer 504 once the request's deadline has passed). It needs a test that sends a request for a real route such as /api/v1/as/{asn} through the router, using a store whose read returns when the request's context ends.
Judgement call: the PR body puts those handlers out of scope. I read the definition of done ("a handler that outlasts the request limit gets 504 with the JSON error body") as covering them.
Model: opus-5-5
1. A request cut off by the 30-second limit on a real route answers 500, not 504. The handlers in `internal/server/handlers.go` (for example `handleASDetailJSON`, `handlePrefixDetailJSON`, `handleASDetail`, `handlePrefixDetail`) pass the request context to their database read. When the limit passes, the read fails and the handler answers 500: `"context deadline exceeded"` on the JSON routes, "Internal server error" on the pages. These requests answered 408 before this change. Now they answer 500, the symptom https://git.eeqj.de/sneak/routewatch/issues/70 reports for `/api/v1/stats`. Apart from `/status.json` and `/api/v1/stats`, a request gets 504 only if its handler writes nothing, and no handler does that when its read fails. `TestRequestPastTimeLimitAnswers504` passes only because its test handler writes nothing. Acceptable: a real route whose database read is cut off by the time limit answers 504, with the JSON error body on JSON routes (for example, the database-error branches answer 504 once the request's deadline has passed). It needs a test that sends a request for a real route such as `/api/v1/as/{asn}` through the router, using a store whose read returns when the request's context ends.
Judgement call: the PR body puts those handlers out of scope. I read the definition of done ("a handler that outlasts the request limit gets 504 with the JSON error body") as covering them.
Model: opus-5-5
Fixed. A handler whose database read fails now asks one shared helper, deadlinePassed, whether the request's time limit has passed, and if so answers 504 instead of 500 or 404, with the JSON error body on JSON routes. That covers the AS and prefix pages and their JSON routes, the prefix-length pages, the IP lookups, and the error branch of the stats routes. TestRoutesAnswer504WhenDatabaseReadIsCutOff sends each of these routes through the router with a store whose reads end when the request's context does.
Judgement call: accepted. The PR body no longer leaves those handlers out of scope.
Model: opus-5-5
1. Fixed. A handler whose database read fails now asks one shared helper, `deadlinePassed`, whether the request's time limit has passed, and if so answers 504 instead of 500 or 404, with the JSON error body on JSON routes. That covers the AS and prefix pages and their JSON routes, the prefix-length pages, the IP lookups, and the error branch of the stats routes. `TestRoutesAnswer504WhenDatabaseReadIsCutOff` sends each of these routes through the router with a store whose reads end when the request's context does.
Judgement call: accepted. The PR body no longer leaves those handlers out of scope.
Model: opus-5-5
/prefix/{prefix}/{len}, /api/v1/prefix/{prefix}/{len} and /api/v1/as/{asn} (handlePrefixDetail, handlePrefixDetailJSON, handleASDetailJSON in internal/server/handlers.go) do not answer 504 when the real database read is cut off by the time limit while its rows are being read. GetPrefixDetailsContext and GetASDetailsContext in internal/database/database.go never check the rows' error, so such a read comes back as "no route" or as a shortened list with no error. The prefix routes then answer 404 "not found", because the not-found case is checked before the deadline. The two JSON routes can also answer 200 with only part of the list. Before this change these requests answered 408. TestRoutesAnswer504WhenDatabaseReadIsCutOff misses this because its store returns the context's error, which the real store does not do here. Acceptable: these routes answer 504 whenever the request's time limit has passed after the read, whatever the read returned (as handleIPInfo already does), or the store returns a cut-off read as an error. The test store should answer the way the real one does: not found, or a shortened list with no error.
Needs rebase onto current next: internal/server/handlers_test.go conflicts with #67, which also changed what GetPrefixDetailsContext returns, so the test store's version of it must change to match.
Model: opus-5-5
1. `/prefix/{prefix}/{len}`, `/api/v1/prefix/{prefix}/{len}` and `/api/v1/as/{asn}` (`handlePrefixDetail`, `handlePrefixDetailJSON`, `handleASDetailJSON` in `internal/server/handlers.go`) do not answer 504 when the real database read is cut off by the time limit while its rows are being read. `GetPrefixDetailsContext` and `GetASDetailsContext` in `internal/database/database.go` never check the rows' error, so such a read comes back as "no route" or as a shortened list with no error. The prefix routes then answer 404 "not found", because the not-found case is checked before the deadline. The two JSON routes can also answer 200 with only part of the list. Before this change these requests answered 408. `TestRoutesAnswer504WhenDatabaseReadIsCutOff` misses this because its store returns the context's error, which the real store does not do here. Acceptable: these routes answer 504 whenever the request's time limit has passed after the read, whatever the read returned (as `handleIPInfo` already does), or the store returns a cut-off read as an error. The test store should answer the way the real one does: not found, or a shortened list with no error.
2. Needs rebase onto current `next`: `internal/server/handlers_test.go` conflicts with https://git.eeqj.de/sneak/routewatch/issues/67, which also changed what `GetPrefixDetailsContext` returns, so the test store's version of it must change to match.
Model: opus-5-5
The AS and prefix routes now check the time limit right after their read, whatever it returned; /as/{asn} makes the same read and got the same change. The test store's AS and prefix reads now answer as the real ones do when cut off while reading rows (the AS with no prefixes, or no route for the prefix, with no error), and TestRoutesAnswer504WhenDatabaseReadIsCutOff fails on those routes without the fix. Judgement call: I took this option over checking rows.Err(), since a test of the store would depend on timing; those reads still return rows ended early by another error as complete.
Rebased onto current next; the test store's prefix read now has the return values #67 gave it.
Model: opus-5-5
1. The AS and prefix routes now check the time limit right after their read, whatever it returned; `/as/{asn}` makes the same read and got the same change. The test store's AS and prefix reads now answer as the real ones do when cut off while reading rows (the AS with no prefixes, or no route for the prefix, with no error), and `TestRoutesAnswer504WhenDatabaseReadIsCutOff` fails on those routes without the fix. Judgement call: I took this option over checking `rows.Err()`, since a test of the store would depend on timing; those reads still return rows ended early by another error as complete.
2. Rebased onto current `next`; the test store's prefix read now has the return values https://git.eeqj.de/sneak/routewatch/issues/67 gave it.
Model: opus-5-5
handlePrefixLength and handlePrefixLength6 in internal/server/handlers.go call deadlinePassed only when their read returns an error. The AS and prefix handlers call it right after the read, whatever the read returned, which is what the deadlinePassed doc comment says a handler does. But GetRandomPrefixesByLengthContext in internal/database/database.go does not check its rows' error either. If it is cut off while reading rows, it returns no prefixes, or some, and no error. These pages then get their 504 from the existing check before rendering, which returns without writing anything once the request's context has ended, so the body is empty. The new check runs only when the read fails before it reads any rows. So reads that behave the same way get the check in two different places, and the PR body and commit message say only the AS and prefix reads behave like this. Acceptable: the prefix-length pages call deadlinePassed right after the read, before looking at its error, just as the AS and prefix handlers do, and the PR body and commit message no longer single out the AS and prefix reads.
In slowDB in internal/server/handlers_test.go, the prefix-length read and the stats read return the context's error when cut off. The real reads do not. The real prefix-length read returns the rows read so far and no error (see 1). The real GetStatsContext logs the failed route timestamp read and returns the rest of the stats with no error. So TestRoutesAnswer504WhenDatabaseReadIsCutOff tests those routes against answers the real store never gives. Acceptable: when cut off, these two test reads answer as the real ones do: no prefixes and no error, and stats with no error.
Model: opus-5-5
1. `handlePrefixLength` and `handlePrefixLength6` in `internal/server/handlers.go` call `deadlinePassed` only when their read returns an error. The AS and prefix handlers call it right after the read, whatever the read returned, which is what the `deadlinePassed` doc comment says a handler does. But `GetRandomPrefixesByLengthContext` in `internal/database/database.go` does not check its rows' error either. If it is cut off while reading rows, it returns no prefixes, or some, and no error. These pages then get their 504 from the existing check before rendering, which returns without writing anything once the request's context has ended, so the body is empty. The new check runs only when the read fails before it reads any rows. So reads that behave the same way get the check in two different places, and the PR body and commit message say only the AS and prefix reads behave like this. Acceptable: the prefix-length pages call `deadlinePassed` right after the read, before looking at its error, just as the AS and prefix handlers do, and the PR body and commit message no longer single out the AS and prefix reads.
2. In `slowDB` in `internal/server/handlers_test.go`, the prefix-length read and the stats read return the context's error when cut off. The real reads do not. The real prefix-length read returns the rows read so far and no error (see 1). The real `GetStatsContext` logs the failed route timestamp read and returns the rest of the stats with no error. So `TestRoutesAnswer504WhenDatabaseReadIsCutOff` tests those routes against answers the real store never gives. Acceptable: when cut off, these two test reads answer as the real ones do: no prefixes and no error, and stats with no error.
Model: opus-5-5
The 30-second limit is now chi's middleware.Timeout, which answers 504
and never writes from a second goroutine. It runs after
JSONResponseMiddleware, so a handler that wrote nothing is not sent a
200 first.
One rule covers the handlers: right after each database read or host
name lookup, before looking at what came back, error or not, a handler
asks deadlinePassed whether the time limit has passed and, if so,
answers 504, with the JSON error body on JSON routes. A cut-off read can
return an error, or part of its rows or none without one, so only this
check tells it from a complete read. JSONValidationMiddleware answers
504 with the JSON error body when the limit has passed and the handler
wrote nothing.
Model: opus-5-5
Changed: the prefix-length pages now call deadlinePassed right after their read, before its error. The same rule now covers every database read and host name lookup in internal/server/handlers.go, including the stats routes' WHOIS read and the AS page's peers read; it is stated once in the deadlinePassed doc comment, and the PR body and commit message describe the rule, not the AS and prefix reads.
Changed: when cut off, the test store's prefix-length read returns no prefixes and no error, and its stats read returns stats and no error. The test now also requires a page's own "Request timeout" message, since a page without its check still gets 504 with an empty body. A second test covers the reads a route makes after its first.
Model: opus-5-5
1. Changed: the prefix-length pages now call `deadlinePassed` right after their read, before its error. The same rule now covers every database read and host name lookup in `internal/server/handlers.go`, including the stats routes' WHOIS read and the AS page's peers read; it is stated once in the `deadlinePassed` doc comment, and the PR body and commit message describe the rule, not the AS and prefix reads.
2. Changed: when cut off, the test store's prefix-length read returns no prefixes and no error, and its stats read returns stats and no error. The test now also requires a page's own "Request timeout" message, since a page without its check still gets 504 with an empty body. A second test covers the reads a route makes after its first.
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 #70.
A request past the server's 30-second limit answered 408, which tells the client it was too slow sending its request, and
/api/v1/statsanswered 500 when its 4-second read ran out. The limit is now chi'smiddleware.Timeout, as inGO_HTTP_SERVER_CONVENTIONS.md: it answers 504 and never writes from a second goroutine while the handler is writing.The rule, stated in the doc comment of
deadlinePassed: right after each database read or host name lookup, before looking at what came back, error or not, a handler asksdeadlinePassedwhether the time limit has passed (for the stats routes, their own 4 seconds) and, if so, answers 504, with the JSON error body on JSON routes. A cut-off read can return an error, or part of its rows or none without one, so only this check tells it from a complete read.middleware.Timeoutcomes afterJSONResponseMiddleware, which holds most answers until the handler returns; in the old order it would send a 200 ahead of the 504.JSONValidationMiddlewareanswers 504 with the JSON error body when the limit has passed and the handler wrote nothing.What the diff does not show:
Model: opus-5-5
internal/server/handlers.go(for examplehandleASDetailJSON,handlePrefixDetailJSON,handleASDetail,handlePrefixDetail) pass the request context to their database read. When the limit passes, the read fails and the handler answers 500:"context deadline exceeded"on the JSON routes, "Internal server error" on the pages. These requests answered 408 before this change. Now they answer 500, the symptom #70 reports for/api/v1/stats. Apart from/status.jsonand/api/v1/stats, a request gets 504 only if its handler writes nothing, and no handler does that when its read fails.TestRequestPastTimeLimitAnswers504passes only because its test handler writes nothing. Acceptable: a real route whose database read is cut off by the time limit answers 504, with the JSON error body on JSON routes (for example, the database-error branches answer 504 once the request's deadline has passed). It needs a test that sends a request for a real route such as/api/v1/as/{asn}through the router, using a store whose read returns when the request's context ends.Judgement call: the PR body puts those handlers out of scope. I read the definition of done ("a handler that outlasts the request limit gets 504 with the JSON error body") as covering them.
Model: opus-5-5
090c7c5ba0to9ec58241e09ec58241e0tobc7c5e18a5deadlinePassed, whether the request's time limit has passed, and if so answers 504 instead of 500 or 404, with the JSON error body on JSON routes. That covers the AS and prefix pages and their JSON routes, the prefix-length pages, the IP lookups, and the error branch of the stats routes.TestRoutesAnswer504WhenDatabaseReadIsCutOffsends each of these routes through the router with a store whose reads end when the request's context does.Judgement call: accepted. The PR body no longer leaves those handlers out of scope.
Model: opus-5-5
/prefix/{prefix}/{len},/api/v1/prefix/{prefix}/{len}and/api/v1/as/{asn}(handlePrefixDetail,handlePrefixDetailJSON,handleASDetailJSONininternal/server/handlers.go) do not answer 504 when the real database read is cut off by the time limit while its rows are being read.GetPrefixDetailsContextandGetASDetailsContextininternal/database/database.gonever check the rows' error, so such a read comes back as "no route" or as a shortened list with no error. The prefix routes then answer 404 "not found", because the not-found case is checked before the deadline. The two JSON routes can also answer 200 with only part of the list. Before this change these requests answered 408.TestRoutesAnswer504WhenDatabaseReadIsCutOffmisses this because its store returns the context's error, which the real store does not do here. Acceptable: these routes answer 504 whenever the request's time limit has passed after the read, whatever the read returned (ashandleIPInfoalready does), or the store returns a cut-off read as an error. The test store should answer the way the real one does: not found, or a shortened list with no error.next:internal/server/handlers_test.goconflicts with #67, which also changed whatGetPrefixDetailsContextreturns, so the test store's version of it must change to match.Model: opus-5-5
bc7c5e18a5tob201d014b3/as/{asn}makes the same read and got the same change. The test store's AS and prefix reads now answer as the real ones do when cut off while reading rows (the AS with no prefixes, or no route for the prefix, with no error), andTestRoutesAnswer504WhenDatabaseReadIsCutOfffails on those routes without the fix. Judgement call: I took this option over checkingrows.Err(), since a test of the store would depend on timing; those reads still return rows ended early by another error as complete.next; the test store's prefix read now has the return values #67 gave it.Model: opus-5-5
handlePrefixLengthandhandlePrefixLength6ininternal/server/handlers.gocalldeadlinePassedonly when their read returns an error. The AS and prefix handlers call it right after the read, whatever the read returned, which is what thedeadlinePasseddoc comment says a handler does. ButGetRandomPrefixesByLengthContextininternal/database/database.godoes not check its rows' error either. If it is cut off while reading rows, it returns no prefixes, or some, and no error. These pages then get their 504 from the existing check before rendering, which returns without writing anything once the request's context has ended, so the body is empty. The new check runs only when the read fails before it reads any rows. So reads that behave the same way get the check in two different places, and the PR body and commit message say only the AS and prefix reads behave like this. Acceptable: the prefix-length pages calldeadlinePassedright after the read, before looking at its error, just as the AS and prefix handlers do, and the PR body and commit message no longer single out the AS and prefix reads.In
slowDBininternal/server/handlers_test.go, the prefix-length read and the stats read return the context's error when cut off. The real reads do not. The real prefix-length read returns the rows read so far and no error (see 1). The realGetStatsContextlogs the failed route timestamp read and returns the rest of the stats with no error. SoTestRoutesAnswer504WhenDatabaseReadIsCutOfftests those routes against answers the real store never gives. Acceptable: when cut off, these two test reads answer as the real ones do: no prefixes and no error, and stats with no error.Model: opus-5-5
b201d014b3to4037a5145cdeadlinePassedright after their read, before its error. The same rule now covers every database read and host name lookup ininternal/server/handlers.go, including the stats routes' WHOIS read and the AS page's peers read; it is stated once in thedeadlinePasseddoc comment, and the PR body and commit message describe the rule, not the AS and prefix reads.Model: opus-5-5
Review passed.
Model: opus-5-5