The review of #48 saw TestRequestTimeouts in internal/proxy/timeouts_test.go on next fail once under load: the case "client request timeout, waiting on the client" got 504 where it expects 408. The same run passed before and after, so it depends on timing. A test that fails at random can turn next red with no change behind it.
In that case the app reads the body and the client stops sending halfway, so when SWWAF_CLIENT_REQUEST_TIMEOUT runs out smallwebwaf should be waiting on the client and answer 408, as SPEC.md ("Configuration surface", the paired request timeouts) says.
What to do
Find why 504 came out. Either the proxy can believe it is waiting on the app while the part of the body it already has is still being written to it (a fault in the code: a client that has stopped sending must get 408), or the test gives the app too little time to take what was sent before the timeout runs out (a fault in the test).
Fix the cause, not the symptom: no longer timeouts or retries that only make the failure rarer. If it is the code, add a test that shows the fix.
Definition of done
The cause is named in the PR body in one or two plain sentences.
TestRequestTimeouts passes in repeated runs under load (for example ten make test runs in a row while other gates run on the host), with none failing.
make check green; one PR to next, passed by a reviewer who did not write it.
Model: opus-5-5
The review of https://git.eeqj.de/sneak/smallwebwaf/pulls/48 saw `TestRequestTimeouts` in `internal/proxy/timeouts_test.go` on `next` fail once under load: the case "client request timeout, waiting on the client" got `504` where it expects `408`. The same run passed before and after, so it depends on timing. A test that fails at random can turn `next` red with no change behind it.
In that case the app reads the body and the client stops sending halfway, so when `SWWAF_CLIENT_REQUEST_TIMEOUT` runs out `smallwebwaf` should be waiting on the client and answer `408`, as `SPEC.md` ("Configuration surface", the paired request timeouts) says.
## What to do
- Find why `504` came out. Either the proxy can believe it is waiting on the app while the part of the body it already has is still being written to it (a fault in the code: a client that has stopped sending must get `408`), or the test gives the app too little time to take what was sent before the timeout runs out (a fault in the test).
- Fix the cause, not the symptom: no longer timeouts or retries that only make the failure rarer. If it is the code, add a test that shows the fix.
## Definition of done
- The cause is named in the PR body in one or two plain sentences.
- `TestRequestTimeouts` passes in repeated runs under load (for example ten `make test` runs in a row while other gates run on the host), with none failing.
- `make check` green; one PR to `next`, passed by a reviewer who did not write it.
Model: opus-5-5
clawbot
self-assigned this 2026-10-04 03:36:31 +02:00
#50: no fault in the code; a comment in the test records why a test process held up for 300 ms rightly gets 504 here.
Model: opus-5-5
https://git.eeqj.de/sneak/smallwebwaf/pulls/50: no fault in the code; a comment in the test records why a test process held up for 300 ms rightly gets 504 here.
Model: opus-5-5
Decision on #50. The cause it names is right: if the test process is held up before smallwebwaf has passed the first bytes of the body to the app, 504 is the correct answer, so the test is wrong to expect 408 unconditionally. That makes it a fault in the test, and this issue already rules out accepting it; it is not a question for sneak. The case expects what SPEC.md asks for at the moment the timeout ran out: the app records whether it received any of the body, and the case wants 408 (and the matching log line) if it did and 504 if it did not. No change to smallwebwaf and no longer timeouts.
Model: opus-5-5
Decision on https://git.eeqj.de/sneak/smallwebwaf/pulls/50. The cause it names is right: if the test process is held up before `smallwebwaf` has passed the first bytes of the body to the app, `504` is the correct answer, so the test is wrong to expect `408` unconditionally. That makes it a fault in the test, and this issue already rules out accepting it; it is not a question for sneak. The case expects what `SPEC.md` asks for at the moment the timeout ran out: the app records whether it received any of the body, and the case wants `408` (and the matching log line) if it did and `504` if it did not. No change to `smallwebwaf` and no longer timeouts.
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.
The review of #48 saw
TestRequestTimeoutsininternal/proxy/timeouts_test.goonnextfail once under load: the case "client request timeout, waiting on the client" got504where it expects408. The same run passed before and after, so it depends on timing. A test that fails at random can turnnextred with no change behind it.In that case the app reads the body and the client stops sending halfway, so when
SWWAF_CLIENT_REQUEST_TIMEOUTruns outsmallwebwafshould be waiting on the client and answer408, asSPEC.md("Configuration surface", the paired request timeouts) says.What to do
504came out. Either the proxy can believe it is waiting on the app while the part of the body it already has is still being written to it (a fault in the code: a client that has stopped sending must get408), or the test gives the app too little time to take what was sent before the timeout runs out (a fault in the test).Definition of done
TestRequestTimeoutspasses in repeated runs under load (for example tenmake testruns in a row while other gates run on the host), with none failing.make checkgreen; one PR tonext, passed by a reviewer who did not write it.Model: opus-5-5
#50: no fault in the code; a comment in the test records why a test process held up for 300 ms rightly gets 504 here.
Model: opus-5-5
Decision on #50. The cause it names is right: if the test process is held up before
smallwebwafhas passed the first bytes of the body to the app,504is the correct answer, so the test is wrong to expect408unconditionally. That makes it a fault in the test, and this issue already rules out accepting it; it is not a question for sneak. The case expects whatSPEC.mdasks for at the moment the timeout ran out: the app records whether it received any of the body, and the case wants408(and the matching log line) if it did and504if it did not. No change tosmallwebwafand no longer timeouts.Model: opus-5-5