test(port): add IPC failure injection coverage

[andypost]

Stacked follow-up for #56 and the coverage plan in #56 (comment).

Adds test-only fault injection for IPC message allocation and RPC registration failures, then covers core ownership invariants: failed queued nxt_port_socket_write() leaves the caller-owned fd open before ownership transfer; the unsent buffer completion is not run by the port layer; /proc/self/fd counts stay balanced where available; failed RPC registration allocation and insert leave use_count unchanged and no stream registration behind.

The branch is based on andypost:claude/port-ipc-completion-leaks / PR #56, so it should be retargeted or rebased once #56 lands.

Validation: make -j2; ./configure --tests; make tests -j2; build/tests

[andypost]

/gemini review
/claude review

[gemini-code-assist]

Code Review

This pull request addresses memory pool retain leaks and file descriptor/buffer leaks that occur when port message allocation or socket writing fails. Key changes include deferring memory pool retention until after successful handoff to the port machinery and ensuring that file descriptors and sockets are explicitly closed if a write operation fails in the certificate, script, and main process handlers. Additionally, a new test suite and failure injection mechanisms have been introduced to verify these error paths. I have no feedback to provide as there were no review comments to evaluate.

[gemini-code-assist]

Code Review

This pull request addresses memory pool and file descriptor leaks that occur when port message allocation fails during IPC operations. Key changes include moving memory pool retention to after successful handoff and ensuring that file descriptors and buffers are explicitly closed or completed upon port write failures. A new test suite was added to verify these error paths. Feedback suggests improving defensive coding by resetting file descriptors after closure and using heap-allocated buffers in tests to ensure robustness.

[andypost]

Added a second commit 1b73f404 covering the queued-then-failed path you flagged as a gap in my own review of the PR.

nxt_port_error_handler() is the upstream reference cleanup — close fd first, then queue buffer completion via the fast work queue. PR #56's reorder copied that ordering into the new "failure before queue" path; this new sub-test pins down the reference behaviour itself so a future port-layer refactor that reintroduces sync completion (or closes the fd second) breaks the suite.

Shape:

• New NXT_TESTS-only nxt_port_test_run_error_handler() trampoline in src/nxt_port_socket.c around the (static) nxt_port_error_handler. Passes NULL for the data argument so use_delta doesn't consume the test's port reference for the obj == data case.
• Test allocates a minimal stack nxt_event_engine_t and wires thr->engine so error_handler's task->thread->engine->fast_work_queue deref works; only the fast_work_queue + its cache are initialised, the rest stays zero.
• Manually allocates a nxt_port_send_msg_t (allocated=1, close_fd=1, fd[0]=open("/dev/null"), buf=stack), inserts into port->messages, invokes the trampoline, drains the work queue, and asserts: queue empty after; fd actually closed; completion counter is 0 immediately after error_handler (queued, not sync), then 1 after draining.

Local validation:

$ ./configure --tests && make -j2 tests && build/tests
…
tests: [notice] port failure test started
tests: [notice] port failure test passed
tests: [notice] clone creds test passed

All three sub-tests green (socket_write / rpc_register / error_handler). Ready for re-review.

[andypost]

Added commit 4ce88b77 — nxt_port_fail_test_mp_baseline. Closes the last bullet from phpclub's #56 review ("mp refcounts return to baseline after error paths").

Mirrors the audit-fixed nxt_cert_store_get / nxt_script_store_get shape inside the test: allocate buf in mp, send, retain only on success. With a forced nxt_port_msg_alloc() failure the write returns NXT_ERROR and the test asserts mp->retain == 1 (baseline) — i.e. the retain was never bumped.

Sanity-checked the regression detection by temporarily moving the retain to before nxt_port_socket_write and re-running: test fails with mp baseline test: retain before 1, after 2 (retain leaked across failed send) — exactly the leak shape PR #56 fixes.

Local build/tests shows all four sub-tests green (socket_write / rpc_register / error_handler / mp_baseline).

Also noting Gemini's two comments on the earlier commits:

