Skip to content

Follow-ups from #698: shutdown leaves a deferred transplant's descriptor open (pre-existing), EPOLLOUT comment premise, CI visibility of the new test, stale comments #727

Description

@FumingPower3925

Follow-ups from PR #698 (#669, #668), deferred under the maintainer's two-round review cap (2026-09-27). Both independent re-reviews of #698's last round, at head ae35481, APPROVED with nits only. Line numbers below are at ae35481. Item 1 is a pre-existing defect, identical on main 9f4d89b and not widened by #698; the rest are text or CI-visibility fixes.

  1. Pre-existing: shutdown neither finishes nor closes a deferred transplant whose goroutine's exit entry is still queued, so its descriptor stays open and no engine owns it. shutdown() (engine/epoll/loop.go:3250-3366) walks only liveConns and never drains the detach queue. A transplantPending conn is already out of liveConns, and its conns slot is nil, because detachFromEpoll cleared both. If ctx is cancelled after tryTransplant sets quiesce and before the exit entry is drained, asyncWG.Wait still returns (the goroutine leaves by the quiesce branch), and the fd is neither handed to io_uring nor closed. The same holds for a single loop that shuts itself down after a listener re-create failure (loop.go:493/:503) while the engine keeps running. Argued by reading only: it needs a failing-first test (cancel between the quiesce and the drain of the exit entry, then check the fd) and a fix (drain the detach queue in shutdown, or track transplantPending conns until they are handed off).

  2. The EPOLLOUT comments give a wrong premise for why no wakeup is lost. loop.go:2697-2701 (armEpollOut: "every flush stops at EAGAIN, after which the send buffer gaining room is a new edge") and loop.go:597-599 ("a partial flush stops at EAGAIN"). flushWrites and flushWritesV make a single write(2)/writev(2); on a short count they return nil without reaching EAGAIN (writer.go:47-73, 142-177). The conclusion holds for TCP (a short non-blocking write only happens with the send buffer full, tcp_sendmsg sets SOCK_NOSPACE, so the next write-space callback is a new EPOLLET edge). Suggested wording: "a partial flush (short write or EAGAIN) leaves the send buffer full". Related and older than fix(epoll): never park the loop on a running async handler, and leave the live set to the loop on an async hijack (celeris#669, celeris#668) #698: driver.go:240-244 says "EPOLLIN stays edge-triggered", but that MOD drops EPOLLET for EPOLLIN too.

  3. CI shows the new transplant test only at package level. TestADeferredTransplantFinishesOnlyAfterItsGoroutineExits appears by name in no job log of run 36280312424, because the Unit job runs ./engine/epoll without -v (ok engine/epoll). The async deferred-transplant path did run by name elsewhere (the Unit adaptive: forced switches leave keep-alive connections behind — async conns miss the 1.2 s flap window in every run, and a one-worker io_uring strands sync conns until their requests fail #657 -v step and the Adaptive job). Add the test to the Unit job's named -v step so a silent skip would show.

  4. TestATransplantWaitsForARelink's comments still give the round-1 reason for relinkPending. async_handler_stall_linux_test.go:311-331 says a queued entry names cs "and a transplant would return cs to the pool under it", and its setup clears asyncRun without asyncClosed ("a later close, say"). Since ae35481, finishTransplantHandoff no longer pools cs, and conn.go:239-243 and transplant.go:217-219 say relinkPending only waits for the loop to see the conn. The test (and mutant M16) stays as defence in depth; its comments should say so.

  5. The epoll: a timeout reap parks the whole loop thread on cs.detachMu for as long as an async handler runs (the epoll twin of the closed celeris#593) #669 and epoll: hijackConn mutates worker-thread-only liveConns and connCount from the dispatch goroutine (the epoll twin of celeris#539) #668 issue comments are one round stale. Comments 5847230139 (epoll: a timeout reap parks the whole loop thread on cs.detachMu for as long as an async handler runs (the epoll twin of the closed celeris#593) #669) and 5847230293 (epoll: hijackConn mutates worker-thread-only liveConns and connCount from the dispatch goroutine (the epoll twin of celeris#539) #668) present e038a40 as the fixed head (120/0/6, 340 PASS, 18/18 mutants, run 36245610547). The final round's numbers (whole package 124/0/6/0, lane x10 380/0/0/0, 21/21 mutants, CI run 36280312424) are only in the PR body and the PR comment. A one-line addendum on each issue pointing to the merged commit would close the gap.

  6. Process: the PR comment's local vet, cross-build and golangci-lint claim has no saved log in the lane's round2/ evidence (MANIFEST.txt lists none). CI corroborates it (Lint prints "0 issues." for every module; both Build jobs are green), so this is only traceability.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingdocumentationImprovements or additions to documentationengine/epollEpoll engine specifics

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions