router: keep a worker the deadline gave up on out of the idle economy

[andypost]

Second half of #347; the first half (arming the deadline for a request parked in app->ack_waiting_req, and retracting it with a CAS before answering) is #405, which this is stacked on. No change to libunit, to the language modules or to the configuration.

The branch is rebased onto the current base tip, 2db15e78: fix/321-detached-worker-lifecycle was rewritten after this branch was cut (a8fe289f is no longer an ancestor), so the diff against the base is the seven files below rather than a merge of the two histories. The release-build recovery in nxt_router_adjust_idle_timer() that the rebase brings in is present and untouched.

B2 first: the finish edge does not cover this

The hard exit does not apply, and this is the chain that shows it.

A limits.timeout expiry for a request a worker is running falls through nxt_router_app_timeout() (src/nxt_router.c:7919-7921):

    nxt_http_request_error(task, r, NXT_HTTP_SERVICE_UNAVAILABLE);

    nxt_request_rpc_data_unlink(task, req_rpc_data);

nxt_request_rpc_data_unlink() (:1272) calls nxt_router_app_port_release() (:1282) with apr_action == NXT_APR_REQUEST_FAILED, set when the request was parked (:7327). That case is dec_requests = 1; inc_use = -1 (:6785-6789), and the release then does:

    main_app_port->active_requests -= got_response + dec_requests;   /* :6894 */
    app->active_requests -= got_response + dec_requests;             /* :6895 */
    ...
    adjust_idle_timer = nxt_router_app_port_idle(task, app, main_app_port);  /* :6903 */

nxt_router_app_port_idle() is the idle transition, and its guard is (:6644-6649):

    if (port->pair[1] != -1
        && port->active_requests == 0
        && port->active_websockets == 0
        && port->detached == 0
        && port->idle_link.next == NULL)

The finish edge that would set the detached term is emitted by libunit's nxt_unit_ctx_detached_done() (src/nxt_unit.c:3691), and it returns immediately unless a detached response was reported first:

    if (ctx_impl->detached == NXT_UNIT_DETACHED_NONE) {   /* :3712-3714 */
        return;
    }

That flag is set only by nxt_unit_request_done_detached() (:3612), called from fastcgi_finish_request() (src/nxt_php_sapi.c:306). A request that is simply slow emits neither edge, so nothing holds the port out of idle_ports: the deadline does release a worker that is still running. The exit does not apply and code is needed.

What changes

The guard term is port->detached == 0. When the acknowledgement has moved the request's port off the shared queue and onto a worker -- the fact nxt_router_msg_retract() already reads at :1197-1200 -- the deadline marks that worker's main port detached before releasing the request (nxt_router_app_abandon(), :6703). The release then still runs, but nxt_router_app_port_idle() finds the detached term false and does not insert the port into spare_ports or idle_ports, so the reaper never sees it and the port keeps counting against "processes": {"max"}. nxt_router_app_port_busy() is called under the same app->mutex for the port that is already in a queue, as the acknowledgement and the detached START edge do.

The state is settled by

• the worker's last message for the stream -- the message whose answer nobody wants, which nxt_router_response_ready_handler() already drops (:5369-5385). The registration is kept alive until then (rpc_cancel cleared before the unlink) because the unlink would cancel it and the port layer would then drop the answer with nothing left to settle the port;
• the port closing, which settles a detached worker today (nxt_router_app_port_close(), :6993-6999);
• or an application START edge, which clears the router's mark and hands the clear to its own FINISH edge (nxt_router_detached_apply(), :8039 and :8078). That is what keeps a worker which answered and kept running from being marked free by the response to the request the router gave up on: the existing case test_php_detached_start_after_the_worker_went_idle now finds the port already held out instead of parked.

The late reply (B3)

With the registration kept, a response that arrives after the unlink reaches nxt_router_response_ready_handler() with req_rpc_data->request == NULL. The handler returns without touching the client (:5369-5385), closes no descriptor it did not open (the port dispatcher completes mmap buffers the handler leaves, src/nxt_port_socket.c:2049-2067), and touches no freed accounting: the request's pool and its app reference are already released, and the only state it reads is the registration's own. The last message settles the port once; a later message cannot settle it twice, because the pointers are cleared before anything else runs and the port bit is checked under app->mutex.

A second expiry cannot produce a second 503 or a second release: the fall-through unlinks, and nxt_request_rpc_data_unlink() re-arms r->timer for the pool release (nxt_router_http_request_release_post()), so the deadline handler is no longer armed for that request.

limits.timeout (B4)

The user-facing documentation (unit-docs, source/configuration/index.rst) says:

timeout -- Integer; request timeout in seconds. If an app process exceeds it while handling a request, Unit cancels the request and returns a 503 "Service Unavailable" response to the client.

This option covers a request that the router still tracks. Unit doesn't detect freezes. If an app process hangs after the request is finished, it stays in the app's process pool. To limit how long Unit waits for an app to start, use start_timeout.

What this change alters is not the knob's meaning but its consequence: the cancellation still happens at the same moment and still answers 503, but it no longer returns the worker to the idle economy. The note's "it stays in the app's process pool" becomes true for a worker that is still executing the cancelled request; today it can be reaped instead.

The issue's scope note answers "does a post-acknowledgement timeout abandon the worker?" for this half -- only "the worker finished" should return capacity -- which is what this implements. What is not decided anywhere, and is left as an open question here, is what should eventually happen to that worker: this PR keeps it counted and waits for it to answer or die. If the intent is that the router should tell it to stop (a cancel message, a kill), that is a separate decision and this PR does not make it.

What does not change

• The 503 and its timing, the client sees exactly one answer, and the request still cannot be executed twice.
• Before the acknowledgement the router does not know which worker holds the request, so that window is unchanged. It is the one-CAS race src/test/nxt_router_app_timeout_test.c drives, where the request is retracted or claimed by a worker that then has to answer; the deadline still bounds the wait.
• /status gains no field. A worker in this state is reported under the existing detached count, which is the router's own view of a worker it may not hand out. That is the one semantic extension in this PR and it is deliberate: the count now means "held out of the idle economy", not only "the application said it was still running".

Verification

Environment: freeunit-harness/ci alpine base plus python3-dev openjdk21 nodejs npm openssl, ./configure --tests --openssl --debug then the php module, built in the same image as the run.

what	before (pre-fix head a8fe289f, the branch point at the time)	after
pytest -q test/test_php_detached_max.py	13 collected, the new test fails	13 passed
pytest -q -k test_php_timed_out_request_keeps_the_worker_busy	FAILED: the worker running the failed request is not held out of the idle economy: {'running': 0, ..., 'idle': 0, 'detached': 0} (the worker had already been reaped)	passed
./build/tests (C unit tests, incl. router app timeout test)	--	exit 0, router app timeout test passed

Re-run after the rebase onto 2db15e78 (full rebuild, rm -rf build): test_php_detached_max.py 13 passed, ./build/tests exit 0, and test_app_start_timeout.py + test_status.py 12 passed / 6 skipped.

The new case holds the worker 12 s against a 2 s limits.timeout and then answers normally, so no detached edge reports anything. It asserts, 4 s after the 503 and with the script still running (a done marker says so), that /status shows detached: 1 and idle: 0, that the process is still there, that the ran marker was appended exactly once, and that the port settles when the answer arrives.

Wire protocol

Nothing new, nothing renumbered.

• Every NXT_PORT_MSG_* value is identical to 2db15e78 (and to a8fe289f: the base rebase does not touch src/nxt_port.h); the list in src/nxt_port.h is unchanged.
• nxt_port_handlers_t is byte-identical, in the same order, with the same size; NXT_PORT_MSG_MAX = sizeof(nxt_port_handlers_t) does not move.
• The only additions are an internal field (nxt_port_t.detached_router) and two pointers on the router's private nxt_request_rpc_data_t.

Review

The branch is one squashed commit. Four review rounds are in the comments; what they changed is in it:

• detached_router counts every abandoned request instead of holding one port-wide flag, and the application's reason moved to its own bit: two concurrent requests that exceed limits.timeout on one worker each hold a reason, and the port leaves the detached state only when the last of them is answered (or the port closes) and no detached work of the application's own is running;
• both application references and the port reference are taken before the critical section that publishes port->detached, the order nxt_router_detached_apply() already uses: a port close taking the same mutex could otherwise drop a reference this call had not taken yet, and the increment that followed would resurrect an application whose free was already queued;
• the nxt_router_app_port_busy() call in the abandon path is commented as defensive (a port with a live request is not in an idle queue, so it normally unwinds nothing and starts nothing), and the state's main_app_port identity is recorded next to the pid-keyed one the detached edge uses;
• the new test's observation window is twice as wide and checks itself with a done marker, so a loaded runner fails loudly instead of reading /status after the script answered;
• detached_router is as wide as active_requests (uint32_t): threads is validated up to NXT_INT32_T_MAX, and a wrap would leave a settle unable to clear the state at all. The struct is the same size either way -- the four bytes of padding after active_requests move ahead of the counter;
• the fixture's before=N comment and the test_php_detached_start_after_the_worker_went_idle docstring describe where idle_ports can still be reached now that the abandon holds the port out first, so the nxt_router_app_port_busy() unwind is not read as dead code.

Known limitation, inherited (not fixed here)

detached_app is still a flag, and libunit's detached state is per-context: with threads > 1 a worker emits START, START, FINISH, FINISH for two concurrent fastcgi_finish_request() calls, and the first FINISH clears the port while the second context is still running its detached work. That is fix/321-detached-worker-lifecycle's state, not this change's, and making it a count needs one thing established first: libunit retries an edge whose send failed, and a duplicate START that a flag absorbs would inflate a count and pin the worker out of the idle economy for good. Auditing nxt_unit_send_detached() / nxt_unit_ctx_detached_retry() for that is #405's path, so it is recorded here rather than changed.

The imprecision now has a second clearing site: nxt_router_app_abandoned_settle() reads detached_app to decide whether the last router reason may clear the state, so a FINISH that ends one context's work can let a settle return the port to the idle economy while another context is still running. Same root cause, different caller; the eventual fix touches both.

The count itself has no test either. A deterministic one needs either a Python ASGI fixture that holds two concurrent requests past limits.timeout (PHP has no threads option in this tree) or a C-level case driving two abandons on one port, which needs nxt_router_app_abandon() exported for tests the way nxt_router_test_app_timeout() is. test_php_timed_out_request_keeps_the_worker_busy pins the mechanism end to end for one request; the count itself is verified by reading.

Files

 CHANGES                            |   5 +-
 docs/changes.xml                   |   5 +-
 src/nxt_port.h                     |  22 ++++
 src/nxt_router.c                   | 231 +++++++++++++++++++++++++++++++++++--
 src/nxt_router_request.h           |  11 ++
 test/php/detached_worker/index.php |  38 ++++--
 test/test_php_detached_max.py      | 117 +++++++++++++++++--
 7 files changed, 401 insertions(+), 28 deletions(-)

[andypost]

@claude review

[claude]

Claude finished @andypost's task in 7m 10s —— View job

---

Review: PR #412 — keep a worker the deadline gave up on out of the idle economy

• Gather context (diff vs fix/321-detached-worker-lifecycle)
• Review src/nxt_port.h / src/nxt_router_request.h additions
• Review src/nxt_router.c abandon / settle / apply paths
• Trace reference and counter balance across all four settle paths
• Review tests
• Post review

I read the whole chain rather than the diff alone: nxt_router_app_timeout() → nxt_router_app_abandon() → nxt_request_rpc_data_unlink() → nxt_router_app_port_release() → nxt_router_app_port_idle(), plus every settle site (nxt_router_response_ready_handler(), nxt_router_response_error_handler(), nxt_router_app_port_close(), nxt_router_detached_apply()) and the RPC registration lifetime in src/nxt_port_rpc.c.

The core argument holds. The B2 exit really does not apply — a merely slow request emits no detached edge, so port->detached == 0 at src/nxt_router.c:6644 and the release parks a busy worker. The mechanism chosen (reuse the detached state the reaper already refuses to touch, via the nxt_assert(port->detached == 0) at :7039) is the right lever.

I checked the two things most likely to be wrong and both are correct:

• Registration lifetime. Clearing rpc_cancel before the unlink does not leak: nxt_port_rpc_handler() deletes and frees the registration itself on last != 0 (src/nxt_port_rpc.c:419,461), the peer is set to the worker pid at acknowledgement time (src/nxt_router.c:5686) so nxt_port_rpc_remove_peer() reaches the error handler on worker death, and nxt_port_rpc_close() sweeps the rest at shutdown. The nxt_request_rpc_data_t is nxt_mp_zalloc'd (src/nxt_port_rpc.c:215), so the two new pointers start NULL without an explicit initialiser.
• Reference balance. All four paths net out: changed → two app refs, settled as 1+1; changed == 0 → one, settled as 0+1; the START/FINISH handover works because nxt_router_detached_apply() uses drop = 2 on a changed FINISH, which returns exactly the detached ref nxt_router_app_abandon() took. Port refs balance too.

One finding below is, I think, a real hole in the mechanism.

---

1. detached_router is a flag, so the first settle frees a port that other abandoned requests are still running on — src/nxt_port.h:357, src/nxt_router.c:6706, :6753

main_app_port->active_requests is incremented per request (:5721), and apps with threads > 1 run several requests in one process, each with its own limits.timeout deadline. Two concurrent expiries on one process:

• first nxt_router_app_abandon(): port->detached 0→1, detached_router = 1, detached_processes++;
• second: port->detached is already 1, so changed == 0 and no second mark is taken — the flag is already 1 and there is nothing to count;
• both nxt_request_rpc_data_unlink()s run nxt_router_app_port_release(), so main_app_port->active_requests is back to 0;
• request A answers → nxt_router_app_abandoned_settle() sees detached_router != 0, clears detached, and nxt_router_app_port_idle() finds active_requests == 0 && detached == 0 → the port goes into idle_ports while request B is still executing. The reaper then QUITs a worker mid-request, which is the failure this PR exists to prevent.

Same root cause, second manifestation: if port->detached was already 1 from an application START edge when the abandon happens, changed == 0 leaves detached_router == 0 entirely, and the application's FINISH edge then returns the port to the idle economy while the abandoned request still runs. And because the mark is per-port while the abandonment is per-request, the port->detached_router = 0 handover at :7980 can be performed by a START edge belonging to a different context of the same worker.

None of this is reachable from the PHP tests (single-threaded, one request at a time), which is why the suite passes. Suggested shape: make detached_router a uint32_t count, increment it unconditionally in the abandon, decrement it in the settle, and clear port->detached only when the count reaches 0 and the application is not itself detached — which also means the START-edge handover has to move the whole count rather than zero it.

Fix this →

2. nxt_router_app_port_busy(task, app, port, "abandoned") is unreachable work — src/nxt_router.c:6714

A port with a live request cannot be in idle_ports/spare_ports: nxt_router_app_port_idle() only inserts when active_requests == 0, and the acknowledgement already took it out with nxt_router_app_port_busy(..., "ack") at :5719. So nxt_queue_chk_remove() always returns 0 here, start_process is always 0, and the nxt_router_start_app_process() call below it is dead. Harmless and symmetric with the START edge, but worth a one-line comment saying it is defensive — otherwise a reader assumes a timeout can trigger a replacement spawn, which it cannot.

3. app_port->main_app_port is the field nxt_router_detached_apply() deliberately avoids — src/nxt_router.c:6700

The comment at :7900 says that field "is never cleared, and the main port can be released while a sibling port of the same process is still registered", which is why the detached edge looks the port up by pid instead. nxt_router_app_port_release() follows the same pointer at the same instant, so this is not new — but the abandon additionally pins state and a reference on that object for an unbounded time, and nxt_router_detached_apply() will later act on whatever nxt_runtime_port_find() returns, which may not be the same object. The app-level counter stays balanced either way, so the impact looks confined to a desynchronised per-port flag on a port that is out of the idle lists regardless; the lookup alternative is not available here anyway (rt->ports is main-thread-only and the abandon runs on the request's engine). Worth a sentence in the comment acknowledging the divergence, since the two functions now write the same field from two different port identities.

4. Test margin is about 2 s — test/test_php_detached_max.py:373

hold = 8 against a 503 at ~timeout (2 s) plus time.sleep(IDLE_TIMEOUT * 2) (4 s) plus four app_processes() round-trips leaves roughly 2 s before the script answers and the assertions become meaningless (they would fail rather than pass spuriously, so it is a flake risk, not a false green). A larger hold, or deriving it from the measured elapsed, would be steadier on a loaded runner.

5. Nit — declaration order, src/nxt_router.c:6741

    nxt_app_t   *app;
    nxt_port_t  *port;
    nxt_bool_t  adjust_idle_timer, ours;

ascends in length; the file's convention (and nxt_router_app_abandon() right above it) is descending.

---

Things I checked that are fine

• No second 503 / second release: the unlink nulls r->timer_data and re-arms r->timer for the release handler, so nxt_router_app_timeout() is no longer the armed handler.
• The late-reply path closes its descriptors at the top of nxt_router_response_ready_handler() before the r == NULL branch, and settles once — the pointers are cleared before anything else runs.
• nxt_router_response_error_handler() calling the settle before nxt_request_rpc_data_unlink() is safe on an already-unlinked rpc_data: msg_info.buf is NULL, app_port/request/app are NULL, body_fd is -1, rpc_cancel is 0, so the second unlink is a no-op.
• nxt_assert(app->detached_processes == 0) in nxt_router_free_app() (:7184) cannot fire while a worker hangs forever, because the abandon's own references keep the app alive.
• The pre-acknowledgement window (retract CAS lost, worker running, router does not know which port) is genuinely unfixable here and the PR says so.

The /status semantic extension — detached now meaning "held out of the idle economy" rather than "the application said so" — reads right to me, and the open question about what should eventually happen to the worker is the correct thing to leave open.
· branch fix/347-timeout-accounting

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e199df1021

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

[andypost]

Addressed in 581c73f.

detached_router is a count now, one per abandoned request, and the application's own reason moved to its own bit (detached_app). Two concurrent requests that exceed limits.timeout on one worker each hold a reason: the first answer decrements the count and leaves the port detached, and only the last one to settle -- with no detached work of the application's own running -- clears it and returns the port to the idle economy. A FINISH edge cannot drop a request's reason either, and the port closing clears all three at once.

Re-verified with the count: pytest -q test/test_php_detached_max.py 13 passed, ./build/tests exit 0 including router app timeout test passed.

[andypost]

@claude review

[claude]

Claude finished @andypost's task in 4m 27s —— View job

---

Review: PR #412 — keep a worker the deadline gave up on out of the idle economy

• Gather context (diff vs base, incl. the new commit 581c73f)
• Re-check the counting fix against my earlier finding SIGSEGV (signal 11, core dump) with Flask + MySQL [#1606] #1
• Review the settle / close / apply paths for the three-reason state
• Trace reference and counter balance across all reason orderings
• Review tests
• Post review

---

581c73f resolves finding #1

I re-traced the state machine with detached (the state), detached_app (the application's reason) and detached_router (the count of requests the router gave up on) as three separate things, and exercised every interleaving I could construct. The two concurrent expiries that broke the flag are fixed, and so is the second manifestation I raised — an abandon landing on a port that is already detached for the application's own reason now records its own reason instead of silently taking none.

Reference accounting stays balanced in all six orderings, which is the part most likely to have broken when the clear grew conditions. The invariant that makes it work: exactly one "detached" app reference exists while port->detached != 0, held by whoever set it, and returned by whoever clears it.

order	who takes the detached ref	who returns it
abandon, settle	nxt_router_app_abandon() (changed)	settle (ours)
abandon ×2, settle ×2	first abandon	second settle (detached_router 1→0)
abandon, START, FINISH, settle	abandon	settle — FINISH is a no-op (drop = 1) while detached_router != 0
abandon, START, settle, FINISH	abandon	FINISH (changed, drop = 2)
START, abandon, settle, FINISH	START (drop = 0)	FINISH (drop = 2)
START, abandon, FINISH, settle	START	settle (ours)
any, then port close	—	nxt_router_app_port_close(), which now zeroes all three (:6963-6966)

The handover in both directions is the interesting case and both work: nxt_router_detached_apply()'s FINISH at :8033 refuses to clear while the router still has reasons, and the settle at :6782 refuses while the application does. Also correct that the settle only decrements under if (port->detached_router != 0) — after a port close zeroed it, a straggling settle drops its own two references and touches nothing else. And nxt_router_app_port_idle() still gates on active_requests == 0, so clearing the last reason cannot park a port that has ordinary live requests on it.

Field layout is uint8_t / uint8_t / uint16_t / uint32_t — nxt_port_t gains nothing in size.

Everything from my previous pass still holds: registration lifetime, the late reply, no second 503, and the nxt_router_response_error_handler() → settle → unlink order being safe on an already-unlinked rpc_data.

One new finding, of the same shape as the one just fixed.

---

1. detached_app is still a flag, so one FINISH edge ends another context's detached work — src/nxt_port.h:356, src/nxt_router.c:7994, :8025

libunit's detached state is per-context, not per-process: ctx_impl->detached (src/nxt_unit.c:448) is set by nxt_unit_request_done_detached() (:3644) and cleared by nxt_unit_ctx_detached_done() (:3680), and nxt_unit_send_detached() reports on each edge. With threads > 1 a process has several contexts, so two concurrent fastcgi_finish_request() responses in one worker emit START, START, FINISH, FINISH — all four mapping onto the same port, since nxt_router_detached_apply() is keyed by pid.

The router collapses them onto one bit:

• START₁: detached_app = 1, detached 0→1, changed = 1, keeps the ref;
• START₂: detached_app is already 1 and detached != 0, so changed = 0 and no reason is recorded for context 2;
• FINISH₁: detached_app = 0, detached_router == 0, detached != 0 → clears, detached_processes--, nxt_router_app_port_idle() → the port goes into idle_ports while context 2 is still running its detached work, and the reaper can QUIT it;
• FINISH₂: detached == 0, changed = 0, no-op.

References balance; the state does not. This is the exact failure mode you just fixed one field over, and it is inherited from fix/321-detached-worker-lifecycle rather than introduced here — but the commit message's reasoning ("a count rather than a flag because one worker can run several of them at once") applies verbatim to detached_app, and having the two reasons side by side with different arities is now the most confusing thing in the struct. Making both uint16_t counts, incremented on START and decremented on FINISH (with the same "clear only when both reach zero" rule), would make nxt_router_detached_apply() and nxt_router_app_abandoned_settle() structurally identical.

One wrinkle if you do: libunit retries a failed START/FINISH (nxt_unit_ctx_detached_retry(), src/nxt_unit.c:3716), and a duplicate START delivered on the wire would inflate a count where it is idempotent against a flag. The retry only fires when the previous send failed, so a duplicate should not be observable — worth confirming before switching, since the flag currently absorbs that class of mistake and a count would not.

Fix this →

2. Nothing in the suite can tell the count from the flag — test/test_php_detached_max.py

test_php_timed_out_request_keeps_the_worker_busy is a good test of the mechanism, but it drives a single request on a single-threaded worker, so it passes identically against e199df1's flag. The bug the count exists for needs two requests over limits.timeout in one process at once, which needs threads > 1 in the app config. Since src/test/nxt_router_app_timeout_test.c already exists and drives this area directly, a C-level case — two nxt_router_app_abandon() calls on one port, settle one, assert the port is still out of idle_ports, settle the other, assert it is back — would be cheaper and more deterministic than a threaded PHP test. Without either, the next refactor can quietly turn the count back into a flag.

3–5. Still open from the previous pass (all minor)

• nxt_router_app_port_busy(task, app, port, "abandoned") is unreachable work — src/nxt_router.c:6714. A port with a live request cannot be in idle_ports/spare_ports, so start_process is always 0 and the nxt_router_start_app_process() below it is dead. A one-line "defensive, symmetric with the START edge" comment stops a reader concluding a timeout can trigger a replacement spawn.
• app_port->main_app_port is the field nxt_router_detached_apply() deliberately avoids — :6700 vs the comment at :7912. Not new (nxt_router_app_port_release() follows the same pointer at the same instant) and the app-level counter stays balanced either way, but the abandon now pins per-port state and a reference on that object for an unbounded time while the detached edge acts on whatever nxt_runtime_port_find() returns. Worth a sentence acknowledging that the two functions write the same field from two different port identities.
• Test margin is about 2 s — test/test_php_detached_max.py:373. hold = 8 minus a 503 at ~2 s, time.sleep(IDLE_TIMEOUT * 2) (4 s) and four app_processes() round-trips. It would fail rather than pass spuriously, so it is a flake risk, not a false green; deriving hold from the measured elapsed would be steadier on a loaded runner.

6. Nit — declaration order, src/nxt_router.c:6741

    nxt_app_t   *app;
    nxt_port_t  *port;
    nxt_bool_t  adjust_idle_timer, ours;

ascends in length; the file's convention (and nxt_router_app_abandon() right above it) is descending.

---

Not verified here

I reviewed by reading; I could not configure or build in this job (./configure is not in my allowed tools, and there is no pre-existing build/). The 13 passed / ./build/tests exit 0 results are yours as reported, not reproduced by me.
· branch fix/347-timeout-accounting

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 581c73f24f

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

[andypost]

Round 2 addressed in d0f14cd.

1. detached_app stays a flag, and the PR says why. Your analysis is right, and the fix is the same shape as the one I just made -- but the state is fix/321-detached-worker-lifecycle's, not this change's, and the wrinkle you flagged is the reason not to touch it here: libunit retries an undelivered edge, and a flag absorbs a duplicate START where a count would inflate and pin the worker out of the idle economy for good. Confirming that a duplicate START cannot arrive means auditing nxt_unit_send_detached() / nxt_unit_ctx_detached_retry() against a partially-delivered send, which is #405's path and not this PR's. I have recorded it in the body as inherited and unfixed rather than silently leaving it for the next reader.

2. The count has no test, and the body says so. The two deterministic routes both need something this PR does not have: PHP has no threads option in this tree (nxt_conf_vldt_php_common_members, src/nxt_conf_validation.c:1077), so a threaded test needs the Python module plus a concurrent-delay ASGI fixture; and a C-level case needs nxt_router_app_abandon()/_settle() exported for tests the way nxt_router_test_app_timeout() is, plus a worker port in the fixture. Both are worth doing and neither belongs in this diff. What the new test does pin is the mechanism end to end on one request; the count is verified by reading, and the body says which is which.

3-5. The nxt_router_app_port_busy() call is now commented as defensive and symmetric, with why start_process is normally 0; the main_app_port note is in nxt_router_app_abandon()'s comment, naming the pid-keyed identity the detached edge uses; and the observation window is twice as wide with a self-check -- the script writes a done marker before it answers and the test asserts it is absent when the counters are read, so a loaded runner fails with "the script finished within 4s of the deadline; raise hold" instead of reading /status late.

6. Declaration order fixed.

Re-verified on d0f14cd: test_php_detached_max.py 13 passed, ./build/tests exit 0 including router app timeout test passed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0f14cdb38

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

[andypost]

@claude review

[claude]

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

---

Review: PR #412 — keep a worker the deadline gave up on out of the idle economy

• Gather context (branch point, base tip, per-commit diff)
• Re-check the round-2 items against d0f14cd
• Re-verify reference/counter balance with detached_app + detached_router
• Cross-check the Codex P1 on reference ordering
• Review tests
• Post review

---

First, a branch-state problem that outweighs the code

origin/fix/321-detached-worker-lifecycle is at 2db15e7, and this branch's parent commit is a8fe289. They are not related by ancestry — both carry the subject "unit: retry detached lifecycle notifications", so the base was amended/force-pushed after this branch was cut. git merge-base reports none, which is why GitHub's file list for this PR is 18 files with src/nxt_unit.c, src/nxt_unit.h, src/nxt_php_sapi.c and src/test/nxt_unit_port_recv_test.c in it, and why Codex's two nxt_unit.c P1s landed on a PR whose own three commits never touch libunit.

git diff 2db15e7 a8fe289 is 7 files, +87/−493. The base tip has work this branch point does not, including one piece that is directly in this PR's blast radius — the release-build recovery in nxt_router_adjust_idle_timer():

        nxt_assert(port->detached == 0);

        if (nxt_slow_path(port->detached != 0)) {
-            /*
-             * A release build.  A bare continue selects the same link again
-             * and spins with app->mutex held. ...
-             */
-            nxt_queue_remove(lnk);
-            lnk->next = NULL;
-            app->idle_processes--;
            continue;
        }

That block exists on the base tip and not here. This PR is the change that makes "a detached port in the idle queue" a state the router can reach for a new reason, so losing the release-build guard in the merge would be the worst possible regression to pick up. nxt_atomic_int_t c → nxt_atomic_t c in nxt_router_detached_apply() is in the same diff.

Please rebase onto the current fix/321-detached-worker-lifecycle before this is merged, and re-run the suite there — the three-way merge will conflict in nxt_unit.c and nxt_router.c, and a conflict resolved the wrong way silently reverts base work. I reviewed the three commits a8fe289..HEAD on their own terms below; everything I say is about those.

---

Round 2 items: all addressed

d0f14cd does what it says. The nxt_router_app_port_busy() call is commented as defensive with the reason start_process is normally 0; the main_app_port identity note is in the function comment naming the pid-keyed alternative; declarations descend again; and the test now runs hold = 12 with a done marker asserted absent at the observation point, so a slow runner fails with "raise hold" instead of reading /status after the script answered. That is the right shape — the margin is now ~6 s and self-checking rather than ~2 s and silent.

The detached_app-is-still-a-flag limitation and the missing test for the count are both recorded in the body with the reason for deferring. I agree with deferring the first: a count that a retried START could inflate would pin a worker out of the idle economy permanently, which is strictly worse than the flag's current failure, and confirming that needs the libunit audit. (Amusingly, the base tip you are not yet rebased onto is where that audit lives.)

I re-traced the reference invariant — one detached app reference exists while port->detached != 0, held by whoever set it — across all six abandon/START/FINISH/settle orderings plus port close, and it still balances, including the two handovers (drop = 2 on a changed FINISH returning the abandon's reference, and the settle declining to clear while detached_app != 0).

One finding stands.

---

1. nxt_router_app_abandon() publishes port->detached before it holds either reference — src/nxt_router.c:6727–6745

This confirms Codex's P1 on :6730, and the port reference has the same problem.

    port->detached_router++;
    start_process = nxt_router_app_port_busy(task, app, port, "abandoned");

    nxt_thread_mutex_unlock(&app->mutex);          /* :6727 — state is now public */

    if (changed) {
        nxt_router_app_use(task, app, 1);          /* :6730 — the detached ref */
    }
    ...
    nxt_router_app_use(task, app, 1);              /* :6744 */
    nxt_port_inc_use(port);                        /* :6745 */

nxt_router_app_timeout() runs on the request's engine; nxt_router_app_port_close() runs on the main thread and takes the same app->mutex. Once the unlock at :6727 retires, the close path can observe port->detached and execute its settle (:6969-6974 clearing all three, then nxt_router_app_use(task, app, -1) at :7003) — returning a reference the abandon has not taken yet. At that instant the only app reference this request contributes is req_rpc_data->app, so after a configuration removal has dropped the configuration's reference, use_count goes 1 → 0, nxt_router_free_app() is posted to app->engine, and the abandon then resurrects a queued-free application with +1 and hands it to nxt_router_start_app_process().

The file already knows this hazard and defends against it one function away — nxt_router_detached_apply() spends a whole comment and a CAS loop on it, and takes its reference before the mutex so that drop = 0 on a changed START merely keeps what it already holds. The abandon inverted that order.

The port reference has the same shape and is not saved by the rpc_data. nxt_router_req_headers_ack_handler() resolves app_port by (pid, reply_port) (:5703) and takes its reference inside the critical section (:5723); port here is app_port->main_app_port, which for a worker with a non-zero reply port is a different object that the rpc_data holds no reference on at all.

Both are one-line moves:

    port = app_port->main_app_port;

    nxt_router_app_use(task, app, 2);   /* the rpc_data's, and the detached one */

    nxt_thread_mutex_lock(&app->mutex);
    ...
    nxt_port_inc_use(port);             /* as the ack does, under the lock */
    nxt_thread_mutex_unlock(&app->mutex);

    if (!changed) {
        nxt_router_app_use(task, app, -1);   /* the detached ref was not needed */
    }

Dropping a reference you demonstrably held is always safe; taking one after you have published the state that authorises someone else to drop it is not. nxt_router_app_use(task, app, 1) is a bare nxt_atomic_fetch_add (:6522) and takes no lock, so it is fine inside the critical section if you prefer that placement.

Fix this →

---

2. Minor — req_rpc_data->app is dereferenced with no guard where app_port gets one — src/nxt_router.c:7869

    if (req_rpc_data->app_port != NULL
        && req_rpc_data->app_port->id != NXT_SHARED_PORT_ID)
    {
        nxt_router_app_abandon(task, req_rpc_data->app, ...);

nxt_router_app_abandon() immediately locks app->mutex. The handler already derefs app->timeout unconditionally on the CLAIMED path above, so this is consistent with what is there — but the new condition null-checks one field and not the other, which reads as if app might be NULL. An nxt_assert(req_rpc_data->app != NULL) would settle it either way.

3. Note, not a defect — a permanently hung worker now holds its slot forever

app->processes still counts the abandoned port, so nxt_router_app_can_start() will not start a replacement. With "processes": {"max": 1} and a worker that never answers, the application is now blocked indefinitely where today the reaper would (wrongly, but effectively) QUIT it and a fresh process would take over. The body calls this out as the deliberate open question and I agree with the choice, but it is a user-visible behaviour change on the failure path and the CHANGES/docs/changes.xml entry should say so in a clause — "the worker is no longer returned to the idle pool and continues to count against processes.max until it answers or exits" — rather than leaving operators to discover it from a stalled app.

---

Things I checked that are fine

• nxt_router_response_ready_handler() settles only on port_msg.last != 0, after nxt_port_recv_msg_close_fds(), and nxt_port_rpc_handler() frees the registration (and therefore the nxt_request_rpc_data_t) only after the handler returns (src/nxt_port_rpc.c:450-464), so the settle cannot run against freed state and cannot run twice — the pointers are nulled before the lock is taken.
• Clearing rpc_cancel before the unlink does not leak the registration: last, nxt_port_rpc_remove_peer() on worker death (peer set at ack, :5686), and nxt_port_rpc_close() at shutdown all reach it.
• nxt_router_app_abandoned_settle() before nxt_request_rpc_data_unlink() in the error handler is safe on an already-unlinked rpc_data (app_port/request/app NULL, body_fd −1, rpc_cancel 0).
• The NXT_SHARED_PORT_ID discriminator matches the one nxt_router_msg_retract() uses (:1200), so the pre-acknowledgement window behaves exactly as before.
• A straggling settle after nxt_router_app_port_close() zeroed the three fields decrements nothing and drops only its own references.
• nxt_router_app_port_idle() still gates on active_requests == 0, so clearing the last reason cannot park a port with ordinary live requests.
• nxt_port_t gains no size: uint8_t / uint8_t / uint16_t lands ahead of the existing uint32_t active_requests.
• "running": 1 in the new test is app->processes (src/nxt_status.c:222), which is the right assertion for a detached-but-alive worker.

Not verified here

Read-only review; I did not configure or build, so the 13 passed and ./build/tests exit 0 results are yours as reported. Given the branch-state issue above, those numbers describe a8fe289..d0f14cd and not what will actually be merged — worth re-running after the rebase.

--- · branch fix/347-timeout-accounting

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67adeb3e22

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

[andypost]

Round 3 addressed in the rebased, squashed head (d1198aab).

Branch state. Rebased onto the current base tip 2db15e78, so the PR is back to seven files (the five it owns plus the CHANGES/docs/changes.xml clause below) instead of a merge of the two histories. I re-checked the release-build recovery in nxt_router_adjust_idle_timer() is present and untouched, and the whole suite was re-run on the rebased tree after rm -rf build: test_php_detached_max.py 13 passed, ./build/tests exit 0, test_app_start_timeout.py + test_status.py 12 passed / 6 skipped.

1. Reference ordering. Fixed exactly as you and Codex describe, and it was a real hole: both application references (nxt_router_app_use(task, app, 2)) are now taken before app->mutex is taken, so the state is never published before the references exist; nxt_port_inc_use(port) moved inside the critical section, as nxt_router_req_headers_ack_handler() does at :5723; and the extra application reference is dropped after the unlock when changed == 0, so an already-detached application keeps its edge's reference and this call keeps only the rpc_data's. The comment states the ordering and why (nxt_router_detached_apply()'s shape), and notes that the request still holds its own reference here so the increment cannot resurrect a freed application.

2. Minor. nxt_assert(req_rpc_data->app != NULL) is in the new condition, with a line saying why (the request is still linked at that point, so it holds the application).

3. CHANGES / docs/changes.xml. Clause added to the 1.36.2 bugfix entry, in both files: a request whose "limits": {"timeout"} deadline has passed no longer returns its worker to the idle pool either, and the worker keeps counting against "max" until it answers or exits. That is the user-visible change on the failure path, and it is now where an operator will look for it.

The detached_app-as-a-flag limitation and the missing test for the count stay recorded as inherited/deferred, per your agreement.

[andypost]

@claude review

[claude]

Claude finished @andypost's task in 6m 27s —— View job

---

Review: PR #412 — keep a worker the deadline gave up on out of the idle economy

• Gather context (diff vs fix/321-detached-worker-lifecycle, confirm the rebase)
• Re-check round-3 items on the rebased head
• Re-verify reference ordering and counter balance
• Cross-check Codex's P2 on the counter width
• Review tests and the CHANGES clause
• Post review

---

Branch state is fixed

git merge-base HEAD origin/fix/321-detached-worker-lifecycle is now 2db15e7, the base tip, and the diff is the seven files the body lists. The release-build recovery in nxt_router_adjust_idle_timer() is present, and src/nxt_unit.c is untouched by this branch — so the two Codex nxt_unit.c P1s from the earlier rounds belong to the base, not here.

Round-3 items: all addressed

The reference ordering is fixed the way it needed to be. nxt_router_app_use(task, app, 2) now runs before nxt_thread_mutex_lock(&app->mutex) (src/nxt_router.c:6720), nxt_port_inc_use(port) moved inside the critical section next to the state write (:6742), and the extra application reference is returned after the unlock when changed == 0 (:6757). The nxt_assert(req_rpc_data->app != NULL) is in the new condition, and the CHANGES / docs/changes.xml clause states the user-visible consequence.

I re-traced the reference invariant — exactly one detached application reference exists while port->detached != 0, held by whoever set it — across all six abandon/START/FINISH/settle interleavings plus port close, and it balances in every one, including the two handovers (drop = 2 on a changed FINISH returning the abandon's reference; the settle declining to clear while detached_app != 0, leaving the FINISH to return it). Port references balance too: one nxt_port_inc_use() per abandon, one nxt_port_use(-1) per settle, and a settle that arrives after nxt_router_app_port_close() zeroed the three fields decrements no counter and drops only its own two.

The drop order in nxt_router_app_abandoned_settle() (:6793-6801) is also correct in the sharp case: when ours is set the settle holds two references, so the ours drop cannot reach zero and the synchronous nxt_router_free_app() in nxt_router_app_use() (:6528) can only fire on the last call — after app->detached_processes-- has already run, which is what the assert in nxt_router_free_app() checks.

Three things stand.

---

1. The abandon disarms the regression test for fix/321's unwind, and leaves the fixture's comment saying the opposite — test/php/detached_worker/index.php:19-22, test/test_php_detached_max.py:268

test_php_detached_start_after_the_worker_went_idle exists for one path: a limits.timeout expiry parks the worker in idle_ports, and the START edge that arrives two seconds later has to take it back out (nxt_router_app_port_busy(..., "detached"), src/nxt_router.c:8065). That parking is exactly what this PR removes. With timeout = 2 and before = 4, the deadline at t≈2 now runs nxt_router_app_abandon() first, so nxt_router_app_port_idle() sees detached != 0 and the port is never in idle_ports between t=2 and t=4.

At t=4 the script calls fastcgi_finish_request(), which puts the START edge on the router's main port and the last response message on the engine port — two ports, two threads, no ordering between them:

• START processed first (the likely order, since it is written first and the main thread is idle): port->detached is already 1, changed == 0, nxt_router_app_port_busy() finds an empty idle_link and unwinds nothing. The scenario the test is named for does not occur.
• the settle wins instead: detached_router → 0 with detached_app still 0, the port is briefly parked, and the START then unwinds it — the original path.

So the test still passes either way, but its coverage of the fix/321 bug is now a coin flip on thread scheduling rather than a deterministic two-second window. The docstring was rewritten to describe only the first ordering; the fixture's own comment was not rewritten at all and now states something this PR makes false:

 *   before=N     hold the worker for N seconds *before* answering.  Past
 *                "limits": {"timeout"} the router fails the request and
 *                parks the worker as idle, so the detached report that
 *                follows arrives for a port that is already in idle_ports.

Two things worth doing: correct that comment, and say in the test how the port can still reach idle_ports at all now — otherwise the next reader deletes the unwind at :8065 as dead code, and the only remaining reachable route to it is the race above. If you want the old window back deterministically, a case where the deadline fires before the acknowledgement (no abandon, so the release still parks) and the worker later reports detached would do it.

Fix this →

2. Codex's P2 on the counter width is right, and widening it is free — src/nxt_port.h:368

I agree with the reachability argument (nxt_conf_vldt_threads() accepts up to NXT_INT32_T_MAX, src/nxt_conf_validation.c:2812), and the failure is worse than a miscount in both directions: at exactly 65536 abandonments detached_router wraps to 0, every later settle takes the if (port->detached_router != 0) branch not at all, port->detached is never cleared and the detached application reference is never returned — a permanently pinned worker plus the nxt_assert(app->detached_processes == 0) in nxt_router_free_app().

What makes it worth taking rather than arguing about: it costs nothing. The current layout is

uint32_t active_websockets;   /* M+8  */
uint8_t  detached;            /* M+12 */
uint8_t  detached_app;        /* M+13 */
uint16_t detached_router;     /* M+14 */
uint32_t active_requests;     /* M+16 */
nxt_port_handler_t handler;   /* 8-aligned -> M+24, four bytes of padding at M+20 */

A uint32_t detached_router lands at M+16 with two bytes of padding at M+14, active_requests at M+20, and handler still at M+24. The padding just moves; nxt_port_t is the same size either way, and it matches active_requests, which counts the same population.

Fix this →

3. The recorded detached_app limitation now has a second clearing site — src/nxt_router.c:6782, :8060

The body records the flag as fix/321's state and defers it on the libunit-retry argument, which I still agree with. One line should be added to that record though: the PR introduces a second place that reads detached_app to decide whether to clear, so the imprecision now bites from two directions. With threads > 1: START₁, START₂ (flag already 1, no reason recorded), FINISH₁ sets detached_app = 0, and then the settle at :6782 finds detached_router == 0 && detached_app == 0 and returns the port to the idle economy while context 2 is still running its detached work. Same root cause as the nxt_router_detached_apply() case already written up, different caller — worth naming so the eventual fix is known to touch both.

---

Things I checked that are fine

• NXT_SHARED_PORT_ID is 0xFFFFu (src/nxt_router.h:37) and the discriminator at :7910 is the same one nxt_router_msg_retract() uses at :1199, so the pre-acknowledgement window is untouched.
• app_port->main_app_port is set for every worker port (:1497, :4228, :6017) and is the same object the acknowledgement (:5717) and the release (:6890) follow, so the abandon cannot land on a different port than the release it precedes.
• A port closed between the abandon's reference-taking and the mutex is handled: nxt_router_app_port_idle()'s port->pair[1] != -1 guard stops the settle putting a dead port back in idle_ports.
• The settle is gated on msg->port_msg.last != 0 and runs before nxt_port_rpc_handler() frees the registration (src/nxt_port_rpc.c:450-464); abandoned_port/abandoned_app are nulled first, so it cannot run twice, and both callers are on the request's engine, so the two pointers need no synchronisation.
• nxt_router_response_error_handler() settling before nxt_request_rpc_data_unlink() is safe on an already-unlinked rpc_data (app_port/request/app NULL, body_fd −1, rpc_cancel 0).
• Clearing rpc_cancel before the unlink does not leak the registration: last, nxt_port_rpc_remove_peer() on worker death (peer set at ack), and nxt_port_rpc_close() at shutdown all reach a handler that settles.
• marker() removes the file first, so FILE_APPEND on ran is safe even though timed_out_ran is shared with test_php_detached_parked_request_times_out, whose not os.path.exists(ran) assertions are unaffected.
• The new test's window is now ~6 s and self-checking: hold = 12, 503 at ≈2 s, IDLE_TIMEOUT * 2 sleep, and the done marker asserted absent before the counters are read, so a loaded runner fails with "raise hold" instead of reading /status late.

Not verified here

Read-only review; I did not configure or build, so the 13 passed, ./build/tests exit 0 and 12 passed / 6 skipped results are yours as reported.
· branch fix/347-timeout-accounting

[andypost]

Head moved -> : the squashed commit message lost its trailer block when it was rebuilt. Tree identical ( is empty), so the review above and the re-verification numbers still describe this head.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[andypost]

Round 4 addressed in f10bcb96 (same single commit, amended).

1. The fixture comment and where idle_ports can still be reached. The before=N comment now says what the deadline actually does: the worker stays out of the idle economy for the request it was running, and the detached report is what ends that state -- unless the answer settles first, in which case the report takes the port back out. The test's docstring names both orders: with the router's mark in place the edge finds the port already out and has nothing to unwind, and the nxt_router_app_port_busy(..., "detached") call is still load-bearing for the order where the answer's settle on the request engine wins against the edge on the main thread. I did not add the deterministic pre-acknowledgement case: reaching it needs a request the deadline finds still queued whose worker then reports detached, and the retraction makes that a race of its own -- recording the route seemed better than a test that pins a scheduling accident.

2. detached_router is uint32_t. Size-neutral as you laid it out -- the padding after active_requests moves ahead of the counter, handler stays 8-aligned -- and the comment now says why: threads is validated up to NXT_INT32_T_MAX, and a wrap would leave a settle unable to clear the state at all rather than merely miscounting.

3. The detached_app record names the second clearing site. The body now says nxt_router_app_abandoned_settle() reads the flag too, so the imprecision bites from two callers and the eventual fix has to touch both.

Re-verified on f10bcb96: test_php_detached_max.py 13 passed, ./build/tests exit 0, test_app_start_timeout.py + test_status.py 12 passed / 6 skipped. CI is running on the head (whitespace and prepare already green).

[andypost]

Rebased onto master now that #405 has merged (7545594f). The branch was stacked on an earlier revision of the #405 branch (base tip 2db15e78), whose tree differs from the one that merged, so git rebase --onto origin/master 2db15e78 drops those five commits and replays this one. It applies without conflict, and the added and removed lines are byte-identical to the pre-rebase commit. The diff is now 7 files, +401/-28, rather than 21 files.

On merged master this abandon sits directly after #405's new early return in nxt_router_app_timeout(), and the two conditions are exact complements: #405 returns for a claimed request still on the shared port, before the ack has arrived; this abandons once the ack has moved the request onto the worker's own port. Both load-bearing arguments in the description still hold against the merged code.

Line references have shifted: the fall-through is now :7922-7924, apr_action = NXT_APR_REQUEST_FAILED is :7326, active_requests -= is :6893/6894, nxt_router_app_abandon() is :6702, nxt_router_app_port_close() is :6992-6997. Two were wrong before the rebase: the dec_requests/inc_use case is :6859-6862, not :6785-6789, and fastcgi_finish_request() is src/nxt_php_sapi.c:279, not :306.

Validation after the rebase: debug and release builds with 0 warnings; build/tests 50 passed and build/unit_port_recv_test all passed; test_php_detached_max.py 13 passed, including test_php_timed_out_request_keeps_the_worker_busy, which fails with the described signature when the abandon is reverted.

Note on sequencing: PR #419 and the parked counter commit from #405's follow-up touch this same area. #419 is libunit-only and independent. The counter makes detached itself a count of the application's work, while this PR keeps detached as an aggregate and splits the reasons into detached_app and detached_router; whichever of those two lands second needs reworking rather than a textual merge.

[andypost]

@codex review
@claude review

[claude]

Claude finished @andypost's task in 6m 8s —— View job

---

Review: PR #412 — keep a worker the deadline gave up on out of the idle economy

• Confirm the rebase onto merged master
• Re-check the round-4 items on the new head
• Check the interaction with router: count a worker that answered and kept running #405's merged early return
• Re-verify reference / counter balance
• Review tests and the CHANGES clause
• Post review

---

Branch state

git merge-base HEAD origin/master is 7545594 — the branch sits directly on the merged tip, and git diff origin/master...HEAD --stat is the seven files you list, +401/−28. The release-build recovery in nxt_router_adjust_idle_timer() is present at src/nxt_router.c:7083 and untouched, and src/nxt_unit.c is not in the diff, so the Codex nxt_unit.c P1s from rounds 2–3 are settled by the base having merged.

The two conditions really are complements. nxt_router_app_port_get() sets req_rpc_data->app_port to app->shared_port (:7325) and the acknowledgement replaces it with the worker's port (:5736); those are the only two assignments. #405's early return takes CLAIMED && id == NXT_SHARED_PORT_ID (:7897), the abandon takes id != NXT_SHARED_PORT_ID (:7913), and a won retraction falls through with the shared port still in place and no abandon. Nothing overlaps and nothing is missed.

Round-4 items are addressed

The fixture's before=N comment now describes the post-abandon behaviour, test_php_detached_start_after_the_worker_went_idle names both orders and why nxt_router_app_port_busy(..., "detached") is still load-bearing, detached_router is uint32_t with the reason in the comment, and the body records nxt_router_app_abandoned_settle() as the second site that reads detached_app. Field layout is still size-neutral: uint8_t / uint8_t / uint32_t / uint32_t with handler pointer-aligned behind it.

I re-traced the reference invariant — exactly one detached application reference exists while port->detached != 0, taken by whoever set it and returned by whoever clears it — across all six abandon/START/FINISH/settle interleavings plus port close, and it balances in every one. The nxt_router_app_use(task, app, 2) before the mutex (:6721) with the conditional -1 after the unlock is the right shape, and nxt_port_inc_use(port) is now inside the critical section next to port->detached_router++ (:6733), matching nxt_router_req_headers_ack_handler() at :5717. nxt_port_rpc_handler() frees the registration only after the handler returns (src/nxt_port_rpc.c:450-464), and the settle nulls both pointers first, so it cannot run twice or against freed state.

One new finding.

---

1. The abandon also fires for an upgraded websocket, where its premise does not hold — src/nxt_router.c:7913

The discriminator is "the port is not the shared port", which is true for a websocket long after the request stopped being a request.

A 101 arrives as a non-last message, so nxt_router_response_ready_handler() re-arms the deadline in the else branch at :5418-5422:

        if (app->timeout != 0) {
            r->timer.handler = nxt_router_app_timeout;
            r->timer_data = req_rpc_data;
            nxt_timer_add(task->thread->engine, &r->timer, app->timeout);
        }

The upgrade itself runs nxt_router_app_port_release(..., NXT_APR_UPGRADE) and then sets req_rpc_data->apr_action = NXT_APR_CLOSE (:5581), but it leaves req_rpc_data->app_port pointing at the worker's port, and nothing disables r->timer on the way into nxt_http_websocket (no nxt_timer_disable() for r->timer anywhere). So a websocket that is quiet for longer than limits.timeout reaches nxt_router_app_timeout() with app_port->id != NXT_SHARED_PORT_ID and runs the abandon.

That 503-and-close on an idle websocket is pre-existing and not this PR's problem. What is new is that the abandon's premise is false there:

• the comment says the port must not go back to the idle economy "when the request's accounting is released a moment from now" — but main_app_port->active_requests was already decremented at upgrade time by the NXT_APR_UPGRADE release, and the NXT_APR_CLOSE release that follows decrements nothing;
• the port was never going back to the idle economy anyway: active_websockets is incremented at :5576 and never decremented anywhere in the tree, so nxt_router_app_port_idle()'s active_websockets == 0 term (:6646) already pins it for the process's life;
• so port->detached, detached_router++, app->detached_processes++ and the extra application reference are all pure overhead here, and they are released only by a last message on that stream or by nxt_router_app_port_close(). Until then /status reports the process under detached, which the CHANGES clause you just added defines as "still running a request whose deadline passed" — a quiet websocket is not that.

A clean discriminator is already sitting there. apr_action is NXT_APR_CLOSE only after an upgrade, so:

    if (req_rpc_data->app_port != NULL
        && req_rpc_data->app_port->id != NXT_SHARED_PORT_ID
        && req_rpc_data->apr_action != NXT_APR_CLOSE)

skips the websocket case exactly, leaving the 503/close behaviour on that path as it is today. If you would rather keep the abandon there, the function comment should say why an upgraded stream is included, because the active_websockets argument means it buys nothing.

No test covers this — test_asgi_websockets.py and friends do not set limits.timeout — so the suite is green either way.

Fix this →

---

Still recorded, not re-raising

detached_app as a flag (deferred on the libunit-retry argument, which the merged base now makes auditable), the count having no deterministic test, and the app_port->main_app_port identity divergence — all three are in the body with their reasons, and I agree with each call. Your sequencing note on #419 and the parked-counter commit is right: that commit makes detached itself the application's count while this keeps detached as an aggregate over detached_app + detached_router, so the second to land needs reworking rather than merging.

Things I checked that are fine

• The settle gate on msg->port_msg.last != 0 (:5377) is correct for an ordinary response: the non-last branch re-arms rather than settling, and a streaming response that outruns the deadline mid-body abandons with apr_action still NXT_APR_REQUEST_FAILED, which is the right release.
• nxt_router_response_error_handler() settling at :5830 before nxt_request_rpc_data_unlink() is safe on an already-unlinked rpc_data, and it is what covers worker death (peer set at :5685, reached by nxt_port_rpc_remove_peer()).
• Clearing rpc_cancel before the unlink does not leak the registration: last, peer removal and nxt_port_rpc_close() all reach a handler that settles.
• nxt_router_app_port_release() re-inserting main_app_port into app->ports (:6897) is not the idle economy — that queue is only walked by the shutdown/quit path at :6487 — so an abandoned port rejoining it is correct, not a leak of capacity.
• A settle arriving after nxt_router_app_port_close() zeroed the three fields takes no decrement branch and drops only its own two references; nxt_router_app_port_idle()'s pair[1] != -1 guard stops it re-parking a dead port.
• The new test's window is ~6 s and self-checking (hold = 12, 503 at ≈2 s, IDLE_TIMEOUT * 2 sleep, done asserted absent), marker() removes the file so sharing timed_out_ran with test_php_detached_parked_request_times_out is safe under FILE_APPEND, and running is app->processes, the right assertion for a detached-but-alive worker.

Not verified here

Read-only review — ./configure is not in my allowed tools, so the 13 passed, build/tests 50 passed and 12 passed / 6 skipped numbers are yours as reported, not reproduced.
· branch fix/347-timeout-accounting

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: f7ea188bba

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

[andypost]

Confirmed, and confirmed on the mechanism too: application-to-client frames go out as _NXT_PORT_MSG_DATA with last = 0, so the deadline is re-armed for every frame and it is a quiet upgraded stream that reaches nxt_router_app_timeout() and takes the abandon, whose premise NXT_APR_UPGRADE had already discharged.

Fixed in cb5f8a7b with r->state != &nxt_http_websocket. I avoided the apr_action discriminator deliberately: #420 renames that assignment to NXT_APR_WEBSOCKET_CLOSE, so != NXT_APR_CLOSE would stop matching once it merges, with nothing failing. The websocket state is assigned beside the NXT_APR_UPGRADE release and nowhere else in the tree, and it stays clear both for a handshake the application is still holding and for one that failed — both of which do still hold their accounting, so they keep reaching the abandon. r->websocket_handshake and the status == 101 pair were rejected for exactly that reason.

New test test_node_websockets_timeout_does_not_detach sets limits: {timeout: 1}, upgrades, stays quiet for 3s and asserts "detached": 0; reverting the condition reads 1 and also trips the conftest descriptor-leak check.

The 503 and close on that path are untouched here — that is issue #422, filed separately, since the deadline should not be firing on an established websocket at all.