diff --git a/internal/rdmamode/rdmamode.go b/internal/rdmamode/rdmamode.go index a4e861ea..a3fd4720 100644 --- a/internal/rdmamode/rdmamode.go +++ b/internal/rdmamode/rdmamode.go @@ -156,16 +156,18 @@ func (b *BackendShutdownAfterRC) SetOpsDrainer(d OpsDrainer) { b.ops.Store(&d) } -// Shutdown drains operational publications, closes the RC service, -// then shuts the wrapped backend down, once. The publication drain -// runs before the RC close: RC teardown itself queues publications, -// so the queue must still be moving while the sessions drain. +// Shutdown closes the RC service, drains operational publications, +// then shuts the wrapped backend down, once. The drain runs AFTER +// the RC close: closing RC quiesces the native reaper (every +// teardown callback has returned by the time Close returns), so no +// producer can enqueue behind the drain - enqueue-then-worker-exit +// stranding is impossible by ordering rather than by locking. func (b *BackendShutdownAfterRC) Shutdown() { b.closed.Do(func() { + b.rc.Close() if d := b.ops.Load(); d != nil { (*d).Shutdown() } - b.rc.Close() b.Backend.Shutdown() }) } diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index f33b9b3f..1ee6442e 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -214,6 +214,11 @@ type sessionRecord struct { // exactly once itself), so a completion call that fires the // callback before returning cannot publish a placeholder. reserved bool + // terminal marks a teardown that arrived while the record was + // reserved: the native session is gone and no second callback + // will come, so a later release of the reservation resolves + // the stashed outcome instead of leaving an orphan. + terminal bool } // opsTracker owns terminal publication: each session publishes @@ -452,16 +457,28 @@ func (t *opsTracker) reserve(sessionID string) *opsEmitter { // without publishing: the transfer claim it was held for rolled // back, so the session lives on and the next claimant (another // READY, or the reaper) must still find an unreserved record. +// If the native session already tore down while the record was +// reserved (terminal stashed), the session is gone: consume the +// record and publish the stashed outcome, since no second +// callback will arrive. func (t *opsTracker) releaseReservation(sessionID string, emit *opsEmitter) { if t == nil { return } t.mu.Lock() rec, ok := t.sessions[sessionID] - if ok && rec.reserved && rec.emit == emit { - rec.reserved = false + if !ok || !rec.reserved || rec.emit != emit { + t.mu.Unlock() + return } + if !rec.terminal { + rec.reserved = false + t.mu.Unlock() + return + } + delete(t.sessions, sessionID) t.mu.Unlock() + t.dispatch(pubJob{emit: rec.emit, err: rec.out.err, byt: rec.out.byt}) } // publishReserved publishes through a reserved record and drops it: @@ -471,6 +488,15 @@ func (t *opsTracker) publishReserved(sessionID string, emit *opsEmitter, err err return } t.mu.Lock() + rec, ok := t.sessions[sessionID] + // Ownership check: only the reservation holder publishes. A + // stale holder (its reservation was released or the record + // was replaced) must not delete or publish the current + // owner's record. + if !ok || rec.emit != emit { + t.mu.Unlock() + return + } delete(t.sessions, sessionID) t.mu.Unlock() if emit != nil { @@ -502,8 +528,16 @@ func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { // A reserved record belongs to its request path, which // publishes the real outcome itself: the callback (fired // synchronously by a completion call, before the request - // path could confirm the result) must not touch it. + // path could confirm the result) must not touch it. But the + // terminal is still a fact: if the reservation is released + // later (claim rollback) and no second callback will ever + // come - the native session is gone - the stashed event + // resolves then, instead of being lost. if rec.reserved { + if !rec.out.done { + rec.out = sessionOutcome{err: expiredError(ev), done: true} + } + rec.terminal = true t.mu.Unlock() return } diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 5853773f..2cd957dd 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -274,6 +274,43 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { tr.releaseReservation("s-rel", emit3) tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-rel"}) + // M1 regression: a terminal arriving while reserved is stashed, + // and a later claim-rollback release consumes it and publishes the + // expiry - the record is not orphaned. + tr.register("s-stash", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) + emitS := tr.reserve("s-stash") + if emitS == nil { + t.Fatal("reserve failed") + } + tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-stash"}) + tr.releaseReservation("s-stash", emitS) + + // M2 regression: a second reservation of a live record is + // refused, so a duplicate READY cannot claim the transfer + // while another request owns the publication. The owner then + // completes normally. + tr.register("s-dbl", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) + emitD := tr.reserve("s-dbl") + if emitD == nil { + t.Fatal("first reserve failed") + } + if tr.reserve("s-dbl") != nil { + t.Fatal("double reserve succeeded") + } + tr.publishReserved("s-dbl", emitD, nil, 64) + + // Ownership: a stale emitter must not publish or consume the + // current record; the real owner still can, even after the + // callback fired (stashed) underneath it. + tr.register("s-own", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) + emitO := tr.reserve("s-own") + if emitO == nil { + t.Fatal("reserve failed") + } + tr.publishReserved("s-own", &opsEmitter{}, nil, 999) + tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-own"}) + tr.publishReserved("s-own", emitO, nil, 32) + // Consume-or-noop denial of an unreserved session. tr.register("s-fail", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) tr.failOutcome("s-fail", errors.New("x")) @@ -284,8 +321,8 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { rl.mu.Lock() defer rl.mu.Unlock() - if len(rl.logs) != 5 { - t.Fatalf("published %d records, want 5: %+v", len(rl.logs), rl.logs) + if len(rl.logs) != 8 { + t.Fatalf("published %d records, want 8: %+v", len(rl.logs), rl.logs) } // The recording sink cannot see session IDs directly (they // live in the synthesized context), so assert the observable diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index 252874d9..9dd79661 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -401,6 +401,15 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // the record in the window between the claim and the // reservation. A reserved record is invisible to both. emit := h.ops.reserve(sessionID) + if emit == nil { + // Another READY holds the publication reservation for + // this session. Publication ownership must track native + // transfer ownership: proceeding without the reservation + // would let this request win the native claim while a + // different request owns the publication, losing the + // record on completion. Answer as a duplicate claim. + return fmt.Errorf("transfer in progress: %w", rcserver.ErrDouble) + } publish := func(err error, bytes int64) { h.ops.publishReserved(sessionID, emit, err, bytes) } @@ -494,7 +503,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { finalized = true } if err != nil { - publish(mapRcError(err), 0) + doPublish(mapRcError(err), 0) return err } // The FINAL wire reply carries the stored object's @@ -505,7 +514,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // the same commit metadata. emit.setCommitMeta(put.ETag, put.VersionID) } else if err := h.svc.FinishFinal(sessionID); err != nil { - publish(mapRcError(err), 0) + doPublish(mapRcError(err), 0) return mapRcError(err) } else { finalized = true @@ -513,7 +522,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // The transfer completed: publish the terminal record with // the byte count the data plane reported. - publish(nil, int64(resp.BytesTransferred)) + doPublish(nil, int64(resp.BytesTransferred)) // Wire reply per the hipobj-rc-v2 contract: protocol echo, // cookie echo, transferred bytes, and object metadata.