refactor(engine): track active connections symmetrically with idle (pre-P5)

[andypost]

Summary

Pre-P5 prep PR. Adds the missing engine->active_connections queue and makes idle/active state symmetric, so P5 (connection drain) can enumerate in-flight conns for drain-done detection and force-close-on-graceful_timeout.

The P5 agent stopped with this finding:

The engine has idle_connections queue but no active_connections queue. nxt_conn_active() removed a conn from engine->idle_connections but did not place it onto any other queue, so c->link dangled while a request was in flight. Counters cannot substitute because closed_conns_cnt++ only fired on idle-conn closes.

This PR is purely the data-structure change. No drain logic, no timer logic — those land in P5 on top of this.

What changes

src/nxt_event_engine.h

nxt_queue_t        active_connections;   /* new */
nxt_atomic_uint_t  active_conns_cnt;     /* new */

alongside the pre-existing idle_connections / idle_conns_cnt.

src/nxt_conn.h

c->idle becomes a tri-state field (still uint8_t):

#define NXT_CONN_TRACK_NONE     0   // controller / proxy peer — owned elsewhere
#define NXT_CONN_TRACK_IDLE     1   // on engine->idle_connections
#define NXT_CONN_TRACK_ACTIVE   2   // on engine->active_connections

Pre-P4.5 was a boolean that coincided with TRACK_IDLE. Existing callers that treated c->idle as boolean stay correct because NXT_CONN_TRACK_IDLE == 1.

The macros become symmetric — c->link is always on exactly one of (idle_connections, active_connections) between accept and close, with c->idle as the discriminator.

src/nxt_conn_close.c

Both close handlers (sync and timer-driven) grow an explicit TRACK_ACTIVE branch. closed_conns_cnt is now incremented for both idle and active closes — the pre-P4.5 behaviour effectively did this too because c->idle was never reset to 0 by nxt_conn_active(), so the counter already incremented for every accepted-conn close. The new explicit form makes the contract auditable.

src/nxt_runtime.c

nxt_runtime_close_idle_connections() drops its explicit nxt_queue_remove(link) before nxt_conn_close(). The close handler now performs the unlink itself; doing it twice would corrupt the queue.

Call-site audit

git grep "nxt_conn_idle\|nxt_conn_active" src/:

Site	What
src/nxt_h1proto.c:485	initial idle on accept-side keep-alive
src/nxt_h1proto.c:496	request begin
src/nxt_h1proto.c:1860	keep-alive return after response
src/nxt_h1proto.c:1885	peer keep-alive
src/nxt_h1proto.c:1985	peer request begin
src/nxt_conn_accept.c:264	initial mark on first accept
src/nxt_conn_close.c:122,160	c->idle gating (now tri-state branches)

Every call site is reachable only after the conn has been on either idle_connections (most) or never on a tracked queue at all (controller / proxy peer); no caller invokes nxt_conn_active on a NONE-state link, so the unconditional nxt_queue_remove inside that macro remains safe.

Verification

Plain build

./configure --tests --modules=python && ./configure python --config=python3-config && make -j$(nproc)
                                                                               # clean

Regression slice with --restart:

Suite	Result
test_python_application.py + test_idle_close_wait.py	42 passed, 6 skipped
test_routing.py	4 failed, 109 passed, 2 skipped, 4 errors

The 4 routing failures are all [::1] IPv6 environmental (socket("[::1]:8081") failed: Address family not supported by protocol). Verified by rebuilding git checkout freeunit/master -- src/ and re-running the same suite — identical 4-fail / 4-error pattern. Zero P4.5-attributable regressions.

ASan build (-fsanitize=address -fno-omit-frame-pointer -O1 -g)

test_python_application.py + test_idle_close_wait.py
  => no queue corruption, no use-after-free

Sole LeakSanitizer reports are 4× 136-byte nxt_var_index_init / nxt_runtime_create entries (one per unitd process), pre-existing on master. Separate issue.

Out of scope

• Walking active_connections for drain — that's P5.
• graceful_timeout config wiring — also P5.
• Anything else in roadmap/plan-graceful-shutdown.md.

Refs

• roadmap/plan-graceful-shutdown.md (P5 prerequisite)
• Upstream nginx/unit PR Fixed nxt_runtime_close_idle_connections(). nginx/unit#334 (defensive queue-traversal pattern; cited as the model for P5)

---

Generated by Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a tri-state connection tracking system (NONE, IDLE, ACTIVE) to manage connection states and counts more effectively. It adds an active_connections queue and an active_conns_cnt counter, updating lifecycle macros and closing handlers accordingly. The review feedback suggests using atomic operations consistently for all shared counters to prevent data races and recommends refactoring the connection closing logic to reduce code duplication.