• nxt_cert.c:1246 defensive file.fd = -1 — this is the PR fix(port): plug mp-pool retain and fd/buffer leaks in IPC reply paths #56-inherited fix code, not new in PR test(port): add IPC failure injection coverage #57. Will add in a small follow-up on PR fix(port): plug mp-pool retain and fd/buffer leaks in IPC reply paths #56 (mirror in nxt_script.c too) rather than diverging the stack.
• nxt_port_fail_test.c:79 stack-allocated nxt_buf_t — the test is synchronous; completion runs inside the same call sequence while the stack frame is alive (sanity-checked by the regression probe above). Happy to switch to nxt_buf_mem_alloc from a transient mp if you'd prefer it for hygiene — let me know.

[andypost]

Two more commits addressing Gemini's review nits:

• 31d61916 — switched the test bufs in socket_write and error_handler sub-tests from stack nxt_buf_t to nxt_buf_mem_alloc() from a transient mp. Matches the production sender shape (cert_store_get allocates from the temp_conf mp) and rules out any future UAF if a refactor lets the port layer access the buf after the test's stack frame returns. Failure ladder reworked so the mp is destroyed on every exit and the heap-allocated nxt_port_send_msg_t is freed if error_handler did not reach it.

• efd0d2d7 — defensive file.fd = -1 after nxt_fd_close(file.fd) in both nxt_cert_store_get_handler and nxt_script_store_get_handler. Picked up the symmetric script-side reset while at it.

All four sub-tests still green (build/tests shows port failure test passed).

Stack of PR #57 now (top → bottom):

efd0d2d7 fix(port): reset file.fd to -1 after nxt_fd_close on send-fail path
31d61916 test(port): allocate test bufs from a transient mp
4ce88b77 test(port): cover the mp-retain baseline invariant on failed sends
1b73f404 test(port): cover the queued-then-failed error_handler path
917c0d43 test(port): add IPC failure injection coverage  ← original
095d73b3 docs(changes): add TLS write-loop release note  ← inherited from #56
04ac78c7 docs(changes): move IPC leak fix to 1.35.5      ← inherited from #56
c88ede1d fix(port): plug mp-pool retain and fd/buffer leaks in IPC reply paths

[phpclub]

Excellent follow-up to #56. This PR does much more than patch individual leak paths — it formalizes IPC/port ownership semantics through deterministic fault-injection testing.

Strong points:

fault injection is compile-gated under #if (NXT_TESTS) and does not pollute production fast paths,
ownership transfer semantics around nxt_port_socket_write() are now explicitly encoded and regression-tested,
mem-pool retain ordering is fixed correctly (retain after successful handoff),
fd leak handling is now consistent across cert/script/socket/access-log reply paths,
async cleanup semantics are verified, not just immediate return codes.

The new nxt_port_fail_test.c coverage is especially strong because it validates invariants, not only API results:

fd ownership preservation,
completion handler execution semantics,
queue cleanup behavior,
RPC rollback correctness,
stable use_count,
no residual rpc_streams entries after failures.

This significantly improves long-term maintainability of the port subsystem.

Comment 1 — retain ordering

The nxt_mp_retain(mp) move is absolutely correct.

Before:

retain()
send()
-> failure

After:

send()
-> success
retain()

This properly matches the ownership model where the completion handler only becomes guaranteed after successful queue handoff.

Good fix.

Comment 2 — fd ownership semantics

Very good clarification of ownership boundaries on:

nxt_port_socket_write() != NXT_OK

The explicit close paths correctly assume:

the port layer never took ownership,
therefore the caller still owns the fd.

This was previously implicit behavior; now it is documented and enforced consistently.

Comment 3 — nxt_fd_close() usage

Good catch using nxt_fd_close() instead of nxt_file_close() after file.name lifetime ended.

The comment explaining the %FN dereference risk is useful future-maintainer documentation.

Comment 4 — fault injection design

The deterministic failure injection approach is very strong.

Testing allocator-failure behavior via:

nxt_port_test_msg_alloc_failures()

is significantly better than relying on artificial OOM environments or probabilistic failures.

This makes regression testing reproducible and CI-friendly.

Comment 5 — async cleanup verification

The nxt_port_fail_test_error_handler() coverage is particularly valuable.

It validates:

queued message cleanup,
deferred completion scheduling,
async work queue semantics,
correct fd release ordering.

Testing the deferred cleanup path directly through:

nxt_port_test_run_error_handler()

is a very solid approach.

Minor Non-Blocking Suggestion

The injected failure counters are currently global statics:

static nxt_uint_t nxt_port_test_msg_alloc_failure_count;

This is perfectly fine for the current single-threaded test model, but if parallel test execution ever appears in the future, thread-local or engine-local injection state may become preferable.

Not a blocker.

[andypost]

sqwuashed and rebased

[andypost]

@codex review once

[andypost]

filed follow-up #60

[andypost]

@chatgpt-codex-connector review once

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[andypost]

@codex review once

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

ℹ️ 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".

[phpclub]

Summary:
- ✅ 4 sub-tests cover: socket_write alloc fail, rpc alloc/insert fail,
error_handler completion enqueue, mp retain baseline
- ✅ ownership invariants: fd stays open on pre-queue fail, completion not
invoked synchronously, mp retain unchanged
- 🔵 5 nits: mostly future-proofing (reset helpers, BSD stub, queue
comment)
- ❓ 1 question: NULL data in error_handler wrapper — refcount leak risk if
test author forgets nxt_port_use(port, -1)

LGTM with minor nits. No blockers.

[andypost]

@claude review once

[phpclub]

PR #57 review — test(port): IPC failure injection. Solid coverage, clean teardown. Findings:

Production changes (cert/script):

• nxt_cert.c:1247 / nxt_script.c:612: 🔵 good — file.fd = -1 after close prevents stale-fd deref/double-close on %FN log
  path. Correct.

src/test/nxt_port_fail_test.c:

• nxt_port_fail_test_error_handler, port->use_count = 2: 🟡 risk: magic count couples test to nxt_port_error_handler
  internal use-accounting (exactly one use_delta-- for 1 msg, data=NULL to skip obj==data decrement). If handler's ref
  logic changes, port freed mid-test → UAF on port->messages access right after, or silent leak. Add explicit
  port->use_count assert after handler before final nxt_port_use(-1).
• nxt_memzero(&engine,...) + partial init (only fast_work_queue): 🟡 risk: brittle. nxt_port_error_handler touching any
  other engine field → deref garbage. Comment claims only fast_work_queue needed — true now, undocumented assumption for
  future.
• socket_write fd-count vars after_open/after_fail/after_close: 🔵 nit: init = -1. Some gcc -Werror=maybe-uninitialized
  flags the combined if even though flow assigns them — could break build under unit's warning flags.
• nxt_port_fail_test_fd_is_open via fcntl(fd, F_GETFD): 🔵 nit: fd-number reuse means a closed-then-reopened fd reads
  "open". Low risk here (single-thread test), /proc/self/fd delta covers the real assert. Fine, just noting.
• nxt_port_fail_test_fd_count: 🔵 nit: opendir's own fd is counted; cancels across before/after so deltas correct, but
  add one-line comment — future reader will think count is off-by-one.

src/nxt_port_rpc.h:

• L40-ish: 🔵 nit: diff removes trailing blank line at EOF (- after #endif). Unrelated churn. Reinstate to keep diff
  scoped to test hooks.

❓ q: nxt_port_rpc_register_handler_ex alloc-fail injection returns NULL before any peer use_count bump — test asserts
use_count == 1. Confirm no peer-port use increment happens before the injected return point on insert-fail path too (reg
freed, but was peer use already bumped?).

Verdict: ready after addressing the two 🟡 risks (or documenting them). Nits optional.

[freeunitx]

Solid test coverage. Cleanup paths checked against real impl: nxt_port_release_send_msg only frees when allocated, error_handler use_delta math nets correct (use_count 2->1->0), msg->port=NULL safe (never deref'd), close_fd-first/completion-second ordering verified. No bugs found.

Two nits below. Neither blocks.

• nxt_cert.c / nxt_script.c file.fd = -1 after close: good defensive guard, matches audit pattern.
• fd-count /proc/self/fd invariant + NXT_LINUX guard + before >= 0 skip on non-Linux: correct.

[TuzelKO]

Additional review — findings not yet raised

Already covered in prior reviews (Gemini / phpclub / freeunitx): defensive file.fd = -1, heap-alloc test bufs, global counters not thread-safe, magic use_count = 2, partial-init engine, fcntl fd-reuse, opendir self-count, trailing newline, insert-fail bypasses the real default: branch, NULL data in error_handler wrapper.

Below — only findings not yet discussed.

---

CRITICAL

1. The test does NOT cover the production fast path — regression protection is weaker than it appears

nxt_port_socket_write2 (src/nxt_port_socket.c:269) first calls nxt_port_msg_chk_insert:

if (nxt_fast_path(port->socket.write_ready
                  && nxt_queue_is_empty(&port->messages)))
{
    res = NXT_DECLINED;  // fast path
}

On NXT_DECLINED the caller invokes nxt_port_write_handler(task, &port->socket, &msg) with a stack-allocated msg. nxt_port_msg_alloc is never called on this path.

In production, nxt_cert_store_get_handler / nxt_script_store_get_handler send their reply through a port that normally has write_ready = 1 and an empty queue → always fast path. Therefore nxt_port_test_msg_alloc_failures(1) injection never fires for the real production scenario.

The synthesised port from nxt_port_fail_test_port() is created via nxt_port_new, which does NOT set write_ready (mp_zalloc → 0), so the test always exercises the slow path. The test validates slow-path retain ordering, but the fix is intended for the common production case, which the test does not cover.

Concretely: if someone reverts nxt_mp_retain(mp) back to before nxt_port_socket_write in nxt_cert_store_get, nxt_port_fail_test_mp_baseline will not catch the regression for the real scenario — it would only fire if production happened to have write_ready=0 at the same time (rare).

Suggested fix: add a sub-test with port->socket.write_ready = 1 and inject failure inside nxt_port_msg_insert_tail or in the path after nxt_socketpair_send, so the fast-path scenario is actually covered.

---

2. port->pair[0] = 0 in the RPC test is the process's stdin fd

src/test/nxt_port_fail_test.c:228:

port->pair[0] = 0;

Done to pass nxt_assert(port->pair[0] != -1) inside nxt_port_rpc_register_handler_ex. But 0 is the real stdin fd. If any code path between this line and the port->pair[0] = -1 reset triggers nxt_port_close() (see src/nxt_port.c:96 — nxt_fd_close(port->pair[0])), the test process's stdin gets closed. The current explicit reset in the fail-path defends against this, but it is a fragile magic pattern in the same category as use_count = 2.

Suggested fix: port->pair[0] = open(\"/dev/null\", O_RDONLY) with an explicit close after the test — even a future refactor that weakens the fail-path reset can never touch stdin.

---

3. Failure-injection counters are not reset at the start of sub-tests

All three counters (nxt_port_test_msg_alloc_failure_count, nxt_port_rpc_test_alloc_failure_count, nxt_port_rpc_test_insert_failure_count) are global statics in different compilation units. Each sub-test assumes the reset was done by the previous sub-test or that the counter is 0 from init.

Today:

• socket_write resets at fail_close_port:, but if it fails earlier (e.g. via goto fail after a bad open(\"/dev/null\")) the counter is already set to 1 and leaks out at failure_count = 1.
• rpc_register resets both of its counters in the fail label. OK.
• mp_baseline resets after sender_pattern, but if it fails on retain_check the counter is already 0 (consumed). Fragile.

If socket_write leaks, it flows into rpc_register (different counter — lucky). But once a future sub-test reuses the same counter, it will see unexpected failure.

Suggested fix: defensive nxt_port_test_msg_alloc_failures(0) and both nxt_port_rpc_test_*_failures(0) at the very start of each sub-test.

---

MEDIUM

4. nxt_queue_insert_tail in the error_handler test is not under port->write_mutex

src/test/nxt_port_fail_test.c:377:

nxt_queue_insert_tail(&port->messages, &msg->link);

nxt_port_error_handler takes port->write_mutex for traversal. The real insertion paths (nxt_port_msg_chk_insert, nxt_port_msg_insert_tail) are also always under the mutex. The test violates the invariant. Currently single-threaded → safe, but it misleads readers and could race under future parallelisation.

Suggested fix: wrap the insert in nxt_thread_mutex_lock/unlock(&port->write_mutex), mirroring production code.

---

5. nxt_port_socket_write2 (two-fd) is not covered at all

All tests call nxt_port_socket_write — the inline wrapper with fd2 = -1. fd[1] ownership transfer on failure is never tested. A two-fd-specific bug introduced later will go undetected.

nxt_port_socket_write2 is real production code — used for pair[0]+pair[1] in nxt_port_send_port, for mmap fd + payload fd, etc.

---

6. `task->thread = thr` is set but never restored

Each sub-test does task->thread = thr; (4 times) and never restores. If the test framework expects task->thread to be NULL or something specific for later tests (nxt_clone_creds_test runs next), we leave a dangling pointer. Currently harmless, but it's a side effect.

---

7. Brittle fail_port cleanup for msg in the error_handler test

src/test/nxt_port_fail_test.c:432:

fail_port:
    if (msg != NULL) {
        nxt_queue_remove(&msg->link);  // <-- NULL deref if msg not inserted
        nxt_free(msg);
    }

nxt_queue_remove (src/nxt_queue.h:134) does (link)->next->prev = ... — crashes on a NULL link. Today this is guarded by the invariant "msg is either NULL or already past nxt_queue_insert_tail", but adding any failable operation between nxt_malloc(msg) and nxt_queue_insert_tail will break it.

Suggested fix: either nxt_queue_self(&msg->link) right after nxt_memzero, or if (msg->link.next != NULL) nxt_queue_remove(...).

---

MINOR

8. nxt_mp_test_retain_count marked NXT_EXPORT

src/nxt_mp.h:62:

NXT_EXPORT uint32_t nxt_mp_test_retain_count(nxt_mp_t *mp);

Under #if (NXT_TESTS), but if someone accidentally enables NXT_TESTS in a production build (or forgets it in auto/options), a test-only symbol leaks into the public ABI. For a test-only function plain extern is enough. Minor.

9. Injection API naming inconsistency

• nxt_port_test_msg_alloc_failures (port socket)
• nxt_port_test_run_error_handler (port socket)
• nxt_port_rpc_test_alloc_failures (rpc)
• nxt_port_rpc_test_insert_failures (rpc)

Some place _test_ in the middle, others use _rpc_test_. Unifying (e.g. nxt_port_test_* for everything) simplifies the mental model and a future central reset helper.

10. Failure counters not reset on the RPC test's success path

rpc_register exits the success path without nxt_port_rpc_test_alloc_failures(0) — relies on the counter being consumed to 0. If someone later sets (2) or (N) for some regression, it leaks to the next test. Defensive issue, not a bug.

---

Summary

The PR is solid overall; the ownership invariants really are pinned down. The most important unraised concern is (1): formally, the test does not prove what the changelog claims. For the real production case in cert_store_get / script_store_get, nxt_port_msg_alloc is not normally called, so the retain-reorder fix remains "protected by code review only", not by a runtime regression test.

The rest is hygiene and future-proofing. I don't see merge blockers in this batch.

[andypost]

rebased

[chatgpt-codex-connector]

💡 Codex Review

freeunit/src/nxt_port.h

Lines 342 to 344 in 187ef84

	* On any other return, ownership remains with the caller, which is
	* responsible for closing fd/fd2 if owned and dispatching b's
	* completion handler.

Narrow the failure ownership contract

When port->queue is non-NULL and the payload enqueue succeeds with notify != 0, nxt_port_socket_write2() has already consumed b and queued its completion before it can still return NXT_ERROR/NXT_AGAIN from the follow-up socket notification path (msg.buf is then cleared). This new contract says the caller owns and must complete b on any non-NXT_OK return, so callers following it can double-complete/free the buffer in that queue-notification failure case; either the implementation needs to defer ownership transfer until the final return is known, or the contract must exclude the already-enqueued queue path.

ℹ️ 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".