From b44105d3ef1b2f8add602e370f0e010355fdc725 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 23:07:42 +0200 Subject: [PATCH 01/12] test(iouring): deterministic check of the #715 recv-theft window Hypothesis (a) of #715 (the #685 class): an io_uring worker prepares a recv SQE that reaches the kernel only at its next submit, and the SQE names the descriptor NUMBER. finishClose / finishCloseDetached queue the recv's cancel behind it and close the descriptor at once, so a sibling worker's accept can be given the number first; the old recv then reads the new connection's request and completes as stale_recv_data_closed. Validation builds only (internal/recvtheft, the internal/zcwindow pattern; production compiles every call site away and connState keeps its layout, pinned by TestRecvTheftWitnessCompilesAway): - witness close_with_unsubmitted_recv: a close reached while the conn's recv SQE is still in the SQ ring (its SQ sequence number vs the kernel's SQ head); - a hold after the close, a hold on the sibling after it accepted the freed number, a gate that lands the dispatch goroutine's close in the promote re-arm's iteration, stale recv exemplars, and the control switch that submits before the close; - a wake hold for hypothesis (c). TestRecvTheft715ArmA asserts the property a fix must restore and fails on main by design when (a) holds; TestRecvTheft715Control (submit before close) and TestRecvTheft715ArmC (hypothesis (c)) assert the same kind of property. All three run only with CELERIS_RECV_THEFT_715=1 under -tags=validation, with two io_uring workers; none is in CI. Refs #715 #685 --- engine/iouring/conn.go | 8 + engine/iouring/handoff_loss.go | 22 +- engine/iouring/recv_theft_715_linux_test.go | 492 +++++++++++++++++++ engine/iouring/recv_theft_prod_linux_test.go | 27 + engine/iouring/ring.go | 14 + engine/iouring/worker.go | 82 ++++ internal/recvtheft/doc.go | 39 ++ internal/recvtheft/recvtheft.go | 294 +++++++++++ internal/recvtheft/recvtheft_off.go | 40 ++ 9 files changed, 1017 insertions(+), 1 deletion(-) create mode 100644 engine/iouring/recv_theft_715_linux_test.go create mode 100644 engine/iouring/recv_theft_prod_linux_test.go create mode 100644 internal/recvtheft/doc.go create mode 100644 internal/recvtheft/recvtheft.go create mode 100644 internal/recvtheft/recvtheft_off.go diff --git a/engine/iouring/conn.go b/engine/iouring/conn.go index 732d3142..a450b9c8 100644 --- a/engine/iouring/conn.go +++ b/engine/iouring/conn.go @@ -8,6 +8,7 @@ import ( "sync/atomic" "github.com/goceleris/celeris/internal/conn" + "github.com/goceleris/celeris/internal/recvtheft" ) // maxSendQueueBytes is the per-connection back-pressure limit for @@ -397,6 +398,13 @@ type connState struct { // has repurposed. drainPendingRelease only releases a connState once // this counter reaches zero (with a wall-clock backstop for kernel // anomalies). Mirrors driverConn.inflightOps. + // + // recvArmSeq (validation builds only; zero-size in production, and not + // the last field, so it adds no padding) is the SQ ring sequence number + // of this conn's latest recv SQE: the close paths compare it with the + // kernel's SQ head to tell a recv the kernel has not consumed yet + // (celeris#715, Worker.recvUnsubmitted). Set by noteRecvPlaced. + recvArmSeq recvtheft.ArmSeq kernelInflight int32 // recvArmed is true while a recv SQE (single-shot or multishot) is // kernel-held for this conn. Set by prepareRecv / flushSendLink's diff --git a/engine/iouring/handoff_loss.go b/engine/iouring/handoff_loss.go index 60879284..1a8ea34f 100644 --- a/engine/iouring/handoff_loss.go +++ b/engine/iouring/handoff_loss.go @@ -2,7 +2,11 @@ package iouring -import "sync/atomic" +import ( + "sync/atomic" + + "github.com/goceleris/celeris/internal/recvtheft" +) // handoffLossStats are the celeris#657 witnesses: the request loss a // reverse (io_uring→epoll) hand-off can cause, counted where it happens. @@ -178,6 +182,22 @@ func (w *Worker) noteStaleRecvData(ud uint64) { } } +// noteStaleRecvExemplar hands an armed recvtheft trial what a stale recv +// with data read: its identity, its result and the first bytes of the closed +// conn's cs.buf, where every single-shot recv except the direct-body one +// lands (celeris#715). Validation builds only (the caller is guarded by +// recvtheft.Enabled). Must run before noteStaleTerminalOp retires the +// closedOps entry. Worker thread only. +func (w *Worker) noteStaleRecvExemplar(c *completionEntry, fd int, ud uint64) { + var head []byte + if e := w.closedOps[connOpKey(ud)]; e != nil && len(e.conns) > 0 && w.bufRing == nil { + buf := e.conns[0].buf + n := min(int(c.Res), len(buf), 64) + head = buf[:n] + } + recvtheft.NoteStaleRecvData(w.id, fd, decodeGen(ud), c.Res, head) +} + // noteHandoffInFlight counts a hand-off that detached cs while the kernel // still held, or was about to be handed, an op on its descriptor. Called at // both hand-off sites after the hand-off is committed and before the close diff --git a/engine/iouring/recv_theft_715_linux_test.go b/engine/iouring/recv_theft_715_linux_test.go new file mode 100644 index 00000000..4d761e97 --- /dev/null +++ b/engine/iouring/recv_theft_715_linux_test.go @@ -0,0 +1,492 @@ +//go:build linux && validation + +package iouring + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "log/slog" + "net" + "os" + "strconv" + "strings" + "sync" + "testing" + "time" + + "golang.org/x/sys/unix" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/internal/recvtheft" + "github.com/goceleris/celeris/protocol/h2/stream" + "github.com/goceleris/celeris/resource" +) + +// celeris#715 hypothesis (a), the celeris#685 class, made deterministic. +// +// The io_uring worker prepares a recv SQE that reaches the kernel only at its +// next submit, and the SQE names the descriptor NUMBER (fixed files are off). +// finishClose / finishCloseDetached queue the recv's cancel behind it and close +// the descriptor at once. The claim under test: if a SIBLING worker's accept is +// given the freed number before this worker submits, the old recv reads the new +// connection's request (stale_recv_data_closed), and the new connection, whose +// own recv then finds an empty socket, is never answered. +// +// The theft needs one ordering: the sibling's accept after the close, and the +// closing worker's submit before the sibling's submit of its own recv. Two +// holds (internal/recvtheft, -tags=validation only) force it: +// - W1, the worker that owns connection A, is parked right after closing A's +// descriptor N with A's recv still unsubmitted (recvtheft.HoldAfterClose); +// - W2, the other worker, is parked right after it accepted B as N and +// prepared B's recv, before it submits (recvtheft.AfterAccept); +// - W1 is released (it submits the old recv), then W2. +// +// A reaches "recv prepared, then closed in the same loop iteration" through the +// engine's own async path: A's first request is on an async route, so the +// inline parse bails, promoteConnToAsync starts the dispatch goroutine and +// re-arms A's recv; the request asks for Connection: close and the handler +// writes nothing, so the goroutine sets asyncClosed with nothing to flush and +// queues A; drainDetachQueue closes A (finishCloseDetached). The one test-only +// step is recvtheft.Options.PromoteGate: the worker waits after the re-arm +// until the goroutine has queued, so the close lands in the same iteration +// instead of racing it. +// +// Arms (one trial per test run; tally the --- lines of -count=N): +// - A (TestRecvTheft715ArmA): the tree as it is. Asserts the property the +// fix must restore: B is answered and no stale recv read B's bytes. +// FAILS on main by design when hypothesis (a) holds. Not in CI; the +// celeris#685 fix enables it. +// - control (TestRecvTheft715Control): the same trial with +// recvtheft.Options.SubmitBeforeClose, the close paths submitting before +// they close (the fix direction). Asserts the same property. +// - C (TestRecvTheft715ArmC): hypothesis (c), the promoted connection's +// hand-off to its dispatch goroutine, with the window between the +// worker's asyncInMu unlock and its Signal / goroutine start widened +// (recvtheft.SetWakeHold). Asserts every request is answered. +// +// All three need two io_uring workers (so RLIMIT_MEMLOCK of at least 24 MiB) +// and run only with CELERIS_RECV_THEFT_715=1 in a -tags=validation build. + +const envRecvTheft715 = "CELERIS_RECV_THEFT_715" + +func requireRecvTheft715(t *testing.T) { + t.Helper() + if os.Getenv(envRecvTheft715) != "1" { + t.Skipf("celeris#715 recv-theft measurement: set %s=1 to run (arm A fails on main by design)", envRecvTheft715) + } +} + +// recvTheftHandler answers every path with "ok" except /close, which writes +// nothing. /close and /async are async routes. +type recvTheftHandler struct{} + +func (recvTheftHandler) HandleStream(_ context.Context, s *stream.Stream) error { + if s.ResponseWriter == nil || s.Path == "/close" { + return nil + } + return s.ResponseWriter.WriteResponse(s, 200, + [][2]string{{"content-type", "text/plain"}, {"content-length", "2"}}, []byte("ok")) +} +func (recvTheftHandler) RouteAsync(_, path string) bool { return path == "/close" || path == "/async" } +func (recvTheftHandler) HasAsyncRoutes() bool { return true } + +// startRecvTheftEngine runs an async-handler io_uring engine with two workers +// on a free loopback port and returns it with its port. +func startRecvTheftEngine(t *testing.T) (*Engine, int) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("pick port: %v", err) + } + addr := ln.Addr().String() + port := ln.Addr().(*net.TCPAddr).Port + _ = ln.Close() + e, err := New(resource.Config{ + Addr: addr, + Protocol: engine.HTTP1, + Resources: resource.Resources{Workers: 2}, + AsyncHandlers: true, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }, recvTheftHandler{}) + if err != nil { + skipOrFail656(t, "iouring engine unavailable: %v", err) + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- e.Listen(ctx) }() + t.Cleanup(func() { + cancel() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Error("engine did not stop within 5s") + } + }) + for deadline := time.Now().Add(8 * time.Second); e.Addr() == nil; { + select { + case err := <-done: + skipOrFail656(t, "iouring engine failed to start: %v", err) + default: + } + if time.Now().After(deadline) { + t.Fatal("engine did not start listening within 8s") + } + time.Sleep(10 * time.Millisecond) + } + n := e.NumWorkers() + t.Logf("RECVTHEFT715 engine workers=%d", n) + if n < 2 { + skipOrFail656(t, "celeris#715 needs a sibling io_uring worker: workers=%d (RLIMIT_MEMLOCK funds one per 12 MiB)", n) + } + return e, port +} + +// fillFDHoles opens /dev/null until the kernel hands out a number above every +// descriptor this process has open, so the next descriptors are allocated at +// the top and a number freed later is the lowest free one. The fillers are +// closed when the test ends. +func fillFDHoles(t *testing.T) { + t.Helper() + ents, err := os.ReadDir("/proc/self/fd") + if err != nil { + t.Fatalf("read /proc/self/fd: %v", err) + } + top := -1 + for _, ent := range ents { + if fd, err := strconv.Atoi(ent.Name()); err == nil && fd > top { + top = fd + } + } + var fillers []int + for { + fd, err := unix.Open("/dev/null", unix.O_RDONLY|unix.O_CLOEXEC, 0) + if err != nil { + t.Fatalf("open /dev/null: %v", err) + } + if fd > top { + _ = unix.Close(fd) + break + } + fillers = append(fillers, fd) + } + t.Cleanup(func() { + for _, fd := range fillers { + _ = unix.Close(fd) + } + }) +} + +// readHead reads from a blocking socket until the response head is complete, +// the peer closes, or d passes. +func readHead(fd int, d time.Duration) (string, error) { + tv := unix.NsecToTimeval(int64(50 * time.Millisecond)) + _ = unix.SetsockoptTimeval(fd, unix.SOL_SOCKET, unix.SO_RCVTIMEO, &tv) + var got []byte + buf := make([]byte, 4096) + for deadline := time.Now().Add(d); time.Now().Before(deadline); { + n, err := unix.Read(fd, buf) + if n > 0 { + got = append(got, buf[:n]...) + if bytes.Contains(got, []byte("\r\n\r\n")) { + return string(got), nil + } + continue + } + if errors.Is(err, unix.EAGAIN) || errors.Is(err, unix.EINTR) { + continue + } + if err != nil { + return string(got), err + } + return string(got), io.EOF + } + return string(got), fmt.Errorf("no response head within %v", d) +} + +// theftResult is one trial of arm A or the control. +type theftResult struct { + attempts int + hit bool + fd int + closer, sibl int + gate bool + witness uint64 + staleClosed uint64 + stale []recvtheft.StaleRecv + stolen bool + answered bool + answer string + answerErr error + missNoClose int + missQueued int + missOtherFD int + candidatesHit int +} + +const ( + theftMaxAttempts = 10 + theftCandidates = 6 + theftAcceptWait = 300 * time.Millisecond + theftSubmitWait = 300 * time.Millisecond + theftAnswerWait = 2 * time.Second + theftClosePath = "/close" + theftRequestAHead = "GET " + theftClosePath + " HTTP/1.1\r\nHost: recv-theft-715\r\nConnection: close\r\n\r\n" + theftRequestB = "GET /b HTTP/1.1\r\nHost: recv-theft-715\r\n\r\n" +) + +// runRecvTheftTrial drives attempts until the sibling worker accepts a fresh +// connection B as the number A's held close released (a hit), then releases +// the closer, then the sibling, and reports what happened to B's request. +func runRecvTheftTrial(t *testing.T, arm string, submitBeforeClose bool) theftResult { + e, port := startRecvTheftEngine(t) + sa := &unix.SockaddrInet4{Port: port, Addr: [4]byte{127, 0, 0, 1}} + var r theftResult + for r.attempts < theftMaxAttempts { + r.attempts++ + if done := recvTheftAttempt(t, e, sa, arm, submitBeforeClose, &r); done { + return r + } + } + return r +} + +func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm string, submitBeforeClose bool, r *theftResult) bool { + // The previous attempt's connections are closed by the engine as their + // clients go; a server-side close after the fill below would open a hole + // under the number this attempt frees, and the sibling's accept would take + // the hole. So start from an engine with no connection. + for deadline := time.Now().Add(3 * time.Second); e.metrics.activeConns.Load() != 0; { + if time.Now().After(deadline) { + t.Fatalf("engine still holds %d connections from the previous attempt", e.metrics.activeConns.Load()) + } + time.Sleep(5 * time.Millisecond) + } + fillFDHoles(t) + // B's candidate sockets exist before A is accepted, so they hold numbers + // below A's and none of them can take the number A's close frees. + var cands []int + defer func() { + for _, fd := range cands { + _ = unix.Close(fd) + } + }() + for range theftCandidates { + fd, err := unix.Socket(unix.AF_INET, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) + if err != nil { + t.Fatalf("socket: %v", err) + } + cands = append(cands, fd) + } + + witness0 := recvtheft.CloseWithUnsubmittedRecv() + stale0 := e.metrics.handoffLoss.staleRecvDataClosed.Load() + tr := recvtheft.Arm(recvtheft.Options{ + SubmitBeforeClose: submitBeforeClose, + PromoteGate: 2 * time.Second, + HoldMax: 10 * time.Second, + }) + defer tr.Disarm() + + a, err := net.DialTimeout("tcp", sockaddrString(sa), time.Second) + if err != nil { + t.Fatalf("dial A: %v", err) + } + defer func() { _ = a.Close() }() + if _, err := a.Write([]byte(theftRequestAHead)); err != nil { + t.Fatalf("write A: %v", err) + } + ce, ok := tr.WaitClose(3 * time.Second) + if !ok { + r.missNoClose++ + t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-close-hold gate=%v witness=+%d", arm, r.attempts, tr.GateUsed(), + recvtheft.CloseWithUnsubmittedRecv()-witness0) + return false + } + + // W1 is parked with N closed and A's recv unsubmitted. Dial B candidates + // until the sibling accepts one as N; one that hashes to W1's listener + // waits in W1's accept queue, one the sibling accepts as another number is + // served normally. + var hit *recvtheft.AcceptEvent + var hitFD int + for i, fd := range cands { + if err := unix.Connect(fd, sa); err != nil { + t.Fatalf("connect candidate %d: %v", i, err) + } + if _, err := unix.Write(fd, []byte(theftRequestB)); err != nil { + t.Fatalf("write candidate %d: %v", i, err) + } + ev, ok := tr.NextAccept(theftAcceptWait) + if !ok { + r.missQueued++ + continue + } + if ev.FD != ce.FD || ev.Worker == ce.Worker { + r.missOtherFD++ + t.Logf("RECVTHEFT715 arm=%s attempt=%d candidate=%d accepted by worker %d as fd %d (target %d)", arm, r.attempts, i, ev.Worker, ev.FD, ce.FD) + continue + } + held, ok := tr.WaitAcceptHeld(time.Second) + if !ok { + t.Fatalf("accept of fd %d by worker %d reported but its hold did not fire", ev.FD, ev.Worker) + } + hit, hitFD = &held, fd + r.candidatesHit = i + 1 + break + } + if hit == nil { + t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-sibling-accept-of-target fd=%d closer=%d queued=%d other_fd=%d", + arm, r.attempts, ce.FD, ce.Worker, r.missQueued, r.missOtherFD) + return false + } + r.hit, r.fd, r.closer, r.sibl, r.gate = true, ce.FD, ce.Worker, hit.Worker, tr.GateUsed() + + // Release W1: its next submit issues whatever it still holds for N. + tr.ReleaseClose() + for deadline := time.Now().Add(theftSubmitWait); time.Now().Before(deadline) && len(tr.Stale()) == 0; { + time.Sleep(time.Millisecond) + } + // Then the sibling: it submits B's own recv. + tr.ReleaseAccept() + r.answer, r.answerErr = readHead(hitFD, theftAnswerWait) + r.answered = strings.HasPrefix(r.answer, "HTTP/1.1 200") + + r.witness = recvtheft.CloseWithUnsubmittedRecv() - witness0 + r.staleClosed = e.metrics.handoffLoss.staleRecvDataClosed.Load() - stale0 + r.stale = tr.Stale() + for _, s := range r.stale { + if s.FD == r.fd && int(s.Res) == len(theftRequestB) && len(s.Head) > 0 && + bytes.Equal(s.Head, []byte(theftRequestB)[:len(s.Head)]) { + r.stolen = true + } + } + return true +} + +func logTheftResult(t *testing.T, arm string, r theftResult) { + t.Helper() + var heads []string + for _, s := range r.stale { + heads = append(heads, fmt.Sprintf("{worker=%d fd=%d gen=%d res=%d head=%q}", s.Worker, s.FD, s.Gen, s.Res, s.Head)) + } + errText := "" + if r.answerErr != nil { + errText = r.answerErr.Error() + } + t.Logf("RECVTHEFT715 arm=%s result attempts=%d hit=%v fd=%d closer=%d sibling=%d gate=%v candidates=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d other_fd=%d}", + arm, r.attempts, r.hit, r.fd, r.closer, r.sibl, r.gate, r.candidatesHit, r.witness, r.staleClosed, r.stolen, r.answered, + errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missOtherFD) +} + +func judgeTheftTrial(t *testing.T, arm string, r theftResult) { + t.Helper() + logTheftResult(t, arm, r) + if !r.hit { + skipOrFail656(t, "INCONCLUSIVE: no sibling accept of the released number in %d attempts", r.attempts) + } + if r.witness == 0 { + t.Fatalf("the close hold fired but close_with_unsubmitted_recv did not move") + } + if r.stolen || !r.answered { + t.Fatalf("B (fd %d on worker %d) lost its request: stolen by the closed conn's recv=%v (stale_recv_data_closed +%d), answered=%v (%v)", + r.fd, r.sibl, r.stolen, r.staleClosed, r.answered, r.answerErr) + } +} + +// TestRecvTheft715ArmA is hypothesis (a) on the tree as it is. It asserts the +// property the celeris#685 close-path fix must restore, and fails on main by +// design when (a) holds: B's request read by A's unsubmitted recv. +func TestRecvTheft715ArmA(t *testing.T) { + requireRecvTheft715(t) + judgeTheftTrial(t, "A", runRecvTheftTrial(t, "A", false)) +} + +// TestRecvTheft715Control is arm A's trial with the close paths submitting the +// ring before they close (recvtheft.Options.SubmitBeforeClose): A's recv is +// issued while N still names A's socket. Predicted: B answered, nothing stolen. +func TestRecvTheft715Control(t *testing.T) { + requireRecvTheft715(t) + judgeTheftTrial(t, "control", runRecvTheftTrial(t, "control", true)) +} + +// TestRecvTheft715ArmC is hypothesis (c): a request the promoted connection's +// own recv read but that never reached its dispatch goroutine. The window +// between the worker's asyncInMu unlock and its Signal / goroutine start is +// widened to 2 ms on every hand-off (recvtheft.SetWakeHold) while keep-alive +// and pipelined requests hit an async route. Asserts every one is answered and +// that the hold actually ran. +func TestRecvTheft715ArmC(t *testing.T) { + requireRecvTheft715(t) + _, port := startRecvTheftEngine(t) + recvtheft.SetWakeHold(2 * time.Millisecond) + defer recvtheft.SetWakeHold(0) + holds0 := recvtheft.WakeHolds() + + const conns, rounds, pipeline = 8, 20, 3 + req := "GET /async HTTP/1.1\r\nHost: recv-theft-715\r\n\r\n" + var mu sync.Mutex + var lost []string + answered := 0 + var wg sync.WaitGroup + for c := range conns { + wg.Go(func() { + conn, err := net.DialTimeout("tcp", "127.0.0.1:"+strconv.Itoa(port), time.Second) + if err != nil { + mu.Lock() + lost = append(lost, fmt.Sprintf("conn %d: dial: %v", c, err)) + mu.Unlock() + return + } + defer func() { _ = conn.Close() }() + buf := make([]byte, 0, 4096) + tmp := make([]byte, 4096) + for round := range rounds { + n := 1 + if round%4 == 3 { + n = pipeline + } + _ = conn.SetDeadline(time.Now().Add(2 * time.Second)) + if _, err := conn.Write([]byte(strings.Repeat(req, n))); err != nil { + mu.Lock() + lost = append(lost, fmt.Sprintf("conn %d round %d: write: %v", c, round, err)) + mu.Unlock() + return + } + for got := 0; got < n; { + if i := bytes.Index(buf, []byte("\r\n\r\nok")); i >= 0 { + buf = buf[i+len("\r\n\r\nok"):] + got++ + mu.Lock() + answered++ + mu.Unlock() + continue + } + k, err := conn.Read(tmp) + if err != nil { + mu.Lock() + lost = append(lost, fmt.Sprintf("conn %d round %d: %d of %d answered: %v", c, round, got, n, err)) + mu.Unlock() + return + } + buf = append(buf, tmp[:k]...) + } + } + }) + } + wg.Wait() + holds := recvtheft.WakeHolds() - holds0 + want := conns * (rounds/4*(3+pipeline) + rounds%4) + t.Logf("RECVTHEFT715 arm=C result conns=%d requests=%d answered=%d lost=%d wake_holds=+%d", conns, want, answered, len(lost), holds) + for _, l := range lost { + t.Logf("RECVTHEFT715 arm=C lost %s", l) + } + if holds == 0 { + t.Fatal("the hand-off window hold never ran: the async hand-off was not exercised") + } + if len(lost) > 0 || answered != want { + t.Fatalf("%d of %d requests answered with the hand-off window widened; lost: %v", answered, want, lost) + } +} diff --git a/engine/iouring/recv_theft_prod_linux_test.go b/engine/iouring/recv_theft_prod_linux_test.go new file mode 100644 index 00000000..f5453ad5 --- /dev/null +++ b/engine/iouring/recv_theft_prod_linux_test.go @@ -0,0 +1,27 @@ +//go:build linux && !validation + +package iouring + +import ( + "testing" + "unsafe" + + "github.com/goceleris/celeris/internal/recvtheft" +) + +// TestRecvTheftWitnessCompilesAway pins that the celeris#715 witness costs a +// production build nothing: recvtheft.Enabled is false, so its call sites +// compile away, and connState.recvArmSeq is zero-size and shares its offset +// with the field after it, so the connState layout is unchanged. +func TestRecvTheftWitnessCompilesAway(t *testing.T) { + if recvtheft.Enabled { + t.Fatal("recvtheft.Enabled is true in a build without the validation tag") + } + var cs connState + if n := unsafe.Sizeof(cs.recvArmSeq); n != 0 { + t.Fatalf("connState.recvArmSeq is %d bytes in production, want 0", n) + } + if a, b := unsafe.Offsetof(cs.recvArmSeq), unsafe.Offsetof(cs.kernelInflight); a != b { + t.Fatalf("connState.recvArmSeq at offset %d, kernelInflight at %d: the zero-size field must not add padding", a, b) + } +} diff --git a/engine/iouring/ring.go b/engine/iouring/ring.go index 63ab1576..4c99aba1 100644 --- a/engine/iouring/ring.go +++ b/engine/iouring/ring.go @@ -394,6 +394,20 @@ func retryPending(n uint32, errno unix.Errno) uint32 { // Pending returns the number of SQEs submitted but not yet sent to the kernel. func (r *Ring) Pending() uint32 { return r.pending } +// sqPlaced returns the SQ ring's tail: the sequence number the next GetSQE +// will take, so the SQE placed last has sqPlaced()-1. +func (r *Ring) sqPlaced() uint32 { + if r.singleIssuer { + return *(*uint32)(r.sqTail) + } + return atomic.LoadUint32((*uint32)(r.sqTail)) +} + +// sqConsumed returns the SQ ring's head, which the kernel advances as it +// consumes SQEs: an SQE placed at sequence s has not reached the kernel yet +// while int32(s-sqConsumed()) >= 0 (celeris#715). +func (r *Ring) sqConsumed() uint32 { return atomic.LoadUint32((*uint32)(r.sqHead)) } + // ClearPending resets the pending counter without issuing a syscall. Used with // SQPOLL where the kernel thread submits SQEs automatically. func (r *Ring) ClearPending() { r.pending = 0 } diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go index b734d332..fc506db9 100644 --- a/engine/iouring/worker.go +++ b/engine/iouring/worker.go @@ -26,6 +26,7 @@ import ( "github.com/goceleris/celeris/internal/ctxkit" "github.com/goceleris/celeris/internal/deferlinger" "github.com/goceleris/celeris/internal/platform" + "github.com/goceleris/celeris/internal/recvtheft" "github.com/goceleris/celeris/internal/sockopts" "github.com/goceleris/celeris/internal/wakefd" "github.com/goceleris/celeris/internal/zcwindow" @@ -801,6 +802,40 @@ func (w *Worker) noteRecvPlaced(cs *connState) { if cs.recvOutstanding >= 2 { w.recvArm.noteDoubleArmed() } + // celeris#715, validation builds only (recvtheft.Enabled is a false + // constant otherwise): both placement sites call this right after the + // recv's GetSQE, so the SQE placed last is the recv. + if recvtheft.Enabled { + cs.recvArmSeq.Set(w.ring.sqPlaced() - 1) + } +} + +// recvUnsubmitted reports whether cs's armed recv SQE is still in the SQ +// ring, not yet consumed by the kernel: it was placed after this worker's +// last submit. Validation builds only: recvArmSeq is recorded only there. +// +// Why the close paths ask (celeris#715 hypothesis (a), the celeris#685 +// class). Such a recv names the descriptor NUMBER (fixed files are off), and +// the kernel resolves the number when the next submit issues the recv. +// finishClose and finishCloseDetached queue the recv's ASYNC_CANCEL behind it +// and close the descriptor at once. If another thread's accept is given the +// freed number before this worker's next submit, and its connection's request +// is already in the socket (TCP_DEFER_ACCEPT hands over only connections that +// have sent), the recv reads that request and completes under the closed +// conn's (fd, generation): staleConnCQE drops it as stale_recv_data_closed, +// and the new connection's own recv then waits on an empty socket. So, under +// -tags=validation, the close paths +// - count the close (recvtheft.CloseWithUnsubmittedRecv, the witness), +// - park the worker right after the descriptor is closed when a +// recvtheft trial is armed (recvtheft.HoldAfterClose), and +// - in the trial's control arm, submit the ring before closing +// (recvtheft.SubmitBeforeClose), so the recv is issued while the number +// still names this conn's socket. +// +// None of it exists in production: recvtheft.Enabled is a false constant +// there and cs.recvArmSeq is zero-size. +func (w *Worker) recvUnsubmitted(cs *connState) bool { + return cs.recvArmed && w.ring != nil && int32(cs.recvArmSeq.Get()-w.ring.sqConsumed()) >= 0 } // retireRecvCancel accounts for one of cs's outstanding backpressure-pause @@ -1550,6 +1585,11 @@ func (w *Worker) staleConnCQE(c *completionEntry, fd int, ud uint64) bool { // identity (celeris#657). if op == udRecv && c.Res > 0 { w.noteStaleRecvData(ud) + // celeris#715, validation builds only: record what the stale recv + // read, while closedOps still holds its connState. + if recvtheft.Enabled { + w.noteStaleRecvExemplar(c, fd, ud) + } } if terminalOp { w.noteStaleTerminalOp(ud) @@ -2035,6 +2075,13 @@ func (w *Worker) onAcceptedFD(ctx context.Context, newFD int, now int64, isFixed cs.needsRecv = true w.markDirty(cs) } + // celeris#715, validation builds only: report the accept to an armed + // recvtheft trial, which parks this worker here (its recv prepared, not + // submitted) when it was given the number another worker's held close + // released. + if recvtheft.Enabled { + recvtheft.AfterAccept(w.id, newFD) + } } // stepAcceptPause runs one step of the accept pause on the worker thread, @@ -2920,6 +2967,11 @@ func (w *Worker) handleRecv(c *completionEntry, fd int, now int64) { w.bufRing.PushBuffer(providedBufID) w.hasBufReturns = true } + // celeris#715 hypothesis (c), validation builds only: the same + // window as promoteConnToAsync's (recvtheft.SetWakeHold). + if recvtheft.Enabled { + recvtheft.WakeHold() + } if starting { w.asyncWG.Add(1) go w.runAsyncHandler(cs) @@ -4029,9 +4081,18 @@ func (w *Worker) finishClose(fd int) { // straggler bytes into memory Go has repurposed — the #256 stackalloc // SIGSEGV / Green-Tea-GC span-corruption class. if cs != nil { + // celeris#715 witness, hold and control, validation builds only: + // see recvUnsubmitted. + if recvtheft.Enabled && w.recvUnsubmitted(cs) { + recvtheft.NoteCloseWithUnsubmittedRecv() + defer recvtheft.HoldAfterClose(w.id, fd) + } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) w.queuePendingRelease(cs) + if recvtheft.Enabled && recvtheft.SubmitBeforeClose() { + _, _ = w.ring.Submit() + } } if fixedFile { @@ -4145,9 +4206,18 @@ func (w *Worker) finishCloseDetached(fd int, cs *connState) { // "s.allocCount != s.nelems" span corruption (v1.4.15/7beebb9: the old 100 ms // wall-clock hold sat below TCP's 200 ms RTO_MIN, so retransmitted // POST segments landed after release). + // celeris#715 witness, hold and control, validation builds only: see + // recvUnsubmitted. + if recvtheft.Enabled && w.recvUnsubmitted(cs) { + recvtheft.NoteCloseWithUnsubmittedRecv() + defer recvtheft.HoldAfterClose(w.id, fd) + } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) w.queuePendingReleaseDetached(cs) + if recvtheft.Enabled && recvtheft.SubmitBeforeClose() { + _, _ = w.ring.Submit() + } if fixedFile { sqe := w.ring.GetSQE() @@ -4227,6 +4297,11 @@ func (w *Worker) promoteConnToAsync(cs *connState, _ int, stashed []byte, c *com cs.asyncRun = true } cs.asyncInMu.Unlock() + // celeris#715 hypothesis (c), validation builds only: widen the window + // between the unlock and the goroutine's wake-up (recvtheft.SetWakeHold). + if recvtheft.Enabled { + recvtheft.WakeHold() + } if starting { w.asyncWG.Add(1) go w.runAsyncHandler(cs) @@ -4243,6 +4318,13 @@ func (w *Worker) promoteConnToAsync(cs *connState, _ int, stashed []byte, c *com w.markDirty(cs) } } + // celeris#715 hypothesis (a), validation builds only: with a trial armed, + // let the dispatch goroutine queue its close before this iteration + // reaches drainDetachQueue, so the recv just re-armed is still + // unsubmitted when closeConn runs (recvtheft.Options.PromoteGate). + if recvtheft.Enabled { + recvtheft.AfterPromoteArm(func() bool { return w.detachQPending.Load() != 0 }) + } } // asyncHeaderDeadline returns the header deadline the worker should arm a diff --git a/internal/recvtheft/doc.go b/internal/recvtheft/doc.go new file mode 100644 index 00000000..eb0de3fb --- /dev/null +++ b/internal/recvtheft/doc.go @@ -0,0 +1,39 @@ +// Package recvtheft holds the witness and the hold points of the celeris#715 +// recv-theft measurement (hypothesis (a) of that issue, the celeris#685 class). +// Everything here is compiled in only under -tags=validation (recvtheft.go); +// production builds get the no-ops in recvtheft_off.go, whose false Enabled +// constant compiles the engine's call sites away, and whose ArmSeq is a +// zero-size struct. It is internal, like internal/zcwindow, so nothing outside +// this module can call it. +// +// The window. The io_uring worker only PREPARES a recv SQE; it reaches the +// kernel at the next io_uring_enter, at the top of the next loop iteration. +// Fixed files are off, so the SQE names the descriptor NUMBER, and the kernel +// resolves that number when it issues the op, not when the SQE was written. +// finishClose and finishCloseDetached queue an ASYNC_CANCEL behind such an +// unsubmitted recv and then close the descriptor synchronously. If another +// thread's accept is given the freed number before this worker submits, the +// recv reads the new connection's request, completes under the old +// connection's (fd, generation), and is dropped as stale; the new connection +// then waits on an empty socket. +// +// The pieces: +// - a witness counter, [CloseWithUnsubmittedRecv]: a close reached while +// the connection's recv SQE had not been consumed by the kernel; +// - a close hold ([HoldAfterClose]) that parks the closing worker right +// after the close, and an accept hold ([AfterAccept]) that parks a +// sibling worker right after it accepted the freed number, before it +// submits its own recv for it. Together they force the one ordering the +// theft needs: the sibling's accept after the close, the closing worker's +// submit before the sibling's; +// - a gate ([AfterPromoteArm]) that makes the dispatch goroutine's close +// land in the same loop iteration as the promote's recv re-arm; +// - the control switch ([Options.SubmitBeforeClose]): submit before the +// close, so the recv is issued while the number still names the closing +// connection's socket; +// - an exemplar record of every stale recv completion that carried data +// ([NoteStaleRecvData]), with its first bytes; +// - a separate hold for hypothesis (c) ([WakeHold]): the dispatch +// goroutine's wake-up delayed between the worker's asyncInMu unlock and +// its Signal / goroutine start. +package recvtheft diff --git a/internal/recvtheft/recvtheft.go b/internal/recvtheft/recvtheft.go new file mode 100644 index 00000000..92130ff8 --- /dev/null +++ b/internal/recvtheft/recvtheft.go @@ -0,0 +1,294 @@ +//go:build validation + +package recvtheft + +import ( + "sync" + "sync/atomic" + "time" +) + +// Enabled is true in a -tags=validation build and false (a constant, so +// guarded code compiles away) in production (recvtheft_off.go). +const Enabled = true + +// ArmSeq records the SQ ring sequence number at which a connection's latest +// recv SQE was placed (the ring's tail minus one right after the placement). +// Compared with the kernel's SQ head it says whether the kernel has consumed +// that SQE yet. Worker-thread only, like every other recv field of the +// connection. Zero-size in production. +type ArmSeq struct{ seq uint32 } + +// Set records the sequence number of the recv SQE just placed. +func (a *ArmSeq) Set(seq uint32) { a.seq = seq } + +// Get returns the recorded sequence number. +func (a *ArmSeq) Get() uint32 { return a.seq } + +// closeWithUnsubmittedRecv is the witness: closes (finishClose, +// finishCloseDetached) reached while the connection's recv SQE was still in +// the SQ ring, not yet consumed by the kernel. +var closeWithUnsubmittedRecv atomic.Uint64 + +// NoteCloseWithUnsubmittedRecv counts one such close. Engine hook. +func NoteCloseWithUnsubmittedRecv() { closeWithUnsubmittedRecv.Add(1) } + +// CloseWithUnsubmittedRecv returns the witness count since process start. +func CloseWithUnsubmittedRecv() uint64 { return closeWithUnsubmittedRecv.Load() } + +// Options configures one [Trial]. +type Options struct { + // SubmitBeforeClose is the control arm: the close paths submit the SQ + // ring after queueing their cancels and before closing the descriptor, + // so a recv prepared earlier in the iteration is issued while its + // number still names the closing connection's socket. + SubmitBeforeClose bool + // PromoteGate bounds how long a worker that has just re-armed a + // promoted connection's recv (promoteConnToAsync) waits for the + // dispatch goroutine to queue a close. Zero disables the gate. The + // gate fires once per trial. + PromoteGate time.Duration + // HoldMax bounds each hold, so a test that stops driving the trial + // cannot park a worker for longer than this. Zero means 5 s. + HoldMax time.Duration +} + +// CloseEvent is the close hold firing: worker Worker closed descriptor FD +// with a recv SQE for it still unsubmitted, and is parked. +type CloseEvent struct{ Worker, FD int } + +// AcceptEvent is an accept the engine completed while a trial had a target: +// worker Worker accepted a connection as descriptor FD. +type AcceptEvent struct{ Worker, FD int } + +// StaleRecv is one stale recv completion that carried data: the recv's +// identity (FD, Gen), its result and the first bytes it wrote. +type StaleRecv struct { + Worker int + FD int + Gen uint32 + Res int32 + Head []byte +} + +// Trial is one armed run of the measurement. At most one is armed at a time. +type Trial struct { + opts Options + + gateUsed atomic.Bool + + closeFired atomic.Bool + closeCh chan CloseEvent + releaseClose chan struct{} + relCloseOnce sync.Once + target atomic.Int64 + holder atomic.Int64 + + acceptCh chan AcceptEvent + acceptFired atomic.Bool + acceptHeldCh chan AcceptEvent + releaseAccept chan struct{} + relAcceptOnce sync.Once + + mu sync.Mutex + stale []StaleRecv +} + +var current atomic.Pointer[Trial] + +// Arm arms a new trial and returns it. A trial still armed is disarmed +// (released) first. +func Arm(o Options) *Trial { + if o.HoldMax <= 0 { + o.HoldMax = 5 * time.Second + } + t := &Trial{ + opts: o, + closeCh: make(chan CloseEvent, 1), + releaseClose: make(chan struct{}), + acceptCh: make(chan AcceptEvent, 64), + acceptHeldCh: make(chan AcceptEvent, 1), + releaseAccept: make(chan struct{}), + } + t.target.Store(-1) + t.holder.Store(-1) + if old := current.Swap(t); old != nil { + old.release() + } + return t +} + +// Disarm releases every hold of t and disarms it. +func (t *Trial) Disarm() { + current.CompareAndSwap(t, nil) + t.release() +} + +func (t *Trial) release() { + t.ReleaseClose() + t.ReleaseAccept() +} + +// WaitClose waits up to d for the close hold to fire. +func (t *Trial) WaitClose(d time.Duration) (CloseEvent, bool) { + select { + case ev := <-t.closeCh: + return ev, true + case <-time.After(d): + return CloseEvent{}, false + } +} + +// ReleaseClose lets the worker parked by the close hold continue. +func (t *Trial) ReleaseClose() { t.relCloseOnce.Do(func() { close(t.releaseClose) }) } + +// NextAccept waits up to d for the next accept completed since the close +// hold fired. +func (t *Trial) NextAccept(d time.Duration) (AcceptEvent, bool) { + select { + case ev := <-t.acceptCh: + return ev, true + case <-time.After(d): + return AcceptEvent{}, false + } +} + +// WaitAcceptHeld waits up to d for the accept hold to fire. +func (t *Trial) WaitAcceptHeld(d time.Duration) (AcceptEvent, bool) { + select { + case ev := <-t.acceptHeldCh: + return ev, true + case <-time.After(d): + return AcceptEvent{}, false + } +} + +// ReleaseAccept lets the worker parked by the accept hold continue. +func (t *Trial) ReleaseAccept() { t.relAcceptOnce.Do(func() { close(t.releaseAccept) }) } + +// Stale returns a copy of the stale recv completions with data recorded while +// t was armed. +func (t *Trial) Stale() []StaleRecv { + t.mu.Lock() + defer t.mu.Unlock() + return append([]StaleRecv(nil), t.stale...) +} + +// GateUsed reports whether the promote gate fired in this trial. +func (t *Trial) GateUsed() bool { return t.gateUsed.Load() } + +// SubmitBeforeClose reports whether the armed trial is the control arm. +// Engine hook, worker thread. +func SubmitBeforeClose() bool { + t := current.Load() + return t != nil && t.opts.SubmitBeforeClose +} + +// AfterPromoteArm is called by promoteConnToAsync after it re-armed the +// connection's recv. The first time in a trial with a PromoteGate, it waits +// until queued reports that a detach-queue entry is pending (the dispatch +// goroutine's close) or the gate expires, so the close is drained in the same +// loop iteration as the re-arm. Engine hook, worker thread. +func AfterPromoteArm(queued func() bool) { + t := current.Load() + if t == nil || t.opts.PromoteGate <= 0 || t.closeFired.Load() || !t.gateUsed.CompareAndSwap(false, true) { + return + } + deadline := time.Now().Add(t.opts.PromoteGate) + for !queued() && time.Now().Before(deadline) { + time.Sleep(20 * time.Microsecond) + } +} + +// HoldAfterClose is deferred by finishClose / finishCloseDetached when they +// found the connection's recv SQE unsubmitted, so it runs right after the +// descriptor is closed. The first time in a trial it records (worker, fd) as +// the trial's target and parks the worker until [Trial.ReleaseClose] or +// HoldMax. Engine hook, worker thread. +func HoldAfterClose(worker, fd int) { + t := current.Load() + if t == nil || !t.closeFired.CompareAndSwap(false, true) { + return + } + t.holder.Store(int64(worker)) + t.target.Store(int64(fd)) + t.closeCh <- CloseEvent{Worker: worker, FD: fd} + select { + case <-t.releaseClose: + case <-time.After(t.opts.HoldMax): + } +} + +// AfterAccept is called by onAcceptedFD after it set up the new connection +// and prepared its first recv, before the worker's next submit. Once the +// trial has a target it reports every accept; the first accept of the target +// number by a worker other than the one holding the close parks that worker +// until [Trial.ReleaseAccept] or HoldMax. Engine hook, worker thread. +func AfterAccept(worker, fd int) { + t := current.Load() + if t == nil { + return + } + target := t.target.Load() + if target < 0 { + return + } + ev := AcceptEvent{Worker: worker, FD: fd} + select { + case t.acceptCh <- ev: + default: + } + if int64(fd) != target || int64(worker) == t.holder.Load() || !t.acceptFired.CompareAndSwap(false, true) { + return + } + t.acceptHeldCh <- ev + select { + case <-t.releaseAccept: + case <-time.After(t.opts.HoldMax): + } +} + +// NoteStaleRecvData records a stale recv completion that carried data while +// a trial is armed. head is copied (at most 64 bytes). Engine hook, worker +// thread. +func NoteStaleRecvData(worker, fd int, gen uint32, res int32, head []byte) { + t := current.Load() + if t == nil { + return + } + if len(head) > 64 { + head = head[:64] + } + t.mu.Lock() + t.stale = append(t.stale, StaleRecv{Worker: worker, FD: fd, Gen: gen, Res: res, Head: append([]byte(nil), head...)}) + t.mu.Unlock() +} + +// wakeHoldNanos is the hypothesis (c) hold. See [SetWakeHold]. +var ( + wakeHoldNanos atomic.Int64 + wakeHolds atomic.Uint64 +) + +// SetWakeHold makes every io_uring worker sleep for d between releasing a +// promoted connection's asyncInMu (after appending received bytes to +// asyncInBuf) and waking or starting its dispatch goroutine; d <= 0 turns it +// off. It widens the hand-off window of hypothesis (c) of celeris#715. +func SetWakeHold(d time.Duration) { + if d < 0 { + d = 0 + } + wakeHoldNanos.Store(int64(d)) +} + +// WakeHolds returns how many times [WakeHold] slept. +func WakeHolds() uint64 { return wakeHolds.Load() } + +// WakeHold sleeps for the duration set by [SetWakeHold], if any. Engine hook, +// worker thread. +func WakeHold() { + if d := wakeHoldNanos.Load(); d > 0 { + wakeHolds.Add(1) + time.Sleep(time.Duration(d)) + } +} diff --git a/internal/recvtheft/recvtheft_off.go b/internal/recvtheft/recvtheft_off.go new file mode 100644 index 00000000..b006ad01 --- /dev/null +++ b/internal/recvtheft/recvtheft_off.go @@ -0,0 +1,40 @@ +//go:build !validation + +package recvtheft + +// Enabled is false in production: the engine's call sites are guarded by it +// and compile away. +const Enabled = false + +// ArmSeq is zero-size in production: a connection carries no recv sequence. +type ArmSeq struct{} + +// Set is the production no-op. +func (*ArmSeq) Set(uint32) {} + +// Get is the production no-op. +func (*ArmSeq) Get() uint32 { return 0 } + +// NoteCloseWithUnsubmittedRecv is the production no-op. +func NoteCloseWithUnsubmittedRecv() {} + +// CloseWithUnsubmittedRecv is always 0 in production. +func CloseWithUnsubmittedRecv() uint64 { return 0 } + +// SubmitBeforeClose is always false in production. +func SubmitBeforeClose() bool { return false } + +// HoldAfterClose is the production no-op. +func HoldAfterClose(int, int) {} + +// AfterAccept is the production no-op. +func AfterAccept(int, int) {} + +// AfterPromoteArm is the production no-op. +func AfterPromoteArm(func() bool) {} + +// NoteStaleRecvData is the production no-op. +func NoteStaleRecvData(int, int, uint32, int32, []byte) {} + +// WakeHold is the production no-op. +func WakeHold() {} From 042bab30e269dbfecf728d73fc42d3b7762066ce Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 00:15:30 +0200 Subject: [PATCH 02/12] test(iouring): pin the recvArmSeq layout by alignment, not by a shared offset The zero-size field sits at connState offset 586 and kernelInflight at 588 on linux/arm64: the gap is kernelInflight's own 4-byte alignment, there with or without the field. The first version compared the two offsets and failed on a layout the field does not change. It now checks that kernelInflight is where its alignment alone puts it, and that the field is not the last one (a trailing zero-size field is what pads). Refs #715 --- engine/iouring/recv_theft_prod_linux_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/engine/iouring/recv_theft_prod_linux_test.go b/engine/iouring/recv_theft_prod_linux_test.go index f5453ad5..794cb86b 100644 --- a/engine/iouring/recv_theft_prod_linux_test.go +++ b/engine/iouring/recv_theft_prod_linux_test.go @@ -11,8 +11,10 @@ import ( // TestRecvTheftWitnessCompilesAway pins that the celeris#715 witness costs a // production build nothing: recvtheft.Enabled is false, so its call sites -// compile away, and connState.recvArmSeq is zero-size and shares its offset -// with the field after it, so the connState layout is unchanged. +// compile away, and connState.recvArmSeq is zero-size and not the last field +// (only a trailing zero-size field makes Go pad a struct), so kernelInflight, +// the field after it, sits where its own alignment puts it and the connState +// layout is unchanged. func TestRecvTheftWitnessCompilesAway(t *testing.T) { if recvtheft.Enabled { t.Fatal("recvtheft.Enabled is true in a build without the validation tag") @@ -21,7 +23,11 @@ func TestRecvTheftWitnessCompilesAway(t *testing.T) { if n := unsafe.Sizeof(cs.recvArmSeq); n != 0 { t.Fatalf("connState.recvArmSeq is %d bytes in production, want 0", n) } - if a, b := unsafe.Offsetof(cs.recvArmSeq), unsafe.Offsetof(cs.kernelInflight); a != b { - t.Fatalf("connState.recvArmSeq at offset %d, kernelInflight at %d: the zero-size field must not add padding", a, b) + at, next, align := unsafe.Offsetof(cs.recvArmSeq), unsafe.Offsetof(cs.kernelInflight), unsafe.Alignof(cs.kernelInflight) + if want := (at + align - 1) &^ (align - 1); next != want { + t.Fatalf("connState.recvArmSeq at offset %d moves kernelInflight to %d, want %d (its alignment, %d, alone)", at, next, want, align) + } + if last := unsafe.Offsetof(cs.recvOutstanding); at >= last { + t.Fatalf("connState.recvArmSeq (offset %d) must not be the last field (recvOutstanding is at %d): a trailing zero-size field adds padding", at, last) } } From c468f9e30fc522aa00f82df947186ce50ca39b88 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 01:17:08 +0200 Subject: [PATCH 03/12] test(iouring): record the measured connState size in the layout test's doc Refs #715 --- engine/iouring/recv_theft_prod_linux_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/engine/iouring/recv_theft_prod_linux_test.go b/engine/iouring/recv_theft_prod_linux_test.go index 794cb86b..5561b831 100644 --- a/engine/iouring/recv_theft_prod_linux_test.go +++ b/engine/iouring/recv_theft_prod_linux_test.go @@ -12,9 +12,10 @@ import ( // TestRecvTheftWitnessCompilesAway pins that the celeris#715 witness costs a // production build nothing: recvtheft.Enabled is false, so its call sites // compile away, and connState.recvArmSeq is zero-size and not the last field -// (only a trailing zero-size field makes Go pad a struct), so kernelInflight, -// the field after it, sits where its own alignment puts it and the connState -// layout is unchanged. +// (a trailing zero-size field is the one kind Go may pad for), so +// kernelInflight, the field after it, sits where its own alignment puts it and +// the connState layout is unchanged. Measured at the time of writing: +// linux/amd64 and linux/arm64 both 600 bytes, with and without the field. func TestRecvTheftWitnessCompilesAway(t *testing.T) { if recvtheft.Enabled { t.Fatal("recvtheft.Enabled is true in a build without the validation tag") @@ -28,6 +29,6 @@ func TestRecvTheftWitnessCompilesAway(t *testing.T) { t.Fatalf("connState.recvArmSeq at offset %d moves kernelInflight to %d, want %d (its alignment, %d, alone)", at, next, want, align) } if last := unsafe.Offsetof(cs.recvOutstanding); at >= last { - t.Fatalf("connState.recvArmSeq (offset %d) must not be the last field (recvOutstanding is at %d): a trailing zero-size field adds padding", at, last) + t.Fatalf("connState.recvArmSeq (offset %d) must not be the last field (recvOutstanding is at %d): a trailing zero-size field can add padding", at, last) } } From fd55b1e6db26350dada5f5588efe05a46ce60309 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 03:22:30 +0200 Subject: [PATCH 04/12] test(iouring): judge the #715 trial by what B gets, add the linked-recv trial, run both in CI (celeris#685) The #715 trial (PR #781) counted a hit only when the sibling worker accepted B on the exact number the closer's close had freed. A fix that keeps the number allocated until the closed connection's recv has ended leaves the sibling nothing to take, so every attempt would read INCONCLUSIVE. A hit is now the sibling accepting B while the closer is parked, on whatever number; the trial records whether it was A's number (reused), whether A's number was still open at the hold (held_open), and that a number kept open is released after the closer's release (released). The verdict is unchanged: B answered and no stale recv carrying B's bytes. TestRecvTheft685Linked is the linked form of the same theft: a recv chained behind a SEND (flushSendLink) is consumed with the SEND but issued, and its descriptor number resolved, only when the SEND completes, as task work the enter that posts the SEND's CQE can leave queued. The trial blocks A's response SEND behind a send queue filled from outside the engine, lets WriteTimeout defer the close, drains A so the SEND completes and the deferred close runs with the linked recv owed, and parks the closer there (recvtheft.Options.LinkedRecv, witness close_with_linked_recv). On this tree both fail by design (laptop container, arm64, kernel 7.0): TestRecvTheft715ArmA stolen 3 of 3, TestRecvTheft685Linked stolen 5 of 5, while TestRecvTheft715Control passes. The new `recv-theft` CI job runs the four trials with two io_uring workers on both arches and requires every run to pass; it is red until the close-path fix lands. Refs #685 #715 --- .github/workflows/ci.yml | 62 +++ .../recv_theft_685_linked_linux_test.go | 412 ++++++++++++++++++ engine/iouring/recv_theft_715_linux_test.go | 90 +++- engine/iouring/worker.go | 46 +- internal/recvtheft/recvtheft.go | 31 +- internal/recvtheft/recvtheft_off.go | 8 +- 6 files changed, 616 insertions(+), 33 deletions(-) create mode 100644 engine/iouring/recv_theft_685_linked_linux_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 393bca9a..beda726a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -980,6 +980,68 @@ jobs: tally mutant kill || ok=1 exit "$ok" + recv-theft: + name: io_uring close paths never release a descriptor an op can still resolve (celeris#685, ${{ matrix.os }}) + # Both arches: the theft was measured on both (celeris#715). + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, ubuntu-24.04-arm] + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: "1.27.0" + # The celeris#715 / celeris#685 trials (-tags=validation only; the + # holds are internal/recvtheft). Each parks the worker that closed a + # connection A with a recv still able to resolve A's descriptor number + # (TestRecvTheft715ArmA: a recv SQE not yet submitted; + # TestRecvTheft685Linked: a recv linked behind a SEND, not yet + # issued), lets the sibling worker accept a fresh connection B, and + # asserts that B is answered and that no stale recv read B's bytes. + # On a tree that frees the number at the close, the sibling is given + # it and the old recv reads B's request: both fail in every trial. + # TestRecvTheft715Control (the ring submitted before the close) and + # TestRecvTheft715ArmC (celeris#715 hypothesis (c)) must pass either way. + # + # The trials need TWO io_uring workers (12 MiB of RLIMIT_MEMLOCK each; + # the runner defaults to 8 MiB), so memlock is raised as in the + # `iouring` job, and CELERIS_REQUIRE_IOURING_WORKERS=1 turns a missing + # sibling into a failure. -v, -count=5, and a tally that requires + # every run of every test to PASS, with no FAIL and no SKIP line: a + # renamed test selects nothing and go test would still exit 0. + - name: celeris#685 recv-theft trials (two workers, skipping forbidden) + shell: bash + env: + CELERIS_REQUIRE_IOURING_WORKERS: "1" + CELERIS_RECV_THEFT_715: "1" + run: | + set -o pipefail + sudo prlimit --pid "$$" --memlock=unlimited:unlimited + echo "memlock (KiB): $(ulimit -l)" + names='TestRecvTheft715ArmA|TestRecvTheft715Control|TestRecvTheft715ArmC|TestRecvTheft685Linked' + runs=5 + want=$(( $(tr '|' '\n' <<<"$names" | wc -l) * runs )) + go test -tags=validation -race -count="$runs" -timeout=600s -v -run "^(${names})\$" \ + ./engine/iouring/ 2>&1 | tee /tmp/recvtheft685.log || true + grep -E 'RECVTHEFT(715|685) .*result' /tmp/recvtheft685.log || true + passed=$(grep -cE "^--- PASS: (${names}) \(" /tmp/recvtheft685.log || true) + failed=$(grep -cE '^[[:space:]]*--- FAIL' /tmp/recvtheft685.log || true) + skipped=$(grep -cE '^[[:space:]]*--- SKIP' /tmp/recvtheft685.log || true) + for n in ${names//|/ }; do + echo "$n: PASS $(grep -cE "^--- PASS: $n \(" /tmp/recvtheft685.log || true) FAIL $(grep -cE "^--- FAIL: $n \(" /tmp/recvtheft685.log || true)" + done + echo "celeris#685 recv-theft trials: want $want PASS, passed $passed, FAIL lines $failed, SKIP lines $skipped" + if [ "$passed" -ne "$want" ] || [ "$failed" -ne 0 ] || [ "$skipped" -ne 0 ]; then + echo "expected exactly $want trial runs to PASS with no FAIL or SKIP line -- did a close path" + echo "release a descriptor number a recv could still resolve, or was a test renamed or skipped?" + exit 1 + fi + conformance: name: Conformance runs-on: ubuntu-latest diff --git a/engine/iouring/recv_theft_685_linked_linux_test.go b/engine/iouring/recv_theft_685_linked_linux_test.go new file mode 100644 index 00000000..10d38d4f --- /dev/null +++ b/engine/iouring/recv_theft_685_linked_linux_test.go @@ -0,0 +1,412 @@ +//go:build linux && validation + +package iouring + +import ( + "context" + "errors" + "io" + "log/slog" + "net" + "os" + "strconv" + "strings" + "testing" + "time" + + "golang.org/x/sys/unix" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/internal/recvtheft" + "github.com/goceleris/celeris/protocol/h2/stream" + "github.com/goceleris/celeris/resource" +) + +// celeris#685, the linked form of the celeris#715 theft, made deterministic. +// +// A sync-mode HTTP/1 response is flushed with its connection's next recv +// chained behind the SEND (flushSendLink, IOSQE_IO_LINK). The kernel consumes +// both SQEs at the submit, but it issues the recv, and resolves its descriptor +// NUMBER, only when the SEND completes: on a DEFER_TASKRUN ring as task work, +// which the enter that posts the SEND's CQE can leave queued. A close path +// that runs at that CQE (completeSend of a connection closeConn deferred while +// the SEND was in flight) queues a cancel that cannot find the recv (it is in +// no cancel table yet) and closed the descriptor at once. If a sibling worker +// is given the number before the closer's next enter, that enter issues the +// recv against the new connection's socket. +// +// The trial: +// 1. A sends half a request (TCP_DEFER_ACCEPT holds the accept until it +// does). The test finds A's server-side descriptor N and fills its send +// queue from outside the engine (A never reads), so the next SEND cannot +// complete. +// 2. A sends the rest. The worker W1 answers with a linked SEND+recv; the +// SEND waits for room. +// 3. WriteTimeout fires (once A's header timer has woken W1, see +// linkedHeaderTimeout): checkTimeouts' closeConn finds the SEND in +// flight and defers the close. +// 4. A drains its socket. The SEND completes, and completeSend runs the +// deferred close with the linked recv owed; W1 parks when the close path +// returns (recvtheft.Options.LinkedRecv). +// 5. The test dials B's candidates. The sibling W2 accepts one, as N if the +// close released it (and W2 then parks before submitting B's recv). +// 6. W1 is released (its next enter runs the queued task work, which issues +// the linked recv), then W2. +// +// Asserts what the fix restores: B answered, no stale recv carrying B's +// bytes, and a number kept open at the close released after. Fails on a tree +// that closes the number at once whenever the kernel left the recv unissued +// at the SEND's CQE (see TestRecvTheft685Linked's log: reused, stolen). +// Needs two io_uring workers and CELERIS_RECV_THEFT_715=1, like the #715 arms. + +type linkedTheftHandler struct{} + +func (linkedTheftHandler) HandleStream(_ context.Context, s *stream.Stream) error { + if s.ResponseWriter == nil { + return nil + } + return s.ResponseWriter.WriteResponse(s, 200, + [][2]string{{"content-type", "text/plain"}, {"content-length", "2"}}, []byte("ok")) +} + +const ( + linkedWriteTimeout = 200 * time.Millisecond + // linkedHeaderTimeout is what wakes W1 once its SEND is blocked. A + // worker whose only op is that SEND waits in the ring with no timeout + // (run's mode 3a: a SEND is expected to complete), so its timeout sweep + // does not run until some completion arrives. A's header timer, armed + // by its half request, is that completion: it fires this long after A's + // first bytes, finds the headers complete and does nothing, and from + // the next iteration on the worker waits with a timeout and sweeps. + linkedHeaderTimeout = 1 * time.Second + // linkedCloseWait covers the header timer, WriteTimeout, and the timeout + // sweep's cadence (every 32 iterations, each at most 25 ms with + // ReadHeaderTimeout set). + linkedCloseWait = 2500 * time.Millisecond + linkedReqHead = "GET /a HTTP/1.1\r\n" + linkedReqTail = "Host: recv-theft-685\r\n\r\n" +) + +func startLinkedTheftEngine(t *testing.T) (*Engine, int) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("pick port: %v", err) + } + addr := ln.Addr().String() + port := ln.Addr().(*net.TCPAddr).Port + _ = ln.Close() + e, err := New(resource.Config{ + Addr: addr, + Protocol: engine.HTTP1, + Resources: resource.Resources{Workers: 2}, + WriteTimeout: linkedWriteTimeout, + ReadHeaderTimeout: linkedHeaderTimeout, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }, linkedTheftHandler{}) + if err != nil { + skipOrFail656(t, "iouring engine unavailable: %v", err) + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- e.Listen(ctx) }() + t.Cleanup(func() { + cancel() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Error("engine did not stop within 5s") + } + }) + for deadline := time.Now().Add(8 * time.Second); e.Addr() == nil; { + select { + case err := <-done: + skipOrFail656(t, "iouring engine failed to start: %v", err) + default: + } + if time.Now().After(deadline) { + t.Fatal("engine did not start listening within 8s") + } + time.Sleep(10 * time.Millisecond) + } + if n := e.NumWorkers(); n < 2 { + skipOrFail656(t, "celeris#685 needs a sibling io_uring worker: workers=%d (RLIMIT_MEMLOCK funds one per 12 MiB)", n) + } + return e, port +} + +// serverFDFor returns this process's descriptor whose peer is local (the +// server side of a loopback connection the test dialed), or -1. +func serverFDFor(local string) int { + ents, err := os.ReadDir("/proc/self/fd") + if err != nil { + return -1 + } + for _, ent := range ents { + fd, err := strconv.Atoi(ent.Name()) + if err != nil { + continue + } + sa, err := unix.Getpeername(fd) + if err != nil { + continue + } + if sockaddrString(sa) == local { + return fd + } + } + return -1 +} + +// fillSendQueue writes to fd (a server-side socket whose peer never reads) +// until two passes 50 ms apart add nothing: the peer's window is closed and +// the send queue is full. Returns the bytes written. +func fillSendQueue(fd int) int { + junk := make([]byte, 64<<10) + total, idle := 0, 0 + for idle < 2 { + wrote := 0 + for { + n, err := unix.SendmsgN(fd, junk, nil, nil, unix.MSG_DONTWAIT|unix.MSG_NOSIGNAL) + if n > 0 { + wrote += n + } + if err != nil { + break + } + } + total += wrote + if wrote == 0 { + idle++ + } else { + idle = 0 + } + time.Sleep(50 * time.Millisecond) + } + return total +} + +type linkedTheftResult struct { + attempts int + hit bool + fd, closer, sibl int + bFD int + reused bool + heldOpen bool + released bool + filled int + witness uint64 + staleClosed uint64 + stale []recvtheft.StaleRecv + stolen bool + answered bool + answerErr error + missNoClose int + missQueued int + missCloserAccepts int +} + +func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *linkedTheftResult) bool { + for deadline := time.Now().Add(3 * time.Second); e.metrics.activeConns.Load() != 0; { + if time.Now().After(deadline) { + t.Fatalf("engine still holds %d connections from the previous attempt", e.metrics.activeConns.Load()) + } + time.Sleep(5 * time.Millisecond) + } + fillFDHoles(t) + var cands []int + defer func() { + for _, fd := range cands { + _ = unix.Close(fd) + } + }() + for range theftCandidates { + fd, err := unix.Socket(unix.AF_INET, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) + if err != nil { + t.Fatalf("socket: %v", err) + } + cands = append(cands, fd) + } + // A: a small receive buffer, so the fill below stays small. + a, err := unix.Socket(unix.AF_INET, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) + if err != nil { + t.Fatalf("socket A: %v", err) + } + defer func() { _ = unix.Close(a) }() + _ = unix.SetsockoptInt(a, unix.SOL_SOCKET, unix.SO_RCVBUF, 4096) + + witness0 := recvtheft.CloseWithLinkedRecv() + stale0 := e.metrics.handoffLoss.staleRecvDataClosed.Load() + tr := recvtheft.Arm(recvtheft.Options{LinkedRecv: true, HoldMax: 10 * time.Second}) + defer tr.Disarm() + + if err := unix.Connect(a, sa); err != nil { + t.Fatalf("connect A: %v", err) + } + if _, err := unix.Write(a, []byte(linkedReqHead)); err != nil { + t.Fatalf("write A head: %v", err) + } + aLocalSA, err := unix.Getsockname(a) + if err != nil { + t.Fatalf("getsockname A: %v", err) + } + aLocal := sockaddrString(aLocalSA) + srv := -1 + for deadline := time.Now().Add(2 * time.Second); srv < 0 && time.Now().Before(deadline); { + if srv = serverFDFor(aLocal); srv < 0 { + time.Sleep(2 * time.Millisecond) + } + } + if srv < 0 { + t.Fatalf("A (%s) was not accepted within 2s", aLocal) + } + // The search's own /proc/self/fd reads can leave a hole under N (a read + // that took the lowest number while the accept was still to come); fill + // it, so the number A's close frees is again the lowest free one. + fillFDHoles(t) + r.filled = fillSendQueue(srv) + if _, err := unix.Write(a, []byte(linkedReqTail)); err != nil { + t.Fatalf("write A tail: %v", err) + } + // The response's SEND now waits for room, with the next recv linked + // behind it; WriteTimeout's closeConn defers the close meanwhile. + closes0 := e.metrics.closeCount.Load() + time.Sleep(linkedCloseWait) + outqBefore, _ := unix.IoctlGetInt(srv, unix.SIOCOUTQ) + // Drain A until the close hold fires: the SEND completes, and the + // deferred close runs at its CQE. + drained := make(chan struct{}) + go func() { + defer close(drained) + buf := make([]byte, 64<<10) + tv := unix.NsecToTimeval(int64(20 * time.Millisecond)) + _ = unix.SetsockoptTimeval(a, unix.SOL_SOCKET, unix.SO_RCVTIMEO, &tv) + for end := time.Now().Add(3 * time.Second); time.Now().Before(end); { + n, err := unix.Read(a, buf) + if n == 0 && err == nil { + return + } + if err != nil && !errors.Is(err, unix.EAGAIN) && !errors.Is(err, unix.EINTR) { + return + } + } + }() + ce, ok := tr.WaitClose(4 * time.Second) + if !ok { + r.missNoClose++ + <-drained + outqAfter, _ := unix.IoctlGetInt(srv, unix.SIOCOUTQ) + t.Logf("RECVTHEFT685 linked attempt=%d miss=no-close-hold filled=%d outq_before_drain=%d outq_after=%d srv_after=%q closes=+%d witness=+%d", + r.attempts, r.filled, outqBefore, outqAfter, fdTarget(srv), e.metrics.closeCount.Load()-closes0, recvtheft.CloseWithLinkedRecv()-witness0) + return false + } + aTarget := fdTarget(ce.FD) + r.heldOpen = strings.HasPrefix(aTarget, "socket:") + + var hit *recvtheft.AcceptEvent + var hitFD int + for i, fd := range cands { + if err := unix.Connect(fd, sa); err != nil { + t.Fatalf("connect candidate %d: %v", i, err) + } + if _, err := unix.Write(fd, []byte(theftRequestB)); err != nil { + t.Fatalf("write candidate %d: %v", i, err) + } + ev, ok := tr.NextAccept(theftAcceptWait) + if !ok { + r.missQueued++ + continue + } + if ev.Worker == ce.Worker { + r.missCloserAccepts++ + continue + } + ev0 := ev + if ev.FD == ce.FD { + held, ok := tr.WaitAcceptHeld(time.Second) + if !ok { + t.Fatalf("accept of fd %d by worker %d reported but its hold did not fire", ev.FD, ev.Worker) + } + ev0 = held + r.reused = true + } + hit, hitFD = &ev0, fd + break + } + if hit == nil { + t.Logf("RECVTHEFT685 linked attempt=%d miss=no-sibling-accept fd=%d closer=%d queued=%d closer_accepts=%d", + r.attempts, ce.FD, ce.Worker, r.missQueued, r.missCloserAccepts) + tr.Disarm() + <-drained + return false + } + r.hit, r.fd, r.closer, r.sibl, r.bFD = true, ce.FD, ce.Worker, hit.Worker, hit.FD + + tr.ReleaseClose() + for deadline := time.Now().Add(theftSubmitWait); time.Now().Before(deadline) && len(tr.Stale()) == 0; { + time.Sleep(time.Millisecond) + } + tr.ReleaseAccept() + answer, aerr := readHead(hitFD, theftAnswerWait) + r.answered, r.answerErr = strings.HasPrefix(answer, "HTTP/1.1 200"), aerr + for deadline := time.Now().Add(theftReleaseWait); ; { + if cur := fdTarget(ce.FD); !r.heldOpen || cur != aTarget { + r.released = true + break + } + if time.Now().After(deadline) { + break + } + time.Sleep(time.Millisecond) + } + <-drained + r.witness = recvtheft.CloseWithLinkedRecv() - witness0 + r.staleClosed = e.metrics.handoffLoss.staleRecvDataClosed.Load() - stale0 + r.stale = tr.Stale() + for _, s := range r.stale { + if s.FD == r.fd && int(s.Res) == len(theftRequestB) && len(s.Head) > 0 && + string(s.Head) == theftRequestB[:len(s.Head)] { + r.stolen = true + } + } + return true +} + +// TestRecvTheft685Linked is the linked form's trial (see above). One trial +// per run; tally the --- lines of -count=N. +func TestRecvTheft685Linked(t *testing.T) { + requireRecvTheft715(t) + e, port := startLinkedTheftEngine(t) + sa := &unix.SockaddrInet4{Port: port, Addr: [4]byte{127, 0, 0, 1}} + var r linkedTheftResult + for r.attempts < theftMaxAttempts { + r.attempts++ + if linkedTheftAttempt(t, e, sa, &r) { + break + } + } + var heads []string + for _, s := range r.stale { + heads = append(heads, strconv.Quote(string(s.Head))) + } + errText := "" + if r.answerErr != nil { + errText = r.answerErr.Error() + } + t.Logf("RECVTHEFT685 linked result attempts=%d hit=%v fd=%d closer=%d sibling=%d b_fd=%d reused=%v held_open=%v released=%v filled=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d closer_accepts=%d}", + r.attempts, r.hit, r.fd, r.closer, r.sibl, r.bFD, r.reused, r.heldOpen, r.released, r.filled, r.witness, r.staleClosed, + r.stolen, r.answered, errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missCloserAccepts) + if !r.hit { + skipOrFail656(t, "INCONCLUSIVE: no sibling accept while the closer was parked, in %d attempts", r.attempts) + } + if r.witness == 0 { + t.Fatalf("the close hold fired but close_with_linked_recv did not move") + } + if r.stolen || !r.answered { + t.Fatalf("B (fd %d on worker %d; A's number %d reused=%v, kept open at the close=%v) lost its request: stolen by A's linked recv=%v (stale_recv_data_closed +%d), answered=%v (%v)", + r.bFD, r.sibl, r.fd, r.reused, r.heldOpen, r.stolen, r.staleClosed, r.answered, r.answerErr) + } + if !r.released { + t.Fatalf("A's number %d, kept open at the close, still names A's socket %v after the closer's release: the kept descriptor leaked", r.fd, theftReleaseWait) + } +} diff --git a/engine/iouring/recv_theft_715_linux_test.go b/engine/iouring/recv_theft_715_linux_test.go index 4d761e97..2214fdf0 100644 --- a/engine/iouring/recv_theft_715_linux_test.go +++ b/engine/iouring/recv_theft_715_linux_test.go @@ -207,11 +207,27 @@ func readHead(fd int, d time.Duration) (string, error) { } // theftResult is one trial of arm A or the control. +// +// A hit is the sibling worker accepting one of B's candidates while the +// closer is parked after its close. On a tree that frees the number at the +// close, that accept is given the number (it is the lowest free one), which +// is the theft's precondition: reused is true, and the sibling is parked too. +// On a tree that keeps the number allocated until the owed recv has ended +// (celeris#685), the accept is given another number: reused is false, and B +// is served with no hold at all. heldOpen and released read the closer's +// descriptor through /proc/self/fd: whether the number still named A's +// socket while the closer was parked, and whether it was released after +// (closed, or reused by something else) within theftReleaseWait of the +// closer's release, so a kept descriptor is not a leak. type theftResult struct { attempts int hit bool fd int closer, sibl int + bFD int + reused bool + heldOpen bool + released bool gate bool witness uint64 staleClosed uint64 @@ -226,12 +242,23 @@ type theftResult struct { candidatesHit int } +// fdTarget returns what /proc/self/fd/ names ("socket:[inode]" for a +// socket), or "" when fd is not open. +func fdTarget(fd int) string { + s, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(fd)) + if err != nil { + return "" + } + return s +} + const ( theftMaxAttempts = 10 theftCandidates = 6 theftAcceptWait = 300 * time.Millisecond theftSubmitWait = 300 * time.Millisecond theftAnswerWait = 2 * time.Second + theftReleaseWait = 2 * time.Second theftClosePath = "/close" theftRequestAHead = "GET " + theftClosePath + " HTTP/1.1\r\nHost: recv-theft-715\r\nConnection: close\r\n\r\n" theftRequestB = "GET /b HTTP/1.1\r\nHost: recv-theft-715\r\n\r\n" @@ -306,10 +333,17 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin return false } - // W1 is parked with N closed and A's recv unsubmitted. Dial B candidates - // until the sibling accepts one as N; one that hashes to W1's listener - // waits in W1's accept queue, one the sibling accepts as another number is - // served normally. + // W1 is parked right after its close path, with A's recv unsubmitted. + // Did that close release N? Read it before any candidate is dialed, while + // nothing else can take the number (every hole below it is filled). + aTarget := fdTarget(ce.FD) + r.heldOpen = strings.HasPrefix(aTarget, "socket:") + + // Dial B candidates until the sibling accepts one. One that hashes to + // W1's listener waits in W1's accept queue. The sibling's accept is given + // the lowest free number: N if the close released it, and then the + // sibling is parked too (its recv for B prepared, not submitted); another + // number if N is still A's, and then B is served with no hold. var hit *recvtheft.AcceptEvent var hitFD int for i, fd := range cands { @@ -324,35 +358,52 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin r.missQueued++ continue } - if ev.FD != ce.FD || ev.Worker == ce.Worker { + if ev.Worker == ce.Worker { r.missOtherFD++ - t.Logf("RECVTHEFT715 arm=%s attempt=%d candidate=%d accepted by worker %d as fd %d (target %d)", arm, r.attempts, i, ev.Worker, ev.FD, ce.FD) + t.Logf("RECVTHEFT715 arm=%s attempt=%d candidate=%d accepted by the closer %d as fd %d (target %d)", arm, r.attempts, i, ev.Worker, ev.FD, ce.FD) continue } - held, ok := tr.WaitAcceptHeld(time.Second) - if !ok { - t.Fatalf("accept of fd %d by worker %d reported but its hold did not fire", ev.FD, ev.Worker) + ev0 := ev + if ev.FD == ce.FD { + held, ok := tr.WaitAcceptHeld(time.Second) + if !ok { + t.Fatalf("accept of fd %d by worker %d reported but its hold did not fire", ev.FD, ev.Worker) + } + ev0 = held + r.reused = true } - hit, hitFD = &held, fd + hit, hitFD = &ev0, fd r.candidatesHit = i + 1 break } if hit == nil { - t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-sibling-accept-of-target fd=%d closer=%d queued=%d other_fd=%d", + t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-sibling-accept fd=%d closer=%d queued=%d closer_accepts=%d", arm, r.attempts, ce.FD, ce.Worker, r.missQueued, r.missOtherFD) return false } - r.hit, r.fd, r.closer, r.sibl, r.gate = true, ce.FD, ce.Worker, hit.Worker, tr.GateUsed() + r.hit, r.fd, r.closer, r.sibl, r.bFD, r.gate = true, ce.FD, ce.Worker, hit.Worker, hit.FD, tr.GateUsed() // Release W1: its next submit issues whatever it still holds for N. tr.ReleaseClose() for deadline := time.Now().Add(theftSubmitWait); time.Now().Before(deadline) && len(tr.Stale()) == 0; { time.Sleep(time.Millisecond) } - // Then the sibling: it submits B's own recv. + // Then the sibling, if parked: it submits B's own recv. tr.ReleaseAccept() r.answer, r.answerErr = readHead(hitFD, theftAnswerWait) r.answered = strings.HasPrefix(r.answer, "HTTP/1.1 200") + // A number kept open at the close must be released once A's recv has + // ended: closed, or by now naming something else. + for deadline := time.Now().Add(theftReleaseWait); ; { + if cur := fdTarget(ce.FD); !r.heldOpen || cur != aTarget { + r.released = true + break + } + if time.Now().After(deadline) { + break + } + time.Sleep(time.Millisecond) + } r.witness = recvtheft.CloseWithUnsubmittedRecv() - witness0 r.staleClosed = e.metrics.handoffLoss.staleRecvDataClosed.Load() - stale0 @@ -376,8 +427,8 @@ func logTheftResult(t *testing.T, arm string, r theftResult) { if r.answerErr != nil { errText = r.answerErr.Error() } - t.Logf("RECVTHEFT715 arm=%s result attempts=%d hit=%v fd=%d closer=%d sibling=%d gate=%v candidates=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d other_fd=%d}", - arm, r.attempts, r.hit, r.fd, r.closer, r.sibl, r.gate, r.candidatesHit, r.witness, r.staleClosed, r.stolen, r.answered, + t.Logf("RECVTHEFT715 arm=%s result attempts=%d hit=%v fd=%d closer=%d sibling=%d b_fd=%d reused=%v held_open=%v released=%v gate=%v candidates=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d closer_accepts=%d}", + arm, r.attempts, r.hit, r.fd, r.closer, r.sibl, r.bFD, r.reused, r.heldOpen, r.released, r.gate, r.candidatesHit, r.witness, r.staleClosed, r.stolen, r.answered, errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missOtherFD) } @@ -385,14 +436,17 @@ func judgeTheftTrial(t *testing.T, arm string, r theftResult) { t.Helper() logTheftResult(t, arm, r) if !r.hit { - skipOrFail656(t, "INCONCLUSIVE: no sibling accept of the released number in %d attempts", r.attempts) + skipOrFail656(t, "INCONCLUSIVE: no sibling accept while the closer was parked, in %d attempts", r.attempts) } if r.witness == 0 { t.Fatalf("the close hold fired but close_with_unsubmitted_recv did not move") } if r.stolen || !r.answered { - t.Fatalf("B (fd %d on worker %d) lost its request: stolen by the closed conn's recv=%v (stale_recv_data_closed +%d), answered=%v (%v)", - r.fd, r.sibl, r.stolen, r.staleClosed, r.answered, r.answerErr) + t.Fatalf("B (fd %d on worker %d; A's number %d reused=%v, kept open at the close=%v) lost its request: stolen by the closed conn's recv=%v (stale_recv_data_closed +%d), answered=%v (%v)", + r.bFD, r.sibl, r.fd, r.reused, r.heldOpen, r.stolen, r.staleClosed, r.answered, r.answerErr) + } + if !r.released { + t.Fatalf("A's number %d, kept open at the close, still names A's socket %v after the closer's release: the kept descriptor leaked", r.fd, theftReleaseWait) } } diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go index fc506db9..5e687d5a 100644 --- a/engine/iouring/worker.go +++ b/engine/iouring/worker.go @@ -838,6 +838,34 @@ func (w *Worker) recvUnsubmitted(cs *connState) bool { return cs.recvArmed && w.ring != nil && int32(cs.recvArmSeq.Get()-w.ring.sqConsumed()) >= 0 } +// recvLinkedOwed reports whether cs's armed recv is the one chained behind a +// SEND (flushSendLink) and has not completed: the kernel consumed its SQE with +// the SEND's, but issues it, and resolves its descriptor number, only after +// the SEND completes, as task work on a DEFER_TASKRUN ring, which can still +// be queued when the SEND's CQE is read (celeris#685). recvLinked is cleared +// only by that recv's own completion, so the pair is exact. +func recvLinkedOwed(cs *connState) bool { + return cs.recvArmed && cs.recvLinked +} + +// recvTheftWitness counts a close path's celeris#715 / celeris#685 +// preconditions (validation builds only; the caller is guarded by +// recvtheft.Enabled) and reports whether the close hold applies, and in +// which form: an unsubmitted recv (linked false) or a linked one still owed +// (linked true). The caller defers recvtheft.HoldAfterClose itself, so the +// hold runs when the close path returns. +func (w *Worker) recvTheftWitness(cs *connState) (hold, linked bool) { + switch { + case w.recvUnsubmitted(cs): + recvtheft.NoteCloseWithUnsubmittedRecv() + return true, false + case recvLinkedOwed(cs): + recvtheft.NoteCloseWithLinkedRecv() + return true, true + } + return false, false +} + // retireRecvCancel accounts for one of cs's outstanding backpressure-pause // ASYNC_CANCELs having resolved, whether by cancelling a recv (the recv's // -ECANCELED) or by cancelling nothing (the cancel's own completion). The @@ -4082,10 +4110,11 @@ func (w *Worker) finishClose(fd int) { // SIGSEGV / Green-Tea-GC span-corruption class. if cs != nil { // celeris#715 witness, hold and control, validation builds only: - // see recvUnsubmitted. - if recvtheft.Enabled && w.recvUnsubmitted(cs) { - recvtheft.NoteCloseWithUnsubmittedRecv() - defer recvtheft.HoldAfterClose(w.id, fd) + // see recvUnsubmitted and recvLinkedOwed. + if recvtheft.Enabled { + if hold, linked := w.recvTheftWitness(cs); hold { + defer recvtheft.HoldAfterClose(w.id, fd, linked) + } } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) @@ -4207,10 +4236,11 @@ func (w *Worker) finishCloseDetached(fd int, cs *connState) { // wall-clock hold sat below TCP's 200 ms RTO_MIN, so retransmitted // POST segments landed after release). // celeris#715 witness, hold and control, validation builds only: see - // recvUnsubmitted. - if recvtheft.Enabled && w.recvUnsubmitted(cs) { - recvtheft.NoteCloseWithUnsubmittedRecv() - defer recvtheft.HoldAfterClose(w.id, fd) + // recvUnsubmitted and recvLinkedOwed. + if recvtheft.Enabled { + if hold, linked := w.recvTheftWitness(cs); hold { + defer recvtheft.HoldAfterClose(w.id, fd, linked) + } } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) diff --git a/internal/recvtheft/recvtheft.go b/internal/recvtheft/recvtheft.go index 92130ff8..69bc78dd 100644 --- a/internal/recvtheft/recvtheft.go +++ b/internal/recvtheft/recvtheft.go @@ -36,6 +36,19 @@ func NoteCloseWithUnsubmittedRecv() { closeWithUnsubmittedRecv.Add(1) } // CloseWithUnsubmittedRecv returns the witness count since process start. func CloseWithUnsubmittedRecv() uint64 { return closeWithUnsubmittedRecv.Load() } +// closeWithLinkedRecv is the witness of the linked form (celeris#685): closes +// reached while the connection's recv was chained behind a SEND +// (IOSQE_IO_LINK) and had not completed. Such a recv was submitted, so the +// witness above does not count it, but the kernel issues it, and resolves +// its descriptor number, only after the SEND completes. +var closeWithLinkedRecv atomic.Uint64 + +// NoteCloseWithLinkedRecv counts one such close. Engine hook. +func NoteCloseWithLinkedRecv() { closeWithLinkedRecv.Add(1) } + +// CloseWithLinkedRecv returns the linked witness count since process start. +func CloseWithLinkedRecv() uint64 { return closeWithLinkedRecv.Load() } + // Options configures one [Trial]. type Options struct { // SubmitBeforeClose is the control arm: the close paths submit the SQ @@ -51,6 +64,10 @@ type Options struct { // HoldMax bounds each hold, so a test that stops driving the trial // cannot park a worker for longer than this. Zero means 5 s. HoldMax time.Duration + // LinkedRecv makes the close hold fire on a close with a LINKED recv + // owed (the celeris#685 linked form, [CloseWithLinkedRecv]) instead of + // one with an unsubmitted recv. + LinkedRecv bool } // CloseEvent is the close hold firing: worker Worker closed descriptor FD @@ -201,13 +218,15 @@ func AfterPromoteArm(queued func() bool) { } // HoldAfterClose is deferred by finishClose / finishCloseDetached when they -// found the connection's recv SQE unsubmitted, so it runs right after the -// descriptor is closed. The first time in a trial it records (worker, fd) as -// the trial's target and parks the worker until [Trial.ReleaseClose] or -// HoldMax. Engine hook, worker thread. -func HoldAfterClose(worker, fd int) { +// found the connection's recv SQE unsubmitted (linked false), or its recv +// linked behind a SEND and not completed (linked true), so it runs when the +// close path returns: right after the descriptor is closed, on a tree that +// closes it there. The first time in a trial whose [Options.LinkedRecv] +// matches linked, it records (worker, fd) as the trial's target and parks the +// worker until [Trial.ReleaseClose] or HoldMax. Engine hook, worker thread. +func HoldAfterClose(worker, fd int, linked bool) { t := current.Load() - if t == nil || !t.closeFired.CompareAndSwap(false, true) { + if t == nil || t.opts.LinkedRecv != linked || !t.closeFired.CompareAndSwap(false, true) { return } t.holder.Store(int64(worker)) diff --git a/internal/recvtheft/recvtheft_off.go b/internal/recvtheft/recvtheft_off.go index b006ad01..10a2d9fe 100644 --- a/internal/recvtheft/recvtheft_off.go +++ b/internal/recvtheft/recvtheft_off.go @@ -24,8 +24,14 @@ func CloseWithUnsubmittedRecv() uint64 { return 0 } // SubmitBeforeClose is always false in production. func SubmitBeforeClose() bool { return false } +// NoteCloseWithLinkedRecv is the production no-op. +func NoteCloseWithLinkedRecv() {} + +// CloseWithLinkedRecv is always 0 in production. +func CloseWithLinkedRecv() uint64 { return 0 } + // HoldAfterClose is the production no-op. -func HoldAfterClose(int, int) {} +func HoldAfterClose(int, int, bool) {} // AfterAccept is the production no-op. func AfterAccept(int, int) {} From eb71d8a3353205343a24bf5a0569820c86cbf182 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 03:26:26 +0200 Subject: [PATCH 05/12] test(iouring): hijack arms for the #685 rule: an op owed at Hijack reads the hijacker's first bytes hijackConn hands the socket over and queues the cancel of the connection's ops for the worker's next io_uring_enter. With single-shot recv nothing is owed there (the recv that brought the request has completed). With multishot recv (CELERIS_IOURING_MULTISHOT_RECV=1) the recv stays armed across its request, and its completion runs as task work: on a DEFER_TASKRUN ring only inside that enter, after the cancel, but on a ring without it at the worker thread's next return from any syscall, before the cancel is submitted. Three arms hold the worker right after the hijack (recvtheft.SetHijackHold, validation builds only) and send the hijacker's first bytes during the hold. On this tree (laptop container, arm64, kernel 7.0): single-shot 3/3 PASS with nothing owed, multishot on DEFER_TASKRUN 3/3 PASS with the recv owed, multishot on a COOP_TASKRUN ring 3/3 FAIL: the recv read the payload (stale_recv_data_closed +1) and the hijacker got nothing. The three join the `recv-theft` CI job. Refs #685 --- .github/workflows/ci.yml | 7 +- .../recv_theft_685_hijack_linux_test.go | 221 ++++++++++++++++++ engine/iouring/worker.go | 10 + internal/recvtheft/recvtheft.go | 42 ++++ internal/recvtheft/recvtheft_off.go | 9 + 5 files changed, 288 insertions(+), 1 deletion(-) create mode 100644 engine/iouring/recv_theft_685_hijack_linux_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index beda726a..91b84d1d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1007,6 +1007,11 @@ jobs: # it and the old recv reads B's request: both fail in every trial. # TestRecvTheft715Control (the ring submitted before the close) and # TestRecvTheft715ArmC (celeris#715 hypothesis (c)) must pass either way. + # The three TestRecvTheft685Hijack* arms hold the worker right after a + # Hijack and send the hijacker's first bytes meanwhile: with multishot + # recv (owed across its request) on a ring without DEFER_TASKRUN, the + # recv reads them unless hijackConn submits its cancel first (Coop); + # the DEFER_TASKRUN and single-shot arms pass either way. # # The trials need TWO io_uring workers (12 MiB of RLIMIT_MEMLOCK each; # the runner defaults to 8 MiB), so memlock is raised as in the @@ -1023,7 +1028,7 @@ jobs: set -o pipefail sudo prlimit --pid "$$" --memlock=unlimited:unlimited echo "memlock (KiB): $(ulimit -l)" - names='TestRecvTheft715ArmA|TestRecvTheft715Control|TestRecvTheft715ArmC|TestRecvTheft685Linked' + names='TestRecvTheft715ArmA|TestRecvTheft715Control|TestRecvTheft715ArmC|TestRecvTheft685Linked|TestRecvTheft685HijackSingleShot|TestRecvTheft685HijackMultishotDefer|TestRecvTheft685HijackMultishotCoop' runs=5 want=$(( $(tr '|' '\n' <<<"$names" | wc -l) * runs )) go test -tags=validation -race -count="$runs" -timeout=600s -v -run "^(${names})\$" \ diff --git a/engine/iouring/recv_theft_685_hijack_linux_test.go b/engine/iouring/recv_theft_685_hijack_linux_test.go new file mode 100644 index 00000000..482250e7 --- /dev/null +++ b/engine/iouring/recv_theft_685_hijack_linux_test.go @@ -0,0 +1,221 @@ +//go:build linux && validation + +package iouring + +import ( + "context" + "io" + "log/slog" + "net" + "strings" + "testing" + "time" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/internal/recvtheft" + "github.com/goceleris/celeris/protocol/h2/stream" + "github.com/goceleris/celeris/resource" +) + +// celeris#685, the hijack path. hijackConn hands the socket to the handler as +// a net.Conn and queues the cancel of any op the worker still has on it; the +// cancel reaches the kernel at the worker's next io_uring_enter. An op the +// kernel still owes the connection there can read the hijacker's first bytes +// first. Which op, and on which ring: +// +// - single-shot recv (the default): the recv that brought the request has +// completed before its handler runs, so nothing is owed at the hijack +// (TestRecvTheft685HijackSingleShot: the witness stays 0); +// - multishot recv (CELERIS_IOURING_MULTISHOT_RECV=1): the recv stays +// armed across its request, so it is owed. Its completion runs as task +// work. On a DEFER_TASKRUN ring that work runs only inside the enter, +// after the submit of the queued cancel, so the cancel wins +// (TestRecvTheft685HijackMultishotDefer). On a ring without it (kernels +// before 6.1, or wherever the probe finds DEFER_TASKRUN unusable) it runs +// at the worker thread's next return from any syscall, before that enter +// (TestRecvTheft685HijackMultishotCoop, a COOP_TASKRUN ring as the high +// tier builds without DEFER_TASKRUN). +// +// Each test hijacks one connection with the worker held right after the +// hijack (recvtheft.SetHijackHold), has the client send a payload during the +// hold, and asserts that the hijacker reads it and that no stale recv read +// it. Needs CELERIS_RECV_THEFT_715=1 and an io_uring worker (one is enough). + +const ( + hijack685Hold = 300 * time.Millisecond + hijack685Payload = "PING-685-hijacker-first-bytes\n" +) + +type hijack685Handler struct{ conns chan net.Conn } + +func (h hijack685Handler) HandleStream(_ context.Context, s *stream.Stream) error { + if s.ResponseWriter == nil { + return nil + } + if s.Path != "/hijack" { + return s.ResponseWriter.WriteResponse(s, 200, + [][2]string{{"content-type", "text/plain"}, {"content-length", "2"}}, []byte("ok")) + } + hj, ok := s.ResponseWriter.(stream.Hijacker) + if !ok { + h.conns <- nil + return nil + } + c, err := hj.Hijack(s) + if err != nil { + h.conns <- nil + return nil + } + h.conns <- c + return nil +} + +// hijack685Tier is how a test arm sets the engine's tier up before Listen. +type hijack685Tier int + +const ( + hijackTierDefault hijack685Tier = iota // as probed; single-shot recv + hijackTierMultiDefer // multishot recv, DEFER_TASKRUN ring + hijackTierMultiCoop // multishot recv, COOP_TASKRUN ring +) + +func runHijack685(t *testing.T, tier hijack685Tier) { + t.Helper() + requireRecvTheft715(t) + if tier != hijackTierDefault { + t.Setenv("CELERIS_IOURING_MULTISHOT_RECV", "1") + } + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("pick port: %v", err) + } + addr := ln.Addr().String() + _ = ln.Close() + h := hijack685Handler{conns: make(chan net.Conn, 1)} + e, err := New(resource.Config{ + Addr: addr, + Protocol: engine.HTTP1, + Resources: resource.Resources{Workers: 2}, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }, h) + if err != nil { + skipOrFail656(t, "iouring engine unavailable: %v", err) + } + if tier != hijackTierDefault { + ht, ok := e.tier.(*highTier) + if !ok || !ht.multishotRecv { + skipOrFail656(t, "celeris#685 hijack arm needs the high tier with multishot recv; have %T", e.tier) + } + if tier == hijackTierMultiDefer && !ht.deferTaskrun { + skipOrFail656(t, "celeris#685 hijack arm needs DEFER_TASKRUN") + } + cp := *ht + cp.deferTaskrun = tier == hijackTierMultiDefer + cp.fixedFiles = false + e.tier = &cp + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- e.Listen(ctx) }() + defer func() { + cancel() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Error("engine did not stop within 5s") + } + }() + for deadline := time.Now().Add(8 * time.Second); e.Addr() == nil; { + select { + case err := <-done: + skipOrFail656(t, "iouring engine failed to start: %v", err) + default: + } + if time.Now().After(deadline) { + t.Fatal("engine did not start listening within 8s") + } + time.Sleep(10 * time.Millisecond) + } + var multishot, deferTR bool + for _, w := range e.workers { + multishot = w.bufRing != nil + deferTR = w.tier.SetupFlags()&setupDeferTaskrun != 0 + } + if (tier != hijackTierDefault) != multishot { + skipOrFail656(t, "celeris#685 hijack arm: multishot recv is %v on the worker, want %v", multishot, tier != hijackTierDefault) + } + + recvtheft.SetHijackHold(hijack685Hold) + defer recvtheft.SetHijackHold(0) + holds0 := recvtheft.HijackHolds() + owed0 := recvtheft.HijackWithOpOwed() + stale0 := e.metrics.handoffLoss.staleRecvDataClosed.Load() + + c, err := net.DialTimeout("tcp", addr, time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer func() { _ = c.Close() }() + if _, err := c.Write([]byte("GET /hijack HTTP/1.1\r\nHost: recv-theft-685\r\n\r\n")); err != nil { + t.Fatalf("write request: %v", err) + } + // The payload goes out while the worker is held after the hijack, before + // its next enter. + for deadline := time.Now().Add(2 * time.Second); recvtheft.HijackHolds() == holds0; { + if time.Now().After(deadline) { + t.Fatal("the hijack hold never ran: the request did not reach Hijack") + } + time.Sleep(time.Millisecond) + } + time.Sleep(20 * time.Millisecond) + if _, err := c.Write([]byte(hijack685Payload)); err != nil { + t.Fatalf("write payload: %v", err) + } + var hc net.Conn + select { + case hc = <-h.conns: + case <-time.After(3 * time.Second): + t.Fatal("the handler never returned from Hijack") + } + if hc == nil { + t.Fatal("Hijack failed") + } + defer func() { _ = hc.Close() }() + _ = hc.SetReadDeadline(time.Now().Add(2 * time.Second)) + buf := make([]byte, 256) + var got []byte + for !strings.Contains(string(got), hijack685Payload) { + n, err := hc.Read(buf) + got = append(got, buf[:n]...) + if err != nil { + break + } + } + time.Sleep(50 * time.Millisecond) + owed := recvtheft.HijackWithOpOwed() - owed0 + stale := e.metrics.handoffLoss.staleRecvDataClosed.Load() - stale0 + read := strings.Contains(string(got), hijack685Payload) + t.Logf("RECVTHEFT685 hijack result tier=%d multishot=%v defer_taskrun=%v op_owed=+%d stale_recv_data_closed=+%d hijacker_read=%v got=%q", + tier, multishot, deferTR, owed, stale, read, got) + if tier == hijackTierDefault && owed != 0 { + t.Errorf("a single-shot hijack found an op owed (+%d): the recv that brought the request should have completed", owed) + } + if tier != hijackTierDefault && owed != 1 { + t.Errorf("the multishot hijack found no op owed (+%d): the arm did not reach its precondition", owed) + } + if !read || stale != 0 { + t.Fatalf("the hijacker lost its first bytes: read=%v (got %q), stale recv data on the closed identity +%d", read, got, stale) + } +} + +// TestRecvTheft685HijackSingleShot: nothing is owed at a single-shot hijack. +func TestRecvTheft685HijackSingleShot(t *testing.T) { runHijack685(t, hijackTierDefault) } + +// TestRecvTheft685HijackMultishotDefer: owed, and the queued cancel still +// wins, because the recv's completion waits for the enter. +func TestRecvTheft685HijackMultishotDefer(t *testing.T) { runHijack685(t, hijackTierMultiDefer) } + +// TestRecvTheft685HijackMultishotCoop: owed, and without DEFER_TASKRUN the +// recv's completion runs at the worker's next syscall return, before the +// enter that would submit the cancel queued by hijackConn. +func TestRecvTheft685HijackMultishotCoop(t *testing.T) { runHijack685(t, hijackTierMultiCoop) } diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go index 5e687d5a..12942201 100644 --- a/engine/iouring/worker.go +++ b/engine/iouring/worker.go @@ -2320,6 +2320,16 @@ func (w *Worker) hijackConn(fd int) (net.Conn, error) { // STEAL the first bytes the hijacker tries to read. Cancel it by its // generation-tagged user_data and defer the pool release until the // terminal CQE arrives, exactly like finishClose. + // celeris#685 hijack witness and hold, validation builds only: count a + // hijack with an op still owed on the socket (kernelInflight > 0), and + // hold the worker thread before it returns, and so before its next + // io_uring_enter (recvtheft.SetHijackHold). + if recvtheft.Enabled { + if cs.kernelInflight > 0 { + recvtheft.NoteHijackWithOpOwed() + } + defer recvtheft.HijackHold() + } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) w.queuePendingRelease(cs) diff --git a/internal/recvtheft/recvtheft.go b/internal/recvtheft/recvtheft.go index 69bc78dd..aa00f6fe 100644 --- a/internal/recvtheft/recvtheft.go +++ b/internal/recvtheft/recvtheft.go @@ -311,3 +311,45 @@ func WakeHold() { time.Sleep(time.Duration(d)) } } + +// hijackWithOpOwed is the hijack witness (celeris#685): hijacks that handed +// the socket over while the kernel still owed the connection an op on it (a +// multishot recv stays armed across its request; a single-shot recv has +// completed by the time its request's handler hijacks). +var hijackWithOpOwed atomic.Uint64 + +// NoteHijackWithOpOwed counts one such hijack. Engine hook. +func NoteHijackWithOpOwed() { hijackWithOpOwed.Add(1) } + +// HijackWithOpOwed returns the hijack witness count since process start. +func HijackWithOpOwed() uint64 { return hijackWithOpOwed.Load() } + +// hijackHoldNanos is the hijack hold. See [SetHijackHold]. +var ( + hijackHoldNanos atomic.Int64 + hijackHolds atomic.Uint64 +) + +// SetHijackHold makes hijackConn sleep for d on the worker thread just before +// it returns the hijacked connection, that is before the worker's next +// io_uring_enter; d <= 0 turns it off. It widens the window in which an op +// the kernel still owes the connection can read the hijacker's first bytes +// (celeris#685). +func SetHijackHold(d time.Duration) { + if d < 0 { + d = 0 + } + hijackHoldNanos.Store(int64(d)) +} + +// HijackHolds returns how many times [HijackHold] slept. +func HijackHolds() uint64 { return hijackHolds.Load() } + +// HijackHold sleeps for the duration set by [SetHijackHold], if any. Engine +// hook, worker thread. +func HijackHold() { + if d := hijackHoldNanos.Load(); d > 0 { + hijackHolds.Add(1) + time.Sleep(time.Duration(d)) + } +} diff --git a/internal/recvtheft/recvtheft_off.go b/internal/recvtheft/recvtheft_off.go index 10a2d9fe..9e3a01d6 100644 --- a/internal/recvtheft/recvtheft_off.go +++ b/internal/recvtheft/recvtheft_off.go @@ -44,3 +44,12 @@ func NoteStaleRecvData(int, int, uint32, int32, []byte) {} // WakeHold is the production no-op. func WakeHold() {} + +// NoteHijackWithOpOwed is the production no-op. +func NoteHijackWithOpOwed() {} + +// HijackWithOpOwed is always 0 in production. +func HijackWithOpOwed() uint64 { return 0 } + +// HijackHold is the production no-op. +func HijackHold() {} From 150d8cd4ff0aba54b46cc450b4f4c546eabfcfae Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 03:35:56 +0200 Subject: [PATCH 06/12] fix(iouring): never release a descriptor number while an op can still resolve it: close paths, hijack, shutdown (celeris#685) Fixed files are off, so a recv or send SQE names its descriptor by number and the kernel resolves the number when it issues the op. finishClose and finishCloseDetached queued the ops' cancels and closed the descriptor at once, while an op could still be issued: a recv still in the SQ ring (a promoted connection's re-arm), or one linked behind a SEND that the kernel issues only after the SEND completes. A sibling worker given the freed number before this worker's next enter then had its new connection's request read by the old recv, dropped as stale_recv_data_closed, and the client was never answered (celeris#715: 80 of 80 in #781's trial). The rule, as #681 applied it to the hand-off: a close path does not release the number while the kernel owes an op on it (fdOwed: kernelInflight > 0, which counts every recv and send from the moment its SQE is written). It does everything else as before and hands the descriptor to its pendingRelease entry; drainPendingRelease closes it where it releases the connState, at the last owed op's terminal CQE. The socket's read side is shut down too (SHUT_RD on the H1 fast path, SHUT_RDWR where the path already half-closed), so an owed recv ends as soon as it is issued, even a linked one no cancel can find, and even where cancels fail (#682). No io_uring_enter is added; the close(2) moves one iteration later. The worker does not park while such a close is outstanding. hijackConn keeps the socket open under the hijacker, so it submits its cancels before handing the socket over when an op is owed (a multishot recv stays armed across its request). Worker shutdown ends the ops owed on connection descriptors (cancel, SHUT_RDWR, run the ring until their terminal CQEs, bounded at 250 ms) before it closes any, and before shutdownDrivers, which relies on nothing being submitted after it. EngineMetrics gains CloseFDDeferred (a rate) and CloseFDForced (the backstop closing a descriptor with an op still owed; must stay 0). The recv-theft CI job gets a detector control: with fdOwed forced false the three judging trials must fail every run. Fixes #685 Refs #715 --- .github/scripts/mutant-685-release-owed-fd.py | 45 +++ .github/workflows/ci.yml | 39 ++ adaptive/engine.go | 5 + adaptive/handoff_loss_metrics_test.go | 4 + engine/engine.go | 39 +- engine/iouring/engine.go | 2 + engine/iouring/fd_lifetime.go | 201 ++++++++++ engine/iouring/fd_lifetime_close_test.go | 343 ++++++++++++++++++ engine/iouring/handoff_loss.go | 28 +- engine/iouring/handoff_loss_metrics_test.go | 4 + .../recv_theft_685_linked_linux_test.go | 24 -- engine/iouring/recv_theft_715_linux_test.go | 10 - engine/iouring/worker.go | 197 ++++++++-- internal/recvtheft/doc.go | 40 +- 14 files changed, 882 insertions(+), 99 deletions(-) create mode 100755 .github/scripts/mutant-685-release-owed-fd.py create mode 100644 engine/iouring/fd_lifetime_close_test.go diff --git a/.github/scripts/mutant-685-release-owed-fd.py b/.github/scripts/mutant-685-release-owed-fd.py new file mode 100755 index 00000000..78ea2c56 --- /dev/null +++ b/.github/scripts/mutant-685-release-owed-fd.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python3 +"""celeris#685 detector control: the MUTANT, applied in CI and never committed. + +Makes fdOwed (engine/iouring/fd_lifetime.go) report false for every +connection, which takes the fd-lifetime rule off every path it guards at +once: the close paths close the descriptor at once again (no kept number, +no read-side shutdown), hijackConn no longer submits its cancels before +handing the socket over, and worker shutdown no longer ends the owed ops +before closing descriptors. + +Against the mutated tree the three trials that judge the rule MUST fail in +every run: TestRecvTheft715ArmA (a recv still in the SQ ring at the close), +TestRecvTheft685Linked (a recv linked behind a SEND, not issued at the +close) and TestRecvTheft685HijackMultishotCoop (a multishot recv owed at a +Hijack on a ring without DEFER_TASKRUN). If one passes, it is not watching +the path it claims to judge, and its green run on the tree as committed +proves nothing. + +The function is matched EXACTLY; any drift makes this script exit 2 instead +of silently mutating nothing. +""" +import sys + +PATH = sys.argv[1] if len(sys.argv) > 1 else "engine/iouring/fd_lifetime.go" + +BODY = ( + "func fdOwed(cs *connState) bool {\n" + "\treturn cs != nil && cs.kernelInflight > 0 && !cs.fixedFile\n" + "}\n" +) + +src = open(PATH).read() +if src.count(BODY) != 1: + print(f"mutant-685: expected exactly one fdOwed body, found {src.count(BODY)}", file=sys.stderr) + sys.exit(2) +mutated = src.replace( + BODY, + "func fdOwed(cs *connState) bool {\n" + "\t// MUTANT celeris#685: the fd-lifetime rule is off everywhere\n" + "\t_ = cs\n" + "\treturn false\n" + "}\n", +) +open(PATH, "w").write(mutated) +print(f"mutant-685: fdOwed always false ({PATH})") diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91b84d1d..458bfd3c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1046,6 +1046,45 @@ jobs: echo "release a descriptor number a recv could still resolve, or was a test renamed or skipped?" exit 1 fi + # The detector control. .github/scripts/mutant-685-release-owed-fd.py + # makes fdOwed report false, which takes the fd-lifetime rule off the + # close paths, Hijack and shutdown at once. Against that tree the three + # trials that judge the rule must FAIL in every one of their runs (a + # PASS means the trial does not watch the path it judges), and the + # others must still PASS. The mutation is undone before the step ends. + - name: celeris#685 detector control (the rule removed; the three judging trials must fail every run) + if: ${{ !cancelled() }} + shell: bash + env: + CELERIS_REQUIRE_IOURING_WORKERS: "1" + CELERIS_RECV_THEFT_715: "1" + run: | + set -o pipefail + sudo prlimit --pid "$$" --memlock=unlimited:unlimited + python3 .github/scripts/mutant-685-release-owed-fd.py + judged='TestRecvTheft715ArmA|TestRecvTheft685Linked|TestRecvTheft685HijackMultishotCoop' + others='TestRecvTheft715Control|TestRecvTheft685HijackSingleShot|TestRecvTheft685HijackMultishotDefer' + runs=3 + go test -tags=validation -race -count="$runs" -timeout=600s -v -run "^(${judged}|${others})\$" \ + ./engine/iouring/ 2>&1 | tee /tmp/recvtheft685-mutant.log || true + git checkout -- engine/iouring/fd_lifetime.go + grep -E 'RECVTHEFT(715|685) .*result' /tmp/recvtheft685-mutant.log || true + bad=0 + for n in ${judged//|/ }; do + p=$(grep -cE "^--- PASS: $n \(" /tmp/recvtheft685-mutant.log || true) + f=$(grep -cE "^--- FAIL: $n \(" /tmp/recvtheft685-mutant.log || true) + echo "mutant, judged $n: PASS $p FAIL $f (want FAIL $runs)" + if [ "$f" -ne "$runs" ] || [ "$p" -ne 0 ]; then bad=1; fi + done + for n in ${others//|/ }; do + p=$(grep -cE "^--- PASS: $n \(" /tmp/recvtheft685-mutant.log || true) + f=$(grep -cE "^--- FAIL: $n \(" /tmp/recvtheft685-mutant.log || true) + echo "mutant, control $n: PASS $p FAIL $f (want PASS $runs)" + if [ "$p" -ne "$runs" ] || [ "$f" -ne 0 ]; then bad=1; fi + done + skipped=$(grep -cE '^[[:space:]]*--- SKIP' /tmp/recvtheft685-mutant.log || true) + if [ "$skipped" -ne 0 ]; then echo "a SKIP is not a result ($skipped SKIP lines)"; bad=1; fi + exit "$bad" conformance: name: Conformance diff --git a/adaptive/engine.go b/adaptive/engine.go index df3a9f9f..86900f5e 100644 --- a/adaptive/engine.go +++ b/adaptive/engine.go @@ -1153,6 +1153,11 @@ func (e *Engine) Metrics() engine.EngineMetrics { TransplantClaimDeferred: pm.TransplantClaimDeferred + sm.TransplantClaimDeferred, TransplantReapFailed: pm.TransplantReapFailed + sm.TransplantReapFailed, TransplantReapUnsupported: pm.TransplantReapUnsupported + sm.TransplantReapUnsupported, + // The same rule on the close paths (celeris#685): each close is an + // event on the one sub-engine that owned the connection, and a + // close of the standby's residue lands on the standby. + CloseFDDeferred: pm.CloseFDDeferred + sm.CloseFDDeferred, + CloseFDForced: pm.CloseFDForced + sm.CloseFDForced, // The post-switch sweep (celeris#657 PR-3). Both sub-engines sweep, // in opposite directions, and only the one draining runs passes at // all, so the pass count sums as a rate. The residual entries are diff --git a/adaptive/handoff_loss_metrics_test.go b/adaptive/handoff_loss_metrics_test.go index d5f2dc7a..1b256121 100644 --- a/adaptive/handoff_loss_metrics_test.go +++ b/adaptive/handoff_loss_metrics_test.go @@ -25,6 +25,7 @@ func TestMetricsSumsTheHandoffLossWitnesses(t *testing.T) { TransplantHeld: 5, TransplantReaps: 6, TransplantReapMisses: 7, TransplantHoldRescued: 8, TransplantDoubleClaim: 9, TransplantClaimDeferred: 12, TransplantReapFailed: 13, TransplantReapUnsupported: 14, + CloseFDDeferred: 15, CloseFDForced: 16, }) e.secondary.(*mockEngine).SetMetrics(engine.EngineMetrics{ StaleRecvDataClosed: 10, StaleRecvDataTransplanted: 20, @@ -32,6 +33,7 @@ func TestMetricsSumsTheHandoffLossWitnesses(t *testing.T) { TransplantHeld: 50, TransplantReaps: 60, TransplantReapMisses: 70, TransplantHoldRescued: 80, TransplantDoubleClaim: 90, TransplantClaimDeferred: 120, TransplantReapFailed: 130, TransplantReapUnsupported: 140, + CloseFDDeferred: 150, CloseFDForced: 160, }) m := e.Metrics() @@ -51,6 +53,8 @@ func TestMetricsSumsTheHandoffLossWitnesses(t *testing.T) { {"TransplantClaimDeferred", m.TransplantClaimDeferred, 132}, {"TransplantReapFailed", m.TransplantReapFailed, 143}, {"TransplantReapUnsupported", m.TransplantReapUnsupported, 154}, + {"CloseFDDeferred", m.CloseFDDeferred, 165}, + {"CloseFDForced", m.CloseFDForced, 176}, } { if c.got != c.want { t.Errorf("Metrics().%s = %d, want %d (sum of both sub-engines)", diff --git a/engine/engine.go b/engine/engine.go index f84646e9..166066c5 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -469,15 +469,15 @@ type EngineMetrics struct { //nolint:revive // user-approved name // - Closed: a connection this engine closed or hijacked (the close // paths and Hijack register the identity the same way). Usually // the client's bytes raced a server-side close, and that client - // sees its connection end. It is not always benign, so a non-zero - // Closed does not prove that no live client lost a request. After - // a Hijack the socket lives on under the hijacker's net.Conn, so a - // recv that completes with data before its cancel lands has taken - // the first bytes of a connection its client is still using. And a - // recv that had not reached the kernel when the descriptor was - // closed resolves the descriptor NUMBER when it does; if a new - // connection holds that number by then, the recv reads that - // client's request. + // sees its connection end. It was not always benign: after a + // Hijack, a recv that completed with data before its cancel landed + // took the first bytes of a connection its client was still using, + // and a recv that had not been issued when the descriptor was + // closed resolved the descriptor NUMBER when it was, reading the + // request of whatever new connection held that number by then + // (celeris#715). Since celeris#685 neither can happen (see + // CloseFDDeferred), so what Closed counts is a closed connection's + // recv reading its own client's late bytes. // - Transplanted: a connection this engine handed to the other // sub-engine. A recv armed before the hand-off outlived it and // read a request meant for the new owner — or, through a reused @@ -580,6 +580,27 @@ type EngineMetrics struct { //nolint:revive // user-approved name // probe finds the flags. io_uring-only; on the adaptive engine the sum // over both sub-engines. TransplantReapUnsupported uint64 + // CloseFDDeferred and CloseFDForced count how the io_uring close paths + // keep the same fd-lifetime rule (celeris#685): a connection's descriptor + // NUMBER is released only when no operation that names it can still be + // issued. Closing it earlier let a receive the kernel had not issued yet + // read the request of a new connection that another thread had been + // given the freed number for; that request was dropped as + // StaleRecvDataClosed, and its client was never answered. + // + // - CloseFDDeferred: closes whose descriptor stayed open until the + // last operation the kernel owed on it had completed, and was closed + // then (normally one loop iteration later). A rate: on an + // async-handler engine it is close to one per connection the server + // closes, and on a sync-mode engine it is the server-side closes of + // connections with a receive armed (timeouts). + // - CloseFDForced: such descriptors closed by the release backstop with + // an operation still owed. Must stay 0. + // + // io_uring-only and cumulative; zero on other engines. On the adaptive + // engine each is the sum over both sub-engines. + CloseFDDeferred uint64 + CloseFDForced uint64 // TransplantSweepPasses counts passes of the post-switch sweep, the // re-examination that moves a connection the drain would otherwise // reach only at that connection's own next event — which, for a diff --git a/engine/iouring/engine.go b/engine/iouring/engine.go index ef1485b2..1cdae58d 100644 --- a/engine/iouring/engine.go +++ b/engine/iouring/engine.go @@ -525,6 +525,8 @@ func (e *Engine) Metrics() engine.EngineMetrics { TransplantClaimDeferred: e.metrics.handoffLoss.claimDeferred.Load(), TransplantReapFailed: e.metrics.handoffLoss.reapFailed.Load(), TransplantReapUnsupported: e.metrics.handoffLoss.reapUnsupported.Load(), + CloseFDDeferred: e.metrics.handoffLoss.closeFDDeferred.Load(), + CloseFDForced: e.metrics.handoffLoss.closeFDForced.Load(), TransplantSweepPasses: e.metrics.sweep.passes.Load(), TransplantResidualDetached: e.metrics.sweep.residual[resDetached].Load(), diff --git a/engine/iouring/fd_lifetime.go b/engine/iouring/fd_lifetime.go index 9d0c7755..dc1200c1 100644 --- a/engine/iouring/fd_lifetime.go +++ b/engine/iouring/fd_lifetime.go @@ -3,6 +3,8 @@ package iouring import ( + "time" + "golang.org/x/sys/unix" "github.com/goceleris/celeris/engine" @@ -343,3 +345,202 @@ func (w *Worker) releaseHoldSlow(cs *connState, rescued bool) { func (w *Worker) rescueHold(cs *connState) { w.releaseHoldSlow(cs, true) } + +// The same rule for the close paths (celeris#685): a descriptor NUMBER is +// released only when no op that names it can still be submitted or issued. +// +// Fixed files are off (celeris#541), so a recv or send SQE names the +// descriptor by number, and the kernel resolves the number when it ISSUES the +// op, not when the SQE is written. Two kinds of op are not issued yet when a +// close path runs: +// +// - one still in the SQ ring: prepareRecv (a promoted connection's re-arm, +// a dirty-list retry) and flushSend only place SQEs, and they reach the +// kernel at the loop's next io_uring_enter; +// - one the kernel holds but has not issued: a recv linked behind a SEND +// (flushSendLink) is issued only when the SEND completes, and on a +// DEFER_TASKRUN ring as task work after that, which can still be queued +// when the SEND's CQE is read. +// +// finishClose and finishCloseDetached used to queue the ops' cancels and +// close(2) the descriptor at once. Another thread (a sibling worker's accept, +// the epoll sub-engine) can be given the freed number before this worker's +// next submit. The recv then reads that connection's request and completes +// under the closed connection's (fd, generation); staleConnCQE drops it as +// stale_recv_data_closed, and the new connection waits on an empty socket +// until its header deadline. Measured deterministically by +// TestRecvTheft715ArmA (celeris#715), and the linked form by +// TestRecvTheft685Linked. +// +// The rule is kept by NOT closing while the kernel owes an op on the +// descriptor (fdOwed). The close path does everything else as before (the +// cancels, the closedOps registration, the deferred release) and hands the +// descriptor to its pendingRelease entry, and drainPendingRelease closes it +// where it releases the connState: when every owed op has delivered its +// terminal CQE. Until then the number stays allocated, so no accept or dup +// anywhere in the process can be given it, and an op issued late resolves +// this connection's own socket. To make sure every owed op does end, the +// close path also shuts the socket's read side down (shutdownHow; SHUT_RD +// alone on the fast path, which otherwise makes no shutdown call, see +// fastCloseShutdownHow): an owed recv returns at once when it is issued, even +// one no cancel can find (a linked recv not issued yet), and even on a kernel +// whose cancels fail (celeris#682). +// +// What it costs. No io_uring_enter is added: the close(2) moves from the +// close path to the release, one iteration later, and a path that already +// shut the write side down shuts both down instead. The one added syscall is +// the SHUT_RD of the H1 fast path, taken only when an op is owed there: a +// server-side close of a connection with its recv armed (a timeout); a +// sync-mode Connection: close response has no recv armed when it closes, and +// neither has a client's FIN. What the peer sees barely moves. An issued recv +// holds its own reference to the file, so its socket was never released +// before that recv's cancel landed anyway, and the FIN (or the RST of unread +// data) went out then; it goes out at the same point now. Only a close with +// a recv still in the SQ ring, whose socket nothing held, used to release the +// socket at the close and now does so one enter later. The worker does not +// park while such a close is outstanding (closeFDOwed). +// +// A hijack keeps the socket open under the hijacker, so it cannot wait: it +// submits its cancels before handing the socket over (see hijackConn). Worker +// shutdown ends the ops owed on every descriptor before it closes any +// (endOwedOpsAtShutdown). + +// fdOwed reports whether the kernel still owes cs an op that names its +// descriptor: a recv or send whose SQE was written and whose terminal CQE has +// not been read. kernelInflight counts every such op from the moment its SQE +// is placed (prepareRecv, flushSend, flushSendLink, both halves of a linked +// pair), submitted or not, issued or not, and staleConnCQE retires it at the +// terminal CQE; the header timer and the cancels name no descriptor and are +// not counted. A fixed-file connection names a slot, not a number, and keeps +// its own close (CLOSE_DIRECT). A nil cs (closeMissingConnState) owes +// nothing we can know of. +func fdOwed(cs *connState) bool { + return cs != nil && cs.kernelInflight > 0 && !cs.fixedFile +} + +// keptFD is the descriptor a close path hands to its pendingRelease entry: +// fd when an op is owed, else -1 (the close path closes it at once). +func keptFD(fd int, owed bool) int { + if owed { + return fd + } + return -1 +} + +// closeUnlessOwed is a close path's last step: close(2) now when nothing is +// owed; otherwise leave the descriptor to its pendingRelease entry, which +// drainPendingRelease closes at the last owed op's terminal CQE. +func closeUnlessOwed(fd int, owed bool) { + if !owed { + _ = unix.Close(fd) + } +} + +// shutdownHow is the shutdown(2) of a close path that half-closes: SHUT_WR +// (the FIN goes out now) as before, and the read side too while an op is +// owed, so the owed recv ends as soon as the kernel issues it. +func shutdownHow(owed bool) int { + if owed { + return unix.SHUT_RDWR + } + return unix.SHUT_WR +} + +// fastCloseShutdownHow is the shutdown(2) the H1 fast path adds when an op is +// owed: SHUT_RD, which sends nothing, so the close(2) at the release still +// decides between FIN and RST as the fast path always did. With a SEND still +// owed (the closing-drain sweep reaps a conn whose SEND never completed) the +// write side goes too: a blocked SEND then fails at once, where a cancel +// alone cannot end it on a kernel whose cancels fail (celeris#682). +func fastCloseShutdownHow(cs *connState) int { + if cs.sending || cs.zcNotifPending { + return unix.SHUT_RDWR + } + return unix.SHUT_RD +} + +// shutdownFDDrainNanos bounds how long worker shutdown waits for the ops still +// owed on connection descriptors (endOwedOpsAtShutdown). The cancels and the +// SHUT_RDWR end every one within the first enter or two on a healthy kernel; +// the bound is for a kernel that never answers, and matches the send drain +// before it (shutdownSendDrainNanos). +const shutdownFDDrainNanos int64 = int64(250 * time.Millisecond) + +// endOwedOpsAtShutdown is worker shutdown's half of the rule: it ends every op +// the kernel still owes on a live connection's descriptor, and on a +// descriptor a close path left to its pendingRelease entry (holdsFD), +// and closes the latter. shutdown closes the live ones itself, after it. +// +// Without it, shutdown closed every descriptor and then the ring. An op whose +// SQE was still in the SQ ring was harmless there (nothing submits after the +// closes), and an issued op holds its own file. The exception is an op the +// kernel had consumed and not issued: a recv linked behind a SEND, or queued +// as task work when the SEND's CQE was read. Where the ring's teardown still +// issues such work, it resolves a number shutdown had already freed. So each +// live connection with an op owed (fdOwed) has its ops cancelled and its +// socket shut down both ways (a consumed recv then ends as soon as it is +// issued, a blocked SEND at once), and the ring is run until every such op +// has delivered its terminal CQE: recv and send completions are retired by +// staleConnCQE exactly as the loop retires them, and an accept's new +// descriptor, which nobody will serve, is closed. Everything the SQ ring still +// holds is submitted by the first enter, which is why this runs before +// shutdownDrivers closes the driver descriptors. Bounded by +// shutdownFDDrainNanos; past it (or on a ring error) the descriptors are +// closed anyway, as shutdown always did. Worker thread only; skipped under +// SQPOLL (no tier enables it) and without a ring. +func (w *Worker) endOwedOpsAtShutdown() { + var owed []*connState + for _, fd := range w.liveConns { + cs := w.conns[fd] + if cs == nil || !fdOwed(cs) { + continue + } + owed = append(owed, cs) + } + pending := func() bool { + for _, cs := range owed { + if cs.kernelInflight > 0 { + return true + } + } + for i := range w.pendingRelease { + if e := &w.pendingRelease[i]; e.holdsFD && e.cs.kernelInflight > 0 { + return true + } + } + return false + } + if w.ring != nil && !w.sqpoll && pending() { + for _, cs := range owed { + w.cancelConnOps(cs.fd, cs) + _ = unix.Shutdown(cs.fd, unix.SHUT_RDWR) + } + deadline := time.Now().UnixNano() + shutdownFDDrainNanos + for pending() && time.Now().UnixNano() < deadline { + if err := w.ring.SubmitAndWaitTimeout(10 * time.Millisecond); err != nil { + break + } + head, tail := w.ring.BeginCQ() + for ; head != tail; head++ { + c := w.ring.cqeAt(head) + ud := c.UserData + switch ud & udMask { + case udRecv, udSend: + w.staleConnCQE(c, int(ud&fdMask), ud) + case udAccept: + if c.Res >= 0 && !w.fixedFiles { + _ = unix.Close(int(c.Res)) + } + } + } + w.ring.EndCQ(head) + } + } + for i := range w.pendingRelease { + if e := &w.pendingRelease[i]; e.holdsFD { + _ = unix.Close(int(e.fd)) + e.holdsFD = false + } + } + w.closeFDOwed = 0 +} diff --git a/engine/iouring/fd_lifetime_close_test.go b/engine/iouring/fd_lifetime_close_test.go new file mode 100644 index 00000000..b42d7599 --- /dev/null +++ b/engine/iouring/fd_lifetime_close_test.go @@ -0,0 +1,343 @@ +//go:build linux + +package iouring + +import ( + "context" + "errors" + "io" + "log/slog" + "net" + "os" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + "unsafe" + + "golang.org/x/sys/unix" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/engine/internal/errclass" + "github.com/goceleris/celeris/internal/conn" + "github.com/goceleris/celeris/resource" +) + +// The fd-lifetime rule on the close paths (celeris#685): a descriptor number +// is released only when no op that names it can still be issued. These run in +// every build and need one ring (or one worker), so they run in the CI `unit` +// job's shape; the trials that force the theft itself are the -tags=validation +// TestRecvTheft715ArmA, TestRecvTheft685Linked and TestRecvTheft685Hijack*. + +// fdTarget returns what /proc/self/fd/ names ("socket:[inode]" for a +// socket), or "" when fd is not open. +func fdTarget(fd int) string { + s, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(fd)) + if err != nil { + return "" + } + return s +} + +// serverFDFor returns this process's descriptor whose peer is local (the +// server side of a loopback connection the test dialed), or -1. +func serverFDFor(local string) int { + ents, err := os.ReadDir("/proc/self/fd") + if err != nil { + return -1 + } + for _, ent := range ents { + fd, err := strconv.Atoi(ent.Name()) + if err != nil { + continue + } + sa, err := unix.Getpeername(fd) + if err != nil { + continue + } + if sockaddrString(sa) == local { + return fd + } + } + return -1 +} + +// TestPendingReleaseEntryStaysTwentyFourBytes pins that the celeris#685 hold +// (holdsFD, fd) fits in the padding after detached: the queue is appended to +// on every close. +func TestPendingReleaseEntryStaysTwentyFourBytes(t *testing.T) { + if unsafe.Sizeof(uintptr(0)) != 8 { + t.Skip("64-bit layout only") + } + if n := unsafe.Sizeof(pendingReleaseEntry{}); n != 24 { + t.Fatalf("pendingReleaseEntry is %d bytes, want 24", n) + } +} + +// newOwedCloseWorker returns a synthetic worker with a real ring and one H1 +// connection on a socketpair, and the peer end (never read by the worker). +// detached gives the connection a detachMu, which routes closeConn to +// finishCloseDetached, as for an async-dispatch connection. +func newOwedCloseWorker(t *testing.T, detached bool) (*Worker, *connState, int) { + t.Helper() + ring := newTestRing(t) + pair, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM|unix.SOCK_NONBLOCK, 0) + if err != nil { + t.Skipf("socketpair: %v", err) + } + local, peer := pair[0], pair[1] + localTarget := fdTarget(local) + t.Cleanup(func() { + _ = unix.Close(peer) + // Only if the test left it open: the number may be someone else's + // once the release closed it. + if fdTarget(local) == localTarget { + _ = unix.Close(local) + } + }) + w := &Worker{ + ring: ring, + conns: make([]*connState, local+1), + liveConns: make([]int, 0, 4), + errs: &errclass.Counters{}, + activeConns: &atomic.Int64{}, + closeCount: &atomic.Uint64{}, + recvArm: &recvArmStats{}, + handoffLoss: &handoffLossStats{}, + } + w.cachedNow = time.Now().UnixNano() + cs := &connState{ + fd: local, + liveIdx: -1, + generation: 11, + buf: make([]byte, 4096), + h1State: conn.NewH1State(), + detected: true, + } + cs.protocol.Store(int32(engine.HTTP1)) + if detached { + cs.detachMu = new(sync.Mutex) + } + w.conns[local] = cs + w.addLiveConn(cs) + w.connCount = 1 + w.activeConns.Add(1) + return w, cs, peer +} + +// runRingOnce submits what the ring holds, waits up to d for completions, and +// feeds every recv/send completion to staleConnCQE, as the loop does. +// Returns the recv completions' results. +func runRingOnce(t *testing.T, w *Worker, d time.Duration) []int32 { + t.Helper() + if err := w.ring.SubmitAndWaitTimeout(d); err != nil { + t.Fatalf("submit: %v", err) + } + var res []int32 + head, tail := w.ring.BeginCQ() + for ; head != tail; head++ { + c := w.ring.cqeAt(head) + ud := c.UserData + switch ud & udMask { + case udRecv, udSend: + if ud&udMask == udRecv { + res = append(res, c.Res) + } + w.staleConnCQE(c, int(ud&fdMask), ud) + } + } + w.ring.EndCQ(head) + return res +} + +// peerSawEnd reports whether the peer reads EOF (or a reset) within d. +func peerSawEnd(peer int, d time.Duration) bool { + buf := make([]byte, 64) + for end := time.Now().Add(d); time.Now().Before(end); { + n, err := unix.Read(peer, buf) + if n == 0 && err == nil { + return true + } + if err != nil && !errors.Is(err, unix.EAGAIN) && !errors.Is(err, unix.EINTR) { + return true + } + time.Sleep(time.Millisecond) + } + return false +} + +// TestFinishCloseKeepsDescriptorWhileRecvOwed is the rule's mechanism on the +// H1 fast path: a close with a recv SQE placed and not submitted (the #715 +// precondition) must leave the descriptor open, shut its read side, and hand +// it to its pendingRelease entry; the recv, issued by the next enter, ends at +// once against this socket; the release then closes the descriptor. +func TestFinishCloseKeepsDescriptorWhileRecvOwed(t *testing.T) { + for _, detached := range []bool{false, true} { + t.Run(map[bool]string{false: "fast", true: "detached"}[detached], func(t *testing.T) { + w, cs, peer := newOwedCloseWorker(t, detached) + fd := cs.fd + target := fdTarget(fd) + if !w.prepareRecv(cs, cs.buf) || cs.kernelInflight != 1 { + t.Fatalf("recv not placed: kernelInflight=%d", cs.kernelInflight) + } + w.closeConn(fd) + if w.conns[fd] != nil { + t.Fatal("the conn is still registered after closeConn") + } + if got := fdTarget(fd); got != target { + t.Fatalf("fd %d was released with its recv still unsubmitted (now %q, was %q)", fd, got, target) + } + if len(w.pendingRelease) != 1 || !w.pendingRelease[0].holdsFD || int(w.pendingRelease[0].fd) != fd || w.closeFDOwed != 1 { + t.Fatalf("the kept descriptor is not on its pendingRelease entry: %+v closeFDOwed=%d", w.pendingRelease, w.closeFDOwed) + } + // The async path shuts the write side down too, as it always did: + // its client sees the FIN now, not at the release. + if detached && !peerSawEnd(peer, time.Second) { + t.Error("finishCloseDetached kept the descriptor but its peer saw no FIN") + } + w.drainPendingRelease() + if fdTarget(fd) != target { + t.Fatal("drainPendingRelease closed the descriptor before the owed recv ended") + } + var res []int32 + for end := time.Now().Add(2 * time.Second); cs.kernelInflight > 0 && time.Now().Before(end); { + res = append(res, runRingOnce(t, w, 50*time.Millisecond)...) + } + if cs.kernelInflight != 0 { + t.Fatalf("the owed recv never ended: kernelInflight=%d", cs.kernelInflight) + } + w.cachedNow = time.Now().UnixNano() + w.drainPendingRelease() + if fdTarget(fd) == target { + t.Fatalf("fd %d still open after its owed recv ended (recv results %v)", fd, res) + } + if w.closeFDOwed != 0 || len(w.pendingRelease) != 0 { + t.Fatalf("closeFDOwed=%d pendingRelease=%d after the release, want 0 and 0", w.closeFDOwed, len(w.pendingRelease)) + } + if !peerSawEnd(peer, time.Second) { + t.Error("the peer saw no end of the connection after the release") + } + t.Logf("celeris685 mechanism detached=%v recv_results=%v", detached, res) + }) + } +} + +// TestFinishCloseClosesAtOnceWhenNothingOwed is the other half: with no op +// owed (the sync Connection: close response, a client's FIN) the close is +// what it was, the descriptor closed at once and nothing held. +func TestFinishCloseClosesAtOnceWhenNothingOwed(t *testing.T) { + w, cs, peer := newOwedCloseWorker(t, false) + fd := cs.fd + target := fdTarget(fd) + w.closeConn(fd) + if fdTarget(fd) == target { + t.Fatalf("fd %d kept open with nothing owed", fd) + } + if w.closeFDOwed != 0 || (len(w.pendingRelease) == 1 && w.pendingRelease[0].holdsFD) { + t.Fatalf("a close with nothing owed registered a kept descriptor: closeFDOwed=%d %+v", w.closeFDOwed, w.pendingRelease) + } + if !peerSawEnd(peer, time.Second) { + t.Error("the peer saw no end of the connection") + } +} + +// TestShutdownEndsOwedOpsBeforeClosing drives worker shutdown with idle +// keep-alive connections, each with its next recv armed (linked behind its +// last response's SEND), so every one has an op owed at shutdown. Shutdown +// must end those ops and then close every descriptor: none may be left open, +// the engine must stop promptly, and the release backstop must not be what +// closed any of them. +func TestShutdownEndsOwedOpsBeforeClosing(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := ln.Addr().String() + _ = ln.Close() + e, err := New(resource.Config{ + Addr: addr, + Protocol: engine.HTTP1, + Resources: resource.Resources{Workers: 2}, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }, transplantTestHandler{}) + if err != nil { + skipOrFail656(t, "iouring engine unavailable: %v", err) + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- e.Listen(ctx) }() + defer cancel() + for deadline := time.Now().Add(8 * time.Second); e.Addr() == nil; { + select { + case err := <-done: + skipOrFail656(t, "iouring engine failed to start: %v", err) + default: + } + if time.Now().After(deadline) { + t.Fatal("engine did not start") + } + time.Sleep(10 * time.Millisecond) + } + const n = 8 + var clients []net.Conn + var servers []int + var targets []string + for i := range n { + c, err := net.DialTimeout("tcp", addr, time.Second) + if err != nil { + t.Fatalf("dial %d: %v", i, err) + } + clients = append(clients, c) + _ = c.SetDeadline(time.Now().Add(3 * time.Second)) + if _, err := c.Write([]byte("GET / HTTP/1.1\r\nHost: x\r\n\r\n")); err != nil { + t.Fatalf("write %d: %v", i, err) + } + buf := make([]byte, 512) + var got []byte + for !strings.Contains(string(got), "\r\n\r\nok") { + k, err := c.Read(buf) + got = append(got, buf[:k]...) + if err != nil { + t.Fatalf("read %d: %v (%q)", i, err, got) + } + } + srv := -1 + for end := time.Now().Add(time.Second); srv < 0 && time.Now().Before(end); { + srv = serverFDFor(c.LocalAddr().String()) + } + if srv < 0 { + t.Fatalf("conn %d: no server-side descriptor found", i) + } + servers = append(servers, srv) + targets = append(targets, fdTarget(srv)) + } + defer func() { + for _, c := range clients { + _ = c.Close() + } + }() + start := time.Now() + cancel() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("the engine did not stop within 3s") + } + took := time.Since(start) + left := 0 + for i, fd := range servers { + if fdTarget(fd) == targets[i] { + left++ + } + } + m := e.Metrics() + t.Logf("celeris685 shutdown conns=%d took=%v left_open=%d close_fd_forced=%d", n, took, left, m.CloseFDForced) + if left != 0 { + t.Fatalf("%d of %d server-side descriptors still open after the engine stopped", left, n) + } + if m.CloseFDForced != 0 { + t.Fatalf("CloseFDForced = %d, want 0", m.CloseFDForced) + } +} diff --git a/engine/iouring/handoff_loss.go b/engine/iouring/handoff_loss.go index 1a8ea34f..a36bcd38 100644 --- a/engine/iouring/handoff_loss.go +++ b/engine/iouring/handoff_loss.go @@ -37,8 +37,13 @@ import ( // the socket lives on under the hijacker's net.Conn, so a recv that // completes with data before its cancel lands has read bytes from a // connection its client is still using. After a close, a recv that -// had not reached the kernel yet resolves the fd NUMBER when it does, -// and a new connection may hold that number by then. +// had not been issued yet resolved the fd NUMBER when it was, and a +// new connection could hold that number by then (celeris#715). Both +// are closed off by the fd-lifetime rule on those paths +// (celeris#685): hijackConn submits its cancels before handing the +// socket over, and a close keeps the number while an op is owed. +// What Closed still counts is a closed conn's recv reading its OWN +// client's late bytes, which that client sees end with the close. // // A stale CQE whose (fd, generation) equals the fd's current // occupant's (a generation collision, which needs the process-wide @@ -96,6 +101,17 @@ import ( // paths only, never on the per-request path while no drain is set, and like // the celeris#586 witnesses they are per-event invariants that a // per-iteration batch could lose at loop exit. +// +// The same rule on the close paths (celeris#685; see the close-path section +// of fd_lifetime.go) keeps two more: +// +// - closeFDDeferred: closes whose descriptor was left open because the +// kernel still owed an op on it, and closed at that op's terminal CQE. A +// rate, and on an async-handler engine with Connection: close traffic +// close to one per request, so it is the one counter here that is +// batched per loop iteration (Worker.closeFDDeferredBatch). +// - closeFDForced: such descriptors the pendingRelease backstop closed +// with an op still owed. Must stay 0. type handoffLossStats struct { staleRecvDataClosed atomic.Uint64 staleRecvDataTransplanted atomic.Uint64 @@ -109,6 +125,14 @@ type handoffLossStats struct { claimDeferred atomic.Uint64 reapFailed atomic.Uint64 reapUnsupported atomic.Uint64 + closeFDDeferred atomic.Uint64 + closeFDForced atomic.Uint64 +} + +func (s *handoffLossStats) noteCloseFDForced() { + if s != nil { + s.closeFDForced.Add(1) + } } // The fd-lifetime counters are nil-safe: a hand-built test Worker has none. diff --git a/engine/iouring/handoff_loss_metrics_test.go b/engine/iouring/handoff_loss_metrics_test.go index 773b0354..f62a4e96 100644 --- a/engine/iouring/handoff_loss_metrics_test.go +++ b/engine/iouring/handoff_loss_metrics_test.go @@ -26,6 +26,8 @@ func TestMetricsCarriesTheHandoffLossWitnesses(t *testing.T) { e.metrics.handoffLoss.claimDeferred.Store(31) e.metrics.handoffLoss.reapFailed.Store(37) e.metrics.handoffLoss.reapUnsupported.Store(41) + e.metrics.handoffLoss.closeFDDeferred.Store(43) + e.metrics.handoffLoss.closeFDForced.Store(47) m := e.Metrics() for _, c := range []struct { @@ -44,6 +46,8 @@ func TestMetricsCarriesTheHandoffLossWitnesses(t *testing.T) { {"TransplantClaimDeferred", m.TransplantClaimDeferred, 31}, {"TransplantReapFailed", m.TransplantReapFailed, 37}, {"TransplantReapUnsupported", m.TransplantReapUnsupported, 41}, + {"CloseFDDeferred", m.CloseFDDeferred, 43}, + {"CloseFDForced", m.CloseFDForced, 47}, } { if c.got != c.want { t.Errorf("Metrics().%s = %d, want %d — the witness exists but cannot "+ diff --git a/engine/iouring/recv_theft_685_linked_linux_test.go b/engine/iouring/recv_theft_685_linked_linux_test.go index 10d38d4f..676e0141 100644 --- a/engine/iouring/recv_theft_685_linked_linux_test.go +++ b/engine/iouring/recv_theft_685_linked_linux_test.go @@ -8,7 +8,6 @@ import ( "io" "log/slog" "net" - "os" "strconv" "strings" "testing" @@ -135,29 +134,6 @@ func startLinkedTheftEngine(t *testing.T) (*Engine, int) { return e, port } -// serverFDFor returns this process's descriptor whose peer is local (the -// server side of a loopback connection the test dialed), or -1. -func serverFDFor(local string) int { - ents, err := os.ReadDir("/proc/self/fd") - if err != nil { - return -1 - } - for _, ent := range ents { - fd, err := strconv.Atoi(ent.Name()) - if err != nil { - continue - } - sa, err := unix.Getpeername(fd) - if err != nil { - continue - } - if sockaddrString(sa) == local { - return fd - } - } - return -1 -} - // fillSendQueue writes to fd (a server-side socket whose peer never reads) // until two passes 50 ms apart add nothing: the peer's window is closed and // the send queue is full. Returns the bytes written. diff --git a/engine/iouring/recv_theft_715_linux_test.go b/engine/iouring/recv_theft_715_linux_test.go index 2214fdf0..dff855a6 100644 --- a/engine/iouring/recv_theft_715_linux_test.go +++ b/engine/iouring/recv_theft_715_linux_test.go @@ -242,16 +242,6 @@ type theftResult struct { candidatesHit int } -// fdTarget returns what /proc/self/fd/ names ("socket:[inode]" for a -// socket), or "" when fd is not open. -func fdTarget(fd int) string { - s, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(fd)) - if err != nil { - return "" - } - return s -} - const ( theftMaxAttempts = 10 theftCandidates = 6 diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go index 12942201..bef0d870 100644 --- a/engine/iouring/worker.go +++ b/engine/iouring/worker.go @@ -179,10 +179,20 @@ func clampBufRingCount(n int) int { // goroutine's defer that reads cs.fd / cs.asyncInBuf. (The goroutine's // own closure references remain visible to GC after we drop ours, so // dropping the ref once the kernel ops drained is safe.) +// +// holdsFD marks an entry whose close path left its descriptor, fd, OPEN +// because the kernel still owed an op on it (celeris#685, see closeFDOwed); +// drainPendingRelease closes it at the same point it releases cs: when the +// last owed op has delivered its terminal CQE. A flag rather than an fd of -1 +// so that the zero entry holds nothing. Both sit in detached's padding, so the +// entry stays 24 bytes on 64-bit platforms +// (TestPendingReleaseEntryStaysTwentyFourBytes). type pendingReleaseEntry struct { cs *connState releaseAtNanos int64 detached bool + holdsFD bool + fd int32 } // closedOpsEntry is the Worker.closedOps value: the conn(s) closed under @@ -442,6 +452,14 @@ type Worker struct { // releaseAtNanos is only the anomaly backstop — see // pendingReleaseHoldNanos. pendingRelease []pendingReleaseEntry + // closeFDOwed counts the pendingRelease entries that still hold their + // descriptor open (holdsFD, celeris#685). The worker does not park + // while it is non-zero: the terminal CQEs those closes wait for arrive + // only through the ring, and a parked worker enters no ring. Worker + // thread only. closeFDDeferredBatch is the count of such closes not yet + // added to handoffLoss.closeFDDeferred, flushed once per loop iteration. + closeFDOwed int + closeFDDeferredBatch uint64 // closedOps routes terminal recv/send CQEs that arrive AFTER their // conn was closed (w.conns[fd] already nil or reused) back to the @@ -817,18 +835,20 @@ func (w *Worker) noteRecvPlaced(cs *connState) { // Why the close paths ask (celeris#715 hypothesis (a), the celeris#685 // class). Such a recv names the descriptor NUMBER (fixed files are off), and // the kernel resolves the number when the next submit issues the recv. -// finishClose and finishCloseDetached queue the recv's ASYNC_CANCEL behind it -// and close the descriptor at once. If another thread's accept is given the +// finishClose and finishCloseDetached queued the recv's ASYNC_CANCEL behind it +// and closed the descriptor at once. If another thread's accept was given the // freed number before this worker's next submit, and its connection's request -// is already in the socket (TCP_DEFER_ACCEPT hands over only connections that -// have sent), the recv reads that request and completes under the closed -// conn's (fd, generation): staleConnCQE drops it as stale_recv_data_closed, -// and the new connection's own recv then waits on an empty socket. So, under -// -tags=validation, the close paths -// - count the close (recvtheft.CloseWithUnsubmittedRecv, the witness), -// - park the worker right after the descriptor is closed when a -// recvtheft trial is armed (recvtheft.HoldAfterClose), and -// - in the trial's control arm, submit the ring before closing +// was already in the socket (TCP_DEFER_ACCEPT hands over only connections that +// have sent), the recv read that request and completed under the closed +// conn's (fd, generation): staleConnCQE dropped it as stale_recv_data_closed, +// and the new connection's own recv then waited on an empty socket. The close +// paths now keep the number while the recv is owed (fdOwed); under +// -tags=validation they still +// - count the close (recvtheft.CloseWithUnsubmittedRecv, the witness of +// the precondition), +// - park the worker when the close path returns, when a recvtheft trial is +// armed (recvtheft.HoldAfterClose), and +// - in the trial's control arm, submit the ring before the close // (recvtheft.SubmitBeforeClose), so the recv is issued while the number // still names this conn's socket. // @@ -1385,6 +1405,13 @@ func (w *Worker) run(ctx context.Context) { w.zc.noteRingBytes(w.ringBytesBatch) w.ringBytesBatch = 0 } + // Same cadence for the celeris#685 deferred-close rate. + if w.closeFDDeferredBatch > 0 { + if w.handoffLoss != nil { + w.handoffLoss.closeFDDeferred.Add(w.closeFDDeferredBatch) + } + w.closeFDDeferredBatch = 0 + } // Same cadence for the celeris#607 link-arm exposure witness. if w.linkArmBatch > 0 { if w.recvArm != nil { @@ -1418,6 +1445,13 @@ func (w *Worker) run(ctx context.Context) { // queuePendingRelease docstring). w.iterCount++ if len(w.pendingRelease) > 0 { + // An idle iteration with a descriptor still kept for an owed op + // (celeris#685) refreshes the clock the backstop reads: cachedNow + // moves only with CQE traffic or the timeout sweep, and a kept + // descriptor also keeps this worker from parking. + if w.closeFDOwed > 0 && w.emptyIters > 0 { + w.cachedNow = time.Now().UnixNano() + } w.drainPendingRelease() } @@ -1502,9 +1536,15 @@ func (w *Worker) run(ctx context.Context) { // re-checked under wakeMu below, and every driver-action enqueue // kicks a parked worker through wakeIfSuspended; see there for why // the pair cannot lose a wakeup. + // + // closeFDOwed gate (celeris#685): a close that left its descriptor + // open until the kernel's last op on it completes is finished by + // drainPendingRelease at that op's terminal CQE, which only an + // iteration of this loop reads. Parking first would hold the + // descriptor, and the socket with it, for as long as the park lasts. if w.listenFD < 0 && w.connCount == 0 && !w.hasDriverConns.Load() && w.driverActionPending.Load() == 0 && w.detachQPending.Load() == 0 && - w.acceptPaused.Load() { + w.closeFDOwed == 0 && w.acceptPaused.Load() { // Submit what this iteration queued before parking (celeris#657, // A5). The iteration that closes or hands off the last conn // queues its close-path cancels (the header timer's, a send's) @@ -1592,11 +1632,13 @@ func (w *Worker) staleConnCQE(c *completionEntry, fd int, ud uint64) bool { // under-count the live conn (and may clear recvArmed above // with its recv still kernel-armed), so its own close can // skip the cancel and release early — the UAF class this - // accounting exists to prevent. Reachability is narrow: - // non-SQPOLL task-work ordering posts the close-path - // -ECANCELED during the submit syscall, before the fd can be - // re-accepted; the window needs a dropped cancel SQE (full SQ - // ring) or SQPOLL's decoupled completion ordering ON TOP of + // accounting exists to prevent. Reachability is narrow: a + // close path keeps the number allocated until the closed + // conn's last owed op has delivered its terminal CQE + // (celeris#685), so on this worker the number cannot be + // re-occupied while such a CQE is still to come; the window + // needs the pendingRelease backstop to have closed a number + // with an op still owed (CloseFDForced, must stay 0) ON TOP of // the gen collision. When it fires, the closed conn's // closedOps entry is left orphaned and the 5 s backstop WARN // in drainPendingRelease is the production signal. @@ -2313,13 +2355,16 @@ func (w *Worker) hijackConn(fd int) (net.Conn, error) { w.connCount-- w.activeConns.Add(-1) w.closeCount.Add(1) - // Cancel-then-release discipline, hijack variant: a single-shot - // recv SQE is virtually always still armed on cs.buf here. The fd - // lives on under the caller's net.Conn, so an uncancelled recv would - // not only pin cs.buf past release (the #256-class UAF) but also - // STEAL the first bytes the hijacker tries to read. Cancel it by its + // Cancel-then-release discipline, hijack variant: any op still armed + // on cs (a multishot recv stays armed across its request; the + // single-shot recv that brought the request has completed, measured by + // TestRecvTheft685HijackSingleShot) targets cs.buf. The fd lives on + // under the caller's net.Conn, so an uncancelled recv would not only + // pin cs.buf past release (the #256-class UAF) but also STEAL the + // first bytes the hijacker tries to read. Cancel it by its // generation-tagged user_data and defer the pool release until the // terminal CQE arrives, exactly like finishClose. + // // celeris#685 hijack witness and hold, validation builds only: count a // hijack with an op still owed on the socket (kernelInflight > 0), and // hold the worker thread before it returns, and so before its next @@ -2333,6 +2378,25 @@ func (w *Worker) hijackConn(fd int) (net.Conn, error) { w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) w.queuePendingRelease(cs) + // The fd-lifetime rule, hijack variant (celeris#685). The socket lives + // on under the hijacker's net.Conn, so no op of this worker may read it + // once the hijacker has it, and none may resolve the original number, + // which the f.Close below releases. The cancel above reaches the kernel + // only at the next submit. Until then an owed recv that is already + // issued (a multishot recv stays armed across its request) can take the + // hijacker's first bytes on a ring whose completions run at any syscall + // exit, and one whose SQE is still in the ring would resolve the number + // after the close. So when an op is owed, submit now: an issued recv is + // cancelled before it can read, and a recv SQE still in the ring is + // issued first, against this socket and not a reused number, then + // cancelled. No hijack path leaves such an SQE: a hijack runs inside + // its request's processing, after that request's recv completed. Nor a + // linked recv, which rides behind a SEND: a hijack with a send pending + // is refused above. One io_uring_enter per hijack with an op owed; none + // otherwise, which is every hijack on single-shot recv. + if fdOwed(cs) && !w.sqpoll { + _, _ = w.ring.Submit() + } f := os.NewFile(uintptr(fd), "tcp") c, err := net.FileConn(f) _ = f.Close() @@ -3991,10 +4055,26 @@ func (w *Worker) dropClosedOps(cs *connState) { // fires). See Worker.pendingRelease docstring for the kernel-buffer-lifetime // invariant this enforces. func (w *Worker) queuePendingRelease(cs *connState) { + w.queuePendingReleaseFD(cs, false, -1) +} + +// queuePendingReleaseFD is queuePendingRelease (detached false) or +// queuePendingReleaseDetached (detached true) for a close path that may also +// hand the descriptor over: keptFD >= 0 is closed by drainPendingRelease when +// cs is released, not by the caller (celeris#685, closeFDOwed); -1 means the +// caller closes it. Worker thread only. +func (w *Worker) queuePendingReleaseFD(cs *connState, detached bool, keptFD int) { w.pendingRelease = append(w.pendingRelease, pendingReleaseEntry{ cs: cs, releaseAtNanos: time.Now().UnixNano() + pendingReleaseHoldNanos, + detached: detached, + holdsFD: keptFD >= 0, + fd: int32(keptFD), }) + if keptFD >= 0 { + w.closeFDOwed++ + w.closeFDDeferredBatch++ + } } // queuePendingReleaseDetached holds cs alive past the kernel's recv-SQE @@ -4012,11 +4092,7 @@ func (w *Worker) queuePendingRelease(cs *connState) { // visible on behalf of the kernel's invisible recv pointer, so it must // outlive every pending op targeting cs.buf // span-corruption class). func (w *Worker) queuePendingReleaseDetached(cs *connState) { - w.pendingRelease = append(w.pendingRelease, pendingReleaseEntry{ - cs: cs, - releaseAtNanos: time.Now().UnixNano() + pendingReleaseHoldNanos, - detached: true, - }) + w.queuePendingReleaseFD(cs, true, -1) } // drainPendingRelease releases queued connStates whose kernel-held ops @@ -4054,9 +4130,26 @@ func (w *Worker) drainPendingRelease() { if w.logger != nil { w.logger.Warn("releasing connState with kernel ops unaccounted for after backstop hold", "worker", w.id, "fd", cs.fd, "generation", cs.generation, - "inflight", cs.kernelInflight, "detached", entry.detached) + "inflight", cs.kernelInflight, "detached", entry.detached, + "holds_fd", entry.holdsFD) } w.dropClosedOps(cs) + // The descriptor goes too, though an op may still name it: a + // descriptor held forever is a leak with no end. The close path + // shut the socket's read side down, so an owed recv the kernel + // issues on THIS socket ends at once; one issued after the + // number is reused would not, which is why this is counted and + // must stay 0 (celeris#685). + if entry.holdsFD { + w.handoffLoss.noteCloseFDForced() + } + } + // The close the close path left for this moment (celeris#685): + // every op that named the descriptor has delivered its terminal + // CQE, so none can resolve the number any more. + if entry.holdsFD { + _ = unix.Close(int(entry.fd)) + w.closeFDOwed-- } if !entry.detached { releaseConnState(cs) @@ -4109,6 +4202,9 @@ func (w *Worker) finishClose(fd int) { // Capture close-path decisions before queueing cs for deferred release. fixedFile := cs != nil && cs.fixedFile fastClose := cs != nil && engine.Protocol(cs.protocol.Load()) == engine.HTTP1 && cs.h1State != nil && !cs.h1State.Detached.Load() + // The fd-lifetime rule (celeris#685): while the kernel still owes an op + // that names fd, the number is not released here. See fdOwed. + owed := fdOwed(cs) // Cancel-then-release discipline (v1.4.15/7beebb9 corruption fix): ASYNC_CANCEL any kernel-held // op still targeting cs's buffers (the single-shot recv is virtually // ALWAYS armed here — closing the fd below does NOT complete it), then @@ -4128,7 +4224,7 @@ func (w *Worker) finishClose(fd int) { } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) - w.queuePendingRelease(cs) + w.queuePendingReleaseFD(cs, false, keptFD(fd, owed)) if recvtheft.Enabled && recvtheft.SubmitBeforeClose() { _, _ = w.ring.Submit() } @@ -4174,23 +4270,32 @@ func (w *Worker) finishClose(fd int) { // // forceRSTClose (slowloris-defence): SHUT_RDWR + close. See // finishCloseDetached for the rationale. + // + // Each branch below ends in closeUnlessOwed: with an op still owed the + // descriptor stays open, and drainPendingRelease closes it at that op's + // terminal CQE (celeris#685). A branch that did not shut the read side + // down already does so first, so the owed recv ends as soon as the + // kernel issues it (fdOwed). if cs != nil && cs.forceRSTClose { _ = unix.Shutdown(fd, unix.SHUT_RDWR) - _ = unix.Close(fd) + closeUnlessOwed(fd, owed) return } // fastClose: plain H1 no-body conn → close() alone suffices. if fastClose { - _ = unix.Close(fd) + if owed { + _ = unix.Shutdown(fd, fastCloseShutdownHow(cs)) + } + closeUnlessOwed(fd, owed) return } - _ = unix.Shutdown(fd, unix.SHUT_WR) + _ = unix.Shutdown(fd, shutdownHow(owed)) raddr := "" if cs != nil { raddr = cs.remoteAddr } sockopts.CloseDrain(fd, "iouring/finishClose", raddr) - _ = unix.Close(fd) + closeUnlessOwed(fd, owed) } // finishCloseAny dispatches to finishCloseDetached for detached connections @@ -4229,6 +4334,8 @@ func (w *Worker) finishCloseDetached(fd int, cs *connState) { } fixedFile := cs.fixedFile + // The fd-lifetime rule (celeris#685), as in finishClose: see fdOwed. + owed := fdOwed(cs) // Do NOT call releaseConnState — goroutine closures still reference cs. // // Cancel-then-release discipline (v1.4.15/7beebb9 corruption fix): ASYNC_CANCEL the armed recv @@ -4254,7 +4361,7 @@ func (w *Worker) finishCloseDetached(fd int, cs *connState) { } w.cancelConnOps(fd, cs) w.noteClosedInflight(cs) - w.queuePendingReleaseDetached(cs) + w.queuePendingReleaseFD(cs, true, keptFD(fd, owed)) if recvtheft.Enabled && recvtheft.SubmitBeforeClose() { _, _ = w.ring.Submit() } @@ -4277,9 +4384,12 @@ func (w *Worker) finishCloseDetached(fd int, cs *connState) { // LINGER on iouring. The SHUT_RDWR + LINGER combo wins because // SHUT_RDWR drops the receive queue too (close() with LINGER // only drops TX). Walker observes the abortive close immediately. + // + // Each branch ends in closeUnlessOwed, and shuts the read side down too + // when an op is still owed (shutdownHow), exactly as finishClose does. if cs.forceRSTClose { _ = unix.Shutdown(fd, unix.SHUT_RDWR) - _ = unix.Close(fd) + closeUnlessOwed(fd, owed) return } // Async-mode HTTP1 conns are NOT truly detached (no WS/SSE middleware @@ -4299,17 +4409,17 @@ func (w *Worker) finishCloseDetached(fd int, cs *connState) { // the drain syscall and the close syscall left a multi-µs window in // which a fresh walker drip would queue, making the close → RST). if cs.h1State != nil && !cs.h1State.Detached.Load() { - _ = unix.Shutdown(fd, unix.SHUT_WR) - _ = unix.Close(fd) + _ = unix.Shutdown(fd, shutdownHow(owed)) + closeUnlessOwed(fd, owed) return } // Truly detached (WS/SSE): graceful half-close so middleware-queued // close-frame echoes flush before the FIN. Sync unix.Close (not via // io_uring) to avoid the async-SQE pile-up that plagued the pre-patch // version. - _ = unix.Shutdown(fd, unix.SHUT_WR) + _ = unix.Shutdown(fd, shutdownHow(owed)) sockopts.CloseDrain(fd, "iouring/finishCloseDetached", cs.remoteAddr) - _ = unix.Close(fd) + closeUnlessOwed(fd, owed) } // runAsyncHandler is the dispatch goroutine for an HTTP1 conn when @@ -5765,6 +5875,13 @@ func (w *Worker) shutdown() { // the wakeup eventfd is closed, because addAdoptAction signals it under // the same lock (celeris#658). w.closeAdoptQueue() + // The fd-lifetime rule at shutdown (celeris#685): end every op the kernel + // still owes on a connection's descriptor before any of those + // descriptors is closed (endOwedOpsAtShutdown). Before shutdownDrivers: + // the drain submits what the SQ ring still holds, and shutdownDrivers + // closes the driver descriptors on the promise that nothing is + // submitted after it. + w.endOwedOpsAtShutdown() // Fire onClose for every registered driver conn before tearing down // ring/listen fd. Otherwise driver callbacks are silently dropped. Then // wait for the driver closes handed off the worker before the shutdown, @@ -5821,6 +5938,8 @@ func (w *Worker) shutdown() { if cs.h2State != nil { conn.CloseH2(cs.h2State) } + // endOwedOpsAtShutdown has ended the ops owed on fd, so no op can + // resolve the number once it is free (celeris#685). if !cs.fixedFile { _ = unix.Close(fd) } diff --git a/internal/recvtheft/doc.go b/internal/recvtheft/doc.go index eb0de3fb..c69d88dc 100644 --- a/internal/recvtheft/doc.go +++ b/internal/recvtheft/doc.go @@ -10,22 +10,29 @@ // kernel at the next io_uring_enter, at the top of the next loop iteration. // Fixed files are off, so the SQE names the descriptor NUMBER, and the kernel // resolves that number when it issues the op, not when the SQE was written. -// finishClose and finishCloseDetached queue an ASYNC_CANCEL behind such an -// unsubmitted recv and then close the descriptor synchronously. If another -// thread's accept is given the freed number before this worker submits, the -// recv reads the new connection's request, completes under the old -// connection's (fd, generation), and is dropped as stale; the new connection -// then waits on an empty socket. +// finishClose and finishCloseDetached queued an ASYNC_CANCEL behind such an +// unsubmitted recv and then closed the descriptor synchronously. If another +// thread's accept was given the freed number before this worker submitted, +// the recv read the new connection's request, completed under the old +// connection's (fd, generation), and was dropped as stale; the new connection +// then waited on an empty socket. A recv linked behind a SEND has the same +// window between the SEND's CQE and the task work that issues it. Since +// celeris#685 the close paths keep the number allocated while such an op is +// owed, so the sibling is given another number; the trials assert that the +// new connection is answered either way. // // The pieces: -// - a witness counter, [CloseWithUnsubmittedRecv]: a close reached while -// the connection's recv SQE had not been consumed by the kernel; -// - a close hold ([HoldAfterClose]) that parks the closing worker right -// after the close, and an accept hold ([AfterAccept]) that parks a -// sibling worker right after it accepted the freed number, before it -// submits its own recv for it. Together they force the one ordering the -// theft needs: the sibling's accept after the close, the closing worker's -// submit before the sibling's; +// - witness counters: [CloseWithUnsubmittedRecv], a close reached while the +// connection's recv SQE had not been consumed by the kernel, and +// [CloseWithLinkedRecv], one reached while its recv was linked behind a +// SEND and had not completed; +// - a close hold ([HoldAfterClose]) that parks the closing worker when its +// close path returns (for one form or the other, [Options.LinkedRecv]), +// and an accept hold ([AfterAccept]) that parks a sibling worker right +// after it accepted the closed descriptor's number, before it submits its +// own recv for it. Together they force the one ordering the theft needs: +// the sibling's accept after the close, the closing worker's submit +// before the sibling's; // - a gate ([AfterPromoteArm]) that makes the dispatch goroutine's close // land in the same loop iteration as the promote's recv re-arm; // - the control switch ([Options.SubmitBeforeClose]): submit before the @@ -35,5 +42,8 @@ // ([NoteStaleRecvData]), with its first bytes; // - a separate hold for hypothesis (c) ([WakeHold]): the dispatch // goroutine's wake-up delayed between the worker's asyncInMu unlock and -// its Signal / goroutine start. +// its Signal / goroutine start; +// - the hijack witness ([HijackWithOpOwed]) and hold ([HijackHold]): a +// Hijack made while the kernel still owed the connection an op, and the +// worker held right after it, before its next io_uring_enter. package recvtheft From 0f951ddd644b89b246c8f8bf3a1301b6203bc733 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 03:37:51 +0200 Subject: [PATCH 07/12] test(iouring): move the /proc/self/fd probes the #685 tests share into their own file fdTarget and serverFDFor are used by the fd-lifetime tests (every build) and by the recv-theft trials (-tags=validation). In a file of their own, the fix's unit tests can be removed from a tree without taking the trials' helpers with them (the negative-control tree of celeris#685 does exactly that). Refs #685 --- engine/iouring/fd_lifetime_close_test.go | 35 ----------------- engine/iouring/fd_probe_linux_test.go | 48 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 35 deletions(-) create mode 100644 engine/iouring/fd_probe_linux_test.go diff --git a/engine/iouring/fd_lifetime_close_test.go b/engine/iouring/fd_lifetime_close_test.go index b42d7599..cca03ad9 100644 --- a/engine/iouring/fd_lifetime_close_test.go +++ b/engine/iouring/fd_lifetime_close_test.go @@ -8,8 +8,6 @@ import ( "io" "log/slog" "net" - "os" - "strconv" "strings" "sync" "sync/atomic" @@ -31,39 +29,6 @@ import ( // job's shape; the trials that force the theft itself are the -tags=validation // TestRecvTheft715ArmA, TestRecvTheft685Linked and TestRecvTheft685Hijack*. -// fdTarget returns what /proc/self/fd/ names ("socket:[inode]" for a -// socket), or "" when fd is not open. -func fdTarget(fd int) string { - s, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(fd)) - if err != nil { - return "" - } - return s -} - -// serverFDFor returns this process's descriptor whose peer is local (the -// server side of a loopback connection the test dialed), or -1. -func serverFDFor(local string) int { - ents, err := os.ReadDir("/proc/self/fd") - if err != nil { - return -1 - } - for _, ent := range ents { - fd, err := strconv.Atoi(ent.Name()) - if err != nil { - continue - } - sa, err := unix.Getpeername(fd) - if err != nil { - continue - } - if sockaddrString(sa) == local { - return fd - } - } - return -1 -} - // TestPendingReleaseEntryStaysTwentyFourBytes pins that the celeris#685 hold // (holdsFD, fd) fits in the padding after detached: the queue is appended to // on every close. diff --git a/engine/iouring/fd_probe_linux_test.go b/engine/iouring/fd_probe_linux_test.go new file mode 100644 index 00000000..ce69d0eb --- /dev/null +++ b/engine/iouring/fd_probe_linux_test.go @@ -0,0 +1,48 @@ +//go:build linux + +package iouring + +import ( + "os" + "strconv" + + "golang.org/x/sys/unix" +) + +// Descriptor probes shared by the celeris#685 / celeris#715 tests (the +// fd-lifetime tests in every build, the recv-theft trials under +// -tags=validation). They read /proc/self/fd, so they see this process's +// descriptors only. + +// fdTarget returns what /proc/self/fd/ names ("socket:[inode]" for a +// socket), or "" when fd is not open. +func fdTarget(fd int) string { + s, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(fd)) + if err != nil { + return "" + } + return s +} + +// serverFDFor returns this process's descriptor whose peer is local (the +// server side of a loopback connection the test dialed), or -1. +func serverFDFor(local string) int { + ents, err := os.ReadDir("/proc/self/fd") + if err != nil { + return -1 + } + for _, ent := range ents { + fd, err := strconv.Atoi(ent.Name()) + if err != nil { + continue + } + sa, err := unix.Getpeername(fd) + if err != nil { + continue + } + if sockaddrString(sa) == local { + return fd + } + } + return -1 +} From e48adee5c6311f5424628aaf20810063e5b097a7 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 05:53:29 +0200 Subject: [PATCH 08/12] test(iouring): a sibling accept of a hole below A's number is not a hit; judge the detector control by result lines (celeris#685) Round 2 of the #793 review. The #715 and linked trials counted ANY sibling accept while the closer was parked as a hit. When a free number lay below A's, the sibling accepted B there, A's released number was never tested, and the trial passed a tree without the fix (1 of 101 trials, b_fd=13 under fd=27). - theftHit: a sibling accept tests the theft only when it was given A's number, or when A's number still named A's socket at the hold (held_by_a: the socket's peer is A's local address). Any other sibling accept filled a hole: it is skipped and the next candidate dialed. judge fails a hit that tests nothing. - TestRecvTheft715ArmAHole and TestRecvTheft685LinkedHole close an undialed candidate, below A's number, once the closer is parked, so the hole is there in every trial. - Where the natural holes came from: a candidate a missed attempt left in the closer's accept queue is accepted once the closer is released, and closed after the next attempt filled the holes. Each attempt now starts when the engine has accepted every connection the trial dialed and holds none (waitEngineQuiet), not only when it holds none. - The CI detector control judges each mutant run by its result line (hit=true reused=true stolen=true; the Coop arm op_owed=+1 hijacker_read=false), not by its --- FAIL line, which an INCONCLUSIVE run or a setup error also produces. Both hole arms join the judged set and the trials step. --- .github/scripts/mutant-685-release-owed-fd.py | 22 ++- .github/workflows/ci.yml | 63 ++++-- engine/iouring/fd_probe_linux_test.go | 15 +- .../recv_theft_685_linked_linux_test.go | 72 +++++-- engine/iouring/recv_theft_715_linux_test.go | 187 +++++++++++++----- 5 files changed, 259 insertions(+), 100 deletions(-) diff --git a/.github/scripts/mutant-685-release-owed-fd.py b/.github/scripts/mutant-685-release-owed-fd.py index 78ea2c56..a4ec0b30 100755 --- a/.github/scripts/mutant-685-release-owed-fd.py +++ b/.github/scripts/mutant-685-release-owed-fd.py @@ -8,13 +8,21 @@ handing the socket over, and worker shutdown no longer ends the owed ops before closing descriptors. -Against the mutated tree the three trials that judge the rule MUST fail in -every run: TestRecvTheft715ArmA (a recv still in the SQ ring at the close), -TestRecvTheft685Linked (a recv linked behind a SEND, not issued at the -close) and TestRecvTheft685HijackMultishotCoop (a multishot recv owed at a -Hijack on a ring without DEFER_TASKRUN). If one passes, it is not watching -the path it claims to judge, and its green run on the tree as committed -proves nothing. +Against the mutated tree every run of the five trials that judge the rule +MUST detect the theft, as its result line reports it (the CI step reads +that line, not the --- line, so a failure for another reason does not +count): TestRecvTheft715ArmA and its hole twin TestRecvTheft715ArmAHole (a +recv still in the SQ ring at the close), TestRecvTheft685Linked and +TestRecvTheft685LinkedHole (a recv linked behind a SEND, not issued at the +close), each with hit=true reused=true stolen=true, and +TestRecvTheft685HijackMultishotCoop (a multishot recv owed at a Hijack on a +ring without DEFER_TASKRUN) with hijacker_read=false. If one does not, it +is not watching the path it claims to judge, and its green run on the tree +as committed proves nothing. + +The mutant also takes the rule off worker shutdown (endOwedOpsAtShutdown +ends only the ops fdOwed reports), but no trial judges that half: it is +covered by construction, not by a detector. The function is matched EXACTLY; any drift makes this script exit 2 instead of silently mutating nothing. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 458bfd3c..d067a224 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1005,8 +1005,12 @@ jobs: # asserts that B is answered and that no stale recv read B's bytes. # On a tree that frees the number at the close, the sibling is given # it and the old recv reads B's request: both fail in every trial. - # TestRecvTheft715Control (the ring submitted before the close) and - # TestRecvTheft715ArmC (celeris#715 hypothesis (c)) must pass either way. + # Their *Hole twins open a free number under A's once the closer is + # parked: the sibling's accept of the hole tests nothing and is + # skipped, so they must fail the same way (a verdict that counted it + # passed a tree without the fix). TestRecvTheft715Control (the ring + # submitted before the close) and TestRecvTheft715ArmC (celeris#715 + # hypothesis (c)) must pass either way. # The three TestRecvTheft685Hijack* arms hold the worker right after a # Hijack and send the hijacker's first bytes meanwhile: with multishot # recv (owed across its request) on a ring without DEFER_TASKRUN, the @@ -1028,7 +1032,7 @@ jobs: set -o pipefail sudo prlimit --pid "$$" --memlock=unlimited:unlimited echo "memlock (KiB): $(ulimit -l)" - names='TestRecvTheft715ArmA|TestRecvTheft715Control|TestRecvTheft715ArmC|TestRecvTheft685Linked|TestRecvTheft685HijackSingleShot|TestRecvTheft685HijackMultishotDefer|TestRecvTheft685HijackMultishotCoop' + names='TestRecvTheft715ArmA|TestRecvTheft715ArmAHole|TestRecvTheft715Control|TestRecvTheft715ArmC|TestRecvTheft685Linked|TestRecvTheft685LinkedHole|TestRecvTheft685HijackSingleShot|TestRecvTheft685HijackMultishotDefer|TestRecvTheft685HijackMultishotCoop' runs=5 want=$(( $(tr '|' '\n' <<<"$names" | wc -l) * runs )) go test -tags=validation -race -count="$runs" -timeout=600s -v -run "^(${names})\$" \ @@ -1048,11 +1052,18 @@ jobs: fi # The detector control. .github/scripts/mutant-685-release-owed-fd.py # makes fdOwed report false, which takes the fd-lifetime rule off the - # close paths, Hijack and shutdown at once. Against that tree the three - # trials that judge the rule must FAIL in every one of their runs (a - # PASS means the trial does not watch the path it judges), and the - # others must still PASS. The mutation is undone before the step ends. - - name: celeris#685 detector control (the rule removed; the three judging trials must fail every run) + # close paths, Hijack and shutdown at once. Against that tree every run + # of the five trials that judge the rule must DETECT the theft, read + # from its result line, not from its --- line: a FAIL for any other + # reason (INCONCLUSIVE, a setup error, a race report) is not a + # detection. The close-path trials (ArmA, ArmAHole, Linked, LinkedHole) + # must report hit=true, reused=true and stolen=true: the sibling was + # given A's number and A's stale recv read B's request. The hijack + # trial (MultishotCoop) must report op_owed=+1 and hijacker_read=false. + # The trials that pass on any tree must still PASS. No trial judges the + # shutdown half of the rule (see the script). The mutation is undone + # before the step ends. + - name: celeris#685 detector control (the rule removed; every run of the five judging trials must detect the theft) if: ${{ !cancelled() }} shell: bash env: @@ -1062,27 +1073,37 @@ jobs: set -o pipefail sudo prlimit --pid "$$" --memlock=unlimited:unlimited python3 .github/scripts/mutant-685-release-owed-fd.py - judged='TestRecvTheft715ArmA|TestRecvTheft685Linked|TestRecvTheft685HijackMultishotCoop' others='TestRecvTheft715Control|TestRecvTheft685HijackSingleShot|TestRecvTheft685HijackMultishotDefer' runs=3 - go test -tags=validation -race -count="$runs" -timeout=600s -v -run "^(${judged}|${others})\$" \ - ./engine/iouring/ 2>&1 | tee /tmp/recvtheft685-mutant.log || true + log=/tmp/recvtheft685-mutant.log + go test -tags=validation -race -count="$runs" -timeout=600s -v \ + -run "^(TestRecvTheft715ArmA|TestRecvTheft715ArmAHole|TestRecvTheft685Linked|TestRecvTheft685LinkedHole|TestRecvTheft685HijackMultishotCoop|${others})\$" \ + ./engine/iouring/ 2>&1 | tee "$log" || true git checkout -- engine/iouring/fd_lifetime.go - grep -E 'RECVTHEFT(715|685) .*result' /tmp/recvtheft685-mutant.log || true + grep -E 'RECVTHEFT(715|685) .*result' "$log" || true bad=0 - for n in ${judged//|/ }; do - p=$(grep -cE "^--- PASS: $n \(" /tmp/recvtheft685-mutant.log || true) - f=$(grep -cE "^--- FAIL: $n \(" /tmp/recvtheft685-mutant.log || true) - echo "mutant, judged $n: PASS $p FAIL $f (want FAIL $runs)" - if [ "$f" -ne "$runs" ] || [ "$p" -ne 0 ]; then bad=1; fi - done + # judged + judged() { + local n=$1 line=$2 want=$3 lines det p + lines=$(grep -cE "$line" "$log" || true) + det=$(grep -E "$line" "$log" | grep -cE "$want" || true) + p=$(grep -cE "^--- PASS: $n \(" "$log" || true) + echo "mutant, judged $n: result lines $lines, detected $det, PASS $p (want $runs, $runs, 0)" + if [ "$lines" -ne "$runs" ] || [ "$det" -ne "$runs" ] || [ "$p" -ne 0 ]; then bad=1; fi + } + stolen=' hit=true .* reused=true .* stolen=true ' + judged TestRecvTheft715ArmA 'RECVTHEFT715 arm=A result ' "$stolen" + judged TestRecvTheft715ArmAHole 'RECVTHEFT715 arm=A-hole result ' "$stolen" + judged TestRecvTheft685Linked 'RECVTHEFT685 linked result ' "$stolen" + judged TestRecvTheft685LinkedHole 'RECVTHEFT685 linked-hole result ' "$stolen" + judged TestRecvTheft685HijackMultishotCoop 'RECVTHEFT685 hijack result tier=2 ' ' op_owed=\+1 .* hijacker_read=false ' for n in ${others//|/ }; do - p=$(grep -cE "^--- PASS: $n \(" /tmp/recvtheft685-mutant.log || true) - f=$(grep -cE "^--- FAIL: $n \(" /tmp/recvtheft685-mutant.log || true) + p=$(grep -cE "^--- PASS: $n \(" "$log" || true) + f=$(grep -cE "^--- FAIL: $n \(" "$log" || true) echo "mutant, control $n: PASS $p FAIL $f (want PASS $runs)" if [ "$p" -ne "$runs" ] || [ "$f" -ne 0 ]; then bad=1; fi done - skipped=$(grep -cE '^[[:space:]]*--- SKIP' /tmp/recvtheft685-mutant.log || true) + skipped=$(grep -cE '^[[:space:]]*--- SKIP' "$log" || true) if [ "$skipped" -ne 0 ]; then echo "a SKIP is not a result ($skipped SKIP lines)"; bad=1; fi exit "$bad" diff --git a/engine/iouring/fd_probe_linux_test.go b/engine/iouring/fd_probe_linux_test.go index ce69d0eb..00f29128 100644 --- a/engine/iouring/fd_probe_linux_test.go +++ b/engine/iouring/fd_probe_linux_test.go @@ -36,13 +36,18 @@ func serverFDFor(local string) int { if err != nil { continue } - sa, err := unix.Getpeername(fd) - if err != nil { - continue - } - if sockaddrString(sa) == local { + if namesPeer(fd, local) { return fd } } return -1 } + +// namesPeer reports whether fd is a socket whose peer is local: the server +// side of the loopback connection the test dialed from local. It tells a +// descriptor number still naming that connection's socket from the same +// number freed and given to another socket. +func namesPeer(fd int, local string) bool { + sa, err := unix.Getpeername(fd) + return err == nil && sockaddrString(sa) == local +} diff --git a/engine/iouring/recv_theft_685_linked_linux_test.go b/engine/iouring/recv_theft_685_linked_linux_test.go index 676e0141..dd6a82de 100644 --- a/engine/iouring/recv_theft_685_linked_linux_test.go +++ b/engine/iouring/recv_theft_685_linked_linux_test.go @@ -47,8 +47,11 @@ import ( // 4. A drains its socket. The SEND completes, and completeSend runs the // deferred close with the linked recv owed; W1 parks when the close path // returns (recvtheft.Options.LinkedRecv). -// 5. The test dials B's candidates. The sibling W2 accepts one, as N if the -// close released it (and W2 then parks before submitting B's recv). +// 5. The test dials B's candidates until the sibling W2's accept of one +// tests the theft (theftHit): one given N, because the close released it +// (W2 then parks before submitting B's recv), or any one while N is still +// A's socket. An accept given a lower free number while N is free is +// skipped. TestRecvTheft685LinkedHole opens such a hole on purpose. // 6. W1 is released (its next enter runs the queued task work, which issues // the linked recv), then W2. // @@ -169,7 +172,12 @@ type linkedTheftResult struct { bFD int reused bool heldOpen bool + heldByA bool released bool + hole int + missHole int + accepts0 uint64 + dialed int filled int witness uint64 staleClosed uint64 @@ -182,13 +190,8 @@ type linkedTheftResult struct { missCloserAccepts int } -func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *linkedTheftResult) bool { - for deadline := time.Now().Add(3 * time.Second); e.metrics.activeConns.Load() != 0; { - if time.Now().After(deadline) { - t.Fatalf("engine still holds %d connections from the previous attempt", e.metrics.activeConns.Load()) - } - time.Sleep(5 * time.Millisecond) - } +func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm string, hole bool, r *linkedTheftResult) bool { + waitEngineQuiet(t, e, r.accepts0, r.dialed) fillFDHoles(t) var cands []int defer func() { @@ -219,6 +222,7 @@ func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *link if err := unix.Connect(a, sa); err != nil { t.Fatalf("connect A: %v", err) } + r.dialed++ if _, err := unix.Write(a, []byte(linkedReqHead)); err != nil { t.Fatalf("write A head: %v", err) } @@ -272,12 +276,23 @@ func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *link r.missNoClose++ <-drained outqAfter, _ := unix.IoctlGetInt(srv, unix.SIOCOUTQ) - t.Logf("RECVTHEFT685 linked attempt=%d miss=no-close-hold filled=%d outq_before_drain=%d outq_after=%d srv_after=%q closes=+%d witness=+%d", - r.attempts, r.filled, outqBefore, outqAfter, fdTarget(srv), e.metrics.closeCount.Load()-closes0, recvtheft.CloseWithLinkedRecv()-witness0) + t.Logf("RECVTHEFT685 %s attempt=%d miss=no-close-hold filled=%d outq_before_drain=%d outq_after=%d srv_after=%q closes=+%d witness=+%d", + arm, r.attempts, r.filled, outqBefore, outqAfter, fdTarget(srv), e.metrics.closeCount.Load()-closes0, recvtheft.CloseWithLinkedRecv()-witness0) return false } aTarget := fdTarget(ce.FD) r.heldOpen = strings.HasPrefix(aTarget, "socket:") + r.heldByA = r.heldOpen && namesPeer(ce.FD, aLocal) + if hole { + // A hole under N, as in TestRecvTheft715ArmAHole: the last candidate, + // never dialed, was allocated before A's server-side descriptor. + r.hole = cands[len(cands)-1] + cands = cands[:len(cands)-1] + if r.hole > ce.FD { + t.Fatalf("the hole candidate's number %d is not below A's %d", r.hole, ce.FD) + } + _ = unix.Close(r.hole) + } var hit *recvtheft.AcceptEvent var hitFD int @@ -285,6 +300,7 @@ func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *link if err := unix.Connect(fd, sa); err != nil { t.Fatalf("connect candidate %d: %v", i, err) } + r.dialed++ if _, err := unix.Write(fd, []byte(theftRequestB)); err != nil { t.Fatalf("write candidate %d: %v", i, err) } @@ -297,6 +313,11 @@ func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *link r.missCloserAccepts++ continue } + if !theftHit(ev.FD, ce.FD, r.heldByA) { + r.missHole++ + t.Logf("RECVTHEFT685 %s attempt=%d candidate=%d accepted by the sibling %d as fd %d, a hole below A's released %d: skipped", arm, r.attempts, i, ev.Worker, ev.FD, ce.FD) + continue + } ev0 := ev if ev.FD == ce.FD { held, ok := tr.WaitAcceptHeld(time.Second) @@ -310,8 +331,8 @@ func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *link break } if hit == nil { - t.Logf("RECVTHEFT685 linked attempt=%d miss=no-sibling-accept fd=%d closer=%d queued=%d closer_accepts=%d", - r.attempts, ce.FD, ce.Worker, r.missQueued, r.missCloserAccepts) + t.Logf("RECVTHEFT685 %s attempt=%d miss=no-sibling-accept fd=%d closer=%d held_by_a=%v queued=%d closer_accepts=%d hole_accepts=%d", + arm, r.attempts, ce.FD, ce.Worker, r.heldByA, r.missQueued, r.missCloserAccepts, r.missHole) tr.Disarm() <-drained return false @@ -350,14 +371,22 @@ func linkedTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, r *link // TestRecvTheft685Linked is the linked form's trial (see above). One trial // per run; tally the --- lines of -count=N. -func TestRecvTheft685Linked(t *testing.T) { +func TestRecvTheft685Linked(t *testing.T) { runLinkedTheft(t, "linked", false) } + +// TestRecvTheft685LinkedHole is the linked trial with a hole opened under A's +// number once the closer is parked, as TestRecvTheft715ArmAHole does for arm +// A: on a tree that releases the number at the close it must fail exactly as +// the linked trial does. +func TestRecvTheft685LinkedHole(t *testing.T) { runLinkedTheft(t, "linked-hole", true) } + +func runLinkedTheft(t *testing.T, arm string, hole bool) { requireRecvTheft715(t) e, port := startLinkedTheftEngine(t) sa := &unix.SockaddrInet4{Port: port, Addr: [4]byte{127, 0, 0, 1}} - var r linkedTheftResult + r := linkedTheftResult{hole: -1, accepts0: e.metrics.acceptCount.Load()} for r.attempts < theftMaxAttempts { r.attempts++ - if linkedTheftAttempt(t, e, sa, &r) { + if linkedTheftAttempt(t, e, sa, arm, hole, &r) { break } } @@ -369,11 +398,14 @@ func TestRecvTheft685Linked(t *testing.T) { if r.answerErr != nil { errText = r.answerErr.Error() } - t.Logf("RECVTHEFT685 linked result attempts=%d hit=%v fd=%d closer=%d sibling=%d b_fd=%d reused=%v held_open=%v released=%v filled=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d closer_accepts=%d}", - r.attempts, r.hit, r.fd, r.closer, r.sibl, r.bFD, r.reused, r.heldOpen, r.released, r.filled, r.witness, r.staleClosed, - r.stolen, r.answered, errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missCloserAccepts) + t.Logf("RECVTHEFT685 %s result attempts=%d hit=%v fd=%d closer=%d sibling=%d b_fd=%d reused=%v held_open=%v held_by_a=%v released=%v hole=%d filled=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d closer_accepts=%d hole_accepts=%d}", + arm, r.attempts, r.hit, r.fd, r.closer, r.sibl, r.bFD, r.reused, r.heldOpen, r.heldByA, r.released, r.hole, r.filled, r.witness, r.staleClosed, + r.stolen, r.answered, errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missCloserAccepts, r.missHole) if !r.hit { - skipOrFail656(t, "INCONCLUSIVE: no sibling accept while the closer was parked, in %d attempts", r.attempts) + skipOrFail656(t, "INCONCLUSIVE: no sibling accept that tests the theft while the closer was parked, in %d attempts", r.attempts) + } + if !r.reused && !r.heldByA { + t.Fatalf("a hit that tests nothing: B was given %d, A's number %d was neither reused nor kept as A's socket (see theftHit)", r.bFD, r.fd) } if r.witness == 0 { t.Fatalf("the close hold fired but close_with_linked_recv did not move") diff --git a/engine/iouring/recv_theft_715_linux_test.go b/engine/iouring/recv_theft_715_linux_test.go index dff855a6..e016879d 100644 --- a/engine/iouring/recv_theft_715_linux_test.go +++ b/engine/iouring/recv_theft_715_linux_test.go @@ -62,6 +62,13 @@ import ( // - control (TestRecvTheft715Control): the same trial with // recvtheft.Options.SubmitBeforeClose, the close paths submitting before // they close (the fix direction). Asserts the same property. +// - A with a hole (TestRecvTheft715ArmAHole): arm A with one of B's +// candidate sockets, whose number is below A's, closed once the closer is +// parked, so a free number lies under the one A's close frees. The +// sibling's first accept is given the hole; it tests nothing (see +// theftHit) and is skipped, and the next one is given A's number if the +// close released it. Pins that the verdict cannot pass a tree without +// the fix because a hole took B (celeris#793 review round 2). // - C (TestRecvTheft715ArmC): hypothesis (c), the promoted connection's // hand-off to its dispatch goroutine, with the window between the // worker's asyncInMu unlock and its Signal / goroutine start widened @@ -179,6 +186,34 @@ func fillFDHoles(t *testing.T) { }) } +// waitEngineQuiet waits, before an attempt fills the descriptor holes, until +// the engine has accepted every connection the trial has dialed (dialed, since +// acceptCount read accepts0) and holds none of them. A candidate that a missed +// attempt left in the closer's accept queue is accepted only once the closer +// is released; its close after the fill opened a hole under the next +// attempt's number, and the holes cascaded into further misses (the round-2 +// smoke on the negative control: 4 such accepts skipped in one attempt). +// Holes cannot make a trial pass (theftHit), so a late accept past the +// deadline is logged, not fatal; a connection still open is fatal as before. +func waitEngineQuiet(t *testing.T, e *Engine, accepts0 uint64, dialed int) { + t.Helper() + for deadline := time.Now().Add(3 * time.Second); ; { + acc := e.metrics.acceptCount.Load() - accepts0 + act := e.metrics.activeConns.Load() + if acc >= uint64(dialed) && act == 0 { + return + } + if time.Now().After(deadline) { + if act != 0 { + t.Fatalf("engine still holds %d connections from the previous attempt", act) + } + t.Logf("RECVTHEFT the engine accepted %d of the %d connections dialed so far; going on", acc, dialed) + return + } + time.Sleep(5 * time.Millisecond) + } +} + // readHead reads from a blocking socket until the response head is complete, // the peer closes, or d passes. func readHead(fd int, d time.Duration) (string, error) { @@ -206,19 +241,22 @@ func readHead(fd int, d time.Duration) (string, error) { return string(got), fmt.Errorf("no response head within %v", d) } -// theftResult is one trial of arm A or the control. +// theftResult is one trial of arm A, A with a hole, or the control. // -// A hit is the sibling worker accepting one of B's candidates while the -// closer is parked after its close. On a tree that frees the number at the -// close, that accept is given the number (it is the lowest free one), which -// is the theft's precondition: reused is true, and the sibling is parked too. -// On a tree that keeps the number allocated until the owed recv has ended -// (celeris#685), the accept is given another number: reused is false, and B -// is served with no hold at all. heldOpen and released read the closer's -// descriptor through /proc/self/fd: whether the number still named A's -// socket while the closer was parked, and whether it was released after -// (closed, or reused by something else) within theftReleaseWait of the -// closer's release, so a kept descriptor is not a leak. +// A hit is a sibling accept of one of B's candidates, while the closer is +// parked after its close, that tests the theft (theftHit). On a tree that +// frees the number at the close, that is the accept given the number: reused +// is true, and the sibling is parked too, with B's recv prepared. On a tree +// that keeps the number allocated until the owed recv has ended +// (celeris#685), heldByA is true, the accept is given another number (reused +// is false), and B is served with no hold at all. A sibling accept given +// another number while A's number was free filled a lower hole: it is +// skipped (missHole) and the next candidate is dialed. heldOpen, heldByA and +// released read the closer's descriptor: whether the number still named a +// socket while the closer was parked, whether that socket was A's (its peer +// is A's local address), and whether it was released after (closed, or +// reused by something else) within theftReleaseWait of the closer's release, +// so a kept descriptor is not a leak. type theftResult struct { attempts int hit bool @@ -227,7 +265,12 @@ type theftResult struct { bFD int reused bool heldOpen bool + heldByA bool released bool + hole int + missHole int + accepts0 uint64 + dialed int gate bool witness uint64 staleClosed uint64 @@ -254,33 +297,47 @@ const ( theftRequestB = "GET /b HTTP/1.1\r\nHost: recv-theft-715\r\n\r\n" ) -// runRecvTheftTrial drives attempts until the sibling worker accepts a fresh -// connection B as the number A's held close released (a hit), then releases -// the closer, then the sibling, and reports what happened to B's request. -func runRecvTheftTrial(t *testing.T, arm string, submitBeforeClose bool) theftResult { +// theftArm is one arm of the trial: its name in the result line, whether the +// close paths submit before they close (the control), and whether a hole is +// opened under A's number once the closer is parked (A with a hole). +type theftArm struct { + name string + submitBeforeClose bool + hole bool +} + +// theftHit reports whether the sibling's accept of a candidate as number +// accepted tests the theft of A's number n. It does when the accept was given +// n (the close released it, and B's recv is the one a stale recv can rob), or +// when n was still A's socket while the closer was parked (the close kept it, +// so no accept could be given it, and B must be served). An accept given +// another number while n was free tests nothing: it filled a hole below n, +// which is still free, so B is served with no theft possible on any tree. +// Counting it as a hit passed a tree without the fix (celeris#793 review +// round 2: 1 of 101 trials, b_fd=13 under fd=27). The caller skips it and +// dials the next candidate, whose accept can be given n. +func theftHit(accepted, n int, heldByA bool) bool { + return accepted == n || heldByA +} + +// runRecvTheftTrial drives attempts until the sibling worker's accept of a +// fresh connection B tests the theft (theftHit), then releases the closer, +// then the sibling, and reports what happened to B's request. +func runRecvTheftTrial(t *testing.T, arm theftArm) theftResult { e, port := startRecvTheftEngine(t) sa := &unix.SockaddrInet4{Port: port, Addr: [4]byte{127, 0, 0, 1}} - var r theftResult + r := theftResult{hole: -1, accepts0: e.metrics.acceptCount.Load()} for r.attempts < theftMaxAttempts { r.attempts++ - if done := recvTheftAttempt(t, e, sa, arm, submitBeforeClose, &r); done { + if done := recvTheftAttempt(t, e, sa, arm, &r); done { return r } } return r } -func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm string, submitBeforeClose bool, r *theftResult) bool { - // The previous attempt's connections are closed by the engine as their - // clients go; a server-side close after the fill below would open a hole - // under the number this attempt frees, and the sibling's accept would take - // the hole. So start from an engine with no connection. - for deadline := time.Now().Add(3 * time.Second); e.metrics.activeConns.Load() != 0; { - if time.Now().After(deadline) { - t.Fatalf("engine still holds %d connections from the previous attempt", e.metrics.activeConns.Load()) - } - time.Sleep(5 * time.Millisecond) - } +func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm theftArm, r *theftResult) bool { + waitEngineQuiet(t, e, r.accepts0, r.dialed) fillFDHoles(t) // B's candidate sockets exist before A is accepted, so they hold numbers // below A's and none of them can take the number A's close frees. @@ -301,7 +358,7 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin witness0 := recvtheft.CloseWithUnsubmittedRecv() stale0 := e.metrics.handoffLoss.staleRecvDataClosed.Load() tr := recvtheft.Arm(recvtheft.Options{ - SubmitBeforeClose: submitBeforeClose, + SubmitBeforeClose: arm.submitBeforeClose, PromoteGate: 2 * time.Second, HoldMax: 10 * time.Second, }) @@ -311,6 +368,7 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin if err != nil { t.Fatalf("dial A: %v", err) } + r.dialed++ defer func() { _ = a.Close() }() if _, err := a.Write([]byte(theftRequestAHead)); err != nil { t.Fatalf("write A: %v", err) @@ -318,28 +376,44 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin ce, ok := tr.WaitClose(3 * time.Second) if !ok { r.missNoClose++ - t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-close-hold gate=%v witness=+%d", arm, r.attempts, tr.GateUsed(), + t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-close-hold gate=%v witness=+%d", arm.name, r.attempts, tr.GateUsed(), recvtheft.CloseWithUnsubmittedRecv()-witness0) return false } // W1 is parked right after its close path, with A's recv unsubmitted. - // Did that close release N? Read it before any candidate is dialed, while - // nothing else can take the number (every hole below it is filled). + // Did that close release N? Read it before any candidate is dialed: the + // number still names A's socket (its peer is A's local address), or it + // was released. aTarget := fdTarget(ce.FD) r.heldOpen = strings.HasPrefix(aTarget, "socket:") + r.heldByA = r.heldOpen && namesPeer(ce.FD, a.LocalAddr().String()) + if arm.hole { + // Open a hole under N: close the last candidate, never dialed, whose + // number was allocated before A's. + r.hole = cands[len(cands)-1] + cands = cands[:len(cands)-1] + if r.hole > ce.FD { + t.Fatalf("the hole candidate's number %d is not below A's %d", r.hole, ce.FD) + } + _ = unix.Close(r.hole) + } - // Dial B candidates until the sibling accepts one. One that hashes to - // W1's listener waits in W1's accept queue. The sibling's accept is given - // the lowest free number: N if the close released it, and then the - // sibling is parked too (its recv for B prepared, not submitted); another - // number if N is still A's, and then B is served with no hold. + // Dial B candidates until the sibling's accept of one tests the theft + // (theftHit). One that hashes to W1's listener waits in W1's accept + // queue. The sibling's accept is given the lowest free number: N if the + // close released N and no hole lies below it, and then the sibling is + // parked too (its recv for B prepared, not submitted); a hole below N if + // there is one, and then that B is served, N is still free, and the next + // candidate is dialed; another number if N is still A's, and then B is + // served with no hold. var hit *recvtheft.AcceptEvent var hitFD int for i, fd := range cands { if err := unix.Connect(fd, sa); err != nil { t.Fatalf("connect candidate %d: %v", i, err) } + r.dialed++ if _, err := unix.Write(fd, []byte(theftRequestB)); err != nil { t.Fatalf("write candidate %d: %v", i, err) } @@ -350,7 +424,12 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin } if ev.Worker == ce.Worker { r.missOtherFD++ - t.Logf("RECVTHEFT715 arm=%s attempt=%d candidate=%d accepted by the closer %d as fd %d (target %d)", arm, r.attempts, i, ev.Worker, ev.FD, ce.FD) + t.Logf("RECVTHEFT715 arm=%s attempt=%d candidate=%d accepted by the closer %d as fd %d (target %d)", arm.name, r.attempts, i, ev.Worker, ev.FD, ce.FD) + continue + } + if !theftHit(ev.FD, ce.FD, r.heldByA) { + r.missHole++ + t.Logf("RECVTHEFT715 arm=%s attempt=%d candidate=%d accepted by the sibling %d as fd %d, a hole below A's released %d: skipped", arm.name, r.attempts, i, ev.Worker, ev.FD, ce.FD) continue } ev0 := ev @@ -367,8 +446,8 @@ func recvTheftAttempt(t *testing.T, e *Engine, sa *unix.SockaddrInet4, arm strin break } if hit == nil { - t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-sibling-accept fd=%d closer=%d queued=%d closer_accepts=%d", - arm, r.attempts, ce.FD, ce.Worker, r.missQueued, r.missOtherFD) + t.Logf("RECVTHEFT715 arm=%s attempt=%d miss=no-sibling-accept fd=%d closer=%d held_by_a=%v queued=%d closer_accepts=%d hole_accepts=%d", + arm.name, r.attempts, ce.FD, ce.Worker, r.heldByA, r.missQueued, r.missOtherFD, r.missHole) return false } r.hit, r.fd, r.closer, r.sibl, r.bFD, r.gate = true, ce.FD, ce.Worker, hit.Worker, hit.FD, tr.GateUsed() @@ -417,16 +496,19 @@ func logTheftResult(t *testing.T, arm string, r theftResult) { if r.answerErr != nil { errText = r.answerErr.Error() } - t.Logf("RECVTHEFT715 arm=%s result attempts=%d hit=%v fd=%d closer=%d sibling=%d b_fd=%d reused=%v held_open=%v released=%v gate=%v candidates=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d closer_accepts=%d}", - arm, r.attempts, r.hit, r.fd, r.closer, r.sibl, r.bFD, r.reused, r.heldOpen, r.released, r.gate, r.candidatesHit, r.witness, r.staleClosed, r.stolen, r.answered, - errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missOtherFD) + t.Logf("RECVTHEFT715 arm=%s result attempts=%d hit=%v fd=%d closer=%d sibling=%d b_fd=%d reused=%v held_open=%v held_by_a=%v released=%v hole=%d gate=%v candidates=%d witness=+%d stale_recv_data_closed=+%d stolen=%v answered=%v answer_err=%q stale=[%s] misses{no_close=%d queued=%d closer_accepts=%d hole_accepts=%d}", + arm, r.attempts, r.hit, r.fd, r.closer, r.sibl, r.bFD, r.reused, r.heldOpen, r.heldByA, r.released, r.hole, r.gate, r.candidatesHit, r.witness, r.staleClosed, r.stolen, r.answered, + errText, strings.Join(heads, " "), r.missNoClose, r.missQueued, r.missOtherFD, r.missHole) } func judgeTheftTrial(t *testing.T, arm string, r theftResult) { t.Helper() logTheftResult(t, arm, r) if !r.hit { - skipOrFail656(t, "INCONCLUSIVE: no sibling accept while the closer was parked, in %d attempts", r.attempts) + skipOrFail656(t, "INCONCLUSIVE: no sibling accept that tests the theft while the closer was parked, in %d attempts", r.attempts) + } + if !r.reused && !r.heldByA { + t.Fatalf("a hit that tests nothing: B was given %d, A's number %d was neither reused nor kept as A's socket (see theftHit)", r.bFD, r.fd) } if r.witness == 0 { t.Fatalf("the close hold fired but close_with_unsubmitted_recv did not move") @@ -445,7 +527,18 @@ func judgeTheftTrial(t *testing.T, arm string, r theftResult) { // design when (a) holds: B's request read by A's unsubmitted recv. func TestRecvTheft715ArmA(t *testing.T) { requireRecvTheft715(t) - judgeTheftTrial(t, "A", runRecvTheftTrial(t, "A", false)) + judgeTheftTrial(t, "A", runRecvTheftTrial(t, theftArm{name: "A"})) +} + +// TestRecvTheft715ArmAHole is arm A with a hole opened under A's number once +// the closer is parked (see theftArm). On a tree that releases the number at +// the close the sibling's first accept fills the hole and is skipped, and the +// next is given A's number: it must fail exactly as arm A does. A verdict that +// counted the hole's accept as a hit passed that tree (celeris#793 review +// round 2). +func TestRecvTheft715ArmAHole(t *testing.T) { + requireRecvTheft715(t) + judgeTheftTrial(t, "A-hole", runRecvTheftTrial(t, theftArm{name: "A-hole", hole: true})) } // TestRecvTheft715Control is arm A's trial with the close paths submitting the @@ -453,7 +546,7 @@ func TestRecvTheft715ArmA(t *testing.T) { // issued while N still names A's socket. Predicted: B answered, nothing stolen. func TestRecvTheft715Control(t *testing.T) { requireRecvTheft715(t) - judgeTheftTrial(t, "control", runRecvTheftTrial(t, "control", true)) + judgeTheftTrial(t, "control", runRecvTheftTrial(t, theftArm{name: "control", submitBeforeClose: true})) } // TestRecvTheft715ArmC is hypothesis (c): a request the promoted connection's From 0f36096c4666d9ec71e76191fbb1f8e6b219e73c Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 07:54:48 +0200 Subject: [PATCH 09/12] test(iouring): a pending SEND_ZC notification must not hold a closed connection's descriptor (celeris#798) Failing first. fdOwed counts kernelInflight, which keeps a SEND_ZC until its notification CQE. The notification names no descriptor: the send has been issued and has completed. But a peer that stops reading keeps the unsent part of the send queued, and the notification with it, for as long as the socket is open, and a kept descriptor keeps the socket open. So a close whose only owed op was that notification held its descriptor until the 5 s release backstop forced it and counted CloseFDForced (#798 item 1). TestCloseReleasesDescriptorWithOnlyAZCNotificationOwed drives a synthetic worker with a real ring and SEND_ZC on as the engine turns it on (the probe, then CELERIS_IOURING_SEND_ZC=on), a 64 KiB SEND_ZC to a loopback peer with a 4 KiB receive buffer that reads nothing, and the closing-drain sweep's close (closeConn defers, then finishCloseAny), in three cases: the notification alone owed; a recv armed behind the send too; the send's first CQE still unread at the close. Each must close the descriptor within 200 ms of the close, with CloseFDForced 0, and must keep the connState, whose sendBuf the kernel may still read, queued until the notification arrives once the peer reads, and release it then. TestShutdownDoesNotWaitForAZCNotification: worker shutdown's drain must not wait for the notification either (notification owed, or the send's first CQE still in the ring when the drain starts): within 100 ms, not its 250 ms bound. --- engine/iouring/fd_lifetime_close_test.go | 354 +++++++++++++++++++++++ 1 file changed, 354 insertions(+) diff --git a/engine/iouring/fd_lifetime_close_test.go b/engine/iouring/fd_lifetime_close_test.go index cca03ad9..9032ddbd 100644 --- a/engine/iouring/fd_lifetime_close_test.go +++ b/engine/iouring/fd_lifetime_close_test.go @@ -8,6 +8,7 @@ import ( "io" "log/slog" "net" + "runtime" "strings" "sync" "sync/atomic" @@ -306,3 +307,356 @@ func TestShutdownEndsOwedOpsBeforeClosing(t *testing.T) { t.Fatalf("CloseFDForced = %d, want 0", m.CloseFDForced) } } + +// The SEND_ZC half of the rule (celeris#798). A SEND_ZC completes in two +// CQEs: the send's result (IORING_CQE_F_MORE), then a notification once the +// kernel has let go of the send buffer's pages. The notification names no +// descriptor, so no op can resolve the number through it; it guards the send +// buffer, which the connState holds. But a peer that stops reading keeps the +// unsent part of the send queued, and the notification with it, for as long +// as the socket is open, and keeping the descriptor keeps the socket open. A +// close that counted the notification as an op owed on the descriptor kept +// it until the 5 s release backstop forced it (CloseFDForced, which must +// stay 0), and worker shutdown waited out its whole drain bound for it. + +// zcReleaseBound is how soon after the close the descriptor must be closed +// when all that holds the connection is a SEND_ZC notification and ops that +// end at the close's cancel and shutdown. The rule closes it at the close +// itself when no owed op names it, and otherwise at the drainPendingRelease +// of the first loop pass that reads the last naming op's terminal CQE (a +// cancelled recv) or the send's first CQE: one pass, and a pass here waits +// at most 10 ms for completions. 200 ms leaves 20 passes of headroom for +// -race on a 4-CPU container, and is 25 times shorter than the 5 s backstop +// (pendingReleaseHoldNanos) the descriptor was held to. +const zcReleaseBound = 200 * time.Millisecond + +// zcShutdownBound is the same for worker shutdown's drain: with only a +// notification owed it does not drain at all, and with the send's first CQE +// already in the ring it stops at the first pass, which returns at once. +// Held for the notification, the drain ran its whole 250 ms bound +// (shutdownFDDrainNanos); 100 ms sits well between the two. +const zcShutdownBound = 100 * time.Millisecond + +// zcPayload is the send: 64 KiB, far over sendZCMinBytes (so it goes out as +// SEND_ZC) and over what a peer with a 4 KiB receive buffer can take, so what +// the send queued past the peer's window stays in the server's send queue and +// holds the notification. Small enough that the pages SEND_ZC charges to +// RLIMIT_MEMLOCK fit the CI unit job's 8 MiB next to the test ring. +const zcPayload = 64 << 10 + +// newZCCloseWorker is newOwedCloseWorker over loopback TCP, with SEND_ZC on +// as the engine turns it on: the startup probe must find it functional, and +// CELERIS_IOURING_SEND_ZC=on then enables it (resolveSendZCPolicy); a kernel +// where the engine never sends zero-copy cannot reach the case. The peer has +// a 4 KiB receive buffer, set before the connect so that the window it +// advertises is small from the first segment, and reads nothing until +// drainZCPeer. +func newZCCloseWorker(t *testing.T) (*Worker, *connState, int) { + t.Helper() + res, reason := probeSendZCCached() + if on, _ := resolveSendZCPolicy(res == SendZCTrueZeroCopy || res == SendZCCopyFallback, "on"); !on { + t.Skipf("the engine does not turn SEND_ZC on here (probe: %v %s)", res, reason) + } + ring := newTestRing(t) + lfd, err := unix.Socket(unix.AF_INET, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) + if err != nil { + t.Fatalf("listen socket: %v", err) + } + defer func() { _ = unix.Close(lfd) }() + if err := unix.Bind(lfd, &unix.SockaddrInet4{Addr: [4]byte{127, 0, 0, 1}}); err != nil { + t.Fatalf("bind: %v", err) + } + if err := unix.Listen(lfd, 1); err != nil { + t.Fatalf("listen: %v", err) + } + sa, err := unix.Getsockname(lfd) + if err != nil { + t.Fatalf("getsockname: %v", err) + } + peer, err := unix.Socket(unix.AF_INET, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) + if err != nil { + t.Fatalf("peer socket: %v", err) + } + if err := unix.SetsockoptInt(peer, unix.SOL_SOCKET, unix.SO_RCVBUF, 4096); err != nil { + _ = unix.Close(peer) + t.Fatalf("SO_RCVBUF: %v", err) + } + if err := unix.Connect(peer, sa); err != nil { + _ = unix.Close(peer) + t.Fatalf("connect: %v", err) + } + local, _, err := unix.Accept4(lfd, unix.SOCK_NONBLOCK|unix.SOCK_CLOEXEC) + if err != nil { + _ = unix.Close(peer) + t.Fatalf("accept: %v", err) + } + localTarget := fdTarget(local) + t.Cleanup(func() { + _ = unix.Close(peer) + if fdTarget(local) == localTarget { + _ = unix.Close(local) + } + }) + w := &Worker{ + ring: ring, + conns: make([]*connState, local+1), + liveConns: make([]int, 0, 4), + errs: &errclass.Counters{}, + activeConns: &atomic.Int64{}, + closeCount: &atomic.Uint64{}, + recvArm: &recvArmStats{}, + handoffLoss: &handoffLossStats{}, + sendZC: true, + } + w.cachedNow = time.Now().UnixNano() + cs := &connState{ + fd: local, + liveIdx: -1, + generation: 13, + buf: make([]byte, 4096), + h1State: conn.NewH1State(), + detected: true, + } + cs.protocol.Store(int32(engine.HTTP1)) + w.conns[local] = cs + w.addLiveConn(cs) + w.connCount = 1 + w.activeConns.Add(1) + return w, cs, peer +} + +// startZCSend places one SEND_ZC of zcPayload bytes on cs, submits it, and +// returns what it sent once its first CQE (the send's result, F_MORE) is in +// the completion ring. With reap it then dispatches that CQE as the loop does +// (staleConnCQE, then handleSend, which records zcNotifPending); without, it +// leaves it there unread, as a close that runs before the loop reads it finds +// it. Either way the notification must still be owed 100 ms later, with the +// peer reading nothing, or the case did not form. +func startZCSend(t *testing.T, w *Worker, cs *connState, reap bool) int32 { + t.Helper() + payload := make([]byte, zcPayload) + for i := range payload { + payload[i] = byte(i) + } + cs.writeBuf = payload + if w.flushSend(cs) || !cs.sendIsZC || cs.kernelInflight != 1 { + t.Fatalf("no SEND_ZC placed: sendIsZC=%v kernelInflight=%d", cs.sendIsZC, cs.kernelInflight) + } + if _, err := w.ring.Submit(); err != nil { + t.Fatalf("submit: %v", err) + } + var c *completionEntry + for end := time.Now().Add(2 * time.Second); c == nil; { + if head, tail := w.ring.BeginCQ(); head != tail { + c = w.ring.cqeAt(head) + break + } + if time.Now().After(end) { + t.Fatal("no completion for the SEND_ZC within 2s") + } + time.Sleep(time.Millisecond) + } + if c.UserData&udMask != udSend || !cqeHasMore(c.Flags) || c.Res <= 0 { + t.Fatalf("first completion ud=%#x flags=%#x res=%d, want the SEND_ZC's result (F_MORE, res > 0)", c.UserData, c.Flags, c.Res) + } + sent := c.Res + if reap { + head, _ := w.ring.BeginCQ() + if !w.staleConnCQE(c, cs.fd, c.UserData) { + w.handleSend(c, cs.fd, time.Now().UnixNano()) + } + w.ring.EndCQ(head + 1) + if !cs.zcNotifPending || !cs.sending || cs.kernelInflight != 1 { + t.Fatalf("after the first CQE: zcNotifPending=%v sending=%v kernelInflight=%d, want true true 1", cs.zcNotifPending, cs.sending, cs.kernelInflight) + } + } + time.Sleep(100 * time.Millisecond) + want := uint32(1) + if reap { + want = 0 + } + if head, tail := w.ring.BeginCQ(); tail-head != want { + t.Fatalf("%d completions in the ring 100 ms after the send, want %d: the notification arrived with the peer "+ + "reading nothing (sent %d of %d), so the case did not form", tail-head, want, sent, zcPayload) + } + return sent +} + +// sweepClose closes fd as a connection with a send still owed is closed: +// closeConn defers it (cs.closing), and the closing-drain sweep in +// checkTimeouts reaps it once the peer has read nothing for +// closingDrainTimeoutNanos. The sweep's two calls, without the 5 s wait. +func sweepClose(t *testing.T, w *Worker, cs *connState) { + t.Helper() + fd := cs.fd + w.closeConn(fd) + if w.conns[fd] != cs || !cs.closing { + t.Fatalf("closeConn did not defer the close of a conn with a send owed: registered=%v closing=%v", w.conns[fd] == cs, cs.closing) + } + w.removeDirty(cs) + w.finishCloseAny(fd, cs) + if w.conns[fd] != nil { + t.Fatal("the conn is still registered after the sweep's close") + } +} + +// drainZCPeer lets the peer read everything to EOF while the loop runs, and +// reports what it read and whether the notification arrived (cs's last op +// retired while cs was still queued for release) before cs was released. +func drainZCPeer(t *testing.T, w *Worker, cs *connState, peer int) (got int, eof, notified bool) { + t.Helper() + if err := unix.SetNonblock(peer, true); err != nil { + t.Fatalf("peer nonblock: %v", err) + } + buf := make([]byte, 64<<10) + for end := time.Now().Add(3 * time.Second); time.Now().Before(end); { + for !eof { + n, err := unix.Read(peer, buf) + if n > 0 { + got += n + continue + } + if n == 0 && err == nil { + eof = true + } + break + } + runRingOnce(t, w, 5*time.Millisecond) + if len(w.pendingRelease) == 1 && w.pendingRelease[0].cs == cs && cs.kernelInflight == 0 { + notified = true + } + w.cachedNow = time.Now().UnixNano() + w.drainPendingRelease() + if eof && len(w.pendingRelease) == 0 { + break + } + } + return got, eof, notified +} + +// TestCloseReleasesDescriptorWithOnlyAZCNotificationOwed is celeris#798 on the +// close paths. A connection sent a SEND_ZC to a peer that stopped reading, so +// the notification stays owed, and was closed by the closing-drain sweep: +// +// - notif-only: the send's first CQE was read; nothing else is owed. No +// op names the descriptor, so it must be closed at the close. +// - recv-and-notif: a recv was armed after the send too (the keep-alive +// path arms one behind every unlinked send). The descriptor is kept for +// the recv, which the close's cancel and shutdown end at once, and must +// then be closed although the notification is still owed. +// - send-done-after-close: the send's first CQE was in the ring, unread, +// when the close ran, so the close kept the descriptor for the send. The +// CQE says the send is done, and the descriptor must then be closed. +// +// In each, the descriptor must be closed within zcReleaseBound of the close +// and CloseFDForced must stay 0. The send buffer must not go with it: the +// connState, whose sendBuf the kernel may still read, must stay queued until +// the notification arrives (here, once the peer has read everything), and +// be released then, by the notification and not by the backstop. +func TestCloseReleasesDescriptorWithOnlyAZCNotificationOwed(t *testing.T) { + for _, tc := range []struct { + name string + reap, recv bool + }{ + {"notif-only", true, false}, + {"recv-and-notif", true, true}, + {"send-done-after-close", false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + w, cs, peer := newZCCloseWorker(t) + fd := cs.fd + target := fdTarget(fd) + sent := startZCSend(t, w, cs, tc.reap) + if tc.recv { + if !w.prepareRecv(cs, cs.buf) || cs.kernelInflight != 2 { + t.Fatalf("recv not placed: kernelInflight=%d", cs.kernelInflight) + } + if res := runRingOnce(t, w, 10*time.Millisecond); len(res) != 0 { + t.Fatalf("the recv completed before the close (%v): the peer sent nothing", res) + } + } + closedAt := time.Now() + sweepClose(t, w, cs) + var releasedAfter time.Duration + for { + if fdTarget(fd) != target { + releasedAfter = time.Since(closedAt) + break + } + if time.Since(closedAt) > 6*time.Second { + t.Fatalf("fd %d still open 6s after the close", fd) + } + runRingOnce(t, w, 10*time.Millisecond) + w.cachedNow = time.Now().UnixNano() + w.drainPendingRelease() + } + forced := w.handoffLoss.closeFDForced.Load() + t.Logf("celeris798 close case=%s sent=%d released_after=%v close_fd_forced=%d", tc.name, sent, releasedAfter.Round(time.Microsecond), forced) + if releasedAfter > zcReleaseBound || forced != 0 { + t.Fatalf("fd %d was closed %v after the close (bound %v) with CloseFDForced=%d: a pending SEND_ZC "+ + "notification, which names no descriptor, held it", fd, releasedAfter, zcReleaseBound, forced) + } + if w.closeFDOwed != 0 { + t.Fatalf("closeFDOwed=%d after the descriptor was closed, want 0", w.closeFDOwed) + } + // The send buffer: cs stays queued, holding sendBuf, while the + // notification is owed, however often the release runs. + for end := time.Now().Add(100 * time.Millisecond); time.Now().Before(end); { + runRingOnce(t, w, 10*time.Millisecond) + w.cachedNow = time.Now().UnixNano() + w.drainPendingRelease() + } + if len(w.pendingRelease) != 1 || w.pendingRelease[0].cs != cs || w.pendingRelease[0].holdsFD || cs.kernelInflight == 0 || cs.fd != fd { + t.Fatalf("with the notification still owed the connState must stay queued for release without "+ + "its descriptor: pendingRelease=%+v kernelInflight=%d", w.pendingRelease, cs.kernelInflight) + } + got, eof, notified := drainZCPeer(t, w, cs, peer) + t.Logf("celeris798 close case=%s peer_got=%d eof=%v notified=%v released=%v", tc.name, got, eof, notified, len(w.pendingRelease) == 0) + if !eof || got != int(sent) { + t.Fatalf("the peer read %d bytes (EOF %v), want the %d the send completed with and then EOF", got, eof, sent) + } + if !notified || len(w.pendingRelease) != 0 { + t.Fatalf("the connState was not released at the notification: notified=%v pendingRelease=%d", notified, len(w.pendingRelease)) + } + if forced := w.handoffLoss.closeFDForced.Load(); forced != 0 { + t.Fatalf("CloseFDForced = %d, want 0", forced) + } + }) + } +} + +// TestShutdownDoesNotWaitForAZCNotification is celeris#798 at worker +// shutdown: endOwedOpsAtShutdown waits for the ops that name a live +// connection's descriptor, and a SEND_ZC notification names none. With the +// notification owed after the send's first CQE was read (notif-pending), or +// with that CQE still in the ring when the drain starts +// (send-done-during-drain), the drain must end within zcShutdownBound rather +// than run out its 250 ms bound. +func TestShutdownDoesNotWaitForAZCNotification(t *testing.T) { + for _, tc := range []struct { + name string + reap bool + }{ + {"notif-pending", true}, + {"send-done-during-drain", false}, + } { + t.Run(tc.name, func(t *testing.T) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + w, cs, _ := newZCCloseWorker(t) + sent := startZCSend(t, w, cs, tc.reap) + start := time.Now() + w.endOwedOpsAtShutdown() + took := time.Since(start) + t.Logf("celeris798 shutdown case=%s sent=%d took=%v kernelInflight_after=%d zcNotifPending_after=%v", tc.name, sent, took.Round(time.Microsecond), cs.kernelInflight, cs.zcNotifPending) + if took > zcShutdownBound { + t.Fatalf("endOwedOpsAtShutdown took %v (bound %v) with only a SEND_ZC notification owed", took, zcShutdownBound) + } + if w.closeFDOwed != 0 { + t.Fatalf("closeFDOwed=%d after shutdown's drain, want 0", w.closeFDOwed) + } + }) + } +} From 717155e69b8136b25f1d6e3f51a3b156c05c61bb Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 07:56:07 +0200 Subject: [PATCH 10/12] fix(iouring): a SEND_ZC notification does not hold a closed connection's descriptor (celeris#798) #798 item 1, a regression #793 introduced: fdOwed was kernelInflight > 0, and kernelInflight counts a SEND_ZC until its notification CQE. The notification names no descriptor (the send was issued and has completed), yet a peer that stops reading holds it for as long as the socket is open, and keeping the descriptor kept the socket open: the close held its descriptor to the 5 s release backstop, which forced it and counted CloseFDForced, and worker shutdown's drain ran out its 250 ms bound. The rule now counts only the ops that name the descriptor: - fdOps(cs) = kernelInflight, less one while zcNotifPending (a conn has one send in flight at most). fdOwed and the shutdown drain's pending() use it. - A closed identity keeps the same count in its closedOps entry (fdOps, int16, in the padding after handoff; the entry stays 32 bytes). noteClosedInflight adds the conn's fdOps; staleConnCQE takes one off at a recv's or plain send's terminal CQE and at a SEND_ZC's first CQE (F_MORE), and none at its notification. - drainPendingRelease closes a kept descriptor as soon as no owed op names it (closedFDNamed: a lookup only for a conn whose last send was SEND_ZC), and still releases the connState only when kernelInflight reaches 0: the notification is what says the kernel is done with sendBuf. - Worker shutdown's drain records a live connection's SEND_ZC first CQE as handleSend does (zcSendCompleted, handleSend's F_MORE branch moved into a function), so it does not wait for that notification either. The detector control's mutant anchors on fdOwed's body; its anchor follows. --- .github/scripts/mutant-685-release-owed-fd.py | 2 +- engine/iouring/fd_lifetime.go | 95 ++++++++++++--- engine/iouring/worker.go | 110 +++++++++++++----- 3 files changed, 166 insertions(+), 41 deletions(-) diff --git a/.github/scripts/mutant-685-release-owed-fd.py b/.github/scripts/mutant-685-release-owed-fd.py index a4ec0b30..1816cc9c 100755 --- a/.github/scripts/mutant-685-release-owed-fd.py +++ b/.github/scripts/mutant-685-release-owed-fd.py @@ -33,7 +33,7 @@ BODY = ( "func fdOwed(cs *connState) bool {\n" - "\treturn cs != nil && cs.kernelInflight > 0 && !cs.fixedFile\n" + "\treturn cs != nil && fdOps(cs) > 0 && !cs.fixedFile\n" "}\n" ) diff --git a/engine/iouring/fd_lifetime.go b/engine/iouring/fd_lifetime.go index dc1200c1..c4208444 100644 --- a/engine/iouring/fd_lifetime.go +++ b/engine/iouring/fd_lifetime.go @@ -376,8 +376,11 @@ func (w *Worker) rescueHold(cs *connState) { // descriptor (fdOwed). The close path does everything else as before (the // cancels, the closedOps registration, the deferred release) and hands the // descriptor to its pendingRelease entry, and drainPendingRelease closes it -// where it releases the connState: when every owed op has delivered its -// terminal CQE. Until then the number stays allocated, so no accept or dup +// once no owed op names it: where it releases the connState, when every owed +// op has delivered its terminal CQE, or earlier when all that is left is +// SEND_ZC notifications (celeris#798). Those name no descriptor and guard +// only the send buffer, so the connState waits for them and the descriptor +// does not (fdOps). Until then the number stays allocated, so no accept or dup // anywhere in the process can be given it, and an op issued late resolves // this connection's own socket. To make sure every owed op does end, the // close path also shuts the socket's read side down (shutdownHow; SHUT_RD @@ -406,16 +409,66 @@ func (w *Worker) rescueHold(cs *connState) { // (endOwedOpsAtShutdown). // fdOwed reports whether the kernel still owes cs an op that names its -// descriptor: a recv or send whose SQE was written and whose terminal CQE has -// not been read. kernelInflight counts every such op from the moment its SQE -// is placed (prepareRecv, flushSend, flushSendLink, both halves of a linked -// pair), submitted or not, issued or not, and staleConnCQE retires it at the -// terminal CQE; the header timer and the cancels name no descriptor and are -// not counted. A fixed-file connection names a slot, not a number, and keeps -// its own close (CLOSE_DIRECT). A nil cs (closeMissingConnState) owes +// descriptor: a recv or send whose SQE was written and that has not +// completed (fdOps). A fixed-file connection names a slot, not a number, and +// keeps its own close (CLOSE_DIRECT). A nil cs (closeMissingConnState) owes // nothing we can know of. func fdOwed(cs *connState) bool { - return cs != nil && cs.kernelInflight > 0 && !cs.fixedFile + return cs != nil && fdOps(cs) > 0 && !cs.fixedFile +} + +// fdOps counts the ops the kernel still owes live cs that name its +// descriptor. kernelInflight counts every recv and send from the moment its +// SQE is placed (prepareRecv, flushSend, flushSendLink, both halves of a +// linked pair), submitted or not, issued or not, and staleConnCQE retires it +// at the terminal CQE; the header timer and the cancels name no descriptor +// and are not counted. +// +// One op in that count may name nothing (celeris#798): a SEND_ZC whose send +// has completed (its first CQE, IORING_CQE_F_MORE, set zcNotifPending) keeps +// its count until the notification CQE, and the notification is not an op on +// the descriptor. It says the kernel has let go of the send buffer's pages, +// which a stalled peer's unread data can hold for as long as the socket +// lives. Counting it kept the descriptor, and so the socket, open until the +// 5 s release backstop forced it (CloseFDForced). So it is left out here, +// and only here: kernelInflight still counts it, and the connState, whose +// sendBuf those pages are, is still released only at the notification +// (drainPendingRelease). A connection has one send in flight at most +// (flushSend and flushSendLink wait out cs.sending and cs.zcNotifPending), +// so zcNotifPending stands for exactly one op. +// +// Live connections only: a closed one's count moves in its closedOps entry +// (closedOpsEntry.fdOps, read by closedFDNamed). +func fdOps(cs *connState) int32 { + if cs.zcNotifPending { + return cs.kernelInflight - 1 + } + return cs.kernelInflight +} + +// closedFDNamed reports whether an op the kernel still owes closed cs names +// its descriptor, as drainPendingRelease asks of an entry that kept the +// descriptor (holdsFD) while kernelInflight is not yet 0. Only a SEND_ZC +// notification names none (see fdOps), and only a connection whose last send +// was armed as SEND_ZC (sendIsZC, which nothing changes after the close) can +// owe one; for any other the answer is yes without a lookup. For that one, +// its closedOps entry counts what is left (fdOps). No entry means the +// accounting cannot say, and the descriptor stays kept. +func (w *Worker) closedFDNamed(cs *connState) bool { + if !cs.sendIsZC { + return true + } + e := w.closedOps[encodeConnOpKey(cs.fd, cs.generation)] + return e == nil || e.fdOps > 0 +} + +// releaseKeptFD closes the descriptor a close path left to e (holdsFD): the +// release closes it when cs goes, or earlier, once only a SEND_ZC +// notification is still owed (celeris#798). Worker thread only. +func (w *Worker) releaseKeptFD(e *pendingReleaseEntry) { + _ = unix.Close(int(e.fd)) + e.holdsFD = false + w.closeFDOwed-- } // keptFD is the descriptor a close path hands to its pendingRelease entry: @@ -488,6 +541,12 @@ const shutdownFDDrainNanos int64 = int64(250 * time.Millisecond) // shutdownFDDrainNanos; past it (or on a ring error) the descriptors are // closed anyway, as shutdown always did. Worker thread only; skipped under // SQPOLL (no tier enables it) and without a ring. +// +// It waits for the ops that name a descriptor, not for SEND_ZC +// notifications (celeris#798, see fdOps): a stalled peer can hold one for as +// long as the socket is open, far past this bound. A live connection's +// SEND_ZC that completes during the drain is recorded as handleSend records +// it (zcSendCompleted), so its notification is not waited for either. func (w *Worker) endOwedOpsAtShutdown() { var owed []*connState for _, fd := range w.liveConns { @@ -499,12 +558,12 @@ func (w *Worker) endOwedOpsAtShutdown() { } pending := func() bool { for _, cs := range owed { - if cs.kernelInflight > 0 { + if fdOps(cs) > 0 { return true } } for i := range w.pendingRelease { - if e := &w.pendingRelease[i]; e.holdsFD && e.cs.kernelInflight > 0 { + if e := &w.pendingRelease[i]; e.holdsFD && e.cs.kernelInflight > 0 && w.closedFDNamed(e.cs) { return true } } @@ -526,7 +585,17 @@ func (w *Worker) endOwedOpsAtShutdown() { ud := c.UserData switch ud & udMask { case udRecv, udSend: - w.staleConnCQE(c, int(ud&fdMask), ud) + fd := int(ud & fdMask) + // A live connection's SEND_ZC completing (F_MORE marks + // only that on a send): record it as the loop does, or + // its notification would be waited for as an op on the + // descriptor. A closed one's is counted by staleConnCQE. + if ud&udMask == udSend && cqeHasMore(c.Flags) && fd < len(w.conns) { + if cs := w.conns[fd]; cs != nil && cs.generation == decodeGen(ud) { + zcSendCompleted(cs, c.Res) + } + } + w.staleConnCQE(c, fd, ud) case udAccept: if c.Res >= 0 && !w.fixedFiles { _ = unix.Close(int(c.Res)) diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go index bef0d870..ba20f68d 100644 --- a/engine/iouring/worker.go +++ b/engine/iouring/worker.go @@ -211,7 +211,18 @@ type closedOpsEntry struct { // (TestClosedOpsEntryStaysThirtyTwoBytes). 32-bit platforms have no // padding there, and the entry grows from 16 to 20 bytes. handoff bool - conns []*connState + // fdOps is the part of inflight that still names the descriptor + // (celeris#798): all of it but the SEND_ZC notifications whose send has + // completed. noteClosedInflight adds each conn's fdOps; staleConnCQE + // takes one off at a recv's or a plain send's terminal CQE and at a + // SEND_ZC's first CQE, and none at its notification. closedFDNamed + // reads it, so a close that kept its descriptor lets go of it once only + // notifications are owed, while the connState waits for inflight. A + // conn owes two such ops at most (a recv and a send), so int16 is ample + // even under a collision; it sits in the same padding as handoff, and + // the entry keeps its size on every platform. + fdOps int16 + conns []*connState } // pendingReleaseHoldNanos is the WALL-CLOCK BACKSTOP for releasing a @@ -1633,13 +1644,16 @@ func (w *Worker) staleConnCQE(c *completionEntry, fd int, ud uint64) bool { // with its recv still kernel-armed), so its own close can // skip the cancel and release early — the UAF class this // accounting exists to prevent. Reachability is narrow: a - // close path keeps the number allocated until the closed - // conn's last owed op has delivered its terminal CQE - // (celeris#685), so on this worker the number cannot be - // re-occupied while such a CQE is still to come; the window - // needs the pendingRelease backstop to have closed a number - // with an op still owed (CloseFDForced, must stay 0) ON TOP of - // the gen collision. When it fires, the closed conn's + // close path keeps the number allocated until the last op + // that names it has ended (celeris#685), so on this worker the + // number cannot be re-occupied while a recv or send CQE of the + // closed conn is still to come. Only a SEND_ZC notification + // can be (celeris#798: it names no descriptor, and a stalled + // peer holds it), which leaves this window where it was + // before celeris#685 for that one CQE; any other needs the + // pendingRelease backstop to have closed a number with an op + // still owed (CloseFDForced, must stay 0). Either way the + // gen collision comes ON TOP. When it fires, the closed conn's // closedOps entry is left orphaned and the 5 s backstop WARN // in drainPendingRelease is the production signal. if cs.kernelInflight > 0 { @@ -1662,7 +1676,13 @@ func (w *Worker) staleConnCQE(c *completionEntry, fd int, ud uint64) bool { } } if terminalOp { - w.noteStaleTerminalOp(ud) + // A notification ends an op that named no descriptor any more + // (celeris#798); every other terminal CQE ends one that did. + w.noteStaleTerminalOp(ud, !cqeIsNotif(c.Flags)) + } else if op == udSend { + // F_MORE on a send is a SEND_ZC's first CQE: the send is done, so it + // names the descriptor no more, and its notification is still owed. + w.noteStaleSendCompleted(ud) } if cqeHasBuffer(c.Flags) && w.bufRing != nil { w.bufRing.PushBuffer(cqeBufferID(c.Flags)) @@ -1676,7 +1696,9 @@ func (w *Worker) staleConnCQE(c *completionEntry, fd int, ud uint64) bool { // CQE's (fd, generation) identity, releasing them for drainPendingRelease // once the kernel owes them nothing. No-op when the identity is unknown // (conn closed with zero in-flight ops, or already backstop-released). -func (w *Worker) noteStaleTerminalOp(ud uint64) { +// namedFD says whether the op still named the descriptor until this CQE +// (closedOpsEntry.fdOps): false only for a SEND_ZC notification. +func (w *Worker) noteStaleTerminalOp(ud uint64, namedFD bool) { if len(w.closedOps) == 0 { return } @@ -1686,6 +1708,9 @@ func (w *Worker) noteStaleTerminalOp(ud uint64) { return } e.inflight-- + if namedFD && e.fdOps > 0 { + e.fdOps-- + } if e.inflight > 0 { return } @@ -1695,6 +1720,19 @@ func (w *Worker) noteStaleTerminalOp(ud uint64) { delete(w.closedOps, key) } +// noteStaleSendCompleted takes a closed identity's SEND_ZC off its count of +// ops that name the descriptor at the send's first CQE (celeris#798): the op +// is done with the descriptor, and only its notification, which still +// counts in inflight, is owed. Worker thread only. +func (w *Worker) noteStaleSendCompleted(ud uint64) { + if len(w.closedOps) == 0 { + return + } + if e := w.closedOps[connOpKey(ud)]; e != nil && e.fdOps > 0 { + e.fdOps-- + } +} + func (w *Worker) processCQE(ctx context.Context, c *completionEntry, now int64) { ud := c.UserData op := decodeOp(ud) @@ -3407,6 +3445,30 @@ func (w *Worker) respondAndArm(cs *connState, fd int, c *completionEntry, link, } } +// zcSendCompleted records a SEND_ZC's first completion (IORING_CQE_F_MORE): +// the send is done and its result waits for the notification, which is when +// the kernel lets go of cs.sendBuf. From here the op names no descriptor, so +// fdOps stops counting it (celeris#798). handleSend calls it, and so does +// worker shutdown's drain (endOwedOpsAtShutdown), which dispatches no +// completion to handleSend. Worker thread only. +func zcSendCompleted(cs *connState, res int32) { + // cs.sending / cs.zcNotifPending are read by the inline-egress guard on + // the dispatch goroutine under detachMu; mutate them under the lock. + if mu := cs.detachMu; mu != nil { + mu.Lock() + defer mu.Unlock() + } + if res < 0 { + cs.sending = false + cs.zcNotifPending = true + cs.zcSentBytes = res // store negative for error path on NOTIF + return + } + cs.zcNotifPending = true + cs.zcSentBytes = res + // sending stays true until NOTIF completes the cycle. +} + func (w *Worker) handleSend(c *completionEntry, fd int, now int64) { cs := w.conns[fd] if cs == nil { @@ -3465,21 +3527,7 @@ func (w *Worker) handleSend(c *completionEntry, fd int, now int64) { // process (celeris#519). F_MORE on a udSend completion is set by the // kernel only for SEND_ZC, so it is the accurate test. if cqeHasMore(c.Flags) { - // cs.sending / cs.zcNotifPending are read by the inline-egress guard on - // the dispatch goroutine under detachMu; mutate them under the lock. - if mu := cs.detachMu; mu != nil { - mu.Lock() - defer mu.Unlock() - } - if c.Res < 0 { - cs.sending = false - cs.zcNotifPending = true - cs.zcSentBytes = c.Res // store negative for error path on NOTIF - return - } - cs.zcNotifPending = true - cs.zcSentBytes = c.Res - // sending stays true until NOTIF completes the cycle. + zcSendCompleted(cs, c.Res) return } @@ -4033,6 +4081,7 @@ func (w *Worker) noteClosedInflight(cs *connState) { w.closedOps[key] = e } e.inflight += cs.kernelInflight + e.fdOps += int16(fdOps(cs)) e.conns = append(e.conns, cs) } @@ -4122,6 +4171,14 @@ func (w *Worker) drainPendingRelease() { entry := &w.pendingRelease[i] cs := entry.cs if cs.kernelInflight > 0 { + // A kept descriptor goes as soon as no owed op names it + // (celeris#798): what is left may be SEND_ZC notifications only, + // and one of those can wait on a stalled peer for as long as the + // socket is open. cs itself stays until they arrive: they say the + // kernel is done with its send buffer. + if entry.holdsFD && !w.closedFDNamed(cs) { + w.releaseKeptFD(entry) + } if entry.releaseAtNanos > w.cachedNow { kept = append(kept, *entry) continue @@ -4148,8 +4205,7 @@ func (w *Worker) drainPendingRelease() { // every op that named the descriptor has delivered its terminal // CQE, so none can resolve the number any more. if entry.holdsFD { - _ = unix.Close(int(entry.fd)) - w.closeFDOwed-- + w.releaseKeptFD(entry) } if !entry.detached { releaseConnState(cs) From cd0360044313d91bd4540b16c42355135db9650d Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 08:07:27 +0200 Subject: [PATCH 11/12] ci: run the celeris#798 SEND_ZC tests on both arches, by name, skipping forbidden The unit job runs TestCloseReleasesDescriptorWithOnlyAZCNotificationOwed and TestShutdownDoesNotWaitForAZCNotification in its engine/iouring package step, on x86 only. The recv-theft job, which runs on both arches, now runs them by name at the runner's own 8 MiB memlock (the pages SEND_ZC pins count against it), -race, three runs, and requires every run of each of the five cases to PASS with no FAIL and no SKIP line: a SKIP would mean the runner gave the engine no working SEND_ZC, and nothing was tested. --- .github/workflows/ci.yml | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d067a224..e1ecab7e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1050,6 +1050,43 @@ jobs: echo "release a descriptor number a recv could still resolve, or was a test renamed or skipped?" exit 1 fi + # celeris#798: the rule counts only the ops that name the descriptor. A + # SEND_ZC's notification names none, yet a peer that stops reading holds + # it for as long as the socket is open. So it must not keep a closed + # connection's descriptor (the release backstop forced it after 5 s and + # counted CloseFDForced), nor stall worker shutdown's drain, while the + # connState, whose send buffer the notification guards, still waits for + # it. The unit job runs these two tests in its package step, on x86 only; + # here they run on both arches, by name, at the runner's own 8 MiB + # memlock (the pages SEND_ZC pins count against it), with a tally: every + # run of each of the five cases must PASS, with no FAIL and no SKIP line. + # A SKIP would mean the runner's kernel gave the engine no working + # SEND_ZC, so nothing was tested. + - name: celeris#798 a SEND_ZC notification holds no descriptor (both arches, skipping forbidden) + if: ${{ !cancelled() }} + shell: bash + env: + CELERIS_REQUIRE_IOURING_WORKERS: "1" + run: | + set -o pipefail + echo "memlock (KiB): $(ulimit -l)" + names='TestCloseReleasesDescriptorWithOnlyAZCNotificationOwed|TestShutdownDoesNotWaitForAZCNotification' + runs=3 + want=$(( $(tr '|' '\n' <<<"$names" | wc -l) * runs )) + wantsub=$(( 5 * runs )) + go test -race -count="$runs" -timeout=300s -v -run "^(${names})\$" \ + ./engine/iouring/ 2>&1 | tee /tmp/zc798.log || true + grep -E 'celeris798 ' /tmp/zc798.log || true + passed=$(grep -cE "^--- PASS: (${names}) \(" /tmp/zc798.log || true) + subpassed=$(grep -cE "^ --- PASS: (${names})/" /tmp/zc798.log || true) + failed=$(grep -cE '^[[:space:]]*--- FAIL' /tmp/zc798.log || true) + skipped=$(grep -cE '^[[:space:]]*--- SKIP' /tmp/zc798.log || true) + echo "celeris#798 SEND_ZC tests: want $want PASS ($wantsub cases), passed $passed ($subpassed cases), FAIL lines $failed, SKIP lines $skipped" + if [ "$passed" -ne "$want" ] || [ "$subpassed" -ne "$wantsub" ] || [ "$failed" -ne 0 ] || [ "$skipped" -ne 0 ]; then + echo "expected exactly $want runs ($wantsub cases) to PASS with no FAIL or SKIP line -- did a" + echo "SEND_ZC notification hold a descriptor or stall shutdown, or was a test renamed or skipped?" + exit 1 + fi # The detector control. .github/scripts/mutant-685-release-owed-fd.py # makes fdOwed report false, which takes the fd-lifetime rule off the # close paths, Hijack and shutdown at once. Against that tree every run From bf129c88d32479a311cc8474ab3ac92a5ef80981 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Mon, 28 Sep 2026 08:13:37 +0200 Subject: [PATCH 12/12] fix(iouring): shutdown's drain notes a SEND_ZC completion itself and leaves handleSend as it was (celeris#798) 717155e moved handleSend's SEND_ZC first-completion branch into a function (zcSendCompleted) so that worker shutdown's drain could record a live connection's send completing during the drain. That moved the detachMu acquire the celeris#587 detector control mutates: its script anchors on the lock at the top of handleSend's F_MORE branch, found nothing, and exited 2, so the zc-window job failed on both arches (CI run on cd03600). handleSend is back exactly as it was. The drain no longer writes the connection at all: it keeps its own note of the live connections whose SEND_ZC completed during the drain (zcDone) and counts their notification out of the ops that name the descriptor. The connection's fields stay the worker's and handleSend's, which the dispatch goroutine reads under detachMu. --- engine/iouring/fd_lifetime.go | 31 ++++++++++++++++++++------- engine/iouring/worker.go | 40 +++++++++++++---------------------- 2 files changed, 38 insertions(+), 33 deletions(-) diff --git a/engine/iouring/fd_lifetime.go b/engine/iouring/fd_lifetime.go index c4208444..518f723e 100644 --- a/engine/iouring/fd_lifetime.go +++ b/engine/iouring/fd_lifetime.go @@ -545,8 +545,10 @@ const shutdownFDDrainNanos int64 = int64(250 * time.Millisecond) // It waits for the ops that name a descriptor, not for SEND_ZC // notifications (celeris#798, see fdOps): a stalled peer can hold one for as // long as the socket is open, far past this bound. A live connection's -// SEND_ZC that completes during the drain is recorded as handleSend records -// it (zcSendCompleted), so its notification is not waited for either. +// SEND_ZC whose first CQE the drain reads names the descriptor no more +// either; handleSend, which records that in zcNotifPending, does not run +// here, so the drain keeps its own note (zcDone) and changes nothing on the +// connection, which the dispatch goroutine may still read. func (w *Worker) endOwedOpsAtShutdown() { var owed []*connState for _, fd := range w.liveConns { @@ -556,9 +558,18 @@ func (w *Worker) endOwedOpsAtShutdown() { } owed = append(owed, cs) } + var zcDone map[*connState]bool + // named is fdOwed for the drain: fdOps, less a SEND_ZC it saw complete. + named := func(cs *connState) bool { + n := fdOps(cs) + if zcDone[cs] { + n-- + } + return n > 0 + } pending := func() bool { for _, cs := range owed { - if fdOps(cs) > 0 { + if named(cs) { return true } } @@ -587,12 +598,16 @@ func (w *Worker) endOwedOpsAtShutdown() { case udRecv, udSend: fd := int(ud & fdMask) // A live connection's SEND_ZC completing (F_MORE marks - // only that on a send): record it as the loop does, or - // its notification would be waited for as an op on the - // descriptor. A closed one's is counted by staleConnCQE. + // only that on a send; a connection has one send in + // flight at most): noted, or its notification would be + // waited for as an op on the descriptor. A closed + // identity's is counted by staleConnCQE. if ud&udMask == udSend && cqeHasMore(c.Flags) && fd < len(w.conns) { - if cs := w.conns[fd]; cs != nil && cs.generation == decodeGen(ud) { - zcSendCompleted(cs, c.Res) + if cs := w.conns[fd]; cs != nil && cs.generation == decodeGen(ud) && !cs.zcNotifPending { + if zcDone == nil { + zcDone = make(map[*connState]bool) + } + zcDone[cs] = true } } w.staleConnCQE(c, fd, ud) diff --git a/engine/iouring/worker.go b/engine/iouring/worker.go index ba20f68d..b7339fb8 100644 --- a/engine/iouring/worker.go +++ b/engine/iouring/worker.go @@ -3445,30 +3445,6 @@ func (w *Worker) respondAndArm(cs *connState, fd int, c *completionEntry, link, } } -// zcSendCompleted records a SEND_ZC's first completion (IORING_CQE_F_MORE): -// the send is done and its result waits for the notification, which is when -// the kernel lets go of cs.sendBuf. From here the op names no descriptor, so -// fdOps stops counting it (celeris#798). handleSend calls it, and so does -// worker shutdown's drain (endOwedOpsAtShutdown), which dispatches no -// completion to handleSend. Worker thread only. -func zcSendCompleted(cs *connState, res int32) { - // cs.sending / cs.zcNotifPending are read by the inline-egress guard on - // the dispatch goroutine under detachMu; mutate them under the lock. - if mu := cs.detachMu; mu != nil { - mu.Lock() - defer mu.Unlock() - } - if res < 0 { - cs.sending = false - cs.zcNotifPending = true - cs.zcSentBytes = res // store negative for error path on NOTIF - return - } - cs.zcNotifPending = true - cs.zcSentBytes = res - // sending stays true until NOTIF completes the cycle. -} - func (w *Worker) handleSend(c *completionEntry, fd int, now int64) { cs := w.conns[fd] if cs == nil { @@ -3527,7 +3503,21 @@ func (w *Worker) handleSend(c *completionEntry, fd int, now int64) { // process (celeris#519). F_MORE on a udSend completion is set by the // kernel only for SEND_ZC, so it is the accurate test. if cqeHasMore(c.Flags) { - zcSendCompleted(cs, c.Res) + // cs.sending / cs.zcNotifPending are read by the inline-egress guard on + // the dispatch goroutine under detachMu; mutate them under the lock. + if mu := cs.detachMu; mu != nil { + mu.Lock() + defer mu.Unlock() + } + if c.Res < 0 { + cs.sending = false + cs.zcNotifPending = true + cs.zcSentBytes = c.Res // store negative for error path on NOTIF + return + } + cs.zcNotifPending = true + cs.zcSentBytes = c.Res + // sending stays true until NOTIF completes the cycle. return }