[gemini-code-assist]

This macro has a potential correctness issue and an inconsistency:

1. Double-counting: If the connection is already in the NXT_CONN_TRACK_ACTIVE state, calling this macro will increment active_conns_cnt again without a corresponding decrement, leading to an incorrect counter value.
2. Atomic Consistency: idle_conns_cnt is updated non-atomically while active_conns_cnt uses nxt_atomic_fetch_add. Both should use atomic operations for consistency.

        if (c->idle != NXT_CONN_TRACK_ACTIVE) {
            nxt_queue_remove(&c->link);

            if (c->idle == NXT_CONN_TRACK_IDLE) {
                nxt_atomic_fetch_add(&e->idle_conns_cnt, -1);
            }

            nxt_queue_insert_head(&e->active_connections, &c->link);

            c->idle = NXT_CONN_TRACK_ACTIVE;
            nxt_atomic_fetch_add(&e->active_conns_cnt, 1);
        }

[gemini-code-assist]

The update to idle_conns_cnt (line 338) uses a non-atomic increment, while the update to active_conns_cnt (line 332) uses nxt_atomic_fetch_add. Since both are defined as nxt_atomic_uint_t in nxt_event_engine_t, they should be updated consistently using atomic operations to avoid potential data races when these counters are read by other threads (e.g., for status monitoring).

        if (c->idle == NXT_CONN_TRACK_ACTIVE) {
            nxt_queue_remove(&c->link);
            nxt_atomic_fetch_add(&e->active_conns_cnt, -1);
        }

        nxt_queue_insert_head(&e->idle_connections, &c->link);

        c->idle = NXT_CONN_TRACK_IDLE;
        nxt_atomic_fetch_add(&e->idle_conns_cnt, 1);

[gemini-code-assist]

The logic for unlinking and updating counters can be refactored to be more concise and avoid duplication. Additionally, using atomic operations consistently for all nxt_atomic_uint_t fields (idle_conns_cnt, active_conns_cnt, and closed_conns_cnt) is recommended to avoid potential data races.

Suggested change

	if (c->idle == NXT_CONN_TRACK_IDLE) {
	nxt_queue_remove(&c->link);
	engine->idle_conns_cnt--;
	c->idle = NXT_CONN_TRACK_NONE;
	engine->closed_conns_cnt++;
	} else if (c->idle == NXT_CONN_TRACK_ACTIVE) {
	nxt_queue_remove(&c->link);
	nxt_atomic_fetch_add(&engine->active_conns_cnt, -1);
	c->idle = NXT_CONN_TRACK_NONE;
	engine->closed_conns_cnt++;
	}
	if (c->idle != NXT_CONN_TRACK_NONE) {
	nxt_queue_remove(&c->link);
	if (c->idle == NXT_CONN_TRACK_IDLE) {
	nxt_atomic_fetch_add(&engine->idle_conns_cnt, -1);
	} else {
	nxt_atomic_fetch_add(&engine->active_conns_cnt, -1);
	}
	c->idle = NXT_CONN_TRACK_NONE;
	nxt_atomic_fetch_add(&engine->closed_conns_cnt, 1);
	}

[gemini-code-assist]

This block is identical to the one in nxt_conn_close_handler. Consolidating the logic and using atomic operations consistently for all tracking counters is recommended.

Suggested change

	if (c->idle == NXT_CONN_TRACK_IDLE) {
	nxt_queue_remove(&c->link);
	engine->idle_conns_cnt--;
	c->idle = NXT_CONN_TRACK_NONE;
	engine->closed_conns_cnt++;
	} else if (c->idle == NXT_CONN_TRACK_ACTIVE) {
	nxt_queue_remove(&c->link);
	nxt_atomic_fetch_add(&engine->active_conns_cnt, -1);
	c->idle = NXT_CONN_TRACK_NONE;
	engine->closed_conns_cnt++;
	}
	if (c->idle != NXT_CONN_TRACK_NONE) {
	nxt_queue_remove(&c->link);
	if (c->idle == NXT_CONN_TRACK_IDLE) {
	nxt_atomic_fetch_add(&engine->idle_conns_cnt, -1);
	} else {
	nxt_atomic_fetch_add(&engine->active_conns_cnt, -1);
	}
	c->idle = NXT_CONN_TRACK_NONE;
	nxt_atomic_fetch_add(&engine->closed_conns_cnt, 1);
	}