port: bound the queued messages that hold a descriptor

[andypost]

Closes #394.

Follows #388 (a queued message dups the descriptors it names) and #389, both
merged. This branch is on master, not on #393.

What the bound is, and what it is not

port->messages stays unbounded. This bounds only the entries that carry a
file descriptor.

The cost #388 introduced is descriptor pressure, not memory pressure. A
message with no descriptor costs the sender memory exactly as it did before
#388, and that is not what #394 is about — bounding the queue as a whole
would change behaviour on paths that never hold a descriptor at all: every
ordinary reply, every request body fragment. So NXT_PORT_MAX_FD_MSGS
(src/nxt_port.h) caps the descriptor-carrying entries of one port at 128,
which is at most 256 held descriptors, and the queue itself is untouched.

128 is a constant with the reasoning next to it. The traffic being bounded is
control-plane and one message per event — a new port, a process start, a
listening socket, a certificate, a script, a shared memory segment. A port
with 128 of them outstanding is a peer that has stopped reading, not a busy
port.

The count is port->fd_messages, maintained under port->write_mutex on one
invariant: an entry is counted exactly while it is in port->messages and
carries a descriptor. It is decremented at the two moments that end it — the
message leaves the queue (nxt_port_socket_cancel(),
nxt_port_error_handler()), or it stays queued but its descriptors have just
gone out (nxt_port_write_msgs(), before nxt_port_msg_close_fd() clears
msg->fd[]).

The refusal, and the caller audit

A send that would pass the bound is refused with NXT_ERROR, not dropped
silently. That answer already carries a defined meaning — nothing was
consumed, the message is still the caller's (src/nxt_port.h) — and it is
already what nxt_port_msg_chk_insert() answers when the heap copy cannot be
allocated. So the refusal is not a new code path for any caller; it is an
existing one reached for a new reason.

Every caller that can reach it, and what it already does:

caller	on a non-NXT_OK return
nxt_port_send_port(), src/nxt_port.c:487 (NEW_PORT, two borrowed fds)	queues the buffer's completion, returns NXT_ERROR to its own caller. The descriptors are borrowed from new_port and stay with it, as on the success path.
nxt_router_start_app_process(), src/nxt_router.c:616 (START_PROCESS)	nxt_port_rpc_cancel() on the stream, then the failure path.
nxt_router_app_prefork(), src/nxt_router.c:4074 (START_PROCESS)	same: nxt_port_rpc_cancel(), then fail:.
nxt_cert_store_get() reply, src/nxt_cert.c:1306	closes file.fd explicitly, with a comment saying the port layer never took it.
nxt_script_store_get() reply, src/nxt_script.c:604	same.
nxt_main_port_socket_handler(), src/nxt_main_process.c:1399 (listening socket)	closes ls.socket and queues the buffer's completion.
nxt_main_port_access_log_handler(), src/nxt_main_process.c:2225	closes the file.
nxt_port_change_log_file(), src/nxt_port.c:1075 (CHANGE_FILE)	queues the buffer's completion; the fd is the runtime's and is not the port's to close.
nxt_router_get_mmap_handler(), src/nxt_router.c:7669 (MMAP)	ignores the return. Correct here: no NXT_PORT_MSG_CLOSE_FD, so the descriptor stays with mmap_handler either way. The peer does not get the segment — degraded, not leaked.

No caller loses a descriptor or a buffer to the refusal, and none of them
treats NXT_ERROR as fatal to the process. That is the result that made this
shape safe to ship; had one of them been unable to survive a refusal the
answer would have had to be different.

Tradeoff

A refused send is a behaviour change. Before this, a send to a stalled peer
always succeeded and the queue grew. Now, past 128 descriptor-carrying
entries on one port, a NEW_PORT or a START_PROCESS or a certificate reply can
fail where it used to be accepted, and the caller takes its failure path —
typically an RPC cancel, which surfaces as a failed process start rather than
a silent stall. That is the intended trade: a bounded, reported failure
instead of an unbounded descriptor hold.

Tests

Third leg in src/test/nxt_port_queued_fd_test.c, the file that already
covers #388's ownership change, on the same stub-engine fixture with a real
socketpair. It fills a port to the bound, then asserts:

• the bound is enforced — the 129th descriptor-carrying send answers
  NXT_ERROR;
• nothing was consumed — no buffer completion ran, the caller's descriptor is
  still open, the queue is unchanged;
• a message with no descriptor is still accepted on the same full port;
• messages queued below the bound are delivered — the peer receives the
  descriptor over the socketpair and it names the right file;
• no descriptor is leaked — the process's open-descriptor count is measured
  before the fill and after the drain and must match;
• the drained port takes descriptor-carrying messages again.

The leg reads no internal counter; it observes the bound the way a caller
does.

Proof the leg ran, rather than a green suite: the refusal logs once, and
exactly once, in both configurations —

tests: [alert] port{2387828,1} 4: 128 queued messages already hold a descriptor, refusing to queue another

Fail-before / pass-after, measured, not inferred:

tree	./build/tests
base df9843b0 + this test file only (with the constant declared so it compiles)	exit 1, port queued fd test: a message past the bound of 128 was accepted
this branch, --tests --openssl --debug	exit 0, port queued fd test passed
this branch, --tests --openssl (release)	exit 0 — run because nxt_queue_remove() nulls links only in debug

Mutation check: with both bound checks neutered to has_fd && 0, the
suite goes to exit 1 on this same line; restored, back to exit 0.

NXT_HAVE_EPOLL_EDGE forced to 0 at src/nxt_unix.h:193: make tests
builds clean, exit 0 (then restored). No new static is inside an
#if (NXT_HAVE_EPOLL_EDGE) block, so -Wunused-function has nothing to
catch, but the build was run rather than reasoned about.

What this does not do

• It does not touch port: a wake-up failure can leave a queued message with no notification, permanently #392. A lost wake-up still leaves a queued message
  unnoticed; this only caps how many descriptors can be pinned that way.
• It does not bound port->messages itself, so an unbounded queue of
  descriptor-free messages is still possible. That is the pre-port: a queued NEW_PORT can transmit a closed or reused descriptor #388 memory
  cost and is out of scope here.
• It does not answer port: port->messages is unbounded, and a queued message now holds two descriptors #394's reachability question — whether an unprivileged
  peer can hold a queue open long enough to matter, and against which port.
  The bound makes the ceiling finite without needing that answer.
• The python suite was not run: nothing here is reachable from a request
  path, and the C suite covers the change directly.

🤖 Generated with Claude Code

[andypost]

@claude review
@codex review

[claude]

Claude finished @andypost's task in 5m 30s —— View job

---

Review: bound the queued messages that hold a descriptor

• Gather context (diff vs origin/master, surrounding port code)
• Audit the fd_messages invariant (increment/decrement paths)
• Audit the refusal path and the callers
• Review the test leg
• Post review

I could not run ./configure --tests && make tests in this environment (the configure step isn't in my allowed tools), so everything below is from reading the code, not from a run. The PR body reports the measured fail-before/pass-after and the mutation check; I have no reason to doubt it and did not reproduce it.

---

Verdict

The invariant holds as described. I traced every path that puts an entry into port->messages and every path that takes one out or clears its fd[], and the counting is consistent:

• Increments: nxt_port_msg_chk_insert() (src/nxt_port_socket.c:491) and nxt_port_msg_insert_tail() (src/nxt_port_socket.c:1405) — both under write_mutex, both immediately adjacent to the nxt_queue_insert_tail() they pair with.
• Decrements: nxt_port_write_msgs() before nxt_port_msg_close_fd() (src/nxt_port_socket.c:917), nxt_port_socket_cancel() before nxt_queue_remove() (:1159), nxt_port_error_handler() (:2263).
• has_fd is computed on the caller's copy at increment and on the queued copy at decrement, and those agree: nxt_port_msg_dup_fds() replaces -1 with -1 and a real fd with a real fd, so the predicate is stable across nxt_port_msg_alloc().
• port comes from nxt_mp_zalloc() (src/nxt_port.c:129), so fd_messages starts at 0.
• The link.next != NULL marker is safe in release builds even though nxt_queue_remove() doesn't null there — every remover in this file re-nulls explicitly (:1161, :944) or frees the message.
• Fragment rotation can't double-count: msg->fd[] is set to -1 after the uncount and before the re-insert, so the re-queued entry has has_fd == 0.
• A refused fd-send can't be masked by the enqueued fixup in nxt_port_socket_write2() — enqueued is only set on the fd == -1 branch.

Comments are thorough and the "refuse, don't drop" choice is right: NXT_ERROR from nxt_port_msg_chk_insert() already meant "nothing consumed", so no caller gets a new contract. I spot-checked nxt_port_send_port() (src/nxt_port.c:491) and nxt_router_get_mmap_handler() (src/nxt_router.c:7669) against the table in the description and both match.

Findings below, most actionable first. None of them is a correctness hole in the happy path.

---

1. The alert fires on every refused send, not on entering the refusing state

src/nxt_port_socket.c:525 logs at nxt_alert level for each refusal. A port sits at the bound precisely because its peer stopped reading — which is a sustained condition, not an instant. Every subsequent descriptor-carrying send to that port logs another ALERT line for as long as the stall lasts.

That matters most for nxt_router_get_mmap_handler() (src/nxt_router.c:7669), which is the one caller on this list driven by the application process rather than by control-plane events: an app that stops reading its port but keeps sending GET_MMAP gets one ALERT per request. The bound itself is still doing its job; the log is the unbounded part.

Logging on the 0→refusing transition (or every N-th refusal) would keep the signal and drop the flood. The test's "logs once, and exactly once" observation is a property of the test's single refusal, not of the code.

Fix this →

2. nxt_port_msg_fd_uncount() decrements unconditionally after an unlocked pre-check

src/nxt_port_socket.c:1262:

if (msg->link.next == NULL || !nxt_port_msg_has_fd(msg)) {
    return;
}

nxt_thread_mutex_lock(&port->write_mutex);
port->fd_messages--;
nxt_thread_mutex_unlock(&port->write_mutex);

The predicate is read outside the mutex and not re-tested inside it. If nxt_port_socket_cancel() on another thread matched the same message between the two (it can: cancel only skips messages with ->nf set, and ->nf isn't set until a fragment has gone out), the message is uncounted and removed there, and this decrement runs a second time.

The underlying send/cancel race is pre-existing and worse than the counter — nxt_port_socket_cancel() calls nxt_port_release_send_msg() on a message nxt_port_write_msgs() is still dereferencing — so I'm not claiming this PR introduces it. What the PR does change is the consequence: fd_messages is uint32_t, so one extra decrement wraps it to ~4·10⁹ and the port refuses every descriptor-carrying send from then on, permanently. That's a stickier failure mode than the descriptor leak being fixed, for a counter that is otherwise pure bookkeeping.

Cheap hardening, independent of ever fixing the race: re-test the predicate under the mutex, and/or clamp the decrement (if (port->fd_messages > 0)). The _locked variant already does the test in the right place.

Fix this →

3. The nxt_port_socket_cancel() decrement is not covered by the new test leg

The leg in src/test/nxt_port_queued_fd_test.c:530 exercises two of the three decrement sites — nxt_port_write_msgs() (the drain via port->socket.write_handler) and nxt_port_error_handler() (via nxt_port_test_run_error_handler). It never calls nxt_port_socket_cancel().

That's the site most likely to be got wrong on a future edit, because it's the one where the uncount has to happen before nxt_queue_remove() nulls the link it reads. If that call were dropped, the port would stay wedged at the bound and the suite would stay green.

One more assertion would close it: fill to the bound, nxt_port_socket_cancel() one queued descriptor-carrying message, then assert the next descriptor-carrying send is accepted. The fixture already has everything needed — the router's start-process path uses the same shape (src/nxt_router.c:953).

Fix this →

4. Unrelated whitespace change

src/nxt_port_socket.c:16 drops one of the two blank lines after the NXT_PORT_MAX_ENQUEUE_BUF_SIZE macro, which is out of step with the two-blank-line convention used everywhere else in this file and unrelated to the change. Worth reverting to keep the diff to its subject.

5. Smaller notes, no action needed

• A refused fd-send still leaves a wake-up marker in the ring. nxt_port_socket_write2() writes _NXT_PORT_MSG_READ_SOCKET into port->queue (src/nxt_port_socket.c:340) before nxt_port_msg_chk_insert() refuses, so the peer is told to read a socket that got nothing. Harmless — the peer's read is non-blocking and finds nothing — but the ring entry is spent.
• The bound is per port; the pressure is per process. 128 entries × 2 fds caps one stalled port at 256, as the header comment says, but N stalled ports still cost N × 256. The comment's "leaves room for several of them under 1024" is fair for a handful of ports and optimistic for a router with many app processes. Not a reason to hold this — a per-port bound is the right first cut and a process-wide budget would need a different mechanism.
• The two alert strings differ in shape — "port{%d,%d} %d: ..." in nxt_port_msg_chk_insert() vs "port{%d,%d}: ..." in nxt_port_msg_insert_tail() (the latter has no task and so no socket fd). Both are correct; a single grep pattern won't catch both. Trivial.
• nxt_port_msg_insert_tail() now holds write_mutex across nxt_port_msg_alloc(), i.e. across a nxt_malloc() and up to two fcntl(F_DUPFD_CLOEXEC) calls. That widens the critical section, but it matches what nxt_port_msg_chk_insert() has always done, and moving the check outside the lock would reintroduce the TOCTOU the bound exists to prevent. Correct as written.

---

Nothing here blocks the change; (1) and (3) are the two I'd want addressed before merge, and (2) is cheap insurance on a counter whose failure mode is worse than the bug it fixes.
· branch fix/394-bound-port-queue

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aafca410e0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Skip the bound test when the fd limit is too low

When the test runs under a low RLIMIT_NOFILE (for example, 128, or any limit with fewer than 128 descriptors remaining after the harness opens its existing files), one of these sends fails because nxt_port_msg_alloc() cannot duplicate fd, before the queue reaches the configured bound. The test then reports that the implementation refused a message below the bound even though descriptor exhaustion is unrelated to the behavior under test. The later getrlimit() call only limits descriptor counting; this loop should first verify that at least NXT_PORT_MAX_FD_MSGS slots are available and skip or adapt the test otherwise.

Useful? React with 👍 / 👎.