fix: zstd truncation/abort under proxy_buffering off (bug B) + regression tests#38
Merged
Conversation
…n tests
Production (deb.myguard.nl, `proxy_buffering off`) served truncated /
aborted zstd responses; nginx logged "zero size buf in writer". Root
cause, two coupled defects in ngx_http_zstd_filter_compress():
1. The compress directive was selected from ctx->action only;
ctx->flush never mapped to ZSTD_e_flush. The COMPRESS->FLUSH action
transition only happens after a call that returned rc > 0. Under
proxy_buffering off the upstream forces a flush around a chunk that
libzstd consumes/buffers internally with rc == 0, so the directive
stayed ZSTD_e_continue and libzstd was never told to flush.
2. The empty-buffer emit guard had a `!(rc == 0 && ctx->flush)`
exception that *forwarded a zero-size buffer* for such a
content-less flush. nginx's ngx_http_write_filter rejects a
zero-size buffer ("zero size buf in writer") and aborts the
request — the truncation/empty-response users saw.
Fix (both parts are required; each alone is wrong):
- Directive: ZSTD_e_flush when (action == FLUSH || ctx->flush), so a
pending flush forces libzstd to disgorge buffered output (mirrors
the stock nginx gzip/brotli sync-flush behaviour).
- Empty-buffer guard: never forward a zero-size non-terminal buffer.
When it is a content-less *completed* flush (rc == 0 && ctx->flush:
per the libzstd contract rc == 0 from a flush means fully drained),
clear NGX_HTTP_GZIP_BUFFERED and ctx->flush and emit nothing.
Clearing ctx->flush is essential for loop termination: the body
filter's inner loop stays alive while ctx->flush is set
(ngx_http_zstd_filter_add_data keeps returning NGX_OK); a naive
"suppress the empty buffer but leave ctx->flush set" change spins a
worker at 100% CPU (NGX_AGAIN livelock — observed and rejected).
Regression tests (decode-and-diff, the discipline the prior misses
lacked):
- tools/test_proxy_unbuffered_truncation.py — the focused bug-B
repro: proxy_buffering off + chunked no-CL upstream, boundary size
matrix around ZSTD_CStreamInSize, byte-exact full-body compare.
- tools/test_compression_matrix.py — broad ~504-cell matrix:
{proxy buffered, proxy unbuffered, static} x {incompressible,
highly-compressible, css} x boundary sizes x {zstd, gzip+zstd,
gzip-only, identity}; every cell decode-and-diffs or asserts
correct fallback. Both wired into build-test.yml `tests` and
`tests-asan`, with step timeouts so a livelock is a hard FAIL.
Verification: focused test 35/35 byte-exact on the fix and FAILS on
clean master (35x "zero size buf in writer") — Principle-0 fail-first
satisfied. Broad matrix 504/504 green on the fix. Under ASAN+UBSAN the
fix serves valid zstd; the only LeakSanitizer report is an identical
pre-existing artifact of SIGKILLing a master_process-off nginx (byte
-for-byte the same on clean master), not introduced here.
Refs the recurring truncation/terminal-frame class: a209f96, 2af5889,
PR#23/#49, #36.
Extends CI coverage to two bug classes from the module's history that no decode-and-diff test exercised: * tools/test_concurrent_cctx_isolation.py (774b4a5 "use per-request zstd contexts" class): fires many DISTINCT concurrent requests through one worker over reused keepalive connections; each response must decompress byte-exact to its OWN origin. A shared/not-reset ZSTD_CCtx produces a well-formed but wrong body (cross-request bleed) that single-body or identical-body tests cannot see. Verified: 96/96 byte-exact on the bug-B fix build. * tools/test_reload_under_load.py (2c538e0 "register CDict cleanup handler ... on reload" / config-cycle class): steady request stream with zstd_dict_file set, repeated SIGHUP reloads mid-flight; every response (old-cycle, straddling, new-cycle) must decode byte-exact, master must survive, error log must stay clean. Complements tools/test_reload_leak.sh (leaks only, not correctness across reload). Verified: 269 reqs / 5 reloads byte-exact, clean. Both wired into build-test.yml `tests` with step timeouts; the CCtx test also runs under `tests-asan` (a shared-context bug is a lifetime bug — ASAN's domain) at lighter concurrency. A dedicated CPU-spin/livelock test was prototyped and DROPPED: the livelock is input-pattern-specific and intermittent, so a fixed-drive + idle-CPU-sample test gave a false PASS on a known-livelock build — worse than no test. The livelock backstop is the existing reliable test_proxy_unbuffered_truncation.py plus its CI step `timeout-minutes` (verified: hangs and is killed on the known-livelock build → hard CI fail), so coverage is retained without a flaky gate. Rig discipline in both: threaded backend, backend self-check, distinct-per-request bodies, full-body decode-and-diff, bounded timeouts so a hang/livelock is a hard FAIL.
Commit 8edb7bb (zstd_long directive) had ZERO coverage. Add tools/test_zstd_long_ldm.py: a 16 MiB body whose duplicate region repeats beyond the level-6 default match window. Asserts (a) byte-exact decode and (b) ~2:1 smaller output with zstd_long on vs off — proving ZSTD_c_enableLongDistanceMatching actually reached libzstd, engaged, and does not corrupt/truncate the stream (the bug-B failure mode). Fail-first verified: passes on the fix, fails when the directive is a no-op. Also document Gaps 2 and 3 from the coverage audit: * Gap 2 (f7e2ef3): proven untestable via black-box http test (purely defensive hardening; pre-fix and fixed .so produce identical responses). * Gap 3 (HTTP/2 transport, 8281baa): requires TLS/ALPN to test in nginx; not worth the complexity when bug-B is already covered by test_proxy_unbuffered_truncation.py and matrix-under-asan. Wire zstd_long test into both tests and tests-asan jobs.
eilandert
force-pushed
the
fix/bugb-flush-loopsafe
branch
from
May 18, 2026 11:28
96e0554 to
8c5b772
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves bug B: production (deb.myguard.nl,
proxy_buffering off) served truncated / aborted zstd responses withzero size buf in writerin the error log. Two coupled defects inngx_http_zstd_filter_compress():ctx->flush. It was chosen fromctx->actiononly; theCOMPRESS→FLUSHtransition happens only after arc > 0call. Underproxy_buffering offthe upstream forces a flush around a chunk libzstd consumes/buffers internally (rc == 0), so the directive stayedZSTD_e_continueand libzstd was never told to flush.!(rc == 0 && ctx->flush)exception) → nginxngx_http_write_filterrejects it → request aborted/truncated.Fix (both parts required — each alone is wrong)
ZSTD_e_flushwhenaction == FLUSH || ctx->flush→ a pending flush forces libzstd to disgorge buffered output (mirrors stock nginx gzip/brotli sync-flush).rc == 0 && ctx->flush⟹ libzstd fully drained), clearNGX_HTTP_GZIP_BUFFERED+ctx->flushand emit nothing. Clearingctx->flushis essential for loop termination — a naive "suppress but keep flush" change spins a worker at 100% CPU (NGX_AGAIN livelock); that wrong attempt was caught and rejected by the new test (it hung instead of false-passing).Verification (the discipline prior misses lacked: decode-and-diff)
tools/test_proxy_unbuffered_truncation.py→ 35/35 byte-exact on the fix, and FAILS on clean master (35×zero size buf in writer). The test genuinely detects the bug and the fix genuinely resolves it.tools/test_compression_matrix.py→ 504/504 cells green — {proxy buffered, proxy unbuffered, static} × {incompressible, highly-compressible, css} × boundary sizes (aroundZSTD_CStreamInSize) × {zstd, gzip+zstd, gzip-only, identity}. No regression on previously-working paths.master_process offnginx — byte-for-byte identical on clean master, not introduced here.Tests added & wired
Both tests added to
build-test.ymltestsandtests-asan, with step timeouts so a CPU-spin livelock is a hard FAIL, not a hang. Closes the long-standing gap (chunked tests used--- ignore_response; nothing decoded a large body and byte-compared it) behind the recurring truncation/terminal-frame class (a209f96, 2af5889, PR#23/#49, #36).Deploy gate
Do not re-enable zstd on deb.myguard.nl or ship a package until this PR's CI (incl.
tests-asan) is green. Production stays on the brotli mitigation until then.