Follow-ups from #793: a SEND_ZC notification counted as an owed op, the unpinned park gate and shutdown drain, and two counter/exit nits

[FumingPower3925]

The round-2 review of #793 (celeris#685) left four minors and two nits in the engine and its tests. The round-2 fix did not change them: under the two-round review cap they are tracked here. The line numbers are at #793's round-2 head, e48adee, rebased on main dfd044f.

1. A pending SEND_ZC notification counts as an owed op (minor, introduced by #793): FIXED in #793

Fixed in #793, round 3 (head bf129c8), failing first. The fd-lifetime rule now counts only the ops that name the descriptor.

• fdOps is kernelInflight less one while zcNotifPending. fdOwed, and the shutdown drain's pending(), use it.
• A closed identity keeps the same count in its closedOps entry.
• drainPendingRelease closes a kept descriptor as soon as only notifications are owed. The connState, whose send buffer the notification guards, still waits for them.
• The shutdown drain also notes a SEND_ZC first CQE that it reads itself.

The tests are TestCloseReleasesDescriptorWithOnlyAZCNotificationOwed (three cases: the notification alone; a recv owed as well; the send's first CQE unread at the close) and TestShutdownDoesNotWaitForAZCNotification.

• On 0f36096 (e48adee + the tests), all five cases fail: the descriptor is held 5.000 to 5.013 s with CloseFDForced=1, and the drain takes 251 to 263 ms.
• On bf129c8, all five pass 60 of 60 runs across the laptop's m8, ci and unc shapes. The descriptor closes within 1.5 ms of the close with CloseFDForced=0, and only the notification releases the connState.
• CI: failing first in 36384232278, green in 36385419739. A new recv-theft step runs both tests by name on both arches, skipping forbidden.

CloseFDForced is therefore a must-stay-0 gate with SEND_ZC on too. The report below is the original one, kept for the record. Items 2 to 5 are still open.

The defect. fdOwed (engine/iouring/fd_lifetime.go:417) is kernelInflight > 0. kernelInflight keeps counting a SEND_ZC until its notification CQE. By then, though, the op has been issued and has completed (its first CQE, F_MORE), and the notification names no descriptor. So a close with only that notification pending keeps its descriptor for no reason.

Neither the cancel nor SHUT_RDWR ends that notification while the socket is alive. Keeping the descriptor is exactly what keeps the socket alive. The one way out is the 5 s pendingRelease backstop, which force-closes the descriptor and counts CloseFDForced, the counter #793 documents as "must stay 0".

endOwedOpsAtShutdown's pending() (fd_lifetime.go:500-511) uses the same test, so shutdown waits out its full 250 ms bound.

How production gets there. SEND_ZC is on (auto whenever the probe calls it functional, including copy-fallback). A peer has stalled on an unlinked send of 4 KiB or more: a detached WS/SSE write, H2, a partial-send remainder, or a buffer-ring send. Then:

1. closeConn defers while zcNotifPending;
2. the closing-drain sweep reaps the connection after 5 s;
3. finishClose keeps the descriptor;
4. 5 s later the backstop forces the close.

The socket is held for 10 s instead of 5, the worker cannot park for those 5 s, and CloseFDForced goes above 0 with no theft possible.

Evidence. The reviewer's test engine/iouring/zz_review685_zc_test.go (untracked, in the review-a worktree of lane 685) sets up a synthetic worker with sendZC=true, loopback TCP, a peer with SO_RCVBUF=4096 that never reads, and a 1 MiB flushSend. Its log review-a-ev/logs/zc1.log shows:

• first CQE res=1048576;
• before the close: kernelInflight=1 recvArmed=false zcNotifPending=true fdOwed=true;
• after finishClose: kept=true closeFDOwed=1;
• after 1.5 s of ring: kernelInflight=1 (no notification);
• after the backstop: closeFDForced=1.

The shutdown arm took 252-255 ms with kernelInflight=1 left, 3 of 3 runs.

To do, failing-first on that test:

• Count only the ops that name the descriptor: owed = kernelInflight - b2i(zcNotifPending) > 0, in fdOwed and in endOwedOpsAtShutdown's pending().
• drainPendingRelease must close a kept descriptor once no descriptor-naming op remains, separately from releasing the connState. The connState still waits for kernelInflight == 0, because the notification guards the send buffer. A close with a recv owed AND a notification pending otherwise keeps the descriptor until the notification, which is the same stall.
• Until this lands, CloseFDForced is not a must-stay-0 gate on a worker with SEND_ZC on.

2. The park gate is load-bearing and no #793 test pins it (minor)

w.closeFDOwed == 0 in the park predicate (engine/iouring/worker.go:1547) keeps a worker that closed its last connection with an op owed from parking while it holds the kept descriptor. Remove it, and that worker parks in an indefinite channel wait. The descriptor and socket then stay open, and the client sees no close, until something wakes the worker.

The reviewer's mutant M2 (the gate term removed) passes every test in #793's scope: 30 tests at -race -count=2, 0 FAIL (review-a-ev/logs/m2-nopark-ci2.log). With #767's park_close_fin_test.go added, TestParkedWorkerSendsFINForAHeaderTimeoutClose and TestParkedWorkerSendsFINForAReadTimeoutClose FAIL 3/3 each (m2-nopark-x712-ci3.log: the engine counted the close at 401 and 1409 ms, and the client saw no close for 3 s).

To do: once #767 and #793 are both on main, re-run #767's park FIN tests against the M2 mutant and record that they fail. Or, if #767 does not land, add a parked-worker FIN test (or a synthetic equivalent) that fails with the gate removed. See #795 for the named CI interlock of the #712 tests.

3. No test fails without the core of endOwedOpsAtShutdown (minor)

The core is at engine/iouring/fd_lifetime.go:491-538: the cancels, SHUT_RDWR, and running the ring until the terminal CQEs. Its premise is unmeasured: that the ring's teardown still issues a consumed-but-unissued op (a linked recv, or task work queued at the SEND's CQE) after shutdown has closed the descriptors.

TestShutdownEndsOwedOpsBeforeClosing builds only idle keep-alive connections whose linked recv has already been issued. It asserts left_open=0 and CloseFDForced=0, which the pre-#793 shutdown also satisfies. The reviewer's mutant M1 (if false && ... on the drain) passes every shutdown, driver-shutdown and recv-theft test: 24/24 at -race -count=3 (review-a-ev/logs/m1-noshutdrain-ci3.log).

Meanwhile the drain adds new behaviour to every shutdown:

• up to 250 ms per worker on a kernel that never answers;
• every CQE it reaps (driver ops, accepts, requests) is consumed and dropped.

The ENOBUFS re-arm path (multishot recv, CELERIS_IOURING_MULTISHOT_RECV=1, worker.go:2853), which #685's scope names, has no dedicated trial either.

To do: add a shutdown trial that holds the worker between a linked recv's SEND CQE and shutdown(), and that fails with M1. If no kernel issues that work after the descriptors close, delete the drain rather than keep an unmeasured cost. Do the same for the ENOBUFS re-arm with multishot on.

4. endOwedOpsAtShutdown gives up on EBUSY (nit)

fd_lifetime.go:520-521 breaks out of the drain on any SubmitAndWaitTimeout error. SubmitAndWaitTimeout returns an error for every errno except ETIME and EINTR, and that includes EBUSY. The ring's own retryPending treats EBUSY as the CQ ring being full with nothing taken (ring.go:381). That is exactly when the drain should reap the CQ and retry, and it is the likely case at shutdown with many live connections, since each cancel and SHUT_RDWR posts a CQE. When it happens, the drain silently degrades to the pre-#793 close-everything behaviour.

To do: on EBUSY or EAGAIN, reap the CQ and retry within the bound; count a give-up.

5. The deferred-close counter can drop its last batch (nit)

closeFDDeferredBatch is flushed at one site only, worker.go:1409, after CQE processing. A close that drainDetachQueue or checkTimeouts queues later in the iteration is counted one iteration later. If that next iteration leaves through the ctx.Err() check to shutdown(), the count is never added to CloseFDDeferred, because neither shutdown() nor endOwedOpsAtShutdown flushes it. #793 quotes that metric as exact ("2,000 of 2,000 async closes").

To do: flush the batch in shutdown() too.

Refs: #793, #685, #715, #767, #795. The review findings and their evidence are in the round-2 review of #793 (reviewers A and B, lane 685).

6. The #798 tests assert wall-clock bounds (minor, CodeRabbit on #793)

From CodeRabbit's review of #793 at bf129c8 (thread). engine/iouring/fd_lifetime_close_test.go judges both new tests by elapsed time:

• zcShutdownBound is 100 ms (line 338). TestShutdownDoesNotWaitForAZCNotification fails when endOwedOpsAtShutdown takes longer. The regression it catches runs the 250 ms shutdownFDDrainNanos, only 2.5 times the bound, so one GC pause or runner stall over 100 ms under -race fails a correct tree.
• zcReleaseBound is 200 ms (line 331), against a 5 s backstop.

The head measured at most 0.263 ms (shutdown) and 1.455 ms (close) across 60 laptop runs, and CI's recv-theft step passed 15 of 15 cases on both arches, so no failure has been seen. The margin is still the kind the path instructions rule out.

To do:

• notif-pending: assert the drain's precondition, fdOps(cs) == 0 before endOwedOpsAtShutdown, rather than a duration.
• send-done-during-drain: count the drain's passes (a test counter or a hook around SubmitAndWaitTimeout) and assert exactly one.
• Widen zcReleaseBound to 2 s.
• Keep both failing on 0f36096 (the pre-fix tree plus the tests).

7. Deferred-close cost on bare metal (nit, CodeRabbit on #793)

CodeRabbit's nitpick on engine/iouring/worker.go:4251-4253 at bf129c8 asks for the churn cost of the deferred close: fdOwed, the park gate closeFDOwed, and the fast path's shutdown(SHUT_RD). #793 measured it on the laptop only (Close685, Churn685, strace syscall counts; "Cost" in #793's body). The only cost it found was +4.87% on Close685/fast/owed=yes, the one shutdown. The churn rows cannot exclude a regression below about 5% (async) or 10% (sync).

To do: run the bare-metal A/B queued as lane 685 (evidence/_queue/cluster.tsv, the #674 ABA template, both arches) and record its verdict here.

8. The hijack's submit-before-handover has no SQPOLL form (minor, CodeRabbit on #793; unreachable today)

From CodeRabbit's review of #793 at 7ed3a33 (thread). hijackConn (engine/iouring/worker.go:2446-2448 at 7ed3a33) submits its cancels before handing the socket over only when !w.sqpoll. Under SQPOLL it returns at once, and nothing orders the kernel thread's consumption of the cancel before the hijacker's first read, so an owed recv could still take the hijacker's first bytes.

No build reaches it. w.sqpoll is tier.SQPollIdle() > 0 (worker.go:941), and every tier's SQPollIdle returns 0 (tier.go:67, :122, :203; #377: GetSQE advances the shared SQ tail before the SQE is written, so SQPOLL is unsafe as built). The shutdown drain is skipped under SQPOLL for the same reason (fd_lifetime.go:543).

To do: if SQPOLL is ever enabled (#377), hijackConn must wait for the cancel's CQE (or the SQ head passing the cancel) before it returns the socket, and endOwedOpsAtShutdown needs an SQPOLL form too. Until then, no change.