Skip to content

Follow-ups from #696: provider.go close-before-unregister contract text, the #691 CI step's test-group count, and its job not being required #726

Description

@FumingPower3925

Follow-ups from PR #696 (#691, #707), deferred under the maintainer's two-round review cap (2026-09-27). #696 merges at 4779076 (plus the update-branch merge of main) after its last re-review round, where both independent reviews APPROVED with nits only. All three are text or CI-visibility fixes; no engine behaviour changes.

  1. engine/provider.go overstates the close-before-unregister contract. At 4779076, provider.go:85 says "Closing fd before UnregisterConn does not affect this engine." Until UnregisterConn is called the conn is still live (closing is false): a RECV that completes still delivers onRecv for a descriptor the caller has closed, and if an HTTP accept on that worker takes the number, the next re-arm reaches armDriverRecv's refusal, so onClose gets "fd N is already an HTTP connection" and the caller's later UnregisterConn returns ErrUnknownFD. driver.go's own comment at armDriverRecv (:374) calls this state "outside the contract", so the two texts disagree. The PR body's Behaviour changes bullet describes it correctly. Suggested wording: "Closing fd before UnregisterConn keeps the socket open and does not misdirect the cancel on this engine, but it is still outside the contract."

  2. The ci.yml comment on the celeris#691 step accounts for 17 of the 19 new tests. ci.yml:480-487 (4779076) splits them 7 + 2 + 2 + 3 + 3 = 17, but the step's new= list names 19 (all of func Test in engine/iouring/driver_unregister_close_test.go). The two not counted are TestDriverUnregisterWaitForOnCloseThenClose (the R3c control) and TestDriverShutdownReleasesDescriptors. "Seven" dates from round 1. The first group (the leak plus every path that closes the duplicate) is nine. The tally itself is right: 27 names x 5 = 135 PASS, which the step checks.

  3. The skip-forbidding celeris#691 step does not gate merging. It runs in the "io_uring init-failure regression (./engine/iouring)" job, which is not a required status check on main (required: Lint, Unit, Conformance, Driver Conformance, Build ubuntu/macos, Vulnerability Check). The required Unit job runs the package without -v, where startTestEngine can skip silently. So a regression or a new skip in any of the 27 driver tests turns only a non-required job red. This predates fix(iouring): run every driver op through the engine's own duplicate of the socket, and count every cancel until its CQE, so closing after UnregisterConn is safe (celeris#691, celeris#707) #696 (the io_uring: a worker whose ring setup or first submit fails leaves its SO_REUSEPORT listen socket open, and adaptive retries the build every tick (found by reading) #656 step sits in the same job). Either say so where the step is described, or make the job required (an org-admin ruleset change).

  4. The number leaves the driver map before onClose, although the contract says it stays until then (CodeRabbit, outside-diff minor on 85f60f3, engine/iouring/driver.go:710-721; verified at 85f60f3). engine/provider.go:82-85 says that until onClose fires, fd's number stays registered on that worker, and a RegisterConn there of the next socket to get the number fails with "fd already registered". But finalizeDriver deletes dc.fd from w.driverConns and releases driverMu first, then runs dc.retire() (the close of opFD, which can linger: io_uring: closing a driver socket with SO_LINGER blocks the worker for the whole linger time (retire closes the engine's duplicate on the worker) #735), and only then fires onClose. In that window a RegisterConn of the reused number succeeds. Nothing is misrouted, because onClose is taken from dc, not looked up by number; the effect is that the documented refusal does not hold for the whole window. Either correct the text ("until the worker finalizes the conn, just before onClose"), or keep the key through retire and onClose and delete it afterwards only if it still maps to dc, without holding driverMu across the callback. Settle it together with io_uring: closing a driver socket with SO_LINGER blocks the worker for the whole linger time (retire closes the engine's duplicate on the worker) #735: moving the close off the worker widens this window.

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

    area/ciCI/CD pipelinedocumentationImprovements or additions to documentationengine/iouringio_uring engine specifics

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions