Skip to content

closeIdleConnections() severs a completed-but-unflushed reply and the drain reports it clean #357

Description

@codeslake

proxy/server.mjs:1150 on main (verified still present at 78ca948)

What happens

A drain calls closeIdleConnections(). Node's notion of "idle" is res.end()
having been CALLED
, not its bytes having reached the client. A reply that is
finished but still flushing is therefore destroyed at drain start, and
server.close(cb) fires immediately afterwards, so the drain announces success.

Measured end to end against this proxy:

client received       4,217,792 bytes
reply declared       67,108,872 bytes
stderr               [cache-fix] shutdown: drained clean in 0.0s of 9s budget

Reproduced in isolation on plain Node 24 with no proxy involved:
res.finished === true, writableFinished === false, ~63 MB queued —
srv.close(cb) fires the callback in 0 ms and the socket is destroyed.

Why it is the worst shape of this class

The drain exists to stop a handover cutting live replies. Here it cuts one to
6% of its length and prints the phrase that means it cut nothing. The report is
the defect's disguise, so no monitor and no operator sees it: a reader greping
drained clean in counts this as a success.

Every other number the drain produces sits downstream of it.

A second consequence, not obvious from the above

It also masks the stall predicate for finished-but-unflushed responses.
Such a response never survives to be judged, so the branch that would decline
to claim a cut for it is unreachable while this stands. Found while building a
fixture for that branch — the fixture walked into this instead.

Scope

  • NOT the reload path. The reload correctly asks the proxy to drain; the proxy
    answers by severing and reporting success. Whoever triggers a drain gets it.
  • NOT introduced by the per-connection drain work — it predates it. The
    per-connection change tightened the numbers that sit on top of it.
  • Client-detectable, and therefore retryable: a short body against
    Content-Length. That is the only reason it is not worse.

Sketch of a fix

Do not treat "end() called" as idle. Either wait for writableFinished on
responses that have unflushed bytes, or exclude them from the idle sweep and
leave them to the drain predicate, which already ages a connection by bytes
delivered and would end a genuinely stalled one on its own window.

Needs a case in test/shutdown-exit-code.test.mjs: a reply larger than the
socket buffers, a client that stops reading, end() upstream, then signal —
assert the client receives the whole body, and that a drain that truncated one
does not print drained clean.

🤖 Generated with Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions