[atomicdeploy]

Summary

Keep AsyncAbstractResponse's final local buffer alive until its bytes have actually been admitted to the TCP queue, and finish only after admitted header/body bytes have been acknowledged.

The current implementation marks RESPONSE_END while filling the final source buffer. A short AsyncClient::add() can leave a tail in _send_buffer; the next request ACK then considers the response finished and closes it. Partial body adds also bypass _writtenLength accounting, while headers can be counted twice.

Changes

• Separate source EOF from response completion with one boolean.
• Account for every partial TCP add exactly once; retain zero/partial tails.
• Preserve the current RESPONSE_TRY_AGAIN credit fix and avoid spending a header credit when zero bytes were admitted.
• Wait for the final actual ACK; a poll or partial ACK cannot finish an unacknowledged tail.
• Treat a failed send as an explicit transport failure, not successful completion.

Reproduction and verification

Prepared against exact upstream main 4d4eaca1833c3c11940a988fc8a6c0bdf734bbd2.

tests/run-response-backpressure.mjs extracts and compiles the actual write_send_buffs implementation from src/WebResponses.cpp with small transport/source doubles. It is not a separately rewritten response algorithm. Requires Node.js and a C++17 g++ (or CXX).

node tests/run-response-backpressure.mjs

The optional --baseline SOURCE --output EXECUTABLE mode verifies the pinned original source hash before reproduction. The original method fails deterministically with an eight-byte final source buffer, one admitted body byte and seven pending bytes while already exposing RESPONSE_END. Both in-flight-credit configurations reproduce the failure. Both patched configurations pass:

• Partial/zero header and body adds, zero TCP window, and partial final ACKs/polls.
• Empty, known-length, unknown-length, chunked/terminator and 224 KiB bodies with byte-exact reconstruction.
• RESPONSE_TRY_AGAIN, failed sends and invalid sources.
• Consistent actual admitted/acknowledged byte accounting.

This is focused host/mocked-transport evidence, not a full Arduino/lwIP build, packet-level measurement, hardware/Wi-Fi soak or proof that every reported network failure has this cause. No installed SDK was modified; this upstream patch follows a downstream immutable-response workaround and its reproducible short-add regression.

[mathieucarbou]

@atomicdeploy please open an issue first describing the bug you saw and an MRE to reproduce it. Ideally written similar to a project exemple.
We’ll discuss in this issue how to fix it. Opening a PR from AI like that does not help understand the issue you had and also does not help us.
Thank you !

[mathieucarbou]

Review of PR #497 — "fix: drain final response buffers before completing"

• PR: fix: drain final response buffers before completing #497 (atomicdeploy, base 4d4eaca1833c3c11940a988fc8a6c0bdf734bbd2, head 4e0f94ae35)
• Reported issue: Row image download stalls 439 bytes before the end, three offers in a row, before one goes through lvschouwen/split-flap#631
• Files changed: src/WebResponseImpl.h (+2), src/WebResponses.cpp (+28/−19), tests/response_backpressure.cpp (new, +126), tests/run-response-backpressure.mjs (new, +37)
• Review date: 2026-10-23

1. The reported bug — is it real?

Yes, it's a real, genuine bug. A user's board download stalled at byte 330365 of a 330804-byte firmware image (Content-Length: 330804, short by exactly 439 bytes), three times in a row, then succeeded unchanged on the 4th try — see lvschouwen/split-flap#631.

Root cause, traced through the code:

In src/WebResponses.cpp, AsyncAbstractResponse::write_send_buffs() (the engine behind AsyncFileResponse, AsyncStreamResponse, AsyncCallbackResponse, AsyncChunkedResponse, AsyncProgmemResponse — i.e. essentially every streamed/file response):

1. The response source's final chunk is read into the local _send_buffer (2×MSS ≈ 2872 bytes). At that moment the code sets _state = RESPONSE_END immediately (lines 470, 486, 492).
2. The buffer is then offered to the TCP stack with request->client()->add(...). AsyncClient::add() is a partial write: it accepts min(room, size) bytes (AsyncTCP.cpp). If the lwIP send window is momentarily small (memory pressure, a burst on the link, the single-core contention the user describes), part of the final buffer stays in _send_buffer as a pending tail (_send_buffer_offset advanced, break).
3. On the next TCP event — an ACK for previously-sent data, or even a poll (_onPoll → _ack(this, 0, 0)) — src/WebRequest.cpp:249 _onAck sees _response->_finished() is true (because _state == RESPONSE_END already), so it calls _client->close() — while the tail is still sitting in _send_buffer, never admitted to lwIP.
4. AsyncClient::close() → _tcp_close → tcp_close(pcb): the never-written tail is simply never transmitted. The peer receives headers + body − tail and then a FIN — exactly "download stopped 439 bytes short".

The "4th try works" randomness and the "block lifted after 1 minute instead of 10" are consistent: it depends on transient lwIP buffer/window pressure at the moment the final add() runs, not on anything deterministic.

Corroborating evidence in this repo's own history: commit 747223f ("fix: AsyncAbstractResponse might loose part of send buffer", closes #315) already fixed the header side of this exact class of bug (partial add() of headers losing remainder). The PR under review fixes the body/final-buffer side — same bug shape, one block of code over. The tail-retention logic (_send_buffer_offset) exists precisely for this, but the completion logic ignored it.

Secondary real defects the PR names, which I verified in the code:

• When the final fill happens and RESPONSE_END is set mid-iteration, the loop breaks on _state == RESPONSE_END (line 430) — so a partially-admitted final buffer is left pending with completion already flagged.
• _ackedLength += len ran unconditionally at the top, but RESPONSE_WAIT_ACK → RESPONSE_END was unconditional at the bottom ("implicit check", lines 519-522) — so nothing ever actually waited for the final ACK.
• send() failure was not treated as an explicit transport failure.

I reproduced the bug by extracting the actual write_send_buffs from unmodified upstream 4d4eaca into the PR's harness: baseline fails deterministically — final source buffer of 8 bytes, TCP admits 1, 7 bytes stranded, RESPONSE_END exposed while data is undelivered. Both ASYNCWEBSERVER_USE_CHUNK_INFLIGHT=0 and =1 reproduce it. This matches the user's 439-byte truncation exactly in shape.

2. The fix — is it correct?

The core idea is correct and the implementation is sound. The PR:

1. Splits "source EOF" from "response complete" with a new bool _source_ended (src/WebResponseImpl.h). RESPONSE_END is no longer set while filling the final buffer; it's set only when the send buffer is fully drained (_send_buffer_len == 0 after reset) and all admitted bytes are ACKed (_ackedLength >= _writtenLength), else the state parks in RESPONSE_WAIT_ACK.
2. Moves _ackedLength += len + the RESPONSE_WAIT_ACK completion check to the top of the function, so a poll (len==0) or partial ACK can never complete an unacknowledged tail — only a real ACK covering _writtenLength can. This is exactly right given that onAck's len is the lwIP tcp_sent ACK delta (verified in AsyncTCP.cpp:490 tcp_sent).
3. Counts partial tail re-adds exactly once (_writtenLength += added_len at admit time, removing the end-of-function _writtenLength += payloadlen double-add for that path). Header bytes are counted in _writtenLength at admit time only, fixing the double-count concern.
4. Treats send() failure as RESPONSE_FAILED + close instead of silently continuing — correct, since _finished() is _state > RESPONSE_WAIT_ACK, and RESPONSE_FAILED (which is > RESPONSE_WAIT_ACK in the enum) triggers the close path in _onAck. Also guards the header partial-send with pcb_written && and doesn't spend an in-flight credit on a zero-byte admit.
5. Keeps the tail buffer alive: _send_buffer.reset() only happens when _send_buffer_len == 0, so a stranded tail survives across polls/ACKs until space frees up.

I ran the patched code through the full harness: all cases pass under both credit configurations — partial/zero header and body adds, zero TCP window, partial final ACKs, polls, empty/known-length/unknown-length/chunked/224 KiB bodies with byte-exact reconstruction, RESPONSE_TRY_AGAIN, failed sends, invalid sources, and byte-accounting consistency. Exit 0.

One subtlety I checked carefully — the chunked-terminator accounting: in the _chunked branch, _send_buffer_len += readLen + 8 adds the 8 framing bytes, and the completion condition is _ackedLength >= _writtenLength where _writtenLength accumulates exactly what was admitted via add(). Since every byte of _send_buffer (including the 0\r\n\r\n terminator framing) goes through add() and is counted in _writtenLength, the terminator is covered by the wait-for-ACK logic too. Good.

Another subtlety — AsyncResponseStream: its _sourceValid() returns _state < RESPONSE_END. Under the patch, _state now stays in RESPONSE_CONTENT/RESPONSE_WAIT_ACK until fully drained, so _sourceValid() stays true slightly longer — which is correct (the stream is still being drained). No user-visible break: write() on the stream is rejected once _started(), unchanged.

Edge case — empty body / immediate EOF: _source_ended=true with _send_buffer_len==0 on the first call → _state = (_ackedLength >= _writtenLength) ? RESPONSE_END : RESPONSE_WAIT_ACK. _writtenLength includes headers; if the headers were just admitted but not yet ACKed, we correctly wait one RTT instead of closing instantly. This changes timing slightly (one extra ACK wait for tiny responses) but is more correct — it guarantees the peer actually received the bytes before FIN.

Edge case — who else writes to the client outside the response? I audited WebRequest.cpp, WebHandlerImpl.h, Middleware.cpp, WebServer.cpp: no other writer between _respond and completion (SSE and WebSocket responses release the client and manage their own lifecycle; AsyncWebSocketResponse sets RESPONSE_WAIT_ACK itself but overrides _ack() to return 0 and never enters write_send_buffs — so the moved RESPONSE_WAIT_ACK handling in AsyncAbstractResponse does not affect it). AsyncEventSourceResponse likewise writes headers directly and releases the client. So the new _ackedLength >= _writtenLength invariant (all bytes on the wire come from this response) holds for every class that uses AsyncAbstractResponse::write_send_buffs.

3. Impacts of the fix

Positive:

• Fixes truncated final responses under TCP backpressure — the user's exact bug.
• Responses now complete only after the peer has ACKed every admitted byte; close() (FIN) can no longer cut off an undelivered tail.
• send() failure is now an explicit RESPONSE_FAILED (connection closed) instead of silent continuation.
• Header accounting no longer double-counts; in-flight credit isn't spent on zero-byte admits.

Behavioral changes to be aware of (all benign, but worth noting):

• One extra RTT before connection close for responses whose last bytes were admitted but not yet ACKed when the source hit EOF. Previously the code closed eagerly on the next poll/ACK; now it waits for the final real ACK. This is the point of the fix (lwIP tcp_close queues the FIN after unacked data, but the old code could close before the tail was even admitted). Cost: negligible latency; benefit: no truncation.
• RESPONSE_WAIT_ACK is now actually used as a state by AsyncAbstractResponse (previously it was effectively dead for this class — set by nobody, immediately converted to RESPONSE_END). _finished() (_state > RESPONSE_WAIT_ACK) still returns false while waiting — correct.
• Memory: the 2×MSS _send_buffer is now retained until the tail drains instead of being reset early — that's the intended fix; worst case it holds ~2.9 KB a little longer under backpressure.

Not addressed by this PR (pre-existing, out of scope):

• The user's second observation — the offer block lifting after ~1 minute instead of the configured 10 — is in their project code, not this library.
• AsyncBasicResponse::write_send_buffs has its own eager RESPONSE_END but writes the whole body with a single client->write() and doesn't use _send_buffer; a short write() there would have the same shape of issue, but AsyncClient::write() on a small body is a different path and wasn't part of this report. Worth a follow-up issue, not a blocker.

4. The test

The PR adds tests/response_backpressure.cpp + tests/run-response-backpressure.mjs — a host-compiled unit test with mocked transport that extracts the real method source and compiles it with g++.

• The repo has no tests/ directory at all and no host unit-test infrastructure. platformio.ini has no test_* config; CI (.github/workflows) only builds examples for ESP32/ESP8266/RP2040/LibreTiny (build-arduino-emulator.yml, build-esp32.yml, etc.) and runs cpplint/pre-commit/arduino-lint.
• The established MRE convention is an example sketch under examples/arduino/ that compiles on-device and can be run against real hardware/Wi-Fi — e.g. examples/arduino/LargeResponse/LargeResponse.ino (which is itself the MRE for the earlier Lost writes using AsyncCallbackResponse during low memory #242/AsyncAbstractResponse truncates 85 octets in the non-empty segment #315 backpressure fixes and references those issues in its header), examples/arduino/SlowChunkResponse/SlowChunkResponse.ino, examples/arduino/ChunkResponse/ChunkResponse.ino. Commit 747223f followed exactly this pattern ("Added back MRE").
• This is also exactly what the maintainer asked for in the PR's only comment (mathieucarbou, MEMBER, 2026-10-19): "please open an issue first describing the bug you saw and an MRE to reproduce it. Ideally written similar to a project example."

That said, the harness itself is technically well-built: it compiles the actual write_send_buffs from the reviewed source (not a re-implementation), pins the baseline by SHA-256 (ec9481a3… — I verified it matches upstream 4d4eaca), runs both credit configs, and its assertions are meaningful (byte-exact reconstruction, no-finish-before-ACK, no-close-while-healthy). It's good evidence, just delivered in the wrong vehicle for this repo. The right move: keep the harness logic as a maintainer-side verification tool (or drop it from the PR), and add an example sketch that reproduces the stall on-device — e.g. a ThrottledResponse/BackpressureResponse example serving a file or generated body while the client (or a deliberately tiny lwIP TCP_WND/SND_BUF, or a rate-limited downloader) forces short add()s at the tail, asserting the received byte count equals the announced Content-Length. That example would compile in CI like all 47 existing ones and give hardware-level proof.

5. Verdict

Aspect	Assessment
Bug real?	✅ Yes — genuine truncation of the final response tail under TCP backpressure; mechanism fully traced and reproduced
Fix correct?	✅ Core logic is correct and I verified it by compiling/running the actual patched method (baseline fails deterministically, patch passes all cases, both credit configs)
Impacts	✅ Positive; only benign change is waiting for the final real ACK before close (one RTT) — which is the fix itself
Risks/gaps	⚠️ None blocking. Note: AsyncBasicResponse has a similar eager-completion shape (separate path, not covered). SSE/WebSocket unaffected (own lifecycles). The user's "block lifted early" observation is their app, not this lib

[mathieucarbou]

reproducing in real life is very hard since the window when the issue cam happen is small - so it takes external conditions that are hard to control in test.

but to me the fix makes sense - we should wait for all the acks to be received I think before considering closing the connection.

[mathieucarbou]

I am dismissing my approval. I found some issues.

1. I am able to reproduce: I crafted a ResponseTailTruncation_PR497.ino example that is applying some memory pressure at key places.

• Without this fix: the responses are always truncated indeed.
• With the fix: the responses are still truncated 50% of the time.

2. I didn't find a proper deterministic way to test and validate

3. Even if it makes sense IMO from an API perspective to not close until we can be sure that all the bytes are sent, the problem is that receiving acks depends on the target host and it is possible that we did this speed improvement to close connections as fast as we can and avoid dealing with timing out waiting for acks.

4. Also, the AI-generated test in the test folder would need to be reworked and removed: we don't accept such AI slop which is unreadable and cannot be maintained (== garbage). An example need to be created so that we can run it on esp32 (under specific circumstances if needed).

5. lwip will still send the bytes avec after close is called - but we still need to fix the states to avoid truncation and too early close when we still hold some bytes. Once accepted by lwip they will be sent even after a close

This PR definitely needs more discussions and work.

There is way to validate at the moment that this fix is the right one for the bug you saw and if right, what's the impact one request speed now and ack timeout.

[mathieucarbou]

Closing in favor of #499
@atomicdeploy : can you please test #499 ?