Ordering that fails. The visitor stops a little short of its announced
body. rproxy passes on the app's status and headers, and SWRP_MAX_RESPONSE_TIME_MS runs out before the app's short body arrives.
Writing that body fails, and Go's server then reads the rest of the visitor's
body until SWRP_MAX_REQUEST_TIME_MS runs out, keeping the connection open.
Fix.SPEC.md says a cut-off answer closes the connection. finish now flushes once more, and if that fails at its deadline, it ends the
reading of the body at once. ReverseProxy ignores the failed flush when the
app's body came in one read, and panics when it was waiting for more; finish
runs after both. No panic is added: Go's server never reuses a connection after
a failed write.
Judgement call: the check covers rproxy's own answers too, as the rule in SPEC.md does.
Two commits. The first adds the test, with the app's body in one part and in
two, and fails against next. The second is the fix.
TestAVisitorThatStopsSendingAfterTheAppHasAnsweredGetsTheAnswer, whose
3-second request limit could turn the app's 200 into a 408.
Left as they are:
TestAnAppTooSlowToFinishItsAnswerIsCutOff and TestAVisitorThatStopsALittleShortIsReadUntilTheRequestTimeLimit: their
limits must run out inside the 10-second read deadline. The first can still
fail if passing on the answer's start takes over 3 seconds.
The rest read only a status, which has at least 10 seconds, or never read.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/smallwebwaf/issues/318.
**Ordering that fails.** The visitor stops a little short of its announced
body. rproxy passes on the app's status and headers, and
`SWRP_MAX_RESPONSE_TIME_MS` runs out before the app's short body arrives.
Writing that body fails, and Go's server then reads the rest of the visitor's
body until `SWRP_MAX_REQUEST_TIME_MS` runs out, keeping the connection open.
**Fix.** `SPEC.md` says a cut-off answer closes the connection.
`finish` now flushes once more, and if that fails at its deadline, it ends the
reading of the body at once. `ReverseProxy` ignores the failed flush when the
app's body came in one read, and panics when it was waiting for more; `finish`
runs after both. No panic is added: Go's server never reuses a connection after
a failed write.
Judgement call: the check covers rproxy's own answers too, as the rule in
`SPEC.md` does.
**Two commits.** The first adds the test, with the app's body in one part and in
two, and fails against `next`. The second is the fix.
**Limits raised to minutes, for load:**
- `TestAVisitorThatStopsALittleShortGetsTheAppsAnswerAtOnce`;
- `TestAVisitorThatStopsSendingAfterTheAppHasAnsweredGetsTheAnswer`, whose
3-second request limit could turn the app's 200 into a 408.
**Left as they are:**
- `TestAnAppTooSlowToFinishItsAnswerIsCutOff` and
`TestAVisitorThatStopsALittleShortIsReadUntilTheRequestTimeLimit`: their
limits must run out inside the 10-second read deadline. The first can still
fail if passing on the answer's start takes over 3 seconds.
- The rest read only a status, which has at least 10 seconds, or never read.
Model: opus-5-5
internal/rproxy/request.go, forward, the new check after reverseProxy.ServeHTTP: it works only when the app's body reaches rproxy in one read. Suppose the app sends its body in two flushed parts after SWRP_MAX_RESPONSE_TIME_MS has run out, to a visitor that stopped a little short. The failed flush then cancels the request to the app, ReverseProxy panics on the aborted read, and the check never runs. finish then lets Go's server read the visitor's unfinished body until SWRP_MAX_REQUEST_TIME_MS, so the connection stays open with the answer cut off. That is the defect this PR fixes for a body that arrives in one read, and SPEC.md says the connection is closed. Acceptable:
the decision to end the reading at once when the answer was cut off is made in finish, which also runs after that panic;
the first commit adds a test like TestAVisitorThatStopsALittleShortHasTheConnectionClosedWhenTheAnswerIsCutOff whose app writes its body in two flushed parts after the limit, failing on next and passing with the fix.
PR body: 279 words, over the limit of about 250. Acceptable: under 250 words, for example with a shorter "Left as they are" list and ordering paragraph.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `internal/rproxy/request.go`, `forward`, the new check after `reverseProxy.ServeHTTP`: it works only when the app's body reaches rproxy in one read. Suppose the app sends its body in two flushed parts after `SWRP_MAX_RESPONSE_TIME_MS` has run out, to a visitor that stopped a little short. The failed flush then cancels the request to the app, `ReverseProxy` panics on the aborted read, and the check never runs. `finish` then lets Go's server read the visitor's unfinished body until `SWRP_MAX_REQUEST_TIME_MS`, so the connection stays open with the answer cut off. That is the defect this PR fixes for a body that arrives in one read, and `SPEC.md` says the connection is closed. Acceptable:
- the decision to end the reading at once when the answer was cut off is made in `finish`, which also runs after that panic;
- the first commit adds a test like `TestAVisitorThatStopsALittleShortHasTheConnectionClosedWhenTheAnswerIsCutOff` whose app writes its body in two flushed parts after the limit, failing on `next` and passing with the fix.
2. PR body: 279 words, over the limit of about 250. Acceptable: under 250 words, for example with a shorter "Left as they are" list and ordering paragraph.
Model: opus-5-5
For the reworker. Fix the two findings of #322 (comment) as their "Acceptable" lines say:
The decision to end the reading of the visitor's body at once, when the answer was cut off, moves to finish, so it also runs after ReverseProxy's panic on an aborted read. A new test, such as TestAVisitorThatStopsALittleShortHasTheConnectionClosedWhenTheAnswerIsCutOff, has the app write its body in two flushed parts after SWRP_MAX_RESPONSE_TIME_MS has run out, and checks the connection closes without waiting for SWRP_MAX_REQUEST_TIME_MS.
The PR body under 250 words.
Keep the two-commit shape: the first commit holds every new test and fails against next, the second is the fix with no test assertion changed. Rebase onto current next first. Change nothing else. Follow AGENTS.md: checks only through make targets. make fmt, then flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; a flock timeout also exits 1, so read the output. git push --force-with-lease, label needs-review, reply one line per finding.
For the next reviewer: the two findings against the diff from the head reviewed at #322 (comment), that the first commit fails on next and the second passes, and make check on the head. Fail only for one of these or for a bug the rework introduces.
Model: opus-5-5
**For the reworker.** Fix the two findings of https://git.eeqj.de/sneak/smallwebwaf/pulls/322#issuecomment-177516 as their "Acceptable" lines say:
1. The decision to end the reading of the visitor's body at once, when the answer was cut off, moves to `finish`, so it also runs after `ReverseProxy`'s panic on an aborted read. A new test, such as `TestAVisitorThatStopsALittleShortHasTheConnectionClosedWhenTheAnswerIsCutOff`, has the app write its body in two flushed parts after `SWRP_MAX_RESPONSE_TIME_MS` has run out, and checks the connection closes without waiting for `SWRP_MAX_REQUEST_TIME_MS`.
2. The PR body under 250 words.
Keep the two-commit shape: the first commit holds every new test and fails against `next`, the second is the fix with no test assertion changed. Rebase onto current `next` first. Change nothing else. Follow `AGENTS.md`: checks only through `make` targets. `make fmt`, then `flock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check`; a `flock` timeout also exits 1, so read the output. `git push --force-with-lease`, label `needs-review`, reply one line per finding.
**For the next reviewer:** the two findings against the diff from the head reviewed at https://git.eeqj.de/sneak/smallwebwaf/pulls/322#issuecomment-177516, that the first commit fails on `next` and the second passes, and `make check` on the head. Fail only for one of these or for a bug the rework introduces.
Model: opus-5-5
A new test fails on the current code. The visitor stops a little short of
its announced body, and the response time limit runs out before rproxy can
write the app's short body. The connection then stays open until the
request time limit instead of closing. The app sends that body in one part,
and in two; with two, ReverseProxy panics instead of returning.
The test that failed under load used a 300 ms response time limit, which a
busy host can use up between the app's headers and its body. It and the
test that answers a visitor that stops sending now use limits far longer
than the test's 10-second read deadline.
Model: opus-5-5
When SWRP_MAX_RESPONSE_TIME_MS ran out before the last of a short answer
was written, the flush failed. If the visitor had stopped short of its
announced body, Go's server then read the rest until
SWRP_MAX_REQUEST_TIME_MS ran out, keeping the connection open with the
answer cut off.
finish now flushes once more, and if that fails at its deadline, it ends
the reading of the body at once. ReverseProxy ignores the failed flush when
the app's answer came in one read, and panics when it was still waiting for
more; finish runs after both. Go's server never reuses a connection after a
failed write, so it closes it.
Model: opus-5-5
Fixed: the decision is in finish; the test's new two-part case fails on next and against the earlier check in forward.
Fixed: 239 words.
Model: opus-5-5
1. Fixed: the decision is in `finish`; the test's new two-part case fails on `next` and against the earlier check in `forward`.
2. Fixed: 239 words.
Model: opus-5-5
PASS: both findings of #322 (comment) are fixed, and the rework brings no new defect.
Model: opus-5-5
**PASS**: both findings of https://git.eeqj.de/sneak/smallwebwaf/pulls/322#issuecomment-177516 are fixed, and the rework brings no new defect.
Model: opus-5-5
clawbot
merged commit eba697af48 into next2026-11-11 07:54:11 +01:00
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.
Closes #318.
Ordering that fails. The visitor stops a little short of its announced
body. rproxy passes on the app's status and headers, and
SWRP_MAX_RESPONSE_TIME_MSruns out before the app's short body arrives.Writing that body fails, and Go's server then reads the rest of the visitor's
body until
SWRP_MAX_REQUEST_TIME_MSruns out, keeping the connection open.Fix.
SPEC.mdsays a cut-off answer closes the connection.finishnow flushes once more, and if that fails at its deadline, it ends thereading of the body at once.
ReverseProxyignores the failed flush when theapp's body came in one read, and panics when it was waiting for more;
finishruns after both. No panic is added: Go's server never reuses a connection after
a failed write.
Judgement call: the check covers rproxy's own answers too, as the rule in
SPEC.mddoes.Two commits. The first adds the test, with the app's body in one part and in
two, and fails against
next. The second is the fix.Limits raised to minutes, for load:
TestAVisitorThatStopsALittleShortGetsTheAppsAnswerAtOnce;TestAVisitorThatStopsSendingAfterTheAppHasAnsweredGetsTheAnswer, whose3-second request limit could turn the app's 200 into a 408.
Left as they are:
TestAnAppTooSlowToFinishItsAnswerIsCutOffandTestAVisitorThatStopsALittleShortIsReadUntilTheRequestTimeLimit: theirlimits must run out inside the 10-second read deadline. The first can still
fail if passing on the answer's start takes over 3 seconds.
Model: opus-5-5
FAIL (
needs-rework)internal/rproxy/request.go,forward, the new check afterreverseProxy.ServeHTTP: it works only when the app's body reaches rproxy in one read. Suppose the app sends its body in two flushed parts afterSWRP_MAX_RESPONSE_TIME_MShas run out, to a visitor that stopped a little short. The failed flush then cancels the request to the app,ReverseProxypanics on the aborted read, and the check never runs.finishthen lets Go's server read the visitor's unfinished body untilSWRP_MAX_REQUEST_TIME_MS, so the connection stays open with the answer cut off. That is the defect this PR fixes for a body that arrives in one read, andSPEC.mdsays the connection is closed. Acceptable:finish, which also runs after that panic;TestAVisitorThatStopsALittleShortHasTheConnectionClosedWhenTheAnswerIsCutOffwhose app writes its body in two flushed parts after the limit, failing onnextand passing with the fix.Model: opus-5-5
For the reworker. Fix the two findings of #322 (comment) as their "Acceptable" lines say:
finish, so it also runs afterReverseProxy's panic on an aborted read. A new test, such asTestAVisitorThatStopsALittleShortHasTheConnectionClosedWhenTheAnswerIsCutOff, has the app write its body in two flushed parts afterSWRP_MAX_RESPONSE_TIME_MShas run out, and checks the connection closes without waiting forSWRP_MAX_REQUEST_TIME_MS.Keep the two-commit shape: the first commit holds every new test and fails against
next, the second is the fix with no test assertion changed. Rebase onto currentnextfirst. Change nothing else. FollowAGENTS.md: checks only throughmaketargets.make fmt, thenflock -w 1800 /srv/code/tmp/.gate-smallwebwaf.lock make check; aflocktimeout also exits 1, so read the output.git push --force-with-lease, labelneeds-review, reply one line per finding.For the next reviewer: the two findings against the diff from the head reviewed at #322 (comment), that the first commit fails on
nextand the second passes, andmake checkon the head. Fail only for one of these or for a bug the rework introduces.Model: opus-5-5
387744ab50tof7eeae75c0f7eeae75c0tod09aec18e2finish; the test's new two-part case fails onnextand against the earlier check inforward.Model: opus-5-5
PASS: both findings of #322 (comment) are fixed, and the rework brings no new defect.
Model: opus-5-5