From 138504dbc9a02e8fb91eff97de6106e5fd5ed6bb Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 02:55:37 +0900 Subject: [PATCH 01/16] rdma: add terminal notification callback to the RC server ABI RC sessions that never reach a completion (expired, abandoned, or canceled before READY) currently vanish inside the reaper without any trace on the operational surface. Add a terminal notification callback to the C ABI so the server reports the final outcome of every session exactly once, fired from the reaper with no server lock held. --- cuwrapper/rc/rc_server_abi.cpp | 19 +++++++++++++++++++ cuwrapper/rc/rc_server_abi.h | 15 +++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/cuwrapper/rc/rc_server_abi.cpp b/cuwrapper/rc/rc_server_abi.cpp index 7d7c5e6a..948cce72 100644 --- a/cuwrapper/rc/rc_server_abi.cpp +++ b/cuwrapper/rc/rc_server_abi.cpp @@ -101,6 +101,11 @@ struct rc_server { * races an in-flight sink pointer swap. */ rc_log_fn log_fn = nullptr; void *log_ctx = nullptr; + /* Terminal notification sink: same lifetime contract as log_fn + * (installed once at init, cleared by destroy after the reaper + * joined). Fired by reapSession with no lock held. */ + rc_terminal_fn term_fn = nullptr; + void *term_ctx = nullptr; std::atomic epoch_counter{1}; /* resource accounting (global buckets; per-principal map). */ std::mutex acct_mtx; @@ -253,6 +258,13 @@ void reapSession(rc_server *srv, RcSession *s) { * is actually gone: a surviving QP still holds the device. */ if (destroyed) hipObj::v2::releaseDevice(srv->device); limitsRelease(srv, s->principal, s->staging_len); + /* Terminal notification: the sink runs after every server-side + * bookkeeping above so it observes the session as fully gone, + * and no lock is held here per the callback contract. */ + rc_terminal_fn tfn = srv->term_fn; + if (tfn) + tfn(srv->term_ctx, s->core.id.c_str(), s->last_outcome, + (uint64_t)s->staging_len); } /* Runs the reap pass: sessions marked reap_pending (or in the @@ -301,6 +313,13 @@ void rc_server_set_log_sink(rc_server *srv, rc_log_fn fn, void *ctx) { srv->log_ctx = ctx; } +void rc_server_set_terminal_notify(rc_server *srv, rc_terminal_fn fn, + void *ctx) { + if (!srv) return; + srv->term_fn = fn; + srv->term_ctx = ctx; +} + int rc_server_init(const rc_device_opts *opts, rc_server **out) { if (!opts || !out) return RC_E_ARG; if (!hipObj::ibv.ensureLoaded()) { diff --git a/cuwrapper/rc/rc_server_abi.h b/cuwrapper/rc/rc_server_abi.h index f852c19b..9193524a 100644 --- a/cuwrapper/rc/rc_server_abi.h +++ b/cuwrapper/rc/rc_server_abi.h @@ -52,6 +52,21 @@ typedef struct rc_server rc_server; void rc_server_set_log_sink(rc_server *srv, rc_log_fn fn, void *ctx); +/* Terminal session notification. Fired exactly once per session + * when the reaper destroys it (expiry, CANCEL, or destroy), + * carrying the last outcome observed for the session. `id` is + * only valid for the duration of the call. Called with no server + * lock held; the sink must return promptly and must not call back + * into the server. Sessions that end inside a READY/FinishPut + * handler still fire this notification after the handler's own + * terminal bookkeeping, so the sink can treat it as the single + * authoritative "the session is gone" signal. */ +typedef void (*rc_terminal_fn)(void *ctx, const char *id, int outcome, + uint64_t bytes); + +void rc_server_set_terminal_notify(rc_server *srv, rc_terminal_fn fn, + void *ctx); + /* Device selection: matching GID prefix when gid_hint is set, * otherwise the first verbs device. */ typedef struct { From 49b1c6f6bc949867fc9818f045cbf96a2f64bab5 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 02:55:45 +0900 Subject: [PATCH 02/16] rdma: publish RC transfer outcomes into the gateway operational services Two-phase RC transfers were invisible to the access log, metrics, and bucket notifications: every outcome record on the S3 surface is driven by the fiber request context, and the RC wire requests carry the transfer session rather than the object. Publish one record per session by tracking the operational context from PREPARE through completion. A tracker table registers each successfully prepared session with its account, region, object, and start time; whichever path confirms the final outcome first (READY completion, READY failure, or the reaper teardown callback) claims the entry and publishes exactly once. Sessions that end before PREPARE succeeds publish a request record directly. The record is emitted through a synthetic fiber context carrying the object path and the captured locals, so the existing logger, metrics manager, and event sender produce the same schema as the S3 surface without any interface change. Bucket notifications fire for completed PUTs. The gateway creates the operational services inside RunVersityGW, after the RC routes exist. Add an OnServicesReady callback to the embedded gateway config and wire it in vgwrdma to hand the services to the RC routes and install the teardown callback. --- cmd/vgwrdma/main.go | 11 ++ embedgw/embedgw.go | 23 +++ rdma/rcroutes/ops_linux.go | 277 ++++++++++++++++++++++++++++++++ rdma/rcroutes/ops_linux_test.go | 104 ++++++++++++ rdma/rcroutes/routes_linux.go | 41 ++++- rdma/rcroutes/routes_stub.go | 13 ++ rdma/rcserver/rcserver_linux.go | 49 ++++++ rdma/rcserver/rcserver_stub.go | 10 ++ 8 files changed, 526 insertions(+), 2 deletions(-) create mode 100644 rdma/rcroutes/ops_linux.go create mode 100644 rdma/rcroutes/ops_linux_test.go diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index e5d3bf4d..4cf78035 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -1294,6 +1294,17 @@ func runGateway(ctx context.Context, be backend.Backend) error { return ctx.Next() } rcH := rcroutes.New(rcSvc, be, iamSvc, readonly, disableACLs) + // The gateway builds the access logger, metrics manager, + // and event sender inside RunVersityGW; hand them to the + // RC routes as soon as they exist so finished transfers + // publish into them. + cfg.OnServicesReady = func(s embedgw.OpsServices) { + rcH.SetOpsServices(rcroutes.OpsServices{ + Logger: s.Logger, + Metrics: s.Metrics, + Events: s.Events, + }) + } cfg.S3Options = append(s3Opts, s3api.WithRoute("POST", "/.hipobj-rc/prepare", rcAuth, rcH.Prepare), s3api.WithRoute("POST", "/.hipobj-rc/ready", rcAuth, rcH.Ready), diff --git a/embedgw/embedgw.go b/embedgw/embedgw.go index f7c6daa9..9f287b2f 100644 --- a/embedgw/embedgw.go +++ b/embedgw/embedgw.go @@ -520,6 +520,13 @@ type Config struct { // as request middleware. S3Options []s3api.Option + // OnServicesReady runs after the operational services (access + // logger, metrics manager, event sender) are created and before + // the S3 server is built, so embedders can wire them into + // components constructed earlier (such as the RDMA control + // routes). A nil callback is skipped. + OnServicesReady func(OpsServices) + // Version, Build, and BuildTime are displayed in the startup banner. // All three are optional; omit or leave empty to suppress the field. Version string @@ -527,6 +534,14 @@ type Config struct { BuildTime string } +// OpsServices bundles the operational service instances handed to +// the Config.OnServicesReady callback. +type OpsServices struct { + Logger s3log.AuditLogger + Metrics metrics.Manager + Events s3event.S3EventSender +} + // TODO: remove gatewayRunning once package-level globals (bucket-name // validation, debug logging) are eliminated and concurrent calls are safe. var gatewayRunning atomic.Bool @@ -853,6 +868,14 @@ func RunVersityGW(ctx context.Context, be backend.Backend, cfg *Config) error { })) } + if cfg.OnServicesReady != nil { + cfg.OnServicesReady(OpsServices{ + Logger: loggers.S3Logger, + Metrics: metricsManager, + Events: evSender, + }) + } + srv, err := s3api.New(be, middlewares.RootUserConfig{ Access: cfg.RootUserAccess, Secret: cfg.RootUserSecret, diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go new file mode 100644 index 00000000..e89ad6ae --- /dev/null +++ b/rdma/rcroutes/ops_linux.go @@ -0,0 +1,277 @@ +// Copyright 2026 Versity Software +// Copyright 2026 Gluesys Inc. and Jihyeon Gim +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//go:build linux && amd64 && cgo + +package rcroutes + +import ( + "errors" + "fmt" + "sync" + "time" + + "github.com/gofiber/fiber/v3" + "github.com/valyala/fasthttp" + + "github.com/versity/versitygw/auth" + "github.com/versity/versitygw/metrics" + "github.com/versity/versitygw/rdma/rcserver" + "github.com/versity/versitygw/s3api/utils" + "github.com/versity/versitygw/s3err" + "github.com/versity/versitygw/s3event" + "github.com/versity/versitygw/s3log" +) + +// OpsServices carries the operational service instances the RC routes +// report into. All three may be nil; publication then becomes a no-op +// so the control plane works without any configured backend. +type OpsServices struct { + Logger s3log.AuditLogger + Metrics metrics.Manager + Events s3event.S3EventSender +} + +// opsEmitter is the operational context captured at PREPARE and held +// until the final outcome is known: enough to synthesize an access +// record carrying the session's object rather than the wire path. +type opsEmitter struct { + ops OpsServices + app *fiber.App + acct auth.Account + region string + bucket string + key string + isPut bool + start time.Time +} + +// synthesize builds a fiber context whose path and request locals +// describe the session's logical object operation, so the standard +// access-log and event pipelines observe GET/PUT of bucket/key +// instead of the fixed RDMA control path. +func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { + ctx := e.app.AcquireCtx(&fasthttp.RequestCtx{}) + method := fiber.MethodGet + if e.isPut { + method = fiber.MethodPut + } + ctx.Method(method) + // The access logger and the event schema both split this path + // into bucket/key, so the synthesized path must be the object + // path in canonical form. + ctx.Path("/" + e.bucket + "/" + e.key) + utils.ContextKeyAccount.Set(ctx, e.acct) + utils.ContextKeyRegion.Set(ctx, e.region) + utils.ContextKeyStartTime.Set(ctx, e.start) + utils.ContextKeyIsRoot.Set(ctx, false) + requestID, hostID := utils.EnsureRequestIDs(ctx) + ctx.Request().Header.Add("X-Amz-Request-Id", requestID) + ctx.Request().Header.Add("X-Amz-Id-2", hostID) + return ctx, func() { e.app.ReleaseCtx(ctx) } +} + +// publish emits the final audit record, request metric, and (for a +// committed PUT) the object-created event. Exactly-once delivery is +// the tracker's job; this method just performs one emission. +func (e *opsEmitter) publish(err error, bytes int64) { + if e == nil || (e.ops.Logger == nil && e.ops.Metrics == nil && e.ops.Events == nil) { + return + } + ctx, release := e.synthesize() + defer release() + + action := metrics.ActionGetObject + if e.isPut { + action = metrics.ActionPutObject + } + status := httpStatusFromError(err) + + if e.ops.Metrics != nil { + e.ops.Metrics.Send(ctx, err, action, bytes, status) + } + if e.ops.Logger != nil { + e.ops.Logger.Log(ctx, err, nil, s3log.LogMeta{ + Action: action, + }) + } + // The object-created event fires at commit time only; error + // publications never carry it. + if e.ops.Events != nil && err == nil && e.isPut { + e.ops.Events.SendEvent(ctx, s3event.EventMeta{ + EventName: s3event.EventObjectCreatedPut, + ObjectSize: bytes, + }) + } +} + +// httpStatusFromError maps an operation error to the HTTP status +// the S3 surface would have answered with. +func httpStatusFromError(err error) int { + if err == nil { + return 200 + } + var serr s3err.S3Error + if errors.As(err, &serr) { + return serr.StatusCode() + } + return 500 +} + +// sessionRecord is one tracked session with its captured context. +type sessionRecord struct { + emit *opsEmitter + done bool + pending bool +} + +// opsTracker owns terminal publication: each session (and each +// pre-session request) is published exactly once, by whichever path +// confirms the final outcome first. It carries its own throwaway +// fiber app: the synthesized contexts only carry path and locals, +// never route state, so they must not share the gateway app. +type opsTracker struct { + mu sync.Mutex + sessions map[string]*sessionRecord + ops OpsServices + app *fiber.App +} + +func newOpsTracker() *opsTracker { + return &opsTracker{ + sessions: map[string]*sessionRecord{}, + app: fiber.New(), + } +} + +// SetOpsServices installs the operational service instances. The +// gateway creates the logger, metrics manager, and event sender +// after the RC routes exist, so the tracker starts empty and the +// embedder injects them once RunVersityGW has built them. Sessions +// registered before the injection publish nothing (there are none: +// the server is not listening yet). +func (t *opsTracker) SetOpsServices(ops OpsServices) { + t.mu.Lock() + defer t.mu.Unlock() + t.ops = ops +} + +// opsSnapshot returns the current service set under the lock. +func (t *opsTracker) opsSnapshot() OpsServices { + t.mu.Lock() + defer t.mu.Unlock() + return t.ops +} + +// register captures the operational context of a successfully created +// session so a later terminal path can publish its final outcome. +func (t *opsTracker) register(sessionID string, acct auth.Account, + region, bucket, key string, isPut bool, start time.Time) { + emit := &opsEmitter{ + ops: t.opsSnapshot(), + app: t.app, + acct: acct, + region: region, + bucket: bucket, + key: key, + isPut: isPut, + start: start, + } + t.mu.Lock() + defer t.mu.Unlock() + t.sessions[sessionID] = &sessionRecord{emit: emit, pending: true} +} + +// claim removes the session's publication slot and returns its +// captured context; the second caller gets nil and publishes nothing. +func (t *opsTracker) claim(sessionID string) *opsEmitter { + t.mu.Lock() + defer t.mu.Unlock() + rec, ok := t.sessions[sessionID] + if !ok { + return nil + } + delete(t.sessions, sessionID) + if !rec.pending { + return nil + } + return rec.emit +} + +// onTerminal is the native teardown callback path: publish sessions +// that no handler ever claimed (expired, abandoned, or canceled +// without a READY). +func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { + if t == nil { + return + } + emit := t.claim(ev.SessionID) + if emit == nil { + return + } + // An expired or abandoned session never reached a final object + // result; the bytes staged for it did not become a transfer. + emit.publish(errSessionExpired, 0) +} + +// publishClaimed publishes the terminal record from the request +// path (READY completion or failure). A nil tracker (no operational +// services) and an unknown session (already claimed or never +// registered) are both silent no-ops. +func (t *opsTracker) publishClaimed(sessionID string, err error, bytes ...int64) { + if t == nil { + return + } + emit := t.claim(sessionID) + if emit == nil { + return + } + var n int64 + if len(bytes) > 0 { + n = bytes[0] + } + emit.publish(err, n) +} + +// publishRequest emits an operation record for a request that ended +// before any session existed (authentication, authorization, or +// header failures): no tracking table entry, single emission. +func (t *opsTracker) publishRequest(ctx fiber.Ctx, err error, + bucket, key string, isPut bool) { + if t == nil { + return + } + emit := &opsEmitter{ + ops: t.ops, + app: t.app, + region: regionFromCtx(ctx), + bucket: bucket, + key: key, + isPut: isPut, + start: time.Now(), + } + emit.publish(err, 0) +} + +// regionFromCtx reads the region the gateway middleware stored on +// the live request; the synthesized publication reuses it. +func regionFromCtx(ctx fiber.Ctx) string { + if v, ok := utils.ContextKeyRegion.Get(ctx).(string); ok { + return v + } + return "" +} + +var errSessionExpired = fmt.Errorf("session expired") diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go new file mode 100644 index 00000000..d7b34a99 --- /dev/null +++ b/rdma/rcroutes/ops_linux_test.go @@ -0,0 +1,104 @@ +// Copyright 2026 Versity Software +// Copyright 2026 Gluesys Inc. and Jihyeon Gim +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +<<<<<<< Updated upstream +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +// either express or implied. See the License for the specific +// language governing permissions and limitations under the +// License. +======= +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +>>>>>>> Stashed changes + +//go:build linux && amd64 && cgo + +package rcroutes + +import ( + "errors" + "testing" + "time" + + "github.com/versity/versitygw/auth" + "github.com/versity/versitygw/rdma/rcserver" + "github.com/versity/versitygw/s3err" +) + +// The tests exercise the tracker's ownership semantics: the session +// table is the single source of truth for who may publish, so the +// assertions watch table membership rather than the emission itself +// (emission is a no-op without operational services wired in). + +func TestOpsTrackerExpiryPublication(t *testing.T) { + tr := newOpsTracker() + tr.register("sess-1", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", false, time.Now()) + if got := len(tr.sessions); got != 1 { + t.Fatalf("registered sessions = %d, want 1", got) + } + + // The reaper path claims and removes the session. + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-1"}) + if got := len(tr.sessions); got != 0 { + t.Fatalf("session survived terminal: %d", got) + } + + // A second terminal (double reap) finds nothing. + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-1"}) + if got := len(tr.sessions); got != 0 { + t.Fatalf("double terminal left residue: %d", got) + } +} + +func TestOpsTrackerRequestPathClaims(t *testing.T) { + tr := newOpsTracker() + tr.register("sess-2", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", true, time.Now()) + + // READY completion claims the publication first. + tr.publishClaimed("sess-2", nil, 4096) + if got := len(tr.sessions); got != 0 { + t.Fatalf("request path left session: %d", got) + } + + // The late reaper callback finds nothing left. + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-2"}) + if got := len(tr.sessions); got != 0 { + t.Fatalf("reaper re-added session: %d", got) + } +} + +func TestOpsTrackerUnknownSession(t *testing.T) { + tr := newOpsTracker() + // Unknown sessions and the nil tracker are silent no-ops. + var nilTracker *opsTracker + nilTracker.publishClaimed("ghost", nil, 1) + nilTracker.onTerminal(rcserver.TerminalEvent{SessionID: "ghost"}) + tr.publishClaimed("ghost", nil, 1) + tr.onTerminal(rcserver.TerminalEvent{SessionID: "ghost"}) + if got := len(tr.sessions); got != 0 { + t.Fatalf("ghost session materialized: %d", got) + } +} + +func TestHttpStatusFromError(t *testing.T) { + if got := httpStatusFromError(nil); got != 200 { + t.Fatalf("nil error => %d, want 200", got) + } + if got := httpStatusFromError(errors.New("x")); got != 500 { + t.Fatalf("plain error => %d, want 500", got) + } + if got := httpStatusFromError(s3err.GetAPIError(s3err.ErrAccessDenied)); got != 403 { + t.Fatalf("access denied => %d, want 403", got) + } +} diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index e0a6c818..b770708b 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -32,6 +32,7 @@ import ( "net/url" "strconv" "strings" + "time" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -76,13 +77,31 @@ type Handler struct { iam auth.IAMService readonly bool disableACL bool + // ops owns terminal publication into the operational services + // (access log, request metrics, object events); nil keeps the + // routes uninstrumented. + ops *opsTracker } -// New builds the route handler around a started RC service. +// SetOpsServices injects the operational service instances once the +// gateway has created them, and wires the native teardown callback +// that publishes sessions no request path ever completed. +func (h *Handler) SetOpsServices(ops OpsServices) { + if h.ops == nil { + return + } + h.ops.SetOpsServices(ops) + h.svc.SetTerminalNotify(h.ops.onTerminal) +} + +// New builds the route handler around a started RC service. The +// operational services arrive later through SetOpsServices, once +// the gateway has created them. func New(svc *rcserver.RCSvc, be backend.Backend, iam auth.IAMService, readonly, disableACL bool) *Handler { return &Handler{svc: svc, be: be, iam: iam, - readonly: readonly, disableACL: disableACL} + readonly: readonly, disableACL: disableACL, + ops: newOpsTracker()} } // principalID derives the session identity digest from the @@ -159,6 +178,7 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { // Authorize through the regular object-access chain. if err := h.authorize(ctx, acct, isRoot, bucket, key, isPut); err != nil { + h.ops.publishRequest(ctx, err, bucket, key, isPut) return err } @@ -173,6 +193,7 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { ClientToken: ctx.Get(hdrToken), }) if err != nil { + h.ops.publishRequest(ctx, mapRcError(err), bucket, key, isPut) return mapRcError(err) } @@ -181,13 +202,23 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { if !isPut { if err := h.stageGet(ctx, resp.SessionID, bucket, key, offset, size); err != nil { _ = h.svc.FinishPrepare(resp.SessionID, false) + h.ops.publishRequest(ctx, err, bucket, key, isPut) return err } } if err := h.svc.FinishPrepare(resp.SessionID, true); err != nil { + h.ops.publishRequest(ctx, mapRcError(err), bucket, key, isPut) return mapRcError(err) } + // The session now owns the operation record: the terminal + // path (READY/FinishPut completion, CANCEL, or the expiry + // reaper) publishes the final outcome exactly once. + if h.ops != nil { + h.ops.register(resp.SessionID, acct, + regionFromCtx(ctx), bucket, key, isPut, time.Now()) + } + // Wire reply per the hipobj-rc-v2 contract: protocol echo, // the server endpoint as "200:", session id, and PSN. ctx.Set(hdrProtocol, protocolValue) @@ -380,6 +411,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { finalized = true } if err != nil { + h.ops.publishClaimed(sessionID, err) return err } // The FINAL wire reply carries the stored object's @@ -387,11 +419,16 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { resp.Etag = put.ETag resp.VersionID = put.VersionID } else if err := h.svc.FinishFinal(sessionID); err != nil { + h.ops.publishClaimed(sessionID, mapRcError(err)) return mapRcError(err) } else { finalized = true } + // The transfer completed: publish the terminal record with + // the byte count the data plane reported. + h.ops.publishClaimed(sessionID, nil, int64(resp.BytesTransferred)) + // Wire reply per the hipobj-rc-v2 contract: protocol echo, // cookie echo, transferred bytes, and object metadata. ctx.Set(hdrProtocol, protocolValue) diff --git a/rdma/rcroutes/routes_stub.go b/rdma/rcroutes/routes_stub.go index 387f4a5e..bace981b 100644 --- a/rdma/rcroutes/routes_stub.go +++ b/rdma/rcroutes/routes_stub.go @@ -48,3 +48,16 @@ func (h *Handler) Ready(ctx fiber.Ctx) error { return notImplemented(ctx) } // Cancel is a stub handler that answers 501 Not Implemented. func (h *Handler) Cancel(ctx fiber.Ctx) error { return notImplemented(ctx) } + +// OpsServices carries the operational service instances (stub +// mirror; the fields exist only to keep the embedding surface +// platform-independent). +type OpsServices struct { + Logger any + Metrics any + Events any +} + +// SetOpsServices is a stub: without RDMA support there is nothing +// to publish into. +func (h *Handler) SetOpsServices(ops OpsServices) {} diff --git a/rdma/rcserver/rcserver_linux.go b/rdma/rcserver/rcserver_linux.go index 2713c1c7..aad73e0c 100644 --- a/rdma/rcserver/rcserver_linux.go +++ b/rdma/rcserver/rcserver_linux.go @@ -34,6 +34,8 @@ package rcserver extern void rcgo_log_sink(void *ctx, int level, char *msg, char *file, int line); extern void rcgo_snapshot_cb(rc_session_snapshot *rec, void *ctx); +extern void rcgo_terminal_cb(void *ctx, char *id, int outcome, + uint64_t bytes); */ import "C" @@ -547,6 +549,53 @@ var ( snapshotSink *[]SessionSnapshot ) +// TerminalEvent describes one reaped session, delivered through the +// terminal notification sink. +type TerminalEvent struct { + SessionID string + Outcome int // RC_READY_* value observed at teardown + Bytes uint64 +} + +// terminalNotify serializes the trampoline callback and stores the +// subscriber. The RC service has a single instance per gateway, so +// one process-wide sink matches the log-sink pattern. +var ( + terminalNotifyMu sync.Mutex + terminalNotify func(TerminalEvent) +) + +//export rcgo_terminal_cb +func rcgo_terminal_cb(_ unsafe.Pointer, id *C.char, outcome C.int, bytes C.uint64_t) { + if id == nil { + return + } + terminalNotifyMu.Lock() + fn := terminalNotify + terminalNotifyMu.Unlock() + if fn == nil { + return + } + fn(TerminalEvent{ + SessionID: C.GoString(id), + Outcome: int(outcome), + Bytes: uint64(bytes), + }) +} + +// SetTerminalNotify installs the session-teardown callback. The C +// side invokes it with no lock held; the callback must return +// promptly and must not call back into the service. +func (s *RCSvc) SetTerminalNotify(fn func(TerminalEvent)) { + terminalNotifyMu.Lock() + terminalNotify = fn + terminalNotifyMu.Unlock() + if s.srv != nil { + C.rc_server_set_terminal_notify(s.srv, + (*[0]byte)(C.rcgo_terminal_cb), nil) + } +} + //export rcgo_snapshot_cb func rcgo_snapshot_cb(rec *C.rc_session_snapshot, _ unsafe.Pointer) { sink := snapshotSink diff --git a/rdma/rcserver/rcserver_stub.go b/rdma/rcserver/rcserver_stub.go index 99e6b220..493e3b1a 100644 --- a/rdma/rcserver/rcserver_stub.go +++ b/rdma/rcserver/rcserver_stub.go @@ -178,6 +178,16 @@ func (s *RCSvc) SessionsSnapshot() ([]SessionSnapshot, error) { return nil, errNotSupported } +// TerminalEvent is a stub mirror of the linux record. +type TerminalEvent struct { + SessionID string + Outcome int + Bytes uint64 +} + +// SetTerminalNotify is a stub. +func (s *RCSvc) SetTerminalNotify(fn func(TerminalEvent)) {} + // ReadyTransfer is a stub. func (s *RCSvc) ReadyTransfer(req ReadyRequest) (*ReadyResponse, error) { return nil, errNotSupported From 39aae5435cc9e6e07e9b8686470c738a1c6af1c7 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 03:15:02 +0900 Subject: [PATCH 03/16] rdma: close publication gaps for RC pre-session failures Review of the operational publication found four gaps where the records disagreed with the S3 surface or were missing entirely. Authorization-failure records lost the requester because the pre-session publisher did not carry the authenticated account; the account now flows into the record, so the audit trail names who was denied. Signature failures and malformed PREPARE headers ended the request before any publication point. The auth adapter now publishes authentication failures through the route handler, and header validation failures publish with whatever object identity the headers still carried, matching the S3 surface where every denied request still logs. RC requests carried no region: the custom routes run before the middleware that stores the region local, so event records lacked awsRegion and audit host headers read s3..amazonaws.com. The auth adapter sets the region for every verified RC request. Operational records reported generic 500 statuses for protocol errors that the wire answers with a specific status (a resource limit rejection logged 500 while the client saw 429). Status mapping now reuses the route error mapping, so the recorded status always equals the wire status. --- cmd/vgwrdma/main.go | 8 +++++- rdma/rcroutes/ops_linux.go | 17 +++++-------- rdma/rcroutes/routes_linux.go | 48 ++++++++++++++++++++++++----------- rdma/rcroutes/routes_stub.go | 3 +++ 4 files changed, 50 insertions(+), 26 deletions(-) diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index 4cf78035..bbb9d963 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -1287,13 +1287,19 @@ func runGateway(ctx context.Context, be backend.Backend) error { // nil on success without doing so. Wrap it so a verified // request reaches the route handler, while errors end the // chain as usual. + rcH := rcroutes.New(rcSvc, be, iamSvc, readonly, disableACLs) rcAuth := func(ctx fiber.Ctx) error { + // The RC routes run before the default-values + // middleware sets the request locals; the access + // logger and event schema read the region from + // there, so set it for every verified request. + utils.ContextKeyRegion.Set(ctx, region) if err := rcVerify(ctx); err != nil { + rcH.PublishAuthFailure(ctx, err) return rcroutes.WriteRouteError(ctx, err) } return ctx.Next() } - rcH := rcroutes.New(rcSvc, be, iamSvc, readonly, disableACLs) // The gateway builds the access logger, metrics manager, // and event sender inside RunVersityGW; hand them to the // RC routes as soon as they exist so finished transfers diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index e89ad6ae..341ec7ad 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -18,7 +18,6 @@ package rcroutes import ( - "errors" "fmt" "sync" "time" @@ -30,7 +29,6 @@ import ( "github.com/versity/versitygw/metrics" "github.com/versity/versitygw/rdma/rcserver" "github.com/versity/versitygw/s3api/utils" - "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3event" "github.com/versity/versitygw/s3log" ) @@ -118,16 +116,14 @@ func (e *opsEmitter) publish(err error, bytes int64) { } // httpStatusFromError maps an operation error to the HTTP status -// the S3 surface would have answered with. +// the S3 surface would have answered with, using the same route +// error mapping as the wire response so operational records never +// disagree with what the client saw. func httpStatusFromError(err error) int { if err == nil { return 200 } - var serr s3err.S3Error - if errors.As(err, &serr) { - return serr.StatusCode() - } - return 500 + return routeError(err).HTTPStatusCode } // sessionRecord is one tracked session with its captured context. @@ -248,14 +244,15 @@ func (t *opsTracker) publishClaimed(sessionID string, err error, bytes ...int64) // publishRequest emits an operation record for a request that ended // before any session existed (authentication, authorization, or // header failures): no tracking table entry, single emission. -func (t *opsTracker) publishRequest(ctx fiber.Ctx, err error, - bucket, key string, isPut bool) { +func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, + err error, bucket, key string, isPut bool) { if t == nil { return } emit := &opsEmitter{ ops: t.ops, app: t.app, + acct: acct, region: regionFromCtx(ctx), bucket: bucket, key: key, diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index b770708b..cb896f2d 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -83,6 +83,14 @@ type Handler struct { ops *opsTracker } +// PublishAuthFailure emits an operation record for a request whose +// authentication failed before any route logic ran. The gateway +// auth adapter calls it so signature failures appear in the access +// log like they do on the S3 surface. +func (h *Handler) PublishAuthFailure(ctx fiber.Ctx, err error) { + h.ops.publishRequest(ctx, auth.Account{}, err, "", "", false) +} + // SetOpsServices injects the operational service instances once the // gateway has created them, and wires the native teardown callback // that publishes sessions no request path ever completed. @@ -142,43 +150,53 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { } defer h.svc.Leave() - if proto := ctx.Get(hdrProtocol); proto != protocolValue { - return invalidHeader(hdrProtocol, proto) - } - acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) + // Header parse failures end the request before authorization; + // publish them as request records too, with whatever object + // identity the malformed headers still carried. + publishHeaderErr := func(err error) error { + target := ctx.Get(hdrTarget) + bucket, key, _ := splitTarget(target) + h.ops.publishRequest(ctx, acct, err, bucket, key, false) + return err + } + + if proto := ctx.Get(hdrProtocol); proto != protocolValue { + return publishHeaderErr(invalidHeader(hdrProtocol, proto)) + } + op := strings.ToUpper(ctx.Get(hdrOp)) if op != "GET" && op != "PUT" { - return invalidHeader(hdrOp, ctx.Get(hdrOp)) + return publishHeaderErr(invalidHeader(hdrOp, ctx.Get(hdrOp))) } + isPut := op == "PUT" target := ctx.Get(hdrTarget) bucket, key, ok := splitTarget(target) if !ok { - return invalidHeader(hdrTarget, target) + return publishHeaderErr(invalidHeader(hdrTarget, target)) } size, err := parseUint(ctx.Get(hdrSize), 10, 64) if err != nil || size == 0 { - return invalidHeader(hdrSize, ctx.Get(hdrSize)) + return publishHeaderErr(invalidHeader(hdrSize, ctx.Get(hdrSize))) } offset, err := parseUint(ctx.Get(hdrOffset), 10, 64) if err != nil { - return invalidHeader(hdrOffset, ctx.Get(hdrOffset)) + return publishHeaderErr(invalidHeader(hdrOffset, ctx.Get(hdrOffset))) } psn, err := parseUint(ctx.Get(hdrPsn), 16, 32) if err != nil || psn == 0 || psn > 0xffffff { - return invalidHeader(hdrPsn, ctx.Get(hdrPsn)) + return publishHeaderErr(invalidHeader(hdrPsn, ctx.Get(hdrPsn))) } cookie, err := parseUint(ctx.Get(hdrCookie), 16, 32) if err != nil || cookie == 0 { - return invalidHeader(hdrCookie, ctx.Get(hdrCookie)) + return publishHeaderErr(invalidHeader(hdrCookie, ctx.Get(hdrCookie))) } - isPut := op == "PUT" // Authorize through the regular object-access chain. if err := h.authorize(ctx, acct, isRoot, bucket, key, isPut); err != nil { - h.ops.publishRequest(ctx, err, bucket, key, isPut) + h.ops.publishRequest(ctx, acct, err, bucket, key, isPut) return err } @@ -193,7 +211,7 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { ClientToken: ctx.Get(hdrToken), }) if err != nil { - h.ops.publishRequest(ctx, mapRcError(err), bucket, key, isPut) + h.ops.publishRequest(ctx, acct, mapRcError(err), bucket, key, isPut) return mapRcError(err) } @@ -202,12 +220,12 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { if !isPut { if err := h.stageGet(ctx, resp.SessionID, bucket, key, offset, size); err != nil { _ = h.svc.FinishPrepare(resp.SessionID, false) - h.ops.publishRequest(ctx, err, bucket, key, isPut) + h.ops.publishRequest(ctx, acct, err, bucket, key, isPut) return err } } if err := h.svc.FinishPrepare(resp.SessionID, true); err != nil { - h.ops.publishRequest(ctx, mapRcError(err), bucket, key, isPut) + h.ops.publishRequest(ctx, acct, mapRcError(err), bucket, key, isPut) return mapRcError(err) } diff --git a/rdma/rcroutes/routes_stub.go b/rdma/rcroutes/routes_stub.go index bace981b..6a48adab 100644 --- a/rdma/rcroutes/routes_stub.go +++ b/rdma/rcroutes/routes_stub.go @@ -61,3 +61,6 @@ type OpsServices struct { // SetOpsServices is a stub: without RDMA support there is nothing // to publish into. func (h *Handler) SetOpsServices(ops OpsServices) {} + +// PublishAuthFailure is a stub mirror of the linux handler. +func (h *Handler) PublishAuthFailure(ctx fiber.Ctx, err error) {} From 279f5e7d2fe0e9742e9b88b66a6c2379dbff6a31 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 03:32:31 +0900 Subject: [PATCH 04/16] rdma: harden RC outcome publication ownership and record fidelity Review of the publication path found that ownership could change hands at the wrong moment and that records could disagree with both the wire response and the underlying operation. Successful transfers lost their completion record: the native completion calls fire the teardown callback synchronously, so the callback claimed the publication first and logged every completed transfer as an expiry, and committed PUTs produced no object-created events. The READY handler now reserves the publication before invoking any completion call; a reserved record is invisible to the callback, and the handler publishes the real outcome exactly once. The same race existed at creation: the PREPARE finalizer can reap an expired session and fire the callback before the session is registered, leaving an orphan entry whose only notification already happened. Registration now runs before the finalizing call, a notification that arrives first is parked and consumed by the registration, and a failed finalization drops the entry. Retained records referenced the request's pooled header buffer, so a later request could rewrite a tracked session's bucket and key; captured strings are cloned now. The synthesized publication path follows the same rule for the event senders, which serialize asynchronously. Operational sinks classify plain errors as 500 on their own, so a resource-limit rejection logged 500 while the client saw 429. The publication renders non-S3 errors through the route error mapping before the record reaches the sinks, and the expiry record carries a dedicated SessionExpired code instead of a generic one. Malformed PUT headers now preserve the operation in the record. --- rdma/rcroutes/ops_linux.go | 232 ++++++++++++++++++++++++-------- rdma/rcroutes/ops_linux_test.go | 70 ++++++++++ rdma/rcroutes/routes_linux.go | 54 +++++--- 3 files changed, 282 insertions(+), 74 deletions(-) diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 341ec7ad..e055aa29 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -18,7 +18,8 @@ package rcroutes import ( - "fmt" + "errors" + "strings" "sync" "time" @@ -29,6 +30,7 @@ import ( "github.com/versity/versitygw/metrics" "github.com/versity/versitygw/rdma/rcserver" "github.com/versity/versitygw/s3api/utils" + "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3event" "github.com/versity/versitygw/s3log" ) @@ -45,6 +47,9 @@ type OpsServices struct { // opsEmitter is the operational context captured at PREPARE and held // until the final outcome is known: enough to synthesize an access // record carrying the session's object rather than the wire path. +// Every string field is owned storage: nothing may reference the +// request's pooled buffers once PREPARE returns, because fasthttp +// reuses them for the next request. type opsEmitter struct { ops OpsServices app *fiber.App @@ -59,7 +64,9 @@ type opsEmitter struct { // synthesize builds a fiber context whose path and request locals // describe the session's logical object operation, so the standard // access-log and event pipelines observe GET/PUT of bucket/key -// instead of the fixed RDMA control path. +// instead of the fixed RDMA control path. The path string is the +// emitter's own storage: event senders serialize asynchronously, so +// the synthesized context must never hand them pooled buffers. func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { ctx := e.app.AcquireCtx(&fasthttp.RequestCtx{}) method := fiber.MethodGet @@ -69,7 +76,9 @@ func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { ctx.Method(method) // The access logger and the event schema both split this path // into bucket/key, so the synthesized path must be the object - // path in canonical form. + // path in canonical form. fiber copies override strings it + // stores as the path original; the derived c.path below is + // a fresh allocation, which is what outlives the release. ctx.Path("/" + e.bucket + "/" + e.key) utils.ContextKeyAccount.Set(ctx, e.acct) utils.ContextKeyRegion.Set(ctx, e.region) @@ -84,10 +93,23 @@ func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { // publish emits the final audit record, request metric, and (for a // committed PUT) the object-created event. Exactly-once delivery is // the tracker's job; this method just performs one emission. +// +// The operational sinks classify plain errors as 500 on their own, +// which would disagree with the status the client saw. Before the +// record reaches them, the error is rendered as its mapped S3 error, +// so the audit log, the metric, and the wire response all carry the +// same classification. func (e *opsEmitter) publish(err error, bytes int64) { if e == nil || (e.ops.Logger == nil && e.ops.Metrics == nil && e.ops.Events == nil) { return } + sinkErr := err + if err != nil { + var s3Err s3err.S3Error + if !errors.As(err, &s3Err) { + sinkErr = routeError(err) + } + } ctx, release := e.synthesize() defer release() @@ -95,13 +117,13 @@ func (e *opsEmitter) publish(err error, bytes int64) { if e.isPut { action = metrics.ActionPutObject } - status := httpStatusFromError(err) + status := httpStatusFromError(sinkErr) if e.ops.Metrics != nil { - e.ops.Metrics.Send(ctx, err, action, bytes, status) + e.ops.Metrics.Send(ctx, sinkErr, action, bytes, status) } if e.ops.Logger != nil { - e.ops.Logger.Log(ctx, err, nil, s3log.LogMeta{ + e.ops.Logger.Log(ctx, sinkErr, nil, s3log.LogMeta{ Action: action, }) } @@ -116,9 +138,8 @@ func (e *opsEmitter) publish(err error, bytes int64) { } // httpStatusFromError maps an operation error to the HTTP status -// the S3 surface would have answered with, using the same route -// error mapping as the wire response so operational records never -// disagree with what the client saw. +// the S3 surface would have answered with. The callers pass mapped +// S3 errors, so the status is simply the error's own. func httpStatusFromError(err error) int { if err == nil { return 200 @@ -128,81 +149,157 @@ func httpStatusFromError(err error) int { // sessionRecord is one tracked session with its captured context. type sessionRecord struct { - emit *opsEmitter - done bool - pending bool + emit *opsEmitter + // reserved marks a record the request path holds exclusively: + // it took ownership before invoking a native completion call + // that would fire the teardown callback synchronously, so the + // callback must not publish on its behalf. + reserved bool } // opsTracker owns terminal publication: each session (and each // pre-session request) is published exactly once, by whichever path // confirms the final outcome first. It carries its own throwaway -// fiber app: the synthesized contexts only carry path and locals, -// never route state, so they must not share the gateway app. +// fiber.App for synthesizing publication contexts, independent of +// the gateway's request routing. type opsTracker struct { mu sync.Mutex - sessions map[string]*sessionRecord ops OpsServices - app *fiber.App + sessions map[string]*sessionRecord + // earlyTerminals parks teardown notifications that arrived + // before the session's registration; register consumes them. + earlyTerminals map[string]rcserver.TerminalEvent + app *fiber.App } func newOpsTracker() *opsTracker { return &opsTracker{ - sessions: map[string]*sessionRecord{}, - app: fiber.New(), + sessions: map[string]*sessionRecord{}, + earlyTerminals: map[string]rcserver.TerminalEvent{}, + app: fiber.New(), } } // SetOpsServices installs the operational service instances. The // gateway creates the logger, metrics manager, and event sender // after the RC routes exist, so the tracker starts empty and the -// embedder injects them once RunVersityGW has built them. Sessions -// registered before the injection publish nothing (there are none: -// the server is not listening yet). +// services arrive here. Sessions registered before the injection +// publish nothing (there are none: the gateway wires this before +// it starts serving). func (t *opsTracker) SetOpsServices(ops OpsServices) { t.mu.Lock() defer t.mu.Unlock() t.ops = ops } -// opsSnapshot returns the current service set under the lock. -func (t *opsTracker) opsSnapshot() OpsServices { +// register captures the operational context of a successfully created +// session so a later terminal path can publish its final outcome. +// The strings are cloned: they originate from the request's pooled +// header buffer, which does not survive the response. +// +// handleEarlyTerminal covers the FinishPrepare race: the native call +// that finalizes PREPARE can reap an already-expired session and fire +// the teardown callback before register runs. When the callback wins +// that race it parks the event, and register consumes it instead of +// leaving an entry whose only notification already happened. +func (t *opsTracker) register(sessionID string, acct auth.Account, + region, bucket, key string, isPut bool, start time.Time) { + emit := &opsEmitter{ + ops: t.loadOps(), + app: t.app, + acct: acct, + region: region, + bucket: strings.Clone(bucket), + key: strings.Clone(key), + isPut: isPut, + start: start, + } + + t.mu.Lock() + defer t.mu.Unlock() + // A teardown notification that arrived before this registration + // owns the publication: publish now and store nothing. + if _, parked := t.earlyTerminals[sessionID]; parked { + delete(t.earlyTerminals, sessionID) + go emit.publish(errSessionExpired, 0) + return + } + t.sessions[sessionID] = &sessionRecord{emit: emit} +} + +// unregister drops a session entry whose PREPARE finalization +// failed: the native side is gone, so the teardown callback has +// either already published or will find nothing. A parked early +// notification is dropped with it (the failure publication covers +// the outcome). +func (t *opsTracker) unregister(sessionID string) { + if t == nil { + return + } + t.mu.Lock() + defer t.mu.Unlock() + delete(t.sessions, sessionID) + delete(t.earlyTerminals, sessionID) +} + +// reserve takes exclusive ownership of a session's publication +// before the request path invokes a native completion call +// (FinishFinal, FinishPut, or a reap-triggering mutation). Those +// calls fire the teardown callback synchronously while the session +// record is still live; reserving first keeps the callback from +// publishing an expiry record for a transfer that is completing +// right now. +func (t *opsTracker) reserve(sessionID string) *opsEmitter { + t.mu.Lock() + defer t.mu.Unlock() + rec, ok := t.sessions[sessionID] + if !ok || rec.reserved { + return nil + } + rec.reserved = true + return rec.emit +} + +// unreserve restores callback ownership when a reserved completion +// call did not after all retire the session (the caller failed +// before any state change). The record goes back to normal tracking +// unless a teardown notification landed meanwhile. +func (t *opsTracker) unreserve(sessionID string, emit *opsEmitter) { + if t == nil || emit == nil { + return + } + t.mu.Lock() + defer t.mu.Unlock() + rec, ok := t.sessions[sessionID] + if !ok { + // The session is gone: the completion call retired it and + // the reserved emitter is the only remaining owner, so + // nothing to restore. + return + } + rec.reserved = false +} + +func (t *opsTracker) loadOps() OpsServices { t.mu.Lock() defer t.mu.Unlock() return t.ops } -// register captures the operational context of a successfully created -// session so a later terminal path can publish its final outcome. -func (t *opsTracker) register(sessionID string, acct auth.Account, - region, bucket, key string, isPut bool, start time.Time) { - emit := &opsEmitter{ - ops: t.opsSnapshot(), - app: t.app, - acct: acct, - region: region, - bucket: bucket, - key: key, - isPut: isPut, - start: start, - } - t.mu.Lock() - defer t.mu.Unlock() - t.sessions[sessionID] = &sessionRecord{emit: emit, pending: true} -} - -// claim removes the session's publication slot and returns its -// captured context; the second caller gets nil and publishes nothing. -func (t *opsTracker) claim(sessionID string) *opsEmitter { +// claim removes the session from the table and returns its emitter +// to exactly one publisher. A reserved record is only claimable by +// its reserving request path (the callback skips it). +func (t *opsTracker) claim(sessionID string, byRequest bool) *opsEmitter { t.mu.Lock() defer t.mu.Unlock() rec, ok := t.sessions[sessionID] if !ok { return nil } - delete(t.sessions, sessionID) - if !rec.pending { + if rec.reserved && !byRequest { return nil } + delete(t.sessions, sessionID) return rec.emit } @@ -213,13 +310,22 @@ func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { if t == nil { return } - emit := t.claim(ev.SessionID) - if emit == nil { + emit := t.claim(ev.SessionID, false) + if emit != nil { + // An expired or abandoned session never reached a final + // object result; the bytes staged for it did not become + // a transfer. + emit.publish(errSessionExpired, 0) return } - // An expired or abandoned session never reached a final object - // result; the bytes staged for it did not become a transfer. - emit.publish(errSessionExpired, 0) + // A session tearing down before its registration ran: park the + // event so register can publish instead of orphaning a record + // whose only notification already happened. + t.mu.Lock() + if _, live := t.sessions[ev.SessionID]; !live { + t.earlyTerminals[ev.SessionID] = ev + } + t.mu.Unlock() } // publishClaimed publishes the terminal record from the request @@ -230,7 +336,7 @@ func (t *opsTracker) publishClaimed(sessionID string, err error, bytes ...int64) if t == nil { return } - emit := t.claim(sessionID) + emit := t.claim(sessionID, true) if emit == nil { return } @@ -250,12 +356,12 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, return } emit := &opsEmitter{ - ops: t.ops, + ops: t.loadOps(), app: t.app, acct: acct, region: regionFromCtx(ctx), - bucket: bucket, - key: key, + bucket: strings.Clone(bucket), + key: strings.Clone(key), isPut: isPut, start: time.Now(), } @@ -271,4 +377,16 @@ func regionFromCtx(ctx fiber.Ctx) string { return "" } -var errSessionExpired = fmt.Errorf("session expired") +// expiredOutcomeError renders a parked teardown notification as the +// error the publication carries: an internal S3 error whose code +// names the expiry, so the audit log keeps the descriptive code the +// plain error used to carry instead of the generic mapping. +type sessionExpiredError struct { + s3err.APIError +} + +var errSessionExpired = sessionExpiredError{APIError: s3err.APIError{ + Code: "SessionExpired", + Description: "The RDMA transfer session expired before completion", + HTTPStatusCode: 500, +}} diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index d7b34a99..4e4c3003 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -78,6 +78,71 @@ func TestOpsTrackerRequestPathClaims(t *testing.T) { } } +func TestOpsTrackerReserveBlocksCallback(t *testing.T) { + tr := newOpsTracker() + tr.register("sess-3", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", true, time.Now()) + + // The request path reserves before invoking a native + // completion call. + emit := tr.reserve("sess-3") + if emit == nil { + t.Fatal("reserve returned nil for a live session") + } + + // The synchronous teardown callback must not publish on a + // reserved record: the entry stays put. + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-3"}) + if got := len(tr.sessions); got != 1 { + t.Fatalf("reserved session removed by callback: %d", got) + } + + // A double reserve is refused. + if tr.reserve("sess-3") != nil { + t.Fatal("double reserve succeeded") + } + + // The request path publishes through its reserved emitter. + tr.publishClaimed("sess-3", nil, 128) + if got := len(tr.sessions); got != 0 { + t.Fatalf("reserved session survived request publication: %d", got) + } +} + +func TestOpsTrackerEarlyTerminal(t *testing.T) { + tr := newOpsTracker() + + // Teardown notification for an unregistered session parks. + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-4"}) + if got := len(tr.earlyTerminals); got != 1 { + t.Fatalf("early terminal not parked: %d", got) + } + + // Registration consumes it: no live entry remains, so no + // orphan record can outlive the session. + tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", false, time.Now()) + if got := len(tr.earlyTerminals); got != 0 { + t.Fatalf("early terminal not consumed: %d", got) + } + if got := len(tr.sessions); got != 0 { + t.Fatalf("orphan session entry created: %d", got) + } +} + +func TestOpsTrackerUnregister(t *testing.T) { + tr := newOpsTracker() + tr.register("sess-5", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", false, time.Now()) + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-5"}) + tr.register("sess-6", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", false, time.Now()) + tr.unregister("sess-6") + if got := len(tr.sessions); got != 0 { + t.Fatalf("unregister left entries: %d", got) + } +} + func TestOpsTrackerUnknownSession(t *testing.T) { tr := newOpsTracker() // Unknown sessions and the nil tracker are silent no-ops. @@ -101,4 +166,9 @@ func TestHttpStatusFromError(t *testing.T) { if got := httpStatusFromError(s3err.GetAPIError(s3err.ErrAccessDenied)); got != 403 { t.Fatalf("access denied => %d, want 403", got) } + // A resource-limit rejection maps to the wire status, not a + // generic 500. + if got := httpStatusFromError(rcserver.ErrLimit); got != 429 { + t.Fatalf("limit error => %d, want 429", got) + } } diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index cb896f2d..06da7b93 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -155,43 +155,43 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { // Header parse failures end the request before authorization; // publish them as request records too, with whatever object - // identity the malformed headers still carried. - publishHeaderErr := func(err error) error { + // identity and operation the malformed headers still carried. + publishHeaderErr := func(err error, isPut bool) error { target := ctx.Get(hdrTarget) bucket, key, _ := splitTarget(target) - h.ops.publishRequest(ctx, acct, err, bucket, key, false) + h.ops.publishRequest(ctx, acct, err, bucket, key, isPut) return err } if proto := ctx.Get(hdrProtocol); proto != protocolValue { - return publishHeaderErr(invalidHeader(hdrProtocol, proto)) + return publishHeaderErr(invalidHeader(hdrProtocol, proto), false) } op := strings.ToUpper(ctx.Get(hdrOp)) if op != "GET" && op != "PUT" { - return publishHeaderErr(invalidHeader(hdrOp, ctx.Get(hdrOp))) + return publishHeaderErr(invalidHeader(hdrOp, ctx.Get(hdrOp)), false) } isPut := op == "PUT" target := ctx.Get(hdrTarget) bucket, key, ok := splitTarget(target) if !ok { - return publishHeaderErr(invalidHeader(hdrTarget, target)) + return publishHeaderErr(invalidHeader(hdrTarget, target), isPut) } size, err := parseUint(ctx.Get(hdrSize), 10, 64) if err != nil || size == 0 { - return publishHeaderErr(invalidHeader(hdrSize, ctx.Get(hdrSize))) + return publishHeaderErr(invalidHeader(hdrSize, ctx.Get(hdrSize)), isPut) } offset, err := parseUint(ctx.Get(hdrOffset), 10, 64) if err != nil { - return publishHeaderErr(invalidHeader(hdrOffset, ctx.Get(hdrOffset))) + return publishHeaderErr(invalidHeader(hdrOffset, ctx.Get(hdrOffset)), isPut) } psn, err := parseUint(ctx.Get(hdrPsn), 16, 32) if err != nil || psn == 0 || psn > 0xffffff { - return publishHeaderErr(invalidHeader(hdrPsn, ctx.Get(hdrPsn))) + return publishHeaderErr(invalidHeader(hdrPsn, ctx.Get(hdrPsn)), isPut) } cookie, err := parseUint(ctx.Get(hdrCookie), 16, 32) if err != nil || cookie == 0 { - return publishHeaderErr(invalidHeader(hdrCookie, ctx.Get(hdrCookie))) + return publishHeaderErr(invalidHeader(hdrCookie, ctx.Get(hdrCookie)), isPut) } // Authorize through the regular object-access chain. @@ -224,7 +224,18 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { return err } } + + // Register before the finalizing call: FinishPrepare can reap + // an already-expired session and fire the teardown callback + // synchronously, and a registered record (or a parked early + // notification) keeps that publication from being lost. + h.ops.register(resp.SessionID, acct, + regionFromCtx(ctx), bucket, key, isPut, time.Now()) if err := h.svc.FinishPrepare(resp.SessionID, true); err != nil { + // The finalization failed: the session is gone and the + // teardown callback (or this call's own reap) published + // the terminal record; nothing may keep the entry. + h.ops.unregister(resp.SessionID) h.ops.publishRequest(ctx, acct, mapRcError(err), bucket, key, isPut) return mapRcError(err) } @@ -232,10 +243,6 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { // The session now owns the operation record: the terminal // path (READY/FinishPut completion, CANCEL, or the expiry // reaper) publishes the final outcome exactly once. - if h.ops != nil { - h.ops.register(resp.SessionID, acct, - regionFromCtx(ctx), bucket, key, isPut, time.Now()) - } // Wire reply per the hipobj-rc-v2 contract: protocol echo, // the server endpoint as "200:", session id, and PSN. @@ -409,6 +416,19 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // The claim succeeded: from here until the response commits, // this handler owns the completion ref. A panic or early // unwind must still release it so the session can be reaped. + // + // The publication ownership moves here as well: every native + // completion call below (FinishFinal, FinishPut) fires the + // teardown callback synchronously, and a reserved record is + // invisible to that callback, so the outcome is published by + // this handler exactly once. + emit := h.ops.reserve(sessionID) + publish := func(err error, bytes int64) { + if emit != nil { + emit.publish(err, bytes) + emit = nil + } + } finalized := false defer func() { if !finalized { @@ -429,7 +449,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { finalized = true } if err != nil { - h.ops.publishClaimed(sessionID, err) + publish(mapRcError(err), 0) return err } // The FINAL wire reply carries the stored object's @@ -437,7 +457,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { resp.Etag = put.ETag resp.VersionID = put.VersionID } else if err := h.svc.FinishFinal(sessionID); err != nil { - h.ops.publishClaimed(sessionID, mapRcError(err)) + publish(mapRcError(err), 0) return mapRcError(err) } else { finalized = true @@ -445,7 +465,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // The transfer completed: publish the terminal record with // the byte count the data plane reported. - h.ops.publishClaimed(sessionID, nil, int64(resp.BytesTransferred)) + publish(nil, int64(resp.BytesTransferred)) // Wire reply per the hipobj-rc-v2 contract: protocol echo, // cookie echo, transferred bytes, and object metadata. From 23a7121bb809763f0d364ba697e6cc7b109c422a Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 04:04:49 +0900 Subject: [PATCH 05/16] rdma: make the teardown callback the single RC publication owner Round-3 review of the ownership model found that moving publication between the request path and the callback by hand leaves edges where a record is published twice or lost. The publication model is now structural instead: request paths only record outcomes, and the native teardown callback - which the ABI fires exactly once per destroyed session, after every completion call - is the single publisher. The READY handler records its outcome (success with the reported byte count, or the mapped failure) and lets the callback publish. A deferred recorder covers panic unwinds, so every path through the handler leaves a final outcome behind. A failed PREPARE finalization publishes through a consume-or-noop helper: when the finalizing call already reaped the session the entry is gone and the helper is a no-op, otherwise it publishes the failure itself. Unclaimed teardowns no longer all read as expiries: the record is classified from the native outcome, so a transfer that died on the wire, failed verification, or timed out carries its own code and status. The synthesized publication context runs on an immutable app, so string accessors copy instead of exposing the pooled request buffer to the asynchronously serializing event senders. Captured account and region strings are cloned for the same reason. The nil-error publication no longer asserts on the S3 error interface, and the panic marker is a proper internal S3 error. Unit tests cover the table semantics: expiry publication, first-record-wins, consume-or-noop failure publication, unregister, unknown sessions, error normalization, status mapping, and the outcome classification. --- rdma/rcroutes/ops_linux.go | 293 +++++++++++++++++--------------- rdma/rcroutes/ops_linux_test.go | 148 ++++++++-------- rdma/rcroutes/routes_linux.go | 64 ++++--- 3 files changed, 276 insertions(+), 229 deletions(-) diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index e055aa29..096b34a7 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -19,6 +19,7 @@ package rcroutes import ( "errors" + "net/http" "strings" "sync" "time" @@ -64,9 +65,10 @@ type opsEmitter struct { // synthesize builds a fiber context whose path and request locals // describe the session's logical object operation, so the standard // access-log and event pipelines observe GET/PUT of bucket/key -// instead of the fixed RDMA control path. The path string is the -// emitter's own storage: event senders serialize asynchronously, so -// the synthesized context must never hand them pooled buffers. +// instead of the fixed RDMA control path. The app runs with +// Immutable: string accessors copy instead of exposing the pooled +// context buffer, which matters because event senders serialize +// asynchronously and would otherwise read a reused buffer. func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { ctx := e.app.AcquireCtx(&fasthttp.RequestCtx{}) method := fiber.MethodGet @@ -76,9 +78,7 @@ func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { ctx.Method(method) // The access logger and the event schema both split this path // into bucket/key, so the synthesized path must be the object - // path in canonical form. fiber copies override strings it - // stores as the path original; the derived c.path below is - // a fresh allocation, which is what outlives the release. + // path in canonical form. ctx.Path("/" + e.bucket + "/" + e.key) utils.ContextKeyAccount.Set(ctx, e.acct) utils.ContextKeyRegion.Set(ctx, e.region) @@ -94,22 +94,15 @@ func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { // committed PUT) the object-created event. Exactly-once delivery is // the tracker's job; this method just performs one emission. // -// The operational sinks classify plain errors as 500 on their own, -// which would disagree with the status the client saw. Before the -// record reaches them, the error is rendered as its mapped S3 error, -// so the audit log, the metric, and the wire response all carry the -// same classification. +// The operational sinks classify plain errors as 500 on their own +// and unwrap nothing, so the publication always hands them the +// error in its normalized S3 form: the audit log, the metric, and +// the wire response then carry the same classification. func (e *opsEmitter) publish(err error, bytes int64) { if e == nil || (e.ops.Logger == nil && e.ops.Metrics == nil && e.ops.Events == nil) { return } - sinkErr := err - if err != nil { - var s3Err s3err.S3Error - if !errors.As(err, &s3Err) { - sinkErr = routeError(err) - } - } + sinkErr := normalizeSinkError(err) ctx, release := e.synthesize() defer release() @@ -117,7 +110,10 @@ func (e *opsEmitter) publish(err error, bytes int64) { if e.isPut { action = metrics.ActionPutObject } - status := httpStatusFromError(sinkErr) + status := http.StatusOK + if sinkErr != nil { + status = sinkErr.(s3err.APIError).HTTPStatusCode + } if e.ops.Metrics != nil { e.ops.Metrics.Send(ctx, sinkErr, action, bytes, status) @@ -137,9 +133,26 @@ func (e *opsEmitter) publish(err error, bytes int64) { } } +// normalizeSinkError renders any operation error as the plain +// s3err.APIError the sinks expect: wrapped S3 errors keep their +// payload (the audit loggers assert the S3Error interface directly +// and would misclassify a wrapper), and non-S3 errors map through +// the same route error mapping the wire response uses. +func normalizeSinkError(err error) error { + if err == nil { + return nil + } + var s3Err s3err.S3Error + if errors.As(err, &s3Err) { + return s3Err.BaseError() + } + return routeError(err) +} + // httpStatusFromError maps an operation error to the HTTP status -// the S3 surface would have answered with. The callers pass mapped -// S3 errors, so the status is simply the error's own. +// the S3 surface would have answered with, using the same route +// error mapping as the wire response so operational records never +// disagree with what the client saw. func httpStatusFromError(err error) int { if err == nil { return 200 @@ -147,36 +160,42 @@ func httpStatusFromError(err error) int { return routeError(err).HTTPStatusCode } +// sessionOutcome is the terminal outcome of a tracked session, +// recorded by whichever path observes the final result first. A +// recorded outcome is final: the teardown callback consumes it and +// publishes, so exactly one publication happens per session. +type sessionOutcome struct { + err error // nil on success + byt int64 // bytes transferred on success + done bool // outcome recorded +} + // sessionRecord is one tracked session with its captured context. type sessionRecord struct { emit *opsEmitter - // reserved marks a record the request path holds exclusively: - // it took ownership before invoking a native completion call - // that would fire the teardown callback synchronously, so the - // callback must not publish on its behalf. - reserved bool + out sessionOutcome } -// opsTracker owns terminal publication: each session (and each -// pre-session request) is published exactly once, by whichever path -// confirms the final outcome first. It carries its own throwaway -// fiber.App for synthesizing publication contexts, independent of -// the gateway's request routing. +// opsTracker owns terminal publication: each session publishes +// exactly once. The request paths only ever RECORD an outcome; the +// native teardown callback - which the ABI guarantees fires exactly +// once per destroyed session, after every completion call - is the +// single publisher. This removes every ownership race: a recorded +// outcome cannot be double-published, and a record the callback +// already consumed cannot be resurrected. type opsTracker struct { mu sync.Mutex ops OpsServices sessions map[string]*sessionRecord - // earlyTerminals parks teardown notifications that arrived - // before the session's registration; register consumes them. - earlyTerminals map[string]rcserver.TerminalEvent - app *fiber.App + app *fiber.App } func newOpsTracker() *opsTracker { return &opsTracker{ - sessions: map[string]*sessionRecord{}, - earlyTerminals: map[string]rcserver.TerminalEvent{}, - app: fiber.New(), + sessions: map[string]*sessionRecord{}, + app: fiber.New(fiber.Config{ + Immutable: true, + }), } } @@ -192,23 +211,22 @@ func (t *opsTracker) SetOpsServices(ops OpsServices) { t.ops = ops } -// register captures the operational context of a successfully created -// session so a later terminal path can publish its final outcome. +// register captures the operational context of a successfully +// created session so the terminal outcome can be published later. // The strings are cloned: they originate from the request's pooled // header buffer, which does not survive the response. // -// handleEarlyTerminal covers the FinishPrepare race: the native call -// that finalizes PREPARE can reap an already-expired session and fire -// the teardown callback before register runs. When the callback wins -// that race it parks the event, and register consumes it instead of -// leaving an entry whose only notification already happened. +// The account is captured by value but its string fields still +// reference request storage on some IAM paths, so the sink-relevant +// identity is cloned as well. func (t *opsTracker) register(sessionID string, acct auth.Account, region, bucket, key string, isPut bool, start time.Time) { + acct.Access = strings.Clone(acct.Access) emit := &opsEmitter{ ops: t.loadOps(), app: t.app, acct: acct, - region: region, + region: strings.Clone(region), bucket: strings.Clone(bucket), key: strings.Clone(key), isPut: isPut, @@ -217,21 +235,14 @@ func (t *opsTracker) register(sessionID string, acct auth.Account, t.mu.Lock() defer t.mu.Unlock() - // A teardown notification that arrived before this registration - // owns the publication: publish now and store nothing. - if _, parked := t.earlyTerminals[sessionID]; parked { - delete(t.earlyTerminals, sessionID) - go emit.publish(errSessionExpired, 0) - return - } t.sessions[sessionID] = &sessionRecord{emit: emit} } // unregister drops a session entry whose PREPARE finalization -// failed: the native side is gone, so the teardown callback has -// either already published or will find nothing. A parked early -// notification is dropped with it (the failure publication covers -// the outcome). +// failed before the session was committed: the native side either +// rejected it (no callback will come) or already reaped it (the +// callback found no record and published nothing). The failure +// itself is published as a request record by the caller. func (t *opsTracker) unregister(sessionID string) { if t == nil { return @@ -239,45 +250,46 @@ func (t *opsTracker) unregister(sessionID string) { t.mu.Lock() defer t.mu.Unlock() delete(t.sessions, sessionID) - delete(t.earlyTerminals, sessionID) } -// reserve takes exclusive ownership of a session's publication -// before the request path invokes a native completion call -// (FinishFinal, FinishPut, or a reap-triggering mutation). Those -// calls fire the teardown callback synchronously while the session -// record is still live; reserving first keeps the callback from -// publishing an expiry record for a transfer that is completing -// right now. -func (t *opsTracker) reserve(sessionID string) *opsEmitter { - t.mu.Lock() - defer t.mu.Unlock() - rec, ok := t.sessions[sessionID] - if !ok || rec.reserved { - return nil - } - rec.reserved = true - return rec.emit -} - -// unreserve restores callback ownership when a reserved completion -// call did not after all retire the session (the caller failed -// before any state change). The record goes back to normal tracking -// unless a teardown notification landed meanwhile. -func (t *opsTracker) unreserve(sessionID string, emit *opsEmitter) { - if t == nil || emit == nil { +// failOutcome publishes a failed finalization exactly once: when +// the finalizing call already reaped the session its callback +// published (the entry is gone, this is a no-op); when no callback +// will ever come (the native side rejected the call) the entry is +// consumed and published here. +func (t *opsTracker) failOutcome(sessionID string, err error) { + if t == nil { return } t.mu.Lock() - defer t.mu.Unlock() rec, ok := t.sessions[sessionID] + if ok { + delete(t.sessions, sessionID) + } + t.mu.Unlock() if !ok { - // The session is gone: the completion call retired it and - // the reserved emitter is the only remaining owner, so - // nothing to restore. return } - rec.reserved = false + rec.emit.publish(err, 0) +} + +// recordOutcome stores the terminal outcome of a completing +// transfer. It never publishes: publication belongs to the teardown +// callback, which fires exactly once per session after the +// completion call returns. Recording is idempotent - the first +// outcome wins - so a panic-unwind path that records after the +// normal path changed nothing. +func (t *opsTracker) recordOutcome(sessionID string, err error, bytes int64) { + if t == nil { + return + } + t.mu.Lock() + defer t.mu.Unlock() + rec, ok := t.sessions[sessionID] + if !ok || rec.out.done { + return + } + rec.out = sessionOutcome{err: err, byt: bytes, done: true} } func (t *opsTracker) loadOps() OpsServices { @@ -286,65 +298,48 @@ func (t *opsTracker) loadOps() OpsServices { return t.ops } -// claim removes the session from the table and returns its emitter -// to exactly one publisher. A reserved record is only claimable by -// its reserving request path (the callback skips it). -func (t *opsTracker) claim(sessionID string, byRequest bool) *opsEmitter { - t.mu.Lock() - defer t.mu.Unlock() - rec, ok := t.sessions[sessionID] - if !ok { - return nil - } - if rec.reserved && !byRequest { - return nil - } - delete(t.sessions, sessionID) - return rec.emit -} - -// onTerminal is the native teardown callback path: publish sessions -// that no handler ever claimed (expired, abandoned, or canceled -// without a READY). +// onTerminal is the native teardown callback: the single publisher +// of session records. It consumes the recorded outcome (success, +// failure, or expiry when no outcome was ever recorded) and removes +// the entry, so exactly one publication happens per session no +// matter which path confirmed the result. func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { if t == nil { return } - emit := t.claim(ev.SessionID, false) - if emit != nil { - // An expired or abandoned session never reached a final - // object result; the bytes staged for it did not become - // a transfer. - emit.publish(errSessionExpired, 0) + t.mu.Lock() + rec, ok := t.sessions[ev.SessionID] + if !ok { + t.mu.Unlock() return } - // A session tearing down before its registration ran: park the - // event so register can publish instead of orphaning a record - // whose only notification already happened. - t.mu.Lock() - if _, live := t.sessions[ev.SessionID]; !live { - t.earlyTerminals[ev.SessionID] = ev - } + delete(t.sessions, ev.SessionID) t.mu.Unlock() + + if !rec.out.done { + // No request path ever confirmed a result: the session + // expired, was abandoned, or was canceled. The event's + // outcome carries the native reason. + rec.out = sessionOutcome{err: expiredError(ev), done: true} + } + emit := rec.emit + emit.publish(rec.out.err, rec.out.byt) } -// publishClaimed publishes the terminal record from the request -// path (READY completion or failure). A nil tracker (no operational -// services) and an unknown session (already claimed or never -// registered) are both silent no-ops. -func (t *opsTracker) publishClaimed(sessionID string, err error, bytes ...int64) { - if t == nil { - return +// expiredError renders an unclaimed teardown as the error the +// publication carries, derived from the native outcome so the +// record names the real terminal reason instead of a generic one. +func expiredError(ev rcserver.TerminalEvent) error { + switch ev.Outcome { + case int(rcserver.ReadyWireFail): + return rcserver.ErrWire + case int(rcserver.ReadyTimeout): + return errTimeoutExpired + case int(rcserver.ReadyVerifyFail): + return errVerifyFailed + default: + return errSessionExpired } - emit := t.claim(sessionID, true) - if emit == nil { - return - } - var n int64 - if len(bytes) > 0 { - n = bytes[0] - } - emit.publish(err, n) } // publishRequest emits an operation record for a request that ended @@ -355,11 +350,12 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, if t == nil { return } + acct.Access = strings.Clone(acct.Access) emit := &opsEmitter{ ops: t.loadOps(), app: t.app, acct: acct, - region: regionFromCtx(ctx), + region: strings.Clone(regionFromCtx(ctx)), bucket: strings.Clone(bucket), key: strings.Clone(key), isPut: isPut, @@ -377,10 +373,9 @@ func regionFromCtx(ctx fiber.Ctx) string { return "" } -// expiredOutcomeError renders a parked teardown notification as the -// error the publication carries: an internal S3 error whose code -// names the expiry, so the audit log keeps the descriptive code the -// plain error used to carry instead of the generic mapping. +// sessionExpiredError is the S3 error an expired or abandoned +// session publishes: an internal error whose code names the +// expiry, so the audit log keeps a descriptive code. type sessionExpiredError struct { s3err.APIError } @@ -390,3 +385,19 @@ var errSessionExpired = sessionExpiredError{APIError: s3err.APIError{ Description: "The RDMA transfer session expired before completion", HTTPStatusCode: 500, }} + +// errTimeoutExpired and errVerifyFailed classify unclaimed teardowns +// whose native outcome names a specific READY failure: the record +// then matches what the wire response for that failure carries. +var ( + errTimeoutExpired = sessionExpiredError{APIError: s3err.APIError{ + Code: "RdmaTransferTimeout", + Description: "The RDMA transfer timed out before completion", + HTTPStatusCode: 504, + }} + errVerifyFailed = sessionExpiredError{APIError: s3err.APIError{ + Code: "RdmaTransferVerifyFailed", + Description: "The RDMA transfer failed verification before completion", + HTTPStatusCode: 502, + }} +) diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 4e4c3003..58e99f1c 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -34,12 +34,11 @@ import ( "github.com/versity/versitygw/s3err" ) -// The tests exercise the tracker's ownership semantics: the session -// table is the single source of truth for who may publish, so the -// assertions watch table membership rather than the emission itself -// (emission is a no-op without operational services wired in). +// The publication model: request paths only record outcomes; the +// teardown callback is the single publisher. The tests exercise +// the table semantics that guarantee exactly-once publication. -func TestOpsTrackerExpiryPublication(t *testing.T) { +func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { tr := newOpsTracker() tr.register("sess-1", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -47,7 +46,8 @@ func TestOpsTrackerExpiryPublication(t *testing.T) { t.Fatalf("registered sessions = %d, want 1", got) } - // The reaper path claims and removes the session. + // The reaper path publishes for a session no READY ever + // recorded and removes the entry. tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-1"}) if got := len(tr.sessions); got != 0 { t.Fatalf("session survived terminal: %d", got) @@ -60,102 +60,92 @@ func TestOpsTrackerExpiryPublication(t *testing.T) { } } -func TestOpsTrackerRequestPathClaims(t *testing.T) { +func TestOpsTrackerRecordedOutcomeWins(t *testing.T) { tr := newOpsTracker() tr.register("sess-2", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", true, time.Now()) - // READY completion claims the publication first. - tr.publishClaimed("sess-2", nil, 4096) - if got := len(tr.sessions); got != 0 { - t.Fatalf("request path left session: %d", got) - } - - // The late reaper callback finds nothing left. + // The READY path records the real outcome; the callback then + // consumes it instead of publishing an expiry. + tr.recordOutcome("sess-2", nil, 4096) tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-2"}) if got := len(tr.sessions); got != 0 { - t.Fatalf("reaper re-added session: %d", got) + t.Fatalf("session survived terminal: %d", got) } + + // A late second record is a no-op (entry gone). + tr.recordOutcome("sess-2", nil, 1) } -func TestOpsTrackerReserveBlocksCallback(t *testing.T) { +func TestOpsTrackerFirstRecordWins(t *testing.T) { tr := newOpsTracker() tr.register("sess-3", auth.Account{Access: "ak"}, "us-east-1", - "bkt", "obj", true, time.Now()) - - // The request path reserves before invoking a native - // completion call. - emit := tr.reserve("sess-3") - if emit == nil { - t.Fatal("reserve returned nil for a live session") - } - - // The synchronous teardown callback must not publish on a - // reserved record: the entry stays put. - tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-3"}) - if got := len(tr.sessions); got != 1 { - t.Fatalf("reserved session removed by callback: %d", got) - } - - // A double reserve is refused. - if tr.reserve("sess-3") != nil { - t.Fatal("double reserve succeeded") - } - - // The request path publishes through its reserved emitter. - tr.publishClaimed("sess-3", nil, 128) - if got := len(tr.sessions); got != 0 { - t.Fatalf("reserved session survived request publication: %d", got) - } -} - -func TestOpsTrackerEarlyTerminal(t *testing.T) { - tr := newOpsTracker() - - // Teardown notification for an unregistered session parks. - tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-4"}) - if got := len(tr.earlyTerminals); got != 1 { - t.Fatalf("early terminal not parked: %d", got) - } - - // Registration consumes it: no live entry remains, so no - // orphan record can outlive the session. - tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) - if got := len(tr.earlyTerminals); got != 0 { - t.Fatalf("early terminal not consumed: %d", got) - } - if got := len(tr.sessions); got != 0 { - t.Fatalf("orphan session entry created: %d", got) + + tr.recordOutcome("sess-3", nil, 100) + tr.recordOutcome("sess-3", errors.New("late"), 0) + + tr.mu.Lock() + rec := tr.sessions["sess-3"] + tr.mu.Unlock() + if rec == nil || rec.out.err != nil || rec.out.byt != 100 { + t.Fatalf("second record overwrote the first: %+v", rec.out) } } func TestOpsTrackerUnregister(t *testing.T) { tr := newOpsTracker() - tr.register("sess-5", auth.Account{Access: "ak"}, "us-east-1", + tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) - tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-5"}) - tr.register("sess-6", auth.Account{Access: "ak"}, "us-east-1", - "bkt", "obj", false, time.Now()) - tr.unregister("sess-6") + tr.unregister("sess-4") if got := len(tr.sessions); got != 0 { t.Fatalf("unregister left entries: %d", got) } + // The teardown callback for the unregistered session is a + // silent no-op (native side already rejected or reaped it). + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-4"}) + if got := len(tr.sessions); got != 0 { + t.Fatalf("terminal resurrected entry: %d", got) + } } func TestOpsTrackerUnknownSession(t *testing.T) { tr := newOpsTracker() // Unknown sessions and the nil tracker are silent no-ops. var nilTracker *opsTracker - nilTracker.publishClaimed("ghost", nil, 1) + nilTracker.recordOutcome("ghost", nil, 1) nilTracker.onTerminal(rcserver.TerminalEvent{SessionID: "ghost"}) - tr.publishClaimed("ghost", nil, 1) + tr.recordOutcome("ghost", nil, 1) tr.onTerminal(rcserver.TerminalEvent{SessionID: "ghost"}) if got := len(tr.sessions); got != 0 { t.Fatalf("ghost session materialized: %d", got) } } +func TestNormalizeSinkError(t *testing.T) { + if normalizeSinkError(nil) != nil { + t.Fatal("nil error should stay nil") + } + // Plain errors map through the route error mapping. + got := normalizeSinkError(errors.New("x")) + apiErr, ok := got.(s3err.APIError) + if !ok || apiErr.HTTPStatusCode != 500 { + t.Fatalf("plain error => %#v", got) + } + // Wrapped S3 errors are extracted to their base form so the + // audit loggers' direct assertion classifies them correctly. + wrapped := errWrapped{s3err.GetAPIError(s3err.ErrNoSuchBucket)} + got = normalizeSinkError(wrapped) + apiErr, ok = got.(s3err.APIError) + if !ok || apiErr.Code != "NoSuchBucket" || apiErr.HTTPStatusCode != 404 { + t.Fatalf("wrapped error => %#v", got) + } +} + +type errWrapped struct{ s3err.S3Error } + +func (errWrapped) Error() string { return "wrapped" } + func TestHttpStatusFromError(t *testing.T) { if got := httpStatusFromError(nil); got != 200 { t.Fatalf("nil error => %d, want 200", got) @@ -172,3 +162,25 @@ func TestHttpStatusFromError(t *testing.T) { t.Fatalf("limit error => %d, want 429", got) } } + +func TestExpiredErrorClassification(t *testing.T) { + cases := []struct { + outcome int + code string + status int + }{ + {int(rcserver.ReadyWireFail), "RdmaTransferFailed", 502}, + {int(rcserver.ReadyVerifyFail), "RdmaTransferVerifyFailed", 502}, + {int(rcserver.ReadyTimeout), "RdmaTransferTimeout", 504}, + {int(rcserver.ReadyOK), "SessionExpired", 500}, + } + for _, tc := range cases { + err := expiredError(rcserver.TerminalEvent{Outcome: tc.outcome}) + apiErr := normalizeSinkError(err).(s3err.APIError) + if apiErr.Code != tc.code || apiErr.HTTPStatusCode != tc.status { + t.Fatalf("outcome %d => %s/%d, want %s/%d", + tc.outcome, apiErr.Code, apiErr.HTTPStatusCode, + tc.code, tc.status) + } + } +} diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index 06da7b93..580f8d32 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -42,6 +42,7 @@ import ( "github.com/versity/versitygw/backend" "github.com/versity/versitygw/rdma/rcserver" "github.com/versity/versitygw/s3api/utils" + "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3response" ) @@ -232,11 +233,13 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { h.ops.register(resp.SessionID, acct, regionFromCtx(ctx), bucket, key, isPut, time.Now()) if err := h.svc.FinishPrepare(resp.SessionID, true); err != nil { - // The finalization failed: the session is gone and the - // teardown callback (or this call's own reap) published - // the terminal record; nothing may keep the entry. - h.ops.unregister(resp.SessionID) - h.ops.publishRequest(ctx, acct, mapRcError(err), bucket, key, isPut) + // The finalization failed. Exactly one publication + // covers it: the finalizing call already reaped the + // session and its callback published the recorded + // outcome, or the native side rejected the call and no + // callback is coming, in which case the entry is + // published here. + h.ops.failOutcome(resp.SessionID, mapRcError(err)) return mapRcError(err) } @@ -417,21 +420,27 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // this handler owns the completion ref. A panic or early // unwind must still release it so the session can be reaped. // - // The publication ownership moves here as well: every native - // completion call below (FinishFinal, FinishPut) fires the - // teardown callback synchronously, and a reserved record is - // invisible to that callback, so the outcome is published by - // this handler exactly once. - emit := h.ops.reserve(sessionID) - publish := func(err error, bytes int64) { - if emit != nil { - emit.publish(err, bytes) - emit = nil + // This handler only RECORDS the outcome; publication belongs + // to the teardown callback, which every native completion + // call below fires exactly once. The deferred recorder covers + // panic unwinds too, so every path through this handler + // leaves a final outcome behind. + recorded := false + record := func(err error, bytes int64) { + if !recorded { + recorded = true + h.ops.recordOutcome(sessionID, err, bytes) } } finalized := false defer func() { if !finalized { + // The unwind finalizer retires the session, which + // fires the callback; make sure it carries an + // outcome even on a panic path. + if !recorded { + record(errPanicked(), 0) + } _ = h.svc.FinishFinal(sessionID) } }() @@ -449,7 +458,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { finalized = true } if err != nil { - publish(mapRcError(err), 0) + record(mapRcError(err), 0) return err } // The FINAL wire reply carries the stored object's @@ -457,15 +466,19 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { resp.Etag = put.ETag resp.VersionID = put.VersionID } else if err := h.svc.FinishFinal(sessionID); err != nil { - publish(mapRcError(err), 0) + record(mapRcError(err), 0) return mapRcError(err) } else { finalized = true } - // The transfer completed: publish the terminal record with - // the byte count the data plane reported. - publish(nil, int64(resp.BytesTransferred)) + // The transfer completed: record the terminal outcome with + // the byte count the data plane reported. FinishPut (inside + // commitPut) or the FinishFinal above already fired the + // teardown callback, which publishes this outcome - or, on a + // timing edge where the callback ran first, the deferred + // finalizer's FinishFinal does. + record(nil, int64(resp.BytesTransferred)) // Wire reply per the hipobj-rc-v2 contract: protocol echo, // cookie echo, transferred bytes, and object metadata. @@ -699,3 +712,14 @@ func mapRcError(err error) error { } return err } + +// errPanicked is the outcome recorded when a panic unwinds the +// READY handler after the completion ref was claimed: the record +// keeps the failure even though the panic itself propagates. +func errPanicked() error { + return s3err.APIError{ + Code: "InternalRDMAError", + Description: "The RDMA transfer ended without a confirmed result", + HTTPStatusCode: 500, + } +} From 97238d80ad77c1e5fd4f19a4dc36777c64f39663 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 10:54:51 +0900 Subject: [PATCH 06/16] rdma: reserve session publication before completion calls A native completion call fires the teardown callback before it returns, so an outcome recorded after the call is too late: every successful transfer published as an expiry, and PUT failures and panics lost their real outcome to the callback's placeholder. The READY handler now reserves the session record before invoking any completion call. A reserved record is invisible to the callback, and the handler publishes the real outcome exactly once after the result is known. A deferred safety net publishes on panic unwinds before the unwind finalizer retires the session. Successful PUTs forward the backend-assigned ETag and version into the object-created event, and the audit record carries the transferred byte count as the object size. A READY whose re-authorization fails now publishes the denial itself before canceling, instead of letting the cancel publish an expiry. The native side keeps the terminal reason on every failure exit: a verify failure no longer overwrites the wire-failure outcome, and QP transition or re-arm failures record the wire failure they return. The unclaimed-teardown classification follows the wire mapping, so every transfer-level failure the client would see as 502 publishes as the same 502 instead of a diverging per-cause code. The metrics bucket tag is documented as absent on synthesized publications: route parameters come from route matching, which a synthesized request never runs; the audit log derives the bucket from the path and stays accurate. --- cuwrapper/rc/rc_server_abi.cpp | 10 ++- rdma/rcroutes/ops_linux.go | 123 ++++++++++++++++++++++---------- rdma/rcroutes/ops_linux_test.go | 91 ++++++++++++++--------- rdma/rcroutes/routes_linux.go | 60 +++++++++------- 4 files changed, 185 insertions(+), 99 deletions(-) diff --git a/cuwrapper/rc/rc_server_abi.cpp b/cuwrapper/rc/rc_server_abi.cpp index 948cce72..e9787db7 100644 --- a/cuwrapper/rc/rc_server_abi.cpp +++ b/cuwrapper/rc/rc_server_abi.cpp @@ -879,6 +879,7 @@ int rc_ready_transfer(rc_server *srv, const rc_ready_req *req, /*destLid*/ 0, destGid, s->core.clientPsn) != 0) { g.lock(); + s->last_outcome = RC_READY_WIRE_FAIL; s->reap_pending = true; s->active_ref = 0; /* roll the completion ref back: no data * phase will run for this session */ @@ -887,6 +888,7 @@ int rc_ready_transfer(rc_server *srv, const rc_ready_req *req, if (hipObj::v2::transitionQpToRtsV2(conn, srv->device, s->core.serverPsn) != 0) { g.lock(); + s->last_outcome = RC_READY_WIRE_FAIL; s->reap_pending = true; s->active_ref = 0; return RC_E_WIRE; @@ -968,7 +970,9 @@ int rc_ready_transfer(rc_server *srv, const rc_ready_req *req, if (after->active_ref > 0) after->active_ref--; } else { /* Cannot re-arm the QP (or the session died mid-reset): - * not retryable. */ + * not retryable. Record the wire failure so the + * teardown publication classifies it as one. */ + after->last_outcome = RC_READY_WIRE_FAIL; after->reap_pending = true; if (after->active_ref > 0) after->active_ref--; return RC_E_WIRE; @@ -981,6 +985,10 @@ int rc_ready_transfer(rc_server *srv, const rc_ready_req *req, after->active_ref = 0; return RC_E_WIRE; case hipObj::v2::DataPhaseResult::VerifyFail: + after->last_outcome = RC_READY_VERIFY_FAIL; + after->reap_pending = true; + after->active_ref = 0; + return RC_E_WIRE; case hipObj::v2::DataPhaseResult::WireFail: after->last_outcome = RC_READY_WIRE_FAIL; after->reap_pending = true; diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 096b34a7..0adf6630 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -60,6 +60,26 @@ type opsEmitter struct { key string isPut bool start time.Time + // Commit metadata the PUT path fills in before publishing the + // success record, so the object-created event carries the + // backend-assigned ETag and version like the regular put + // pipeline's event does. + etag string + version string + hasEtag bool + hasVer bool +} + +// setCommitMeta records the backend-assigned object metadata for +// the success publication's event payload. +func (e *opsEmitter) setCommitMeta(etag, version string) { + if e == nil { + return + } + e.etag = etag + e.hasEtag = true + e.version = version + e.hasVer = true } // synthesize builds a fiber context whose path and request locals @@ -69,6 +89,11 @@ type opsEmitter struct { // Immutable: string accessors copy instead of exposing the pooled // context buffer, which matters because event senders serialize // asynchronously and would otherwise read a reused buffer. +// +// Route parameters cannot be populated this way (they come from +// route matching, which a synthesized request never runs), so the +// metrics bucket tag is absent on RC publications; the audit log +// derives the bucket from the path instead and stays accurate. func (e *opsEmitter) synthesize() (fiber.Ctx, func()) { ctx := e.app.AcquireCtx(&fasthttp.RequestCtx{}) method := fiber.MethodGet @@ -121,15 +146,28 @@ func (e *opsEmitter) publish(err error, bytes int64) { if e.ops.Logger != nil { e.ops.Logger.Log(ctx, sinkErr, nil, s3log.LogMeta{ Action: action, + // The object size field reports the transferred + // byte count the record carries, so a successful + // GET/PUT shows real bytes instead of zero. + ObjectSize: bytes, }) } // The object-created event fires at commit time only; error // publications never carry it. if e.ops.Events != nil && err == nil && e.isPut { - e.ops.Events.SendEvent(ctx, s3event.EventMeta{ + meta := s3event.EventMeta{ EventName: s3event.EventObjectCreatedPut, ObjectSize: bytes, - }) + } + if e.hasEtag { + etag := e.etag + meta.ObjectETag = &etag + } + if e.hasVer { + ver := e.version + meta.VersionId = &ver + } + e.ops.Events.SendEvent(ctx, meta) } } @@ -160,10 +198,7 @@ func httpStatusFromError(err error) int { return routeError(err).HTTPStatusCode } -// sessionOutcome is the terminal outcome of a tracked session, -// recorded by whichever path observes the final result first. A -// recorded outcome is final: the teardown callback consumes it and -// publishes, so exactly one publication happens per session. +// sessionOutcome is the terminal outcome of a tracked session. type sessionOutcome struct { err error // nil on success byt int64 // bytes transferred on success @@ -174,6 +209,11 @@ type sessionOutcome struct { type sessionRecord struct { emit *opsEmitter out sessionOutcome + // reserved marks a record the request path owns: the native + // teardown callback skips it (the request path will publish + // exactly once itself), so a completion call that fires the + // callback before returning cannot publish a placeholder. + reserved bool } // opsTracker owns terminal publication: each session publishes @@ -273,23 +313,36 @@ func (t *opsTracker) failOutcome(sessionID string, err error) { rec.emit.publish(err, 0) } -// recordOutcome stores the terminal outcome of a completing -// transfer. It never publishes: publication belongs to the teardown -// callback, which fires exactly once per session after the -// completion call returns. Recording is idempotent - the first -// outcome wins - so a panic-unwind path that records after the -// normal path changed nothing. -func (t *opsTracker) recordOutcome(sessionID string, err error, bytes int64) { +// reserve marks a session record as owned by its request path: the +// teardown callback skips a reserved record because the request +// path publishes the real outcome itself. Returns the emitter when +// the record exists and was not reserved yet. +func (t *opsTracker) reserve(sessionID string) *opsEmitter { if t == nil { - return + return nil } t.mu.Lock() defer t.mu.Unlock() rec, ok := t.sessions[sessionID] - if !ok || rec.out.done { + if !ok || rec.reserved { + return nil + } + rec.reserved = true + return rec.emit +} + +// publishReserved publishes through a reserved record and drops it: +// the single publication of a request-owned session outcome. +func (t *opsTracker) publishReserved(sessionID string, emit *opsEmitter, err error, bytes int64) { + if t == nil { return } - rec.out = sessionOutcome{err: err, byt: bytes, done: true} + t.mu.Lock() + delete(t.sessions, sessionID) + t.mu.Unlock() + if emit != nil { + emit.publish(err, bytes) + } } func (t *opsTracker) loadOps() OpsServices { @@ -313,6 +366,14 @@ func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { t.mu.Unlock() return } + // 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. + if rec.reserved { + t.mu.Unlock() + return + } delete(t.sessions, ev.SessionID) t.mu.Unlock() @@ -328,15 +389,17 @@ func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { // expiredError renders an unclaimed teardown as the error the // publication carries, derived from the native outcome so the -// record names the real terminal reason instead of a generic one. +// record names the real terminal reason. The classification stays +// aligned with the wire mapping: every transfer-level failure the +// READY call reports as RC_E_WIRE (wire, verify, or execution +// timeout) publishes as the same 502 the client would have seen, +// and only a session that expired without any transfer attempt +// keeps the expiry code. func expiredError(ev rcserver.TerminalEvent) error { switch ev.Outcome { - case int(rcserver.ReadyWireFail): + case int(rcserver.ReadyWireFail), int(rcserver.ReadyVerifyFail), + int(rcserver.ReadyTimeout): return rcserver.ErrWire - case int(rcserver.ReadyTimeout): - return errTimeoutExpired - case int(rcserver.ReadyVerifyFail): - return errVerifyFailed default: return errSessionExpired } @@ -385,19 +448,3 @@ var errSessionExpired = sessionExpiredError{APIError: s3err.APIError{ Description: "The RDMA transfer session expired before completion", HTTPStatusCode: 500, }} - -// errTimeoutExpired and errVerifyFailed classify unclaimed teardowns -// whose native outcome names a specific READY failure: the record -// then matches what the wire response for that failure carries. -var ( - errTimeoutExpired = sessionExpiredError{APIError: s3err.APIError{ - Code: "RdmaTransferTimeout", - Description: "The RDMA transfer timed out before completion", - HTTPStatusCode: 504, - }} - errVerifyFailed = sessionExpiredError{APIError: s3err.APIError{ - Code: "RdmaTransferVerifyFailed", - Description: "The RDMA transfer failed verification before completion", - HTTPStatusCode: 502, - }} -) diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 58e99f1c..a1ee154f 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -8,17 +8,10 @@ // // Unless required by applicable law or agreed to in writing, // software distributed under the License is distributed on an -<<<<<<< Updated upstream -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, -// either express or implied. See the License for the specific -// language governing permissions and limitations under the -// License. -======= // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. ->>>>>>> Stashed changes //go:build linux && amd64 && cgo @@ -34,9 +27,11 @@ import ( "github.com/versity/versitygw/s3err" ) -// The publication model: request paths only record outcomes; the -// teardown callback is the single publisher. The tests exercise -// the table semantics that guarantee exactly-once publication. +// The publication model: the request path reserves a session +// record before any native completion call (which fires the +// teardown callback synchronously, before the call returns), the +// callback skips reserved records, and the request path publishes +// exactly once. Unreserved records are published by the callback. func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { tr := newOpsTracker() @@ -47,7 +42,7 @@ func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { } // The reaper path publishes for a session no READY ever - // recorded and removes the entry. + // reserved and removes the entry. tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-1"}) if got := len(tr.sessions); got != 0 { t.Fatalf("session survived terminal: %d", got) @@ -60,50 +55,73 @@ func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { } } -func TestOpsTrackerRecordedOutcomeWins(t *testing.T) { +func TestOpsTrackerReserveBlocksCallback(t *testing.T) { tr := newOpsTracker() tr.register("sess-2", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", true, time.Now()) - // The READY path records the real outcome; the callback then - // consumes it instead of publishing an expiry. - tr.recordOutcome("sess-2", nil, 4096) + // The READY path reserves before its completion call; the + // callback the call fires synchronously must skip the record. + emit := tr.reserve("sess-2") + if emit == nil { + t.Fatal("reserve returned nil for a live session") + } tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-2"}) - if got := len(tr.sessions); got != 0 { - t.Fatalf("session survived terminal: %d", got) + if got := len(tr.sessions); got != 1 { + t.Fatalf("callback consumed a reserved record: %d", got) } - // A late second record is a no-op (entry gone). - tr.recordOutcome("sess-2", nil, 1) + // The request path then publishes and drops the entry. + tr.publishReserved("sess-2", emit, nil, 4096) + if got := len(tr.sessions); got != 0 { + t.Fatalf("publishReserved left residue: %d", got) + } + + // A second reserve of the consumed entry is nil. + if again := tr.reserve("sess-2"); again != nil { + t.Fatal("reserve succeeded for a consumed entry") + } } -func TestOpsTrackerFirstRecordWins(t *testing.T) { +func TestOpsTrackerReserveIsExclusive(t *testing.T) { tr := newOpsTracker() tr.register("sess-3", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) - tr.recordOutcome("sess-3", nil, 100) - tr.recordOutcome("sess-3", errors.New("late"), 0) - - tr.mu.Lock() - rec := tr.sessions["sess-3"] - tr.mu.Unlock() - if rec == nil || rec.out.err != nil || rec.out.byt != 100 { - t.Fatalf("second record overwrote the first: %+v", rec.out) + if first := tr.reserve("sess-3"); first == nil { + t.Fatal("first reserve failed") } + if second := tr.reserve("sess-3"); second != nil { + t.Fatal("double reserve succeeded") + } +} + +func TestOpsTrackerFailOutcome(t *testing.T) { + tr := newOpsTracker() + tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", + "bkt", "obj", false, time.Now()) + + // Consume-or-noop: present entry is consumed. + tr.failOutcome("sess-4", errors.New("x")) + if got := len(tr.sessions); got != 0 { + t.Fatalf("failOutcome left residue: %d", got) + } + // A second call after the callback already consumed is a + // silent no-op, not a double publication. + tr.failOutcome("sess-4", errors.New("y")) } func TestOpsTrackerUnregister(t *testing.T) { tr := newOpsTracker() - tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", + tr.register("sess-5", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) - tr.unregister("sess-4") + tr.unregister("sess-5") if got := len(tr.sessions); got != 0 { t.Fatalf("unregister left entries: %d", got) } // The teardown callback for the unregistered session is a // silent no-op (native side already rejected or reaped it). - tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-4"}) + tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-5"}) if got := len(tr.sessions); got != 0 { t.Fatalf("terminal resurrected entry: %d", got) } @@ -113,9 +131,9 @@ func TestOpsTrackerUnknownSession(t *testing.T) { tr := newOpsTracker() // Unknown sessions and the nil tracker are silent no-ops. var nilTracker *opsTracker - nilTracker.recordOutcome("ghost", nil, 1) + nilTracker.reserve("ghost") nilTracker.onTerminal(rcserver.TerminalEvent{SessionID: "ghost"}) - tr.recordOutcome("ghost", nil, 1) + tr.reserve("ghost") tr.onTerminal(rcserver.TerminalEvent{SessionID: "ghost"}) if got := len(tr.sessions); got != 0 { t.Fatalf("ghost session materialized: %d", got) @@ -164,14 +182,17 @@ func TestHttpStatusFromError(t *testing.T) { } func TestExpiredErrorClassification(t *testing.T) { + // Every transfer-level failure the READY call reports as + // RC_E_WIRE publishes as the same 502 the wire response + // carries; only an unattempted session keeps the expiry code. cases := []struct { outcome int code string status int }{ {int(rcserver.ReadyWireFail), "RdmaTransferFailed", 502}, - {int(rcserver.ReadyVerifyFail), "RdmaTransferVerifyFailed", 502}, - {int(rcserver.ReadyTimeout), "RdmaTransferTimeout", 504}, + {int(rcserver.ReadyVerifyFail), "RdmaTransferFailed", 502}, + {int(rcserver.ReadyTimeout), "RdmaTransferFailed", 502}, {int(rcserver.ReadyOK), "SessionExpired", 500}, } for _, tc := range cases { diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index 580f8d32..a120f0cc 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -384,7 +384,13 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { return errors.New("invalid session target") } if err := h.authorize(ctx, acct, isRoot, bucket, key, info.Op == 1); err != nil { - // Permission revoked mid-session: cancel the session. + // Permission revoked mid-session: cancel the session, + // and publish the real denial - not an expiry - as its + // outcome. The cancel tears the session down now; a + // plain publish (consume-or-noop) would race it, so + // the record is consumed here. + err = mapRcError(err) + h.ops.failOutcome(sessionID, err) _ = h.svc.Cancel(sessionID, principal) return err } @@ -420,26 +426,31 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // this handler owns the completion ref. A panic or early // unwind must still release it so the session can be reaped. // - // This handler only RECORDS the outcome; publication belongs - // to the teardown callback, which every native completion - // call below fires exactly once. The deferred recorder covers - // panic unwinds too, so every path through this handler - // leaves a final outcome behind. - recorded := false - record := func(err error, bytes int64) { - if !recorded { - recorded = true - h.ops.recordOutcome(sessionID, err, bytes) + // Every native completion call below (FinishFinal, and + // FinishPut inside commitPut) fires the teardown callback + // synchronously, BEFORE the call returns - so the outcome + // cannot be recorded after the call. The handler reserves the + // publication instead: a reserved record is invisible to the + // callback, and this handler publishes exactly once after the + // result is known. The deferred safety net publishes on any + // unwind that bypassed the normal paths. + emit := h.ops.reserve(sessionID) + published := false + publish := func(err error, bytes int64) { + if !published { + published = true + h.ops.publishReserved(sessionID, emit, err, bytes) } } finalized := false defer func() { if !finalized { - // The unwind finalizer retires the session, which - // fires the callback; make sure it carries an - // outcome even on a panic path. - if !recorded { - record(errPanicked(), 0) + // The unwind finalizer retires the session. The + // reservation keeps its callback a no-op, so the + // publication must come from here on a panic or + // early-unwind path. + if !published { + publish(errPanicked(), 0) } _ = h.svc.FinishFinal(sessionID) } @@ -458,27 +469,26 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { finalized = true } if err != nil { - record(mapRcError(err), 0) + publish(mapRcError(err), 0) return err } // The FINAL wire reply carries the stored object's // metadata, which the backend assigned at commit time. resp.Etag = put.ETag resp.VersionID = put.VersionID + // The success publication's object-created event carries + // the same commit metadata. + emit.setCommitMeta(put.ETag, put.VersionID) } else if err := h.svc.FinishFinal(sessionID); err != nil { - record(mapRcError(err), 0) + publish(mapRcError(err), 0) return mapRcError(err) } else { finalized = true } - // The transfer completed: record the terminal outcome with - // the byte count the data plane reported. FinishPut (inside - // commitPut) or the FinishFinal above already fired the - // teardown callback, which publishes this outcome - or, on a - // timing edge where the callback ran first, the deferred - // finalizer's FinishFinal does. - record(nil, int64(resp.BytesTransferred)) + // The transfer completed: publish the terminal record with + // the byte count the data plane reported. + publish(nil, int64(resp.BytesTransferred)) // Wire reply per the hipobj-rc-v2 contract: protocol echo, // cookie echo, transferred bytes, and object metadata. From d30ace72ad90f3f2d25e9cdcb54f9d5afc08343c Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 12:12:53 +0900 Subject: [PATCH 07/16] rdma: keep reaper callbacks off sink threads The teardown callback ran audit, metrics, and event sinks inline on whatever thread fired it, which is the native reaper thread: one blocking sink (a synchronous file write on a stalled filesystem) would stall reaping for every other session. Publications now hand off to a dedicated worker through a bounded queue; a full queue falls back to a detached goroutine, so the callback never waits on a sink and no record is dropped. A denial issued while a completion owner holds the reservation (concurrent READY re-authorization failure) no longer steals the publication: reserved records are invisible to the denial path, which previously produced a denial record plus the owner's success record for one transfer. The tracker tests now drive a recording audit sink and assert the published record count across expiry, reserved, denied-while- reserved, and consume-or-noop paths: one record per session. --- rdma/rcroutes/ops_linux.go | 74 +++++++++++++++++++++++++++---- rdma/rcroutes/ops_linux_test.go | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 8 deletions(-) diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 0adf6630..9879df7c 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -223,20 +223,75 @@ type sessionRecord struct { // single publisher. This removes every ownership race: a recorded // outcome cannot be double-published, and a record the callback // already consumed cannot be resurrected. +// +// Sink execution never runs on the caller's thread: the native +// reaper invokes the callback, and an operational sink can block +// (a synchronous file write on a stalled filesystem), which would +// stall reaping for every other session. Publications hand off to +// a dedicated worker through a bounded queue; when the queue is +// full the publication runs inline as a last resort, keeping the +// guarantee that no record is silently dropped while still capping +// how long a callback may wait. type opsTracker struct { mu sync.Mutex ops OpsServices sessions map[string]*sessionRecord app *fiber.App + pubq chan pubJob + done chan struct{} } +// pubJob is one deferred publication handed to the worker. +type pubJob struct { + emit *opsEmitter + err error + byt int64 +} + +// pubQueueDepth bounds how many publications may wait in the +// handoff queue before the callback falls back to inline +// execution. +const pubQueueDepth = 256 + func newOpsTracker() *opsTracker { - return &opsTracker{ + t := &opsTracker{ sessions: map[string]*sessionRecord{}, app: fiber.New(fiber.Config{ Immutable: true, }), + pubq: make(chan pubJob, pubQueueDepth), + done: make(chan struct{}), } + go func() { + defer close(t.done) + for job := range t.pubq { + job.emit.publish(job.err, job.byt) + } + }() + return t +} + +// dispatch hands a publication to the worker. It must never block +// indefinitely: when the queue is full (the worker itself stuck in +// a sink), the publication runs inline so the record is still +// delivered and the caller - possibly the native reaper - returns. +func (t *opsTracker) dispatch(job pubJob) { + select { + case t.pubq <- job: + return + default: + } + select { + case <-t.done: + // Worker exited (shutdown path): publish inline. + job.emit.publish(job.err, job.byt) + return + default: + } + // Queue full and worker alive but stalled. Rather than block + // the caller, drop the job onto a goroutine: publication order + // is not part of the contract, and dropping is worse. + go job.emit.publish(job.err, job.byt) } // SetOpsServices installs the operational service instances. The @@ -296,21 +351,25 @@ func (t *opsTracker) unregister(sessionID string) { // the finalizing call already reaped the session its callback // published (the entry is gone, this is a no-op); when no callback // will ever come (the native side rejected the call) the entry is -// consumed and published here. +// consumed and published here. A reserved record belongs to an +// in-flight completion owner (a concurrent READY's denial must not +// steal its publication), so it is left untouched. func (t *opsTracker) failOutcome(sessionID string, err error) { if t == nil { return } t.mu.Lock() rec, ok := t.sessions[sessionID] - if ok { + if ok && !rec.reserved { delete(t.sessions, sessionID) + } else { + ok = false } t.mu.Unlock() if !ok { return } - rec.emit.publish(err, 0) + t.dispatch(pubJob{emit: rec.emit, err: err}) } // reserve marks a session record as owned by its request path: the @@ -341,7 +400,7 @@ func (t *opsTracker) publishReserved(sessionID string, emit *opsEmitter, err err delete(t.sessions, sessionID) t.mu.Unlock() if emit != nil { - emit.publish(err, bytes) + t.dispatch(pubJob{emit: emit, err: err, byt: bytes}) } } @@ -383,8 +442,7 @@ func (t *opsTracker) onTerminal(ev rcserver.TerminalEvent) { // outcome carries the native reason. rec.out = sessionOutcome{err: expiredError(ev), done: true} } - emit := rec.emit - emit.publish(rec.out.err, rec.out.byt) + t.dispatch(pubJob{emit: rec.emit, err: rec.out.err, byt: rec.out.byt}) } // expiredError renders an unclaimed teardown as the error the @@ -424,7 +482,7 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, isPut: isPut, start: time.Now(), } - emit.publish(err, 0) + t.dispatch(pubJob{emit: emit, err: err}) } // regionFromCtx reads the region the gateway middleware stored on diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index a1ee154f..be33847e 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -19,12 +19,16 @@ package rcroutes import ( "errors" + "sync" "testing" "time" + "github.com/gofiber/fiber/v3" + "github.com/versity/versitygw/auth" "github.com/versity/versitygw/rdma/rcserver" "github.com/versity/versitygw/s3err" + "github.com/versity/versitygw/s3log" ) // The publication model: the request path reserves a session @@ -205,3 +209,77 @@ func TestExpiredErrorClassification(t *testing.T) { } } } + +// recordingLogger captures audit publications so tests can assert +// what the sinks actually received. +type recordingLogger struct { + mu sync.Mutex + logs []recLog +} + +type recLog struct { + err error + bytes int64 +} + +func (r *recordingLogger) Log(ctx fiber.Ctx, err error, body []byte, meta s3log.LogMeta) { + r.mu.Lock() + defer r.mu.Unlock() + r.logs = append(r.logs, recLog{err: err, bytes: meta.ObjectSize}) +} + +func (r *recordingLogger) HangUp() error { return nil } +func (r *recordingLogger) Shutdown() error { return nil } + +func (r *recordingLogger) count() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.logs) +} + +// TestOpsTrackerPublishesExactlyOncePerSession drives the tracker +// with a recording sink and asserts the published record count: +// one per registered session no matter how the ownership played +// out (callback expiry, reserved request path, denial). +func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { + rl := &recordingLogger{} + tr := newOpsTracker() + tr.SetOpsServices(OpsServices{Logger: rl}) + + // Expiry path: callback publishes. + tr.register("s-exp", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) + tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-exp"}) + + // Reserved path: reserve, callback fires (skipped), the + // request path publishes. + tr.register("s-res", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) + emit := tr.reserve("s-res") + if emit == nil { + t.Fatal("reserve failed") + } + tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-res"}) + tr.publishReserved("s-res", emit, nil, 128) + + // Denial path while reserved: failOutcome must not steal the + // publication; the request path still owns it. + tr.register("s-den", auth.Account{Access: "ak"}, "r", "b", "o", true, time.Now()) + emit2 := tr.reserve("s-den") + if emit2 == nil { + t.Fatal("reserve failed") + } + tr.failOutcome("s-den", errors.New("denied")) + tr.publishReserved("s-den", emit2, nil, 256) + + // 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")) + + // Let the publication worker drain. + deadline := time.Now().Add(2 * time.Second) + for rl.count() < 4 && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + if got := rl.count(); got != 4 { + t.Fatalf("published %d records, want 4", got) + } +} From 96be4d1407a32e624e992ca99dfbac1b1089e022 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 13:04:43 +0900 Subject: [PATCH 08/16] rdma: bound publication backpressure and close the reservation window Overflow publications ran one goroutine per job: a stalled sink with a full queue grew them without bound (a probe reached a thousand blocked calls). Overflow now runs inline under a bounded semaphore, so at most a fixed number of callers wait and every record still publishes. The publication worker outlived the operational sinks: gateway shutdown closed the RC service and then the sinks while publications were still queued, losing terminal records (reached "file already closed" with the real file logger). The shutdown wrapper now drains the publication queue before closing the RC service, and the route handler exposes the drain; late terminals after the drain publish inline instead of queueing behind a stopped worker. The publication reservation happened after the transfer claim returned: in the window between the claim and the reservation, a concurrent READY's re-authorization denial consumed the unreserved record, so the claimant's successful transfer lost its publication to the denial. The READY handler now reserves before the claim and before re-authorization; a rolled-back claim (wire failure, peer busy) releases the reservation instead of publishing, so the session keeps its record for the next claimant or the reaper. The tracker test now joins the worker through the shutdown drain and asserts per-session outcomes and byte counts instead of an aggregate count that a pending fifth record could satisfy. --- cmd/vgwrdma/main.go | 8 ++- internal/rdmamode/rdmamode.go | 25 ++++++- rdma/rcroutes/ops_linux.go | 120 ++++++++++++++++++++++++++------ rdma/rcroutes/ops_linux_test.go | 63 ++++++++++++----- rdma/rcroutes/routes_linux.go | 59 +++++++++++----- rdma/rcroutes/routes_stub.go | 3 + 6 files changed, 218 insertions(+), 60 deletions(-) diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index bbb9d963..a2db8939 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -1276,7 +1276,6 @@ func runGateway(ctx context.Context, be backend.Backend) error { // rollback closure and the RunVersityGW lifecycle a // single, ordered owner of both steps. be = rdmamode.WrapBackendShutdownAfterRC(be, rcSvc) - rcVerify := middlewares.VerifyV4Signature( middlewares.RootUserConfig{ Access: gwcli.RootUserAccess, @@ -1288,6 +1287,13 @@ func runGateway(ctx context.Context, be backend.Backend) error { // request reaches the route handler, while errors end the // chain as usual. rcH := rcroutes.New(rcSvc, be, iamSvc, readonly, disableACLs) + // The RC shutdown wrapper drains queued operational + // publications before the sinks close; the drain hook is + // the route handler, which is built only now (it needs + // the wrapped backend). + if w, ok := be.(*rdmamode.BackendShutdownAfterRC); ok { + w.SetOpsDrainer(rcH) + } rcAuth := func(ctx fiber.Ctx) error { // The RC routes run before the default-values // middleware sets the request locals; the access diff --git a/internal/rdmamode/rdmamode.go b/internal/rdmamode/rdmamode.go index 92cad9fa..a4e861ea 100644 --- a/internal/rdmamode/rdmamode.go +++ b/internal/rdmamode/rdmamode.go @@ -21,6 +21,7 @@ import ( "math" "strings" "sync" + "sync/atomic" "time" "github.com/versity/versitygw/backend" @@ -124,6 +125,12 @@ func V2ValidationError(s V2Settings) string { // idempotent. type Closer interface{ Close() } +// OpsDrainer drains operational publications (audit records, +// events) that the RC teardown path queued before the RC service +// closes, so nothing is left waiting on sinks that are about to +// close. It is idempotent. +type OpsDrainer interface{ Shutdown() } + // BackendShutdownAfterRC forwards a backend and closes the RC // service before the wrapped backend shuts down. The RC handlers // reference the backend and IAM service, so the RC service must @@ -136,12 +143,28 @@ type Closer interface{ Close() } type BackendShutdownAfterRC struct { backend.Backend rc Closer + ops atomic.Pointer[OpsDrainer] closed sync.Once } -// Shutdown closes the RC service, then the wrapped backend, once. +// SetOpsDrainer installs the operational publication drainer. The +// route handler that owns the publications is built after this +// wrapper (it needs the wrapped backend), so the drainer arrives +// via this setter; installs after Shutdown ran are dropped, since +// the drain window has passed. +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. func (b *BackendShutdownAfterRC) Shutdown() { b.closed.Do(func() { + 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 9879df7c..f33b9b3f 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -233,12 +233,15 @@ type sessionRecord struct { // guarantee that no record is silently dropped while still capping // how long a callback may wait. type opsTracker struct { - mu sync.Mutex - ops OpsServices - sessions map[string]*sessionRecord - app *fiber.App - pubq chan pubJob - done chan struct{} + mu sync.Mutex + ops OpsServices + sessions map[string]*sessionRecord + app *fiber.App + pubq chan pubJob + overflow chan struct{} + done chan struct{} + drain chan struct{} + drainOnce sync.Once } // pubJob is one deferred publication handed to the worker. @@ -249,49 +252,104 @@ type pubJob struct { } // pubQueueDepth bounds how many publications may wait in the -// handoff queue before the callback falls back to inline -// execution. +// handoff queue before the caller falls back to inline execution. const pubQueueDepth = 256 +// pubOverflowSlots bounds how many callers may run overflow +// publications inline at once; further callers block on the +// semaphore until a slot frees. +const pubOverflowSlots = 8 + func newOpsTracker() *opsTracker { t := &opsTracker{ sessions: map[string]*sessionRecord{}, app: fiber.New(fiber.Config{ Immutable: true, }), - pubq: make(chan pubJob, pubQueueDepth), - done: make(chan struct{}), + pubq: make(chan pubJob, pubQueueDepth), + overflow: make(chan struct{}, pubOverflowSlots), + done: make(chan struct{}), + drain: make(chan struct{}), } go func() { defer close(t.done) - for job := range t.pubq { - job.emit.publish(job.err, job.byt) + for { + select { + case job, ok := <-t.pubq: + if !ok { + return + } + job.emit.publish(job.err, job.byt) + case <-t.drain: + // Drain mode: empty whatever is already + // queued, then exit. Producers past this + // point publish inline. + for { + select { + case job, ok := <-t.pubq: + if !ok { + return + } + job.emit.publish(job.err, job.byt) + default: + return + } + } + } } }() return t } +// Shutdown drains pending publications and stops the worker. The +// gateway must call this BEFORE closing the operational sinks: a +// queued publication that runs after its logger closed is lost. +// After Shutdown, dispatch publishes inline (the queue no longer +// moves), so late terminals still record instead of vanishing. +func (t *opsTracker) Shutdown() { + if t == nil { + return + } + t.drainOnce.Do(func() { + close(t.drain) + <-t.done + }) +} + // dispatch hands a publication to the worker. It must never block // indefinitely: when the queue is full (the worker itself stuck in // a sink), the publication runs inline so the record is still // delivered and the caller - possibly the native reaper - returns. +// dispatch hands a publication to the worker. It must never block +// indefinitely: when the queue is full the caller runs the +// publication itself under an overflow semaphore, which bounds how +// many overflow publications may wait at once. Overflow callers +// beyond the semaphore block - the alternative (one goroutine per +// job) grows without bound under a stalled sink, and dropping the +// record loses the publication entirely. The callers that reach +// overflow are the native reaper or a request handler; waiting +// there is bounded by the semaphore and by the worker draining, +// and is the price of never losing a record. func (t *opsTracker) dispatch(job pubJob) { + // After the worker exited (shutdown drain), the queue no + // longer moves: publish inline so the record is not + // stranded behind a send nobody will receive. + select { + case <-t.done: + job.emit.publish(job.err, job.byt) + return + default: + } select { case t.pubq <- job: return default: } - select { - case <-t.done: - // Worker exited (shutdown path): publish inline. - job.emit.publish(job.err, job.byt) - return - default: - } - // Queue full and worker alive but stalled. Rather than block - // the caller, drop the job onto a goroutine: publication order - // is not part of the contract, and dropping is worse. - go job.emit.publish(job.err, job.byt) + // Queue full and worker alive. Run inline under the overflow + // semaphore. + t.overflow <- struct{}{} + defer func() { <-t.overflow }() + job.emit.publish(job.err, job.byt) } // SetOpsServices installs the operational service instances. The @@ -390,6 +448,22 @@ func (t *opsTracker) reserve(sessionID string) *opsEmitter { return rec.emit } +// releaseReservation returns a reserved record to the pool +// 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. +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 + } + t.mu.Unlock() +} + // publishReserved publishes through a reserved record and drops it: // the single publication of a request-owned session outcome. func (t *opsTracker) publishReserved(sessionID string, emit *opsEmitter, err error, bytes int64) { diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index be33847e..5853773f 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -231,27 +231,21 @@ func (r *recordingLogger) Log(ctx fiber.Ctx, err error, body []byte, meta s3log. func (r *recordingLogger) HangUp() error { return nil } func (r *recordingLogger) Shutdown() error { return nil } -func (r *recordingLogger) count() int { - r.mu.Lock() - defer r.mu.Unlock() - return len(r.logs) -} - // TestOpsTrackerPublishesExactlyOncePerSession drives the tracker -// with a recording sink and asserts the published record count: -// one per registered session no matter how the ownership played -// out (callback expiry, reserved request path, denial). +// with a recording sink, joins the publication worker through +// Shutdown, and asserts the per-session record: each session +// publishes exactly one record with its own outcome and bytes. func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { rl := &recordingLogger{} tr := newOpsTracker() tr.SetOpsServices(OpsServices{Logger: rl}) - // Expiry path: callback publishes. + // Expiry path: callback publishes a zero-byte error record. tr.register("s-exp", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-exp"}) // Reserved path: reserve, callback fires (skipped), the - // request path publishes. + // request path publishes success with bytes. tr.register("s-res", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) emit := tr.reserve("s-res") if emit == nil { @@ -261,7 +255,7 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { tr.publishReserved("s-res", emit, nil, 128) // Denial path while reserved: failOutcome must not steal the - // publication; the request path still owns it. + // publication; the owner's success record is the only one. tr.register("s-den", auth.Account{Access: "ak"}, "r", "b", "o", true, time.Now()) emit2 := tr.reserve("s-den") if emit2 == nil { @@ -270,16 +264,49 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { tr.failOutcome("s-den", errors.New("denied")) tr.publishReserved("s-den", emit2, nil, 256) + // Released reservation: the record returns to the pool and + // the reaper (or the next claimant) can still publish it. + tr.register("s-rel", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) + emit3 := tr.reserve("s-rel") + if emit3 == nil { + t.Fatal("reserve failed") + } + tr.releaseReservation("s-rel", emit3) + tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-rel"}) + // 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")) - // Let the publication worker drain. - deadline := time.Now().Add(2 * time.Second) - for rl.count() < 4 && time.Now().Before(deadline) { - time.Sleep(time.Millisecond) + // Join the worker: Shutdown drains everything queued and + // stops it, so counting after Shutdown sees the final state. + tr.Shutdown() + + 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 got := rl.count(); got != 4 { - t.Fatalf("published %d records, want 4", got) + // The recording sink cannot see session IDs directly (they + // live in the synthesized context), so assert the observable + // contract: error/bytes pairings, one per session, in the + // dispatch order above. + type outcome struct { + isErr bool + bytes int64 + } + want := []outcome{ + {true, 0}, // s-exp expiry + {false, 128}, // s-res success + {false, 256}, // s-den success (denial was skipped) + {true, 0}, // s-rel expiry after release + {true, 0}, // s-fail denial + } + for i, w := range want { + got := rl.logs[i] + if (got.err != nil) != w.isErr || got.bytes != w.bytes { + t.Fatalf("record %d = (err=%v, bytes=%d), want (err=%v, bytes=%d)", + i, got.err, got.bytes, w.isErr, w.bytes) + } } } diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index a120f0cc..252874d9 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -103,6 +103,17 @@ func (h *Handler) SetOpsServices(ops OpsServices) { h.svc.SetTerminalNotify(h.ops.onTerminal) } +// Shutdown drains pending operational publications and stops the +// publication worker. Call before the operational sinks (audit +// logger, metrics, events) close: a queued publication that runs +// after its sink closed is lost. +func (h *Handler) Shutdown() { + if h.ops == nil { + return + } + h.ops.Shutdown() +} + // New builds the route handler around a started RC service. The // operational services arrive later through SetOpsServices, once // the gateway has created them. @@ -383,14 +394,22 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { if !ok { return errors.New("invalid session target") } + // Reserve the publication BEFORE the transfer claim and before + // re-authorization: once ReadyTransfer returns this handler + // holds the native completion reference, and a concurrent + // READY's denial (or the reaper) must not be able to consume + // the record in the window between the claim and the + // reservation. A reserved record is invisible to both. + emit := h.ops.reserve(sessionID) + publish := func(err error, bytes int64) { + h.ops.publishReserved(sessionID, emit, err, bytes) + } if err := h.authorize(ctx, acct, isRoot, bucket, key, info.Op == 1); err != nil { - // Permission revoked mid-session: cancel the session, - // and publish the real denial - not an expiry - as its - // outcome. The cancel tears the session down now; a - // plain publish (consume-or-noop) would race it, so - // the record is consumed here. + // Permission revoked mid-session: publish the real + // denial - not an expiry - as the outcome, release the + // reservation, and cancel the session. err = mapRcError(err) - h.ops.failOutcome(sessionID, err) + publish(err, 0) _ = h.svc.Cancel(sessionID, principal) return err } @@ -409,7 +428,11 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // completion ref, so no local finalizer may run // either. A second concurrent READY must not be able // to reap a session the first one is still - // transferring on. + // transferring on. The publication reservation held + // the record for this claim; release it without + // publishing so the surviving path (the other READY, + // or the eventual reaper) still owns it. + h.ops.releaseReservation(sessionID, emit) return mapRcError(err) } @@ -417,8 +440,11 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // the same response (atomic with the transfer result, so a // concurrent READY cannot rewrite it); the server already // rolled the claim back (state Prepared, no completion ref), - // so answer 409 without any finalizer. + // so answer 409 without any finalizer. The reservation is + // released the same way: the session stays with the record + // the next READY (or the reaper) will claim. if resp.Outcome == rcserver.ReadyBusy { + h.ops.releaseReservation(sessionID, emit) return fmt.Errorf("peer busy: %w", rcserver.ErrDouble) } @@ -429,17 +455,16 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // Every native completion call below (FinishFinal, and // FinishPut inside commitPut) fires the teardown callback // synchronously, BEFORE the call returns - so the outcome - // cannot be recorded after the call. The handler reserves the - // publication instead: a reserved record is invisible to the - // callback, and this handler publishes exactly once after the - // result is known. The deferred safety net publishes on any - // unwind that bypassed the normal paths. - emit := h.ops.reserve(sessionID) + // cannot be recorded after the call. The reservation was made + // before the transfer claim (above), so the callback is a + // no-op for this session and this handler publishes exactly + // once after the result is known. The deferred safety net + // publishes on any unwind that bypassed the normal paths. published := false - publish := func(err error, bytes int64) { + doPublish := func(err error, bytes int64) { if !published { published = true - h.ops.publishReserved(sessionID, emit, err, bytes) + publish(err, bytes) } } finalized := false @@ -450,7 +475,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // publication must come from here on a panic or // early-unwind path. if !published { - publish(errPanicked(), 0) + doPublish(errPanicked(), 0) } _ = h.svc.FinishFinal(sessionID) } diff --git a/rdma/rcroutes/routes_stub.go b/rdma/rcroutes/routes_stub.go index 6a48adab..d9c4b1f0 100644 --- a/rdma/rcroutes/routes_stub.go +++ b/rdma/rcroutes/routes_stub.go @@ -64,3 +64,6 @@ func (h *Handler) SetOpsServices(ops OpsServices) {} // PublishAuthFailure is a stub mirror of the linux handler. func (h *Handler) PublishAuthFailure(ctx fiber.Ctx, err error) {} + +// Shutdown is a stub mirror of the linux handler. +func (h *Handler) Shutdown() {} From d3e45fdb410405e1f87014b6f643f39897704050 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 14:21:27 +0900 Subject: [PATCH 09/16] rdma: track terminal events held off by an active reservation A teardown callback that arrived while its record was reserved was dropped: if the READY that held the reservation then rolled its claim back, the session had lost both publication paths - no request owner and no callback owner - and the record stayed forever, unpublished. The record now stashes the terminal event; releasing the reservation after a rollback consumes the stash and publishes the expiry, since the native session is gone and no second callback will arrive. A duplicate READY that lost the reservation race still proceeded to claim the native transfer, so the transfer could complete with no publication owner. The READY handler now refuses to claim when the reservation is not granted, answering as a duplicate claim. Publication also validates ownership against the record's emitter, so a stale reservation cannot publish over or consume the current owner's record. The shutdown drain raced its producers: a callback could enqueue a publication after the drain checked the queue but before the worker exited, stranding the record behind a stopped worker. The drain now runs after the RC service close, which quiesces the native reaper before returning, so no producer can enqueue behind the drain; ordering replaces locking. Error paths that published directly bypassed the single-shot guard, letting the deferred panic safety net attempt a second publication of the same record. All outcome publications in the READY completion flow now go through the guarded path. --- internal/rdmamode/rdmamode.go | 12 ++++++---- rdma/rcroutes/ops_linux.go | 40 +++++++++++++++++++++++++++++--- rdma/rcroutes/ops_linux_test.go | 41 +++++++++++++++++++++++++++++++-- rdma/rcroutes/routes_linux.go | 15 +++++++++--- 4 files changed, 95 insertions(+), 13 deletions(-) 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. From 55c82f5e4e6c7ecd64dc9fc8605991de24ee2f56 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 18:51:11 +0900 Subject: [PATCH 10/16] rdma: size publications to session capacity and preserve committed PUT facts Size the publication queue from the configured session limit instead of a fixed depth with an overflow semaphore: each session publishes exactly one terminal record, so the queue can never fill and native callbacks hand off without waiting. Remove the semaphore fallback. Admit verification failures under the shutdown barrier so an authentication publication cannot land after the sinks close. Give the metrics manager a lifetime independent of the gateway context so shutdown drain updates are counted, and pass the captured bucket explicitly so RC datapoints appear in bucket-filtered metrics. Track reservations by claim generation: release and publication validate the generation, so a stale claim cannot consume a newer owner's record. Install the reservation cleanup defer immediately after acquisition so a panic during authorization cannot orphan it. Preserve committed PUT facts independent of the native finalizer: when the backend object exists, record the commit, keep the committed byte count on the error publication, and still emit the object-created event. --- cmd/vgwrdma/main.go | 10 +++ embedgw/embedgw.go | 11 ++- metrics/metrics.go | 28 +++++++ rdma/rcroutes/ops_linux.go | 139 +++++++++++++++++++------------- rdma/rcroutes/ops_linux_test.go | 71 +++++++++------- rdma/rcroutes/routes_linux.go | 66 ++++++++++----- rdma/rcroutes/routes_stub.go | 7 ++ rdma/rcserver/rcserver_linux.go | 23 ++++-- 8 files changed, 240 insertions(+), 115 deletions(-) diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index a2db8939..354be6e3 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -1300,6 +1300,16 @@ func runGateway(ctx context.Context, be backend.Backend) error { // logger and event schema read the region from // there, so set it for every verified request. utils.ContextKeyRegion.Set(ctx, region) + // Admission barrier: verification can block on IAM + // lookups, and its failure publications must not + // outlive the RC shutdown sequence (sinks close + // right after the RC service drains). Requests that + // lose the race to shutdown are dropped - their + // audit record would land after the sinks closed. + if !rcSvc.TryEnter() { + return rcroutes.WriteRouteError(ctx, rcroutes.ErrNotAdmitted()) + } + defer rcSvc.Leave() if err := rcVerify(ctx); err != nil { rcH.PublishAuthFailure(ctx, err) return rcroutes.WriteRouteError(ctx, err) diff --git a/embedgw/embedgw.go b/embedgw/embedgw.go index 9f287b2f..629e16d5 100644 --- a/embedgw/embedgw.go +++ b/embedgw/embedgw.go @@ -799,7 +799,16 @@ func RunVersityGW(ctx context.Context, be backend.Backend, cfg *Config) error { return fmt.Errorf("setup logger: %w", err) } - metricsManager, err := metrics.NewManager(ctx, metrics.Config{ + // The metrics manager must outlive the gateway context: RC + // teardown publications drain during backend shutdown, after + // this context is cancelled. A manager bound to ctx would + // silently discard those final datapoints, so it runs on its + // own context and closes with the other sinks below. + metricsCtx, metricsStop := context.WithCancel(context.Background()) + // The cancel runs when this function returns - after the + // shutdown sequence below finishes draining every sink. + defer metricsStop() + metricsManager, err := metrics.NewManager(metricsCtx, metrics.Config{ ServiceName: cfg.MetricsService, StatsdServers: cfg.StatsdServers, DogStatsdServers: cfg.DogstatsServers, diff --git a/metrics/metrics.go b/metrics/metrics.go index d96f2ded..0d725dba 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -44,6 +44,12 @@ type Tag struct { // Manager is the interface definition for metrics manager type Manager interface { Send(ctx fiber.Ctx, err error, action string, count int64, status int) + // SendWithBucket is Send with the bucket dimension stated + // by the caller. The S3 middleware derives the bucket from + // the matched route, which synthesized contexts (RDMA + // operational records) cannot reproduce: they pass the + // captured bucket explicitly instead. + SendWithBucket(ctx fiber.Ctx, err error, action string, count int64, status int, bucket string) Close() } @@ -137,6 +143,28 @@ func (m *manager) Send(ctx fiber.Ctx, err error, action string, count int64, sta reqTags = append(reqTags, Tag{Key: "bucket", Value: bucket}) } + m.send(ctx, err, action, count, status, reqTags) +} + +// SendWithBucket reports with the bucket dimension supplied by the +// caller; see the Manager interface. +func (m *manager) SendWithBucket(ctx fiber.Ctx, err error, action string, count int64, status int, bucket string) { + if action == "" { + action = ActionUndetected + } + a := ActionMap[action] + reqTags := []Tag{ + {Key: "method", Value: ctx.Method()}, + {Key: "api", Value: a.Service}, + {Key: "action", Value: a.Name}, + } + if bucket != "" { + reqTags = append(reqTags, Tag{Key: "bucket", Value: bucket}) + } + m.send(ctx, err, action, count, status, reqTags) +} + +func (m *manager) send(ctx fiber.Ctx, err error, action string, count int64, status int, reqTags []Tag) { reqStatus := status if err != nil { diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 1ee6442e..8f23b527 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -68,11 +68,23 @@ type opsEmitter struct { version string hasEtag bool hasVer bool + // committed records that the backend created the object. + // The creation event keys off this fact, not off the final + // publication's error status: a committed PUT whose native + // finalizer later failed still created the object, and its + // creation event must not be lost. + committed bool + // eventSent guards the creation event against a second + // publication path (stashed terminal consumed by a release). + eventSent bool } -// setCommitMeta records the backend-assigned object metadata for -// the success publication's event payload. -func (e *opsEmitter) setCommitMeta(etag, version string) { +// markCommitted records the backend commit fact together with the +// backend-assigned object metadata. The creation event fires for +// any publication after this, including an error publication from +// a failed native finalizer: the object exists regardless of the +// finalizer's fate. +func (e *opsEmitter) markCommitted(etag, version string) { if e == nil { return } @@ -80,6 +92,7 @@ func (e *opsEmitter) setCommitMeta(etag, version string) { e.hasEtag = true e.version = version e.hasVer = true + e.committed = true } // synthesize builds a fiber context whose path and request locals @@ -141,7 +154,10 @@ func (e *opsEmitter) publish(err error, bytes int64) { } if e.ops.Metrics != nil { - e.ops.Metrics.Send(ctx, sinkErr, action, bytes, status) + // The bucket dimension comes from the captured session, + // not the route: the synthesized context has no matched + // route, so Params("bucket") would be empty here. + e.ops.Metrics.SendWithBucket(ctx, sinkErr, action, bytes, status, e.bucket) } if e.ops.Logger != nil { e.ops.Logger.Log(ctx, sinkErr, nil, s3log.LogMeta{ @@ -152,9 +168,12 @@ func (e *opsEmitter) publish(err error, bytes int64) { ObjectSize: bytes, }) } - // The object-created event fires at commit time only; error - // publications never carry it. - if e.ops.Events != nil && err == nil && e.isPut { + // The object-created event keys off the backend commit fact, + // not the publication's error status: a committed PUT whose + // native finalizer failed still created the object, so its + // creation event must survive. Uncommitted PUTs (backend + // failure) never carry it. + if e.ops.Events != nil && e.committed && e.isPut && !e.eventSent { meta := s3event.EventMeta{ EventName: s3event.EventObjectCreatedPut, ObjectSize: bytes, @@ -167,6 +186,7 @@ func (e *opsEmitter) publish(err error, bytes int64) { ver := e.version meta.VersionId = &ver } + e.eventSent = true e.ops.Events.SendEvent(ctx, meta) } } @@ -219,6 +239,11 @@ type sessionRecord struct { // will come, so a later release of the reservation resolves // the stashed outcome instead of leaving an orphan. terminal bool + // claimGen identifies the current reservation. Each reserve + // bumps it, so a release or publication from an earlier + // reservation is rejected even though the record's emitter + // pointer is reused across claims. + claimGen uint64 } // opsTracker owns terminal publication: each session publishes @@ -243,7 +268,6 @@ type opsTracker struct { sessions map[string]*sessionRecord app *fiber.App pubq chan pubJob - overflow chan struct{} done chan struct{} drain chan struct{} drainOnce sync.Once @@ -256,25 +280,30 @@ type pubJob struct { byt int64 } -// pubQueueDepth bounds how many publications may wait in the -// handoff queue before the caller falls back to inline execution. -const pubQueueDepth = 256 +// pubQueueCapacity returns the publication queue depth for a +// given session limit. Each session publishes exactly one +// terminal record (enforced by the reservation protocol), and the +// session count never exceeds the configured limit, so a queue +// this deep can never fill - native callbacks always hand off +// without waiting. A floor keeps tests and degenerate zero +// configs sane. +func pubQueueCapacity(maxSessions uint32) int { + n := int(maxSessions) + 1 // +1: shutdown drain may race one last dispatch + if n < 64 { + n = 64 + } + return n +} -// pubOverflowSlots bounds how many callers may run overflow -// publications inline at once; further callers block on the -// semaphore until a slot frees. -const pubOverflowSlots = 8 - -func newOpsTracker() *opsTracker { +func newOpsTracker(queueDepth int) *opsTracker { t := &opsTracker{ sessions: map[string]*sessionRecord{}, app: fiber.New(fiber.Config{ Immutable: true, }), - pubq: make(chan pubJob, pubQueueDepth), - overflow: make(chan struct{}, pubOverflowSlots), - done: make(chan struct{}), - drain: make(chan struct{}), + pubq: make(chan pubJob, queueDepth), + done: make(chan struct{}), + drain: make(chan struct{}), } go func() { defer close(t.done) @@ -321,24 +350,14 @@ func (t *opsTracker) Shutdown() { }) } -// dispatch hands a publication to the worker. It must never block -// indefinitely: when the queue is full (the worker itself stuck in -// a sink), the publication runs inline so the record is still -// delivered and the caller - possibly the native reaper - returns. -// dispatch hands a publication to the worker. It must never block -// indefinitely: when the queue is full the caller runs the -// publication itself under an overflow semaphore, which bounds how -// many overflow publications may wait at once. Overflow callers -// beyond the semaphore block - the alternative (one goroutine per -// job) grows without bound under a stalled sink, and dropping the -// record loses the publication entirely. The callers that reach -// overflow are the native reaper or a request handler; waiting -// there is bounded by the semaphore and by the worker draining, -// and is the price of never losing a record. +// dispatch hands a publication to the worker without ever +// blocking the caller. The queue is sized to the session limit +// and each session publishes exactly once, so the send below +// always has room while the worker runs; after the worker exits +// (shutdown drain) the publication runs inline, because the queue +// no longer moves. Either way the native callback - which may be +// the reaper - returns promptly and the record is never lost. func (t *opsTracker) dispatch(job pubJob) { - // After the worker exited (shutdown drain), the queue no - // longer moves: publish inline so the record is not - // stranded behind a send nobody will receive. select { case <-t.done: job.emit.publish(job.err, job.byt) @@ -347,14 +366,12 @@ func (t *opsTracker) dispatch(job pubJob) { } select { case t.pubq <- job: - return default: + // Unreachable while the capacity contract holds; kept + // as a safety net so a mis-sized queue degrades to + // inline publication instead of blocking the reaper. + job.emit.publish(job.err, job.byt) } - // Queue full and worker alive. Run inline under the overflow - // semaphore. - t.overflow <- struct{}{} - defer func() { <-t.overflow }() - job.emit.publish(job.err, job.byt) } // SetOpsServices installs the operational service instances. The @@ -435,11 +452,18 @@ func (t *opsTracker) failOutcome(sessionID string, err error) { t.dispatch(pubJob{emit: rec.emit, err: err}) } +// reservation couples the emitter with the generation of the +// claim that owns it: release and publication validate the +// generation, so a stale claim cannot act on a newer one. +type reservation struct { + emit *opsEmitter + gen uint64 +} // reserve marks a session record as owned by its request path: the // teardown callback skips a reserved record because the request -// path publishes the real outcome itself. Returns the emitter when -// the record exists and was not reserved yet. -func (t *opsTracker) reserve(sessionID string) *opsEmitter { +// path publishes the real outcome itself. Returns the reservation +// when the record exists and was not reserved yet. +func (t *opsTracker) reserve(sessionID string) *reservation { if t == nil { return nil } @@ -450,7 +474,8 @@ func (t *opsTracker) reserve(sessionID string) *opsEmitter { return nil } rec.reserved = true - return rec.emit + rec.claimGen++ + return &reservation{emit: rec.emit, gen: rec.claimGen} } // releaseReservation returns a reserved record to the pool @@ -461,13 +486,13 @@ func (t *opsTracker) reserve(sessionID string) *opsEmitter { // 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) { +func (t *opsTracker) releaseReservation(sessionID string, rsv *reservation) { if t == nil { return } t.mu.Lock() rec, ok := t.sessions[sessionID] - if !ok || !rec.reserved || rec.emit != emit { + if !ok || !rec.reserved || rsv == nil || rec.claimGen != rsv.gen { t.mu.Unlock() return } @@ -483,25 +508,23 @@ func (t *opsTracker) releaseReservation(sessionID string, emit *opsEmitter) { // publishReserved publishes through a reserved record and drops it: // the single publication of a request-owned session outcome. -func (t *opsTracker) publishReserved(sessionID string, emit *opsEmitter, err error, bytes int64) { +func (t *opsTracker) publishReserved(sessionID string, rsv *reservation, err error, bytes int64) { if t == nil { 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 + // Ownership check: only the current reservation generation + // publishes. A stale claim (its reservation was released or + // superseded) must not delete or publish the current // owner's record. - if !ok || rec.emit != emit { + if !ok || rsv == nil || rec.claimGen != rsv.gen { t.mu.Unlock() return } delete(t.sessions, sessionID) t.mu.Unlock() - if emit != nil { - t.dispatch(pubJob{emit: emit, err: err, byt: bytes}) - } + t.dispatch(pubJob{emit: rsv.emit, err: err, byt: bytes}) } func (t *opsTracker) loadOps() OpsServices { diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 2cd957dd..12f9dc60 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -38,7 +38,7 @@ import ( // exactly once. Unreserved records are published by the callback. func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) tr.register("sess-1", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) if got := len(tr.sessions); got != 1 { @@ -60,14 +60,14 @@ func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { } func TestOpsTrackerReserveBlocksCallback(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) tr.register("sess-2", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", true, time.Now()) // The READY path reserves before its completion call; the // callback the call fires synchronously must skip the record. - emit := tr.reserve("sess-2") - if emit == nil { + rsv := tr.reserve("sess-2") + if rsv == nil { t.Fatal("reserve returned nil for a live session") } tr.onTerminal(rcserver.TerminalEvent{SessionID: "sess-2"}) @@ -76,7 +76,7 @@ func TestOpsTrackerReserveBlocksCallback(t *testing.T) { } // The request path then publishes and drops the entry. - tr.publishReserved("sess-2", emit, nil, 4096) + tr.publishReserved("sess-2", rsv, nil, 4096) if got := len(tr.sessions); got != 0 { t.Fatalf("publishReserved left residue: %d", got) } @@ -88,7 +88,7 @@ func TestOpsTrackerReserveBlocksCallback(t *testing.T) { } func TestOpsTrackerReserveIsExclusive(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) tr.register("sess-3", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -101,7 +101,7 @@ func TestOpsTrackerReserveIsExclusive(t *testing.T) { } func TestOpsTrackerFailOutcome(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -116,7 +116,7 @@ func TestOpsTrackerFailOutcome(t *testing.T) { } func TestOpsTrackerUnregister(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) tr.register("sess-5", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) tr.unregister("sess-5") @@ -132,7 +132,7 @@ func TestOpsTrackerUnregister(t *testing.T) { } func TestOpsTrackerUnknownSession(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) // Unknown sessions and the nil tracker are silent no-ops. var nilTracker *opsTracker nilTracker.reserve("ghost") @@ -237,7 +237,7 @@ func (r *recordingLogger) Shutdown() error { return nil } // publishes exactly one record with its own outcome and bytes. func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { rl := &recordingLogger{} - tr := newOpsTracker() + tr := newOpsTracker(pubQueueCapacity(0)) tr.SetOpsServices(OpsServices{Logger: rl}) // Expiry path: callback publishes a zero-byte error record. @@ -247,69 +247,79 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { // Reserved path: reserve, callback fires (skipped), the // request path publishes success with bytes. tr.register("s-res", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) - emit := tr.reserve("s-res") - if emit == nil { + rsv := tr.reserve("s-res") + if rsv == nil { t.Fatal("reserve failed") } tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-res"}) - tr.publishReserved("s-res", emit, nil, 128) + tr.publishReserved("s-res", rsv, nil, 128) // Denial path while reserved: failOutcome must not steal the // publication; the owner's success record is the only one. tr.register("s-den", auth.Account{Access: "ak"}, "r", "b", "o", true, time.Now()) - emit2 := tr.reserve("s-den") - if emit2 == nil { + rsv2 := tr.reserve("s-den") + if rsv2 == nil { t.Fatal("reserve failed") } tr.failOutcome("s-den", errors.New("denied")) - tr.publishReserved("s-den", emit2, nil, 256) + tr.publishReserved("s-den", rsv2, nil, 256) // Released reservation: the record returns to the pool and // the reaper (or the next claimant) can still publish it. tr.register("s-rel", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) - emit3 := tr.reserve("s-rel") - if emit3 == nil { + rsv3 := tr.reserve("s-rel") + if rsv3 == nil { t.Fatal("reserve failed") } - tr.releaseReservation("s-rel", emit3) + tr.releaseReservation("s-rel", rsv3) 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 { + rsvS := tr.reserve("s-stash") + if rsvS == nil { t.Fatal("reserve failed") } tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-stash"}) - tr.releaseReservation("s-stash", emitS) + tr.releaseReservation("s-stash", rsvS) // 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 { + rsvD := tr.reserve("s-dbl") + if rsvD == nil { t.Fatal("first reserve failed") } if tr.reserve("s-dbl") != nil { t.Fatal("double reserve succeeded") } - tr.publishReserved("s-dbl", emitD, nil, 64) + tr.publishReserved("s-dbl", rsvD, 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 { + rsvO := tr.reserve("s-own") + if rsvO == nil { t.Fatal("reserve failed") } - tr.publishReserved("s-own", &opsEmitter{}, nil, 999) + // Stale reservation: released, re-reserved by another claim, + // then the stale token tries to publish. The generation check + // must reject the stale token while the current owner still + // publishes. + rsvO2 := tr.reserve("s-own") + tr.releaseReservation("s-own", rsvO) + rsvB := tr.reserve("s-own") + if rsvB == nil { + t.Fatal("re-reserve after release failed") + } + tr.publishReserved("s-own", rsvO2, nil, 999) // stale: no-op tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-own"}) - tr.publishReserved("s-own", emitO, nil, 32) + tr.publishReserved("s-own", rsvB, nil, 32) // Consume-or-noop denial of an unreserved session. tr.register("s-fail", auth.Account{Access: "ak"}, "r", "b", "o", false, time.Now()) @@ -337,6 +347,9 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { {false, 128}, // s-res success {false, 256}, // s-den success (denial was skipped) {true, 0}, // s-rel expiry after release + {true, 0}, // s-stash stashed terminal consumed by release + {false, 64}, // s-dbl owner publishes after refused double reserve + {false, 32}, // s-own current owner publishes; stale token no-op {true, 0}, // s-fail denial } for i, w := range want { diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index 9dd79661..d25b5164 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -121,7 +121,7 @@ func New(svc *rcserver.RCSvc, be backend.Backend, iam auth.IAMService, readonly, disableACL bool) *Handler { return &Handler{svc: svc, be: be, iam: iam, readonly: readonly, disableACL: disableACL, - ops: newOpsTracker()} + ops: newOpsTracker(pubQueueCapacity(svc.MaxSessions()))} } // principalID derives the session identity digest from the @@ -139,6 +139,14 @@ func errNotAdmitted() error { return errRouteUnavailable{} } +// ErrNotAdmitted is the route error for requests that lost the +// race with shutdown: the RC service stopped admitting, so the +// request cannot be served. Exposed for the admission barrier in +// the route middleware, which runs before the handlers. +func ErrNotAdmitted() error { + return errNotAdmitted() +} + func invalidHeader(name, value string) error { return fmt.Errorf("invalid %s header: %q: %w", name, value, errRouteBadRequest{}) @@ -400,8 +408,8 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // READY's denial (or the reaper) must not be able to consume // 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 { + rsv := h.ops.reserve(sessionID) + if rsv == nil { // Another READY holds the publication reservation for // this session. Publication ownership must track native // transfer ownership: proceeding without the reservation @@ -410,8 +418,15 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // record on completion. Answer as a duplicate claim. return fmt.Errorf("transfer in progress: %w", rcserver.ErrDouble) } + // Panic safety starts at the reservation: an unwind anywhere + // below (authorization, backend I/O) must still retire the + // reservation so the record is not orphaned - the terminal + // callback can only stash under a reservation, and nobody + // else would ever release it. releaseReservation is a no-op + // once a later publish consumed the record. + defer h.ops.releaseReservation(sessionID, rsv) publish := func(err error, bytes int64) { - h.ops.publishReserved(sessionID, emit, err, bytes) + h.ops.publishReserved(sessionID, rsv, err, bytes) } if err := h.authorize(ctx, acct, isRoot, bucket, key, info.Op == 1); err != nil { // Permission revoked mid-session: publish the real @@ -441,7 +456,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // the record for this claim; release it without // publishing so the surviving path (the other READY, // or the eventual reaper) still owns it. - h.ops.releaseReservation(sessionID, emit) + h.ops.releaseReservation(sessionID, rsv) return mapRcError(err) } @@ -453,7 +468,7 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // released the same way: the session stays with the record // the next READY (or the reaper) will claim. if resp.Outcome == rcserver.ReadyBusy { - h.ops.releaseReservation(sessionID, emit) + h.ops.releaseReservation(sessionID, rsv) return fmt.Errorf("peer busy: %w", rcserver.ErrDouble) } @@ -498,21 +513,26 @@ func (h *Handler) readyCore(ctx fiber.Ctx) error { // finalizer retires exactly at that point; a failure // *before* the borrow still falls back to the // finalizer path below. - put, gd, err := h.commitPut(ctx, sessionID, bucket, key, sizeOf(resp)) - if gd { + put, viewDone, committed, err := h.commitPut(ctx, sessionID, bucket, key, sizeOf(resp)) + if viewDone { finalized = true } + if put != nil { + // The backend committed the object. Record the + // fact before anything else can fail: the audit + // record and creation event must reflect the + // commit even when the native finalizer below + // errors out. + rsv.emit.markCommitted(put.ETag, put.VersionID) + } if err != nil { - doPublish(mapRcError(err), 0) + doPublish(mapRcError(err), committed) return err } // The FINAL wire reply carries the stored object's // metadata, which the backend assigned at commit time. resp.Etag = put.ETag resp.VersionID = put.VersionID - // The success publication's object-created event carries - // the same commit metadata. - emit.setCommitMeta(put.ETag, put.VersionID) } else if err := h.svc.FinishFinal(sessionID); err != nil { doPublish(mapRcError(err), 0) return mapRcError(err) @@ -553,18 +573,18 @@ func sizeOf(resp *rcserver.ReadyResponse) uint64 { // panic-safe defer releases the view if the handler unwinds before // FinishPut runs. func (h *Handler) commitPut(ctx fiber.Ctx, sessionID, bucket, key string, - size uint64) (*s3response.PutObjectOutput, bool, error) { + size uint64) (put *s3response.PutObjectOutput, viewDone bool, committed int64, err error) { view, err := h.svc.GetPutData(sessionID) if err != nil { - return nil, false, mapRcError(err) + return nil, false, 0, mapRcError(err) } // Panic-safe ownership: if anything below unwinds, the view is // still returned exactly once (the ABI consumes the handle a // single time; a redundant FinishPut after a commit is a // no-op STALE). - committed := false + putDone := false defer func() { - if !committed { + if !putDone { _ = h.svc.FinishPut(*view, false, "", "") } }() @@ -579,13 +599,19 @@ func (h *Handler) commitPut(ctx fiber.Ctx, sessionID, bucket, key string, Body: bytes.NewReader(view.Buf), }) if err != nil { - return nil, true, err + // The object was not created; the audit records zero + // transferred bytes. The view was consumed above. + return nil, true, 0, err } + // The object exists from here on. The value below is the + // committed byte count, reported even when the native + // finalizer fails, so the audit record reflects the commit. + committed = contentLength if err := h.svc.FinishPut(*view, true, res.ETag, res.VersionID); err != nil { - return nil, true, mapRcError(err) + return &res, true, committed, mapRcError(err) } - committed = true - return &res, true, nil + putDone = true + return &res, true, committed, nil } // Cancel handles CANCEL: authenticated owner tears the session down. diff --git a/rdma/rcroutes/routes_stub.go b/rdma/rcroutes/routes_stub.go index d9c4b1f0..b735c47c 100644 --- a/rdma/rcroutes/routes_stub.go +++ b/rdma/rcroutes/routes_stub.go @@ -19,6 +19,8 @@ package rcroutes import ( + "errors" + "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/auth" @@ -65,5 +67,10 @@ func (h *Handler) SetOpsServices(ops OpsServices) {} // PublishAuthFailure is a stub mirror of the linux handler. func (h *Handler) PublishAuthFailure(ctx fiber.Ctx, err error) {} +// ErrNotAdmitted is a stub mirror of the linux helper. +func ErrNotAdmitted() error { + return errors.New("rc routes unavailable") +} + // Shutdown is a stub mirror of the linux handler. func (h *Handler) Shutdown() {} diff --git a/rdma/rcserver/rcserver_linux.go b/rdma/rcserver/rcserver_linux.go index aad73e0c..6a2fef27 100644 --- a/rdma/rcserver/rcserver_linux.go +++ b/rdma/rcserver/rcserver_linux.go @@ -174,12 +174,13 @@ type SessionInfo struct { // unblock), waits for every entered call to leave, then tears the // server down; it is idempotent and safe from any goroutine. type RCSvc struct { - srv *C.rc_server - closing atomic.Bool - ops atomic.Int64 - once sync.Once - ctx context.Context - cancel context.CancelFunc + srv *C.rc_server + closing atomic.Bool + ops atomic.Int64 + once sync.Once + ctx context.Context + cancel context.CancelFunc + maxSessions uint32 } // Context returns the service-lifetime context. Handlers bind @@ -188,6 +189,14 @@ func (s *RCSvc) Context() context.Context { return s.ctx } +// MaxSessions reports the configured global session limit. The +// operational publication pipeline sizes its queue against it: +// each session publishes exactly one terminal record, so a queue +// this deep can never fill. +func (s *RCSvc) MaxSessions() uint32 { + return s.maxSessions +} + // rcLogSink receives every diagnostic line the C server emits. // It is stateless and process-global on purpose: the sink must be // valid from init through destroy, independent of any single @@ -263,7 +272,7 @@ func Init(opts DeviceOpts) (*RCSvc, error) { } installLogSink(srv, opts.Debug) ctx, cancel := context.WithCancel(context.Background()) - return &RCSvc{srv: srv, ctx: ctx, cancel: cancel}, nil + return &RCSvc{srv: srv, ctx: ctx, cancel: cancel, maxSessions: opts.MaxSessions}, nil } // TryEnter admits a request into the service. It returns false once From e9a2a2f98be19519b0e679e022f54dc67920fdb2 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 21:30:57 +0900 Subject: [PATCH 11/16] rdma: never run operational sinks on callback threads Rework the publication handoff so a native callback can never execute a sink: the channel buffer absorbs the common case, and a full buffer appends to an overflow list that the worker drains after the channel instead of falling back to inline publication. Queued records accumulate across successive sessions and failed authentications consume no session at all, so capacity accounting cannot bound the backlog; only removing the fallback closes the stall. Drop the now-unused session-limit accessor. Move signature verification back outside the admission barrier: IAM lookups carry no cancellation, so holding the barrier across verification let one stalled lookup defer RC shutdown indefinitely. The handlers enforce admission themselves, and a failure publication checks the drain state before dispatching, so it cannot land after the sinks close. Synchronize metrics producers with Close: the manager now marks itself closed before closing the datapoint channel, and a producer that still races the closure recovers instead of panicking on a send over a closed channel. Exercise real stale tokens in the reservation generation test: the original token attempts both release and publish after a newer claim took over. --- cmd/vgwrdma/main.go | 18 +++--- metrics/metrics.go | 21 ++++++- rdma/rcroutes/ops_linux.go | 103 +++++++++++++++++++++----------- rdma/rcroutes/ops_linux_test.go | 31 ++++++---- rdma/rcroutes/routes_linux.go | 2 +- rdma/rcserver/rcserver_linux.go | 23 +++---- 6 files changed, 123 insertions(+), 75 deletions(-) diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index 354be6e3..55128c6c 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -1300,16 +1300,14 @@ func runGateway(ctx context.Context, be backend.Backend) error { // logger and event schema read the region from // there, so set it for every verified request. utils.ContextKeyRegion.Set(ctx, region) - // Admission barrier: verification can block on IAM - // lookups, and its failure publications must not - // outlive the RC shutdown sequence (sinks close - // right after the RC service drains). Requests that - // lose the race to shutdown are dropped - their - // audit record would land after the sinks closed. - if !rcSvc.TryEnter() { - return rcroutes.WriteRouteError(ctx, rcroutes.ErrNotAdmitted()) - } - defer rcSvc.Leave() + // Verification runs outside the admission barrier: + // signature checks can block on IAM lookups that + // carry no cancellation, and holding the barrier + // across them would let one stalled lookup defer + // RC shutdown indefinitely. The handlers enforce + // admission themselves; a failure publication + // produced here checks the drain state before + // dispatching, so it cannot outlive the sinks. if err := rcVerify(ctx); err != nil { rcH.PublishAuthFailure(ctx, err) return rcroutes.WriteRouteError(ctx, err) diff --git a/metrics/metrics.go b/metrics/metrics.go index 0d725dba..2b538b1f 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -22,6 +22,7 @@ import ( "os" "strings" "sync" + "sync/atomic" "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/s3err" @@ -62,6 +63,12 @@ type manager struct { publishers []publisher addDataChan chan datapoint + // closed gates senders against Close: the datapoint channel + // is closed to drain the forwarder, and a send on a closed + // channel panics. Producers that lose this race (an S3 + // handler still finishing after the shutdown timeout) drop + // their update instead of taking the process down. + closed atomic.Bool } type Config struct { @@ -214,7 +221,7 @@ func (m *manager) increment(key string, tags ...Tag) { // add adds value to key func (m *manager) add(key string, value int64, tags ...Tag) { - if m.ctx.Err() != nil { + if m.ctx.Err() != nil || m.closed.Load() { return } @@ -224,6 +231,13 @@ func (m *manager) add(key string, value int64, tags ...Tag) { tags: tags, } + // The send races Close for last-producer position: the + // closed check above and the channel close in Close are not + // atomic, so the send below can still observe a closed + // channel. Recovering here turns that race into a dropped + // datapoint, which is the documented contract for late + // producers. + defer func() { _ = recover() }() select { case m.addDataChan <- d: default: @@ -233,6 +247,11 @@ func (m *manager) add(key string, value int64, tags ...Tag) { // Close closes metrics channels, waits for data to complete, closes all plugins func (m *manager) Close() { + // Stop accepting new datapoints before closing the channel: + // producers check the flag and drop their update, so only a + // producer already between the check and the send can race, + // and that producer recovers instead of panicking. + m.closed.Store(true) // drain the datapoint channels close(m.addDataChan) m.wg.Wait() diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 8f23b527..5c5a1e15 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -263,11 +263,18 @@ type sessionRecord struct { // guarantee that no record is silently dropped while still capping // how long a callback may wait. type opsTracker struct { - mu sync.Mutex - ops OpsServices - sessions map[string]*sessionRecord - app *fiber.App - pubq chan pubJob + mu sync.Mutex + ops OpsServices + sessions map[string]*sessionRecord + app *fiber.App + pubq chan pubJob + // overflow holds publications that arrived while the queue + // buffer was full. A native callback must never wait on a + // slow sink, so dispatch appends here (under pubmu) instead + // of blocking or running the sink itself, and the worker + // drains this list after the channel empties. + pubmu sync.Mutex + overflow []pubJob done chan struct{} drain chan struct{} drainOnce sync.Once @@ -280,28 +287,30 @@ type pubJob struct { byt int64 } -// pubQueueCapacity returns the publication queue depth for a -// given session limit. Each session publishes exactly one -// terminal record (enforced by the reservation protocol), and the -// session count never exceeds the configured limit, so a queue -// this deep can never fill - native callbacks always hand off -// without waiting. A floor keeps tests and degenerate zero -// configs sane. -func pubQueueCapacity(maxSessions uint32) int { - n := int(maxSessions) + 1 // +1: shutdown drain may race one last dispatch - if n < 64 { - n = 64 - } - return n -} +// pubQueueSoftCap is the buffered pre-allocation of the +// publication queue, not a bound: the overflow list in dispatch +// holds whatever exceeds it, so a slow sink never blocks a +// native callback. +const pubQueueSoftCap = 256 -func newOpsTracker(queueDepth int) *opsTracker { +// newOpsTracker builds the tracker. The publication queue is +// conceptually unbounded: a callback thread must never run a +// sink (a blocked sink would stall the native reaper and defer +// RC shutdown), so dispatch always hands off without waiting, +// whatever the backlog. Capacity accounting cannot bound the +// backlog - queued records accumulate across successive sessions +// and authentication failures consume no session at all - so the +// worker is the only sink executor and the queue absorbs +// whatever the sinks cannot keep up with. Each job is a few +// pointers; a stalled sink delays records, it does not lose +// them. +func newOpsTracker() *opsTracker { t := &opsTracker{ sessions: map[string]*sessionRecord{}, app: fiber.New(fiber.Config{ Immutable: true, }), - pubq: make(chan pubJob, queueDepth), + pubq: make(chan pubJob, pubQueueSoftCap), done: make(chan struct{}), drain: make(chan struct{}), } @@ -315,9 +324,13 @@ func newOpsTracker(queueDepth int) *opsTracker { } job.emit.publish(job.err, job.byt) case <-t.drain: - // Drain mode: empty whatever is already - // queued, then exit. Producers past this - // point publish inline. + // Drain mode: empty the channel, then the + // overflow list, then exit. Producers past + // this point append to overflow; the final + // sweep below runs only what arrived before + // the drain signal, and a producer racing + // the sweep re-enqueues through dispatch's + // post-drain path. for { select { case job, ok := <-t.pubq: @@ -326,6 +339,13 @@ func newOpsTracker(queueDepth int) *opsTracker { } job.emit.publish(job.err, job.byt) default: + t.pubmu.Lock() + pending := t.overflow + t.overflow = nil + t.pubmu.Unlock() + for _, job := range pending { + job.emit.publish(job.err, job.byt) + } return } } @@ -351,12 +371,17 @@ func (t *opsTracker) Shutdown() { } // dispatch hands a publication to the worker without ever -// blocking the caller. The queue is sized to the session limit -// and each session publishes exactly once, so the send below -// always has room while the worker runs; after the worker exits -// (shutdown drain) the publication runs inline, because the queue -// no longer moves. Either way the native callback - which may be -// the reaper - returns promptly and the record is never lost. +// blocking the caller or running a sink on the calling thread: +// the native reaper invokes terminal callbacks, and an +// operational sink can block indefinitely, which must never +// stall reaping or RC shutdown. The channel buffer absorbs the +// common case; when it is full the job goes to the overflow +// list, which the worker drains after the channel. After the +// worker exits (shutdown drain), a session-terminal job is +// published inline - its producer (Close, after quiescing +// native producers) is not a native callback - while request +// publications are dropped by publishRequest before reaching +// here. func (t *opsTracker) dispatch(job pubJob) { select { case <-t.done: @@ -367,10 +392,9 @@ func (t *opsTracker) dispatch(job pubJob) { select { case t.pubq <- job: default: - // Unreachable while the capacity contract holds; kept - // as a safety net so a mis-sized queue degrades to - // inline publication instead of blocking the reaper. - job.emit.publish(job.err, job.byt) + t.pubmu.Lock() + t.overflow = append(t.overflow, job) + t.pubmu.Unlock() } } @@ -597,11 +621,22 @@ func expiredError(ev rcserver.TerminalEvent) error { // publishRequest emits an operation record for a request that ended // before any session existed (authentication, authorization, or // header failures): no tracking table entry, single emission. +// These requests run outside the admission barrier (verification +// may block on uncancellable IAM lookups), so a record produced +// after the shutdown drain began is dropped rather than published +// into closed sinks. func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, err error, bucket, key string, isPut bool) { if t == nil { return } + select { + case <-t.done: + // The worker already exited through the drain; sinks + // are closing. Drop the record. + return + default: + } acct.Access = strings.Clone(acct.Access) emit := &opsEmitter{ ops: t.loadOps(), diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 12f9dc60..09a2c3d2 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -38,7 +38,7 @@ import ( // exactly once. Unreserved records are published by the callback. func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() tr.register("sess-1", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) if got := len(tr.sessions); got != 1 { @@ -60,7 +60,7 @@ func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { } func TestOpsTrackerReserveBlocksCallback(t *testing.T) { - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() tr.register("sess-2", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", true, time.Now()) @@ -88,7 +88,7 @@ func TestOpsTrackerReserveBlocksCallback(t *testing.T) { } func TestOpsTrackerReserveIsExclusive(t *testing.T) { - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() tr.register("sess-3", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -101,7 +101,7 @@ func TestOpsTrackerReserveIsExclusive(t *testing.T) { } func TestOpsTrackerFailOutcome(t *testing.T) { - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -116,7 +116,7 @@ func TestOpsTrackerFailOutcome(t *testing.T) { } func TestOpsTrackerUnregister(t *testing.T) { - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() tr.register("sess-5", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) tr.unregister("sess-5") @@ -132,7 +132,7 @@ func TestOpsTrackerUnregister(t *testing.T) { } func TestOpsTrackerUnknownSession(t *testing.T) { - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() // Unknown sessions and the nil tracker are silent no-ops. var nilTracker *opsTracker nilTracker.reserve("ghost") @@ -237,7 +237,7 @@ func (r *recordingLogger) Shutdown() error { return nil } // publishes exactly one record with its own outcome and bytes. func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { rl := &recordingLogger{} - tr := newOpsTracker(pubQueueCapacity(0)) + tr := newOpsTracker() tr.SetOpsServices(OpsServices{Logger: rl}) // Expiry path: callback publishes a zero-byte error record. @@ -307,18 +307,23 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { if rsvO == nil { t.Fatal("reserve failed") } - // Stale reservation: released, re-reserved by another claim, - // then the stale token tries to publish. The generation check - // must reject the stale token while the current owner still + // Stale generation: the original owner releases, another + // claim re-reserves, and then the ORIGINAL token tries both + // release and publish. The generation check must reject the + // stale token on both paths while the current owner still // publishes. - rsvO2 := tr.reserve("s-own") + rsvO2 := tr.reserve("s-own") // refused: still reserved by rsvO + if rsvO2 != nil { + t.Fatal("double reserve succeeded") + } tr.releaseReservation("s-own", rsvO) rsvB := tr.reserve("s-own") if rsvB == nil { t.Fatal("re-reserve after release failed") } - tr.publishReserved("s-own", rsvO2, nil, 999) // stale: no-op - tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-own"}) + tr.publishReserved("s-own", rsvO, nil, 999) // stale: no-op + tr.onTerminal(rcserver.TerminalEvent{SessionID: "s-own"}) // stashes under rsvB + tr.releaseReservation("s-own", rsvO) // stale: no-op, keeps rsvB tr.publishReserved("s-own", rsvB, nil, 32) // Consume-or-noop denial of an unreserved session. diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index d25b5164..d1e185dd 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -121,7 +121,7 @@ func New(svc *rcserver.RCSvc, be backend.Backend, iam auth.IAMService, readonly, disableACL bool) *Handler { return &Handler{svc: svc, be: be, iam: iam, readonly: readonly, disableACL: disableACL, - ops: newOpsTracker(pubQueueCapacity(svc.MaxSessions()))} + ops: newOpsTracker()} } // principalID derives the session identity digest from the diff --git a/rdma/rcserver/rcserver_linux.go b/rdma/rcserver/rcserver_linux.go index 6a2fef27..aad73e0c 100644 --- a/rdma/rcserver/rcserver_linux.go +++ b/rdma/rcserver/rcserver_linux.go @@ -174,13 +174,12 @@ type SessionInfo struct { // unblock), waits for every entered call to leave, then tears the // server down; it is idempotent and safe from any goroutine. type RCSvc struct { - srv *C.rc_server - closing atomic.Bool - ops atomic.Int64 - once sync.Once - ctx context.Context - cancel context.CancelFunc - maxSessions uint32 + srv *C.rc_server + closing atomic.Bool + ops atomic.Int64 + once sync.Once + ctx context.Context + cancel context.CancelFunc } // Context returns the service-lifetime context. Handlers bind @@ -189,14 +188,6 @@ func (s *RCSvc) Context() context.Context { return s.ctx } -// MaxSessions reports the configured global session limit. The -// operational publication pipeline sizes its queue against it: -// each session publishes exactly one terminal record, so a queue -// this deep can never fill. -func (s *RCSvc) MaxSessions() uint32 { - return s.maxSessions -} - // rcLogSink receives every diagnostic line the C server emits. // It is stateless and process-global on purpose: the sink must be // valid from init through destroy, independent of any single @@ -272,7 +263,7 @@ func Init(opts DeviceOpts) (*RCSvc, error) { } installLogSink(srv, opts.Debug) ctx, cancel := context.WithCancel(context.Background()) - return &RCSvc{srv: srv, ctx: ctx, cancel: cancel, maxSessions: opts.MaxSessions}, nil + return &RCSvc{srv: srv, ctx: ctx, cancel: cancel}, nil } // TryEnter admits a request into the service. It returns false once From 64a6ed0c3786c09a9ba6a7fb21b3960e932da380 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 21:55:36 +0900 Subject: [PATCH 12/16] rdma: synchronize publication acceptance with the drain boundary Service the overflow list during normal operation: the worker publishes and clears it after every channel job, so a burst that exceeds the channel buffer drains as soon as the sink recovers instead of accumulating until shutdown. Make dispatch and the drain sweep share one critical section. An append either lands before the sweep and is drained, or runs after the worker exited and publishes inline; the check-then-send window that could strand a record between the two is closed. Request publications use the same boundary with drop semantics: once the drain finished, no owner can guarantee the sinks are still open, so the record is dropped rather than published. Never close the metrics datapoint channel. The forwarder exits through the canceled context after draining the buffer, the closed flag turns late producers into drops, and no send can race a closure. Implement the new Manager method in the test mock. --- embedgw/embedgw.go | 7 +++ metrics/metrics.go | 45 +++++++++++++----- rdma/rcroutes/ops_linux.go | 83 +++++++++++++++++++++++++--------- s3api/controllers/base_test.go | 4 +- 4 files changed, 106 insertions(+), 33 deletions(-) diff --git a/embedgw/embedgw.go b/embedgw/embedgw.go index 629e16d5..b5b9d9f8 100644 --- a/embedgw/embedgw.go +++ b/embedgw/embedgw.go @@ -1243,6 +1243,13 @@ Loop: } if metricsManager != nil { + // Cancel the metrics context first: the forwarder exits + // through it, draining the buffered datapoints, and + // Close then only waits for the forwarder and closes the + // publishers. The channel itself never closes, so late + // producers (a handler outliving the HTTP shutdown + // timeout) drop their datapoint instead of panicking. + metricsStop() metricsManager.Close() } diff --git a/metrics/metrics.go b/metrics/metrics.go index 2b538b1f..62f15830 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -245,15 +245,14 @@ func (m *manager) add(key string, value int64, tags ...Tag) { } } -// Close closes metrics channels, waits for data to complete, closes all plugins +// Close stops the manager: producers drop new datapoints, the +// forwarder drains the buffered ones and exits through the +// canceled context, and the publishers flush and close. The +// datapoint channel itself is never closed - a producer racing +// the closure would panic - so the closed flag and the context +// cancellation carry the shutdown instead. func (m *manager) Close() { - // Stop accepting new datapoints before closing the channel: - // producers check the flag and drop their update, so only a - // producer already between the check and the send can race, - // and that producer recovers instead of panicking. m.closed.Store(true) - // drain the datapoint channels - close(m.addDataChan) m.wg.Wait() // close all publishers @@ -269,12 +268,36 @@ type publisher interface { } func (m *manager) addForwarder(addChan <-chan datapoint) { - for data := range addChan { - for _, s := range m.publishers { - s.Add(data.key, data.value, data.tags...) + defer m.wg.Done() + for { + select { + case data, ok := <-addChan: + if !ok { + return + } + for _, s := range m.publishers { + s.Add(data.key, data.value, data.tags...) + } + case <-m.ctx.Done(): + // The channel is never closed (producers race its + // closure otherwise); termination is the context. + // Drain whatever the buffer still holds so late + // datapoints are not lost, then exit. + for { + select { + case data, ok := <-addChan: + if !ok { + return + } + for _, s := range m.publishers { + s.Add(data.key, data.value, data.tags...) + } + default: + return + } + } } } - m.wg.Done() } type datapoint struct { diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 5c5a1e15..ce7633a6 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -323,29 +323,35 @@ func newOpsTracker() *opsTracker { return } job.emit.publish(job.err, job.byt) + // Service the overflow list after every + // channel job: bursts that exceed the + // buffer publish as soon as the sink + // recovers instead of waiting for + // shutdown. + t.serviceOverflow() case <-t.drain: - // Drain mode: empty the channel, then the - // overflow list, then exit. Producers past - // this point append to overflow; the final - // sweep below runs only what arrived before - // the drain signal, and a producer racing - // the sweep re-enqueues through dispatch's - // post-drain path. + // Drain mode, single critical section with + // dispatch: holding pubmu across the + // channel-and-list sweep closes the + // accept-vs-drain race - a dispatch that + // appended before this point is drained, + // one that runs after sees done and + // publishes inline. + t.pubmu.Lock() for { select { case job, ok := <-t.pubq: if !ok { + t.pubmu.Unlock() return } job.emit.publish(job.err, job.byt) default: - t.pubmu.Lock() - pending := t.overflow - t.overflow = nil - t.pubmu.Unlock() - for _, job := range pending { + for _, job := range t.overflow { job.emit.publish(job.err, job.byt) } + t.overflow = nil + t.pubmu.Unlock() return } } @@ -355,6 +361,17 @@ func newOpsTracker() *opsTracker { return t } +// serviceOverflow publishes and clears the overflow list if the +// worker can take it. Called by the worker only. +func (t *opsTracker) serviceOverflow() { + t.pubmu.Lock() + defer t.pubmu.Unlock() + for _, job := range t.overflow { + job.emit.publish(job.err, job.byt) + } + t.overflow = nil +} + // Shutdown drains pending publications and stops the worker. The // gateway must call this BEFORE closing the operational sinks: a // queued publication that runs after its logger closed is lost. @@ -383,16 +400,22 @@ func (t *opsTracker) Shutdown() { // publications are dropped by publishRequest before reaching // here. func (t *opsTracker) dispatch(job pubJob) { + // Fast path: the worker is alive. The pubmu critical section + // is the accept-vs-drain boundary: the drain sweep holds the + // same lock, so an append either lands before the sweep (and + // is drained) or after done closed (and runs inline). + t.pubmu.Lock() select { case <-t.done: + t.pubmu.Unlock() job.emit.publish(job.err, job.byt) return default: } select { case t.pubq <- job: + t.pubmu.Unlock() default: - t.pubmu.Lock() t.overflow = append(t.overflow, job) t.pubmu.Unlock() } @@ -630,13 +653,6 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, if t == nil { return } - select { - case <-t.done: - // The worker already exited through the drain; sinks - // are closing. Drop the record. - return - default: - } acct.Access = strings.Clone(acct.Access) emit := &opsEmitter{ ops: t.loadOps(), @@ -648,7 +664,32 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, isPut: isPut, start: time.Now(), } - t.dispatch(pubJob{emit: emit, err: err}) + // The accept-vs-drain boundary decides: a record accepted + // before the drain sweep is published by the worker; one + // that arrives after is dropped here (not published inline), + // because a request publication has no owner left to + // guarantee its sinks are still open. + t.dispatchOrDrop(pubJob{emit: emit, err: err}) +} + +// dispatchOrDrop is dispatch with request-publication semantics: +// after the worker exited through the drain the job is dropped +// instead of published inline. +func (t *opsTracker) dispatchOrDrop(job pubJob) { + t.pubmu.Lock() + select { + case <-t.done: + t.pubmu.Unlock() + return + default: + } + select { + case t.pubq <- job: + t.pubmu.Unlock() + default: + t.overflow = append(t.overflow, job) + t.pubmu.Unlock() + } } // regionFromCtx reads the region the gateway middleware stored on diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go index 46e01d9b..b2169e92 100644 --- a/s3api/controllers/base_test.go +++ b/s3api/controllers/base_test.go @@ -282,7 +282,9 @@ func (m *mockEvSender) Close() error { return nil type mockMetricsManager struct{} func (m *mockMetricsManager) Send(_ fiber.Ctx, _ error, _ string, _ int64, _ int) {} -func (m *mockMetricsManager) Close() {} +func (m *mockMetricsManager) SendWithBucket(_ fiber.Ctx, _ error, _ string, _ int64, _ int, _ string) { +} +func (m *mockMetricsManager) Close() {} func TestProcessController(t *testing.T) { payload, err := xml.Marshal(s3response.Bucket{ From c9668b40b774c6210448331c4c33774559b62f98 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 22:44:56 +0900 Subject: [PATCH 13/16] rdma: keep sinks outside the publication lock and self-close metrics Detach queued work under the publication mutex and execute the sinks after releasing it: the overflow servicing and the shutdown drain now swap out the pending list inside the critical section and publish outside, so a slow sink delays its own records but never blocks a dispatcher or a native terminal callback. Move the worker-stopped transition under the same mutex. The worker marks itself stopped before emptying the channel and the overflow list, and dispatchers test that flag rather than the done channel inside the critical section, closing the window where an append could land between the sweep completing and the deferred channel close only to be stranded. Give the metrics manager its own cancellation. The forwarder terminates through a child context the manager derives and cancels in Close, so a standalone use of the API shuts down without relying on an external context being canceled. --- metrics/metrics.go | 16 ++++++-- rdma/rcroutes/ops_linux.go | 76 +++++++++++++++++++++++--------------- 2 files changed, 59 insertions(+), 33 deletions(-) diff --git a/metrics/metrics.go b/metrics/metrics.go index 62f15830..980cda90 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -56,8 +56,9 @@ type Manager interface { // manager is a manager of metrics plugins type manager struct { - wg sync.WaitGroup - ctx context.Context + wg sync.WaitGroup + ctx context.Context + cancel context.CancelFunc config Config @@ -93,9 +94,15 @@ func NewManager(ctx context.Context, conf Config) (Manager, error) { addDataChan := make(chan datapoint, dataItemCount) + // Derive a cancellable child of the caller context: closing + // the manager cancels it itself (a standalone user of the + // API has no external cancellation to rely on), while the + // gateway shutdown path keeps its own context propagation. + mctx, mcancel := context.WithCancel(ctx) mgr := &manager{ addDataChan: addDataChan, - ctx: ctx, + ctx: mctx, + cancel: mcancel, config: conf, } @@ -253,6 +260,9 @@ func (m *manager) add(key string, value int64, tags ...Tag) { // cancellation carry the shutdown instead. func (m *manager) Close() { m.closed.Store(true) + // Self-owned cancellation terminates the forwarder wherever + // it is waiting; the external context is only a second path. + m.cancel() m.wg.Wait() // close all publishers diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index ce7633a6..218203d0 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -273,8 +273,15 @@ type opsTracker struct { // slow sink, so dispatch appends here (under pubmu) instead // of blocking or running the sink itself, and the worker // drains this list after the channel empties. + // + // pubmu guards the accept-vs-drain boundary: overflow, and + // the stopped transition, change only under it. Sinks never + // execute under pubmu - the worker detaches queued work and + // publishes outside the lock - so a slow sink delays records + // but never blocks a dispatcher. pubmu sync.Mutex overflow []pubJob + stopped bool done chan struct{} drain chan struct{} drainOnce sync.Once @@ -327,31 +334,44 @@ func newOpsTracker() *opsTracker { // channel job: bursts that exceed the // buffer publish as soon as the sink // recovers instead of waiting for - // shutdown. - t.serviceOverflow() + // shutdown. The list is detached under + // the lock and published outside it, so a + // slow sink never blocks a dispatcher. + for _, job := range t.takeOverflow() { + job.emit.publish(job.err, job.byt) + } case <-t.drain: - // Drain mode, single critical section with - // dispatch: holding pubmu across the - // channel-and-list sweep closes the - // accept-vs-drain race - a dispatch that - // appended before this point is drained, - // one that runs after sees done and - // publishes inline. + // Drain mode. The accept-vs-drain boundary: + // under pubmu the worker marks itself + // stopped, empties the channel and detaches + // the overflow list. A dispatch that + // acquires the mutex before the stopped + // transition is drained here; one that + // acquires it after sees stopped (or done, + // closed only after the unlock) and takes + // its post-drain path. Sinks run after the + // unlock, never under the lock. t.pubmu.Lock() + t.stopped = true + var pending []pubJob for { select { case job, ok := <-t.pubq: if !ok { t.pubmu.Unlock() + for _, job := range pending { + job.emit.publish(job.err, job.byt) + } return } - job.emit.publish(job.err, job.byt) + pending = append(pending, job) default: - for _, job := range t.overflow { - job.emit.publish(job.err, job.byt) - } + pending = append(pending, t.overflow...) t.overflow = nil t.pubmu.Unlock() + for _, job := range pending { + job.emit.publish(job.err, job.byt) + } return } } @@ -361,15 +381,15 @@ func newOpsTracker() *opsTracker { return t } -// serviceOverflow publishes and clears the overflow list if the -// worker can take it. Called by the worker only. -func (t *opsTracker) serviceOverflow() { +// takeOverflow detaches the overflow list under pubmu. Called by +// the worker only; the caller publishes the returned jobs outside +// the lock. +func (t *opsTracker) takeOverflow() []pubJob { t.pubmu.Lock() defer t.pubmu.Unlock() - for _, job := range t.overflow { - job.emit.publish(job.err, job.byt) - } + pending := t.overflow t.overflow = nil + return pending } // Shutdown drains pending publications and stops the worker. The @@ -400,17 +420,15 @@ func (t *opsTracker) Shutdown() { // publications are dropped by publishRequest before reaching // here. func (t *opsTracker) dispatch(job pubJob) { - // Fast path: the worker is alive. The pubmu critical section - // is the accept-vs-drain boundary: the drain sweep holds the - // same lock, so an append either lands before the sweep (and - // is drained) or after done closed (and runs inline). + // The pubmu critical section is the accept-vs-drain boundary: + // the drain sweep marks stopped under the same lock, so an + // append either lands before the sweep (and is drained) or + // observes stopped and runs inline. t.pubmu.Lock() - select { - case <-t.done: + if t.stopped { t.pubmu.Unlock() job.emit.publish(job.err, job.byt) return - default: } select { case t.pubq <- job: @@ -673,15 +691,13 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, } // dispatchOrDrop is dispatch with request-publication semantics: -// after the worker exited through the drain the job is dropped +// after the worker stopped through the drain the job is dropped // instead of published inline. func (t *opsTracker) dispatchOrDrop(job pubJob) { t.pubmu.Lock() - select { - case <-t.done: + if t.stopped { t.pubmu.Unlock() return - default: } select { case t.pubq <- job: From 51e63e26368afde42806a4994100bf425289e0f5 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 23:09:20 +0900 Subject: [PATCH 14/16] rdma: cap the session-less publication backlog Bound the records a stalled sink can accumulate from requests that never opened a session (failed authentications): beyond 4096 queued, dispatchOrDrop drops the record and counts it, and shutdown reports the drop count once. Session publications stay uncapped - each session publishes exactly once and the session table has a hard limit, so their backlog is structurally bounded. Cancel the metrics child context on constructor failure so a malformed publisher endpoint does not leak the derived context onto the parent. --- metrics/metrics.go | 5 +++ rdma/rcroutes/ops_linux.go | 72 ++++++++++++++++++++++++++++++-------- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/metrics/metrics.go b/metrics/metrics.go index 980cda90..ded4c676 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -113,6 +113,7 @@ func NewManager(ctx context.Context, conf Config) (Manager, error) { for server := range statsdServers { statsd, err := newStatsd(server, conf.ServiceName) if err != nil { + mcancel() return nil, err } mgr.publishers = append(mgr.publishers, statsd) @@ -126,6 +127,10 @@ func NewManager(ctx context.Context, conf Config) (Manager, error) { for server := range dogStatsdServers { dogStatsd, err := newDogStatsd(server, conf.ServiceName) if err != nil { + // The derived child context would otherwise stay + // attached to the parent until the parent is + // canceled. + mcancel() return nil, err } mgr.publishers = append(mgr.publishers, dogStatsd) diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 218203d0..b7a33ea0 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -19,9 +19,12 @@ package rcroutes import ( "errors" + "fmt" "net/http" + "os" "strings" "sync" + "sync/atomic" "time" "github.com/gofiber/fiber/v3" @@ -279,19 +282,22 @@ type opsTracker struct { // execute under pubmu - the worker detaches queued work and // publishes outside the lock - so a slow sink delays records // but never blocks a dispatcher. - pubmu sync.Mutex - overflow []pubJob - stopped bool - done chan struct{} - drain chan struct{} - drainOnce sync.Once + pubmu sync.Mutex + overflow []pubJob + reqBacklog atomic.Int64 + reqDropped atomic.Int64 + stopped bool + done chan struct{} + drain chan struct{} + drainOnce sync.Once } // pubJob is one deferred publication handed to the worker. type pubJob struct { - emit *opsEmitter - err error - byt int64 + emit *opsEmitter + err error + byt int64 + isReq bool } // pubQueueSoftCap is the buffered pre-allocation of the @@ -300,6 +306,16 @@ type pubJob struct { // native callback. const pubQueueSoftCap = 256 +// pubRequestBacklogCap bounds the queued records that carry no +// session. Session publications are structurally bounded (each +// session publishes exactly once and the session table has a +// hard limit), but request publications - failed authentications +// - arrive with no session at all, and a stalled sink would let +// them accumulate without limit. Beyond this depth the record is +// dropped and counted, trading a bounded window of lost +// request-audit records for memory safety under overload. +const pubRequestBacklogCap = 4096 + // newOpsTracker builds the tracker. The publication queue is // conceptually unbounded: a callback thread must never run a // sink (a blocked sink would stall the native reaper and defer @@ -329,7 +345,7 @@ func newOpsTracker() *opsTracker { if !ok { return } - job.emit.publish(job.err, job.byt) + t.run(job) // Service the overflow list after every // channel job: bursts that exceed the // buffer publish as soon as the sink @@ -338,7 +354,7 @@ func newOpsTracker() *opsTracker { // the lock and published outside it, so a // slow sink never blocks a dispatcher. for _, job := range t.takeOverflow() { - job.emit.publish(job.err, job.byt) + t.run(job) } case <-t.drain: // Drain mode. The accept-vs-drain boundary: @@ -360,7 +376,7 @@ func newOpsTracker() *opsTracker { if !ok { t.pubmu.Unlock() for _, job := range pending { - job.emit.publish(job.err, job.byt) + t.run(job) } return } @@ -370,7 +386,7 @@ func newOpsTracker() *opsTracker { t.overflow = nil t.pubmu.Unlock() for _, job := range pending { - job.emit.publish(job.err, job.byt) + t.run(job) } return } @@ -392,6 +408,15 @@ func (t *opsTracker) takeOverflow() []pubJob { return pending } +// run publishes one job and releases its request-backlog +// reservation, if any. +func (t *opsTracker) run(job pubJob) { + job.emit.publish(job.err, job.byt) + if job.isReq { + t.reqBacklog.Add(-1) + } +} + // Shutdown drains pending publications and stops the worker. The // gateway must call this BEFORE closing the operational sinks: a // queued publication that runs after its logger closed is lost. @@ -404,6 +429,12 @@ func (t *opsTracker) Shutdown() { t.drainOnce.Do(func() { close(t.drain) <-t.done + if n := t.reqDropped.Load(); n > 0 { + // Overload during shutdown: records without a session + // were dropped once the request backlog hit its cap. + // Surfaced once here rather than per record. + fmt.Fprintf(os.Stderr, "rdma-rc: dropped %d request audit records at the publication backlog cap\n", n) + } }) } @@ -427,7 +458,7 @@ func (t *opsTracker) dispatch(job pubJob) { t.pubmu.Lock() if t.stopped { t.pubmu.Unlock() - job.emit.publish(job.err, job.byt) + t.run(job) return } select { @@ -692,13 +723,24 @@ func (t *opsTracker) publishRequest(ctx fiber.Ctx, acct auth.Account, // dispatchOrDrop is dispatch with request-publication semantics: // after the worker stopped through the drain the job is dropped -// instead of published inline. +// instead of published inline, and the queued backlog of session- +// less records is capped so a stalled sink cannot accumulate them +// without bound. func (t *opsTracker) dispatchOrDrop(job pubJob) { t.pubmu.Lock() if t.stopped { t.pubmu.Unlock() return } + if t.reqBacklog.Load() >= pubRequestBacklogCap { + t.pubmu.Unlock() + // Overload policy: drop and count. The record carries no + // session and no owner can reissue it. + t.reqDropped.Add(1) + return + } + job.isReq = true + t.reqBacklog.Add(1) select { case t.pubq <- job: t.pubmu.Unlock() From 60bec2c00de78a862b7ed7b6f368ee319c3d8e2b Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 23:40:51 +0900 Subject: [PATCH 15/16] rdma: gate session admission on audit publication backlog The native side releases its session quota when it fires the teardown notification, before the audit record lands in a sink, so session turnover can queue more unpublished records than the live-session limit allows. Hold an admission credit per session from registration until its record is published, and refuse new sessions while the backlog of unpublished session records reaches the native session quota: the refusal rolls the prepare back, still publishes the request-level audit record, and answers SlowDown so the client retries. A stalled sink now turns into latency instead of unbounded memory. Count dropped request records under the publication mutex so the shutdown drop-count report cannot miss an increment racing it. --- cmd/vgwrdma/main.go | 2 +- rdma/rcroutes/ops_linux.go | 62 ++++++++++++++++++++++++++------- rdma/rcroutes/ops_linux_test.go | 14 ++++---- rdma/rcroutes/routes_linux.go | 15 +++++--- rdma/rcroutes/routes_stub.go | 2 +- 5 files changed, 69 insertions(+), 26 deletions(-) diff --git a/cmd/vgwrdma/main.go b/cmd/vgwrdma/main.go index 55128c6c..bd7579d9 100644 --- a/cmd/vgwrdma/main.go +++ b/cmd/vgwrdma/main.go @@ -1286,7 +1286,7 @@ func runGateway(ctx context.Context, be backend.Backend) error { // nil on success without doing so. Wrap it so a verified // request reaches the route handler, while errors end the // chain as usual. - rcH := rcroutes.New(rcSvc, be, iamSvc, readonly, disableACLs) + rcH := rcroutes.New(rcSvc, be, iamSvc, readonly, disableACLs, int(rcMaxSessions)) // The RC shutdown wrapper drains queued operational // publications before the sinks close; the drain hook is // the route handler, which is built only now (it needs diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index b7a33ea0..11aa5695 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -286,10 +286,22 @@ type opsTracker struct { overflow []pubJob reqBacklog atomic.Int64 reqDropped atomic.Int64 - stopped bool - done chan struct{} - drain chan struct{} - drainOnce sync.Once + // pubPending counts queued-but-unpublished session records. + // The native side releases its session quota when it fires + // the teardown notification, not when the audit record lands, + // so successive sessions can queue more records than the + // live-session limit allows. Admission control closes that + // gap: a new session is refused while too many of its + // predecessors' records are still unpublished, so a stalled + // sink delays new sessions instead of accumulating memory. + pubPending atomic.Int64 + // sessionLimit is the native concurrent-session quota; the + // admission budget scales with it. + sessionLimit int + stopped bool + done chan struct{} + drain chan struct{} + drainOnce sync.Once } // pubJob is one deferred publication handed to the worker. @@ -327,15 +339,16 @@ const pubRequestBacklogCap = 4096 // whatever the sinks cannot keep up with. Each job is a few // pointers; a stalled sink delays records, it does not lose // them. -func newOpsTracker() *opsTracker { +func newOpsTracker(sessionLimit int) *opsTracker { t := &opsTracker{ sessions: map[string]*sessionRecord{}, app: fiber.New(fiber.Config{ Immutable: true, }), - pubq: make(chan pubJob, pubQueueSoftCap), - done: make(chan struct{}), - drain: make(chan struct{}), + pubq: make(chan pubJob, pubQueueSoftCap), + done: make(chan struct{}), + drain: make(chan struct{}), + sessionLimit: sessionLimit, } go func() { defer close(t.done) @@ -408,13 +421,16 @@ func (t *opsTracker) takeOverflow() []pubJob { return pending } -// run publishes one job and releases its request-backlog -// reservation, if any. +// run publishes one job and releases its reservations: the +// request-backlog slot and the session admission credit the +// record was holding. func (t *opsTracker) run(job pubJob) { job.emit.publish(job.err, job.byt) if job.isReq { t.reqBacklog.Add(-1) + return } + t.pubPending.Add(-1) } // Shutdown drains pending publications and stops the worker. The @@ -490,8 +506,24 @@ func (t *opsTracker) SetOpsServices(ops OpsServices) { // The account is captured by value but its string fields still // reference request storage on some IAM paths, so the sink-relevant // identity is cloned as well. +// errPubBacklog reports admission refusal: too many earlier +// sessions still have unpublished audit records, so accepting +// another would grow the publication backlog without bound while +// a sink is stalled. +var errPubBacklog = errors.New("publication backlog at capacity") + func (t *opsTracker) register(sessionID string, acct auth.Account, - region, bucket, key string, isPut bool, start time.Time) { + region, bucket, key string, isPut bool, start time.Time) error { + // Admission control: the native quota counts live sessions, + // but teardown notifications fire before the audit records + // land, so session turnover can queue more records than the + // quota bounds. Refusing new sessions while the unpublished + // backlog exceeds the quota turns a stalled sink into + // latency (the client retries) instead of unbounded memory. + // Unbounded when sessionLimit is unset (tests). + if t.sessionLimit > 0 && t.pubPending.Load() >= int64(t.sessionLimit) { + return errPubBacklog + } acct.Access = strings.Clone(acct.Access) emit := &opsEmitter{ ops: t.loadOps(), @@ -506,7 +538,9 @@ func (t *opsTracker) register(sessionID string, acct auth.Account, t.mu.Lock() defer t.mu.Unlock() + t.pubPending.Add(1) t.sessions[sessionID] = &sessionRecord{emit: emit} + return nil } // unregister drops a session entry whose PREPARE finalization @@ -733,10 +767,12 @@ func (t *opsTracker) dispatchOrDrop(job pubJob) { return } if t.reqBacklog.Load() >= pubRequestBacklogCap { - t.pubmu.Unlock() // Overload policy: drop and count. The record carries no - // session and no owner can reissue it. + // session and no owner can reissue it. Incremented under + // pubmu so Shutdown's report (also under pubmu via the + // drain's stopped transition) cannot miss it. t.reqDropped.Add(1) + t.pubmu.Unlock() return } job.isReq = true diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 09a2c3d2..312622eb 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -38,7 +38,7 @@ import ( // exactly once. Unreserved records are published by the callback. func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(0) tr.register("sess-1", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) if got := len(tr.sessions); got != 1 { @@ -60,7 +60,7 @@ func TestOpsTrackerCallbackPublishesExpiry(t *testing.T) { } func TestOpsTrackerReserveBlocksCallback(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(0) tr.register("sess-2", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", true, time.Now()) @@ -88,7 +88,7 @@ func TestOpsTrackerReserveBlocksCallback(t *testing.T) { } func TestOpsTrackerReserveIsExclusive(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(0) tr.register("sess-3", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -101,7 +101,7 @@ func TestOpsTrackerReserveIsExclusive(t *testing.T) { } func TestOpsTrackerFailOutcome(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(0) tr.register("sess-4", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) @@ -116,7 +116,7 @@ func TestOpsTrackerFailOutcome(t *testing.T) { } func TestOpsTrackerUnregister(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(0) tr.register("sess-5", auth.Account{Access: "ak"}, "us-east-1", "bkt", "obj", false, time.Now()) tr.unregister("sess-5") @@ -132,7 +132,7 @@ func TestOpsTrackerUnregister(t *testing.T) { } func TestOpsTrackerUnknownSession(t *testing.T) { - tr := newOpsTracker() + tr := newOpsTracker(0) // Unknown sessions and the nil tracker are silent no-ops. var nilTracker *opsTracker nilTracker.reserve("ghost") @@ -237,7 +237,7 @@ func (r *recordingLogger) Shutdown() error { return nil } // publishes exactly one record with its own outcome and bytes. func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { rl := &recordingLogger{} - tr := newOpsTracker() + tr := newOpsTracker(0) tr.SetOpsServices(OpsServices{Logger: rl}) // Expiry path: callback publishes a zero-byte error record. diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index d1e185dd..382beefe 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -118,10 +118,10 @@ func (h *Handler) Shutdown() { // operational services arrive later through SetOpsServices, once // the gateway has created them. func New(svc *rcserver.RCSvc, be backend.Backend, iam auth.IAMService, - readonly, disableACL bool) *Handler { + readonly, disableACL bool, sessionLimit int) *Handler { return &Handler{svc: svc, be: be, iam: iam, readonly: readonly, disableACL: disableACL, - ops: newOpsTracker()} + ops: newOpsTracker(sessionLimit)} } // principalID derives the session identity digest from the @@ -249,8 +249,15 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { // an already-expired session and fire the teardown callback // synchronously, and a registered record (or a parked early // notification) keeps that publication from being lost. - h.ops.register(resp.SessionID, acct, - regionFromCtx(ctx), bucket, key, isPut, time.Now()) + // Refusal here is admission control: earlier sessions still + // hold unpublished audit records, so the new session is + // rejected before the native side commits it. + if err := h.ops.register(resp.SessionID, acct, + regionFromCtx(ctx), bucket, key, isPut, time.Now()); err != nil { + _ = h.svc.FinishPrepare(resp.SessionID, false) + h.ops.publishRequest(ctx, acct, err, bucket, key, isPut) + return s3err.GetAPIError(s3err.ErrSlowDown) + } if err := h.svc.FinishPrepare(resp.SessionID, true); err != nil { // The finalization failed. Exactly one publication // covers it: the finalizing call already reaped the diff --git a/rdma/rcroutes/routes_stub.go b/rdma/rcroutes/routes_stub.go index b735c47c..f3b5e03c 100644 --- a/rdma/rcroutes/routes_stub.go +++ b/rdma/rcroutes/routes_stub.go @@ -32,7 +32,7 @@ type Handler struct{} // New builds a stub route handler; the routes answer 501. func New(svc any, be backend.Backend, iam auth.IAMService, - readonly, disableACL bool) *Handler { + readonly, disableACL bool, sessionLimit int) *Handler { return &Handler{} } From ae2a3a6d5595cef956e899aeca4e508d4de58c1d Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Sun, 6 Sep 2026 23:57:12 +0900 Subject: [PATCH 16/16] rdma: acquire session admission credit atomically Checking the publication backlog and taking the admission credit were separate steps, so concurrent registrations could each observe the same headroom and overshoot the session quota together. Both now share one critical section, and a concurrent test pins the behavior: sixteen registrations against a limit of eight with one record pending admit exactly seven. An admission refusal now publishes the same SlowDown error the wire response carries, so operational accounting matches what the client saw, and unregister releases the credit an unfinalized registration was holding so the admission budget cannot leak. --- rdma/rcroutes/ops_linux.go | 34 +++++++++++++++++++++----- rdma/rcroutes/ops_linux_test.go | 42 +++++++++++++++++++++++++++++++++ rdma/rcroutes/routes_linux.go | 8 +++++-- 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/rdma/rcroutes/ops_linux.go b/rdma/rcroutes/ops_linux.go index 11aa5695..eb8370f0 100644 --- a/rdma/rcroutes/ops_linux.go +++ b/rdma/rcroutes/ops_linux.go @@ -518,11 +518,26 @@ func (t *opsTracker) register(sessionID string, acct auth.Account, // but teardown notifications fire before the audit records // land, so session turnover can queue more records than the // quota bounds. Refusing new sessions while the unpublished - // backlog exceeds the quota turns a stalled sink into + // backlog reaches the quota turns a stalled sink into // latency (the client retries) instead of unbounded memory. - // Unbounded when sessionLimit is unset (tests). - if t.sessionLimit > 0 && t.pubPending.Load() >= int64(t.sessionLimit) { - return errPubBacklog + // The check and the credit acquisition share the session + // mutex so concurrent registrations cannot each observe the + // same headroom and overshoot together. Unbounded when + // sessionLimit is unset (tests). + if t.sessionLimit > 0 { + t.mu.Lock() + full := t.pubPending.Load() >= int64(t.sessionLimit) + if !full { + t.pubPending.Add(1) + } + t.mu.Unlock() + if full { + return errPubBacklog + } + } else { + t.mu.Lock() + t.pubPending.Add(1) + t.mu.Unlock() } acct.Access = strings.Clone(acct.Access) emit := &opsEmitter{ @@ -538,7 +553,6 @@ func (t *opsTracker) register(sessionID string, acct auth.Account, t.mu.Lock() defer t.mu.Unlock() - t.pubPending.Add(1) t.sessions[sessionID] = &sessionRecord{emit: emit} return nil } @@ -554,7 +568,14 @@ func (t *opsTracker) unregister(sessionID string) { } t.mu.Lock() defer t.mu.Unlock() - delete(t.sessions, sessionID) + if _, ok := t.sessions[sessionID]; ok { + delete(t.sessions, sessionID) + // Release the admission credit the registration took: + // no callback will ever publish for this entry, so + // leaving the credit held would permanently shrink the + // admission budget. + t.pubPending.Add(-1) + } } // failOutcome publishes a failed finalization exactly once: when @@ -589,6 +610,7 @@ type reservation struct { emit *opsEmitter gen uint64 } + // reserve marks a session record as owned by its request path: the // teardown callback skips a reserved record because the request // path publishes the real outcome itself. Returns the reservation diff --git a/rdma/rcroutes/ops_linux_test.go b/rdma/rcroutes/ops_linux_test.go index 312622eb..ae1d6e65 100644 --- a/rdma/rcroutes/ops_linux_test.go +++ b/rdma/rcroutes/ops_linux_test.go @@ -19,7 +19,9 @@ package rcroutes import ( "errors" + "fmt" "sync" + "sync/atomic" "testing" "time" @@ -365,3 +367,43 @@ func TestOpsTrackerPublishesExactlyOncePerSession(t *testing.T) { } } } + +func TestOpsTrackerAdmissionAtomicUnderConcurrency(t *testing.T) { + tr := newOpsTracker(8) + // One predecessor record is already pending. + if err := tr.register("s-seed", auth.Account{Access: "a"}, + "r", "b", "k", false, time.Now()); err != nil { + t.Fatalf("seed registration: %v", err) + } + + const rounds = 16 + var wg sync.WaitGroup + var admitted atomic.Int64 + var refused atomic.Int64 + for i := 0; i < rounds; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + err := tr.register(fmt.Sprintf("s-%d", i), auth.Account{Access: "a"}, + "r", "b", "k", false, time.Now()) + if err == nil { + admitted.Add(1) + } else if errors.Is(err, errPubBacklog) { + refused.Add(1) + } else { + t.Errorf("registration %d: unexpected error %v", i, err) + } + }(i) + } + wg.Wait() + + if got := admitted.Load(); got != 7 { + t.Fatalf("admitted %d registrations, want exactly 7 (limit 8, 1 pending)", got) + } + if got := refused.Load(); got != rounds-7 { + t.Fatalf("refused %d registrations, want %d", got, rounds-7) + } + if got := tr.pubPending.Load(); got != 8 { + t.Fatalf("pending credits = %d, want 8", got) + } +} diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index 382beefe..394d9893 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -255,8 +255,12 @@ func (h *Handler) prepareCore(ctx fiber.Ctx) error { if err := h.ops.register(resp.SessionID, acct, regionFromCtx(ctx), bucket, key, isPut, time.Now()); err != nil { _ = h.svc.FinishPrepare(resp.SessionID, false) - h.ops.publishRequest(ctx, acct, err, bucket, key, isPut) - return s3err.GetAPIError(s3err.ErrSlowDown) + // The audit record carries the same SlowDown the wire + // shows, so operator-side accounting matches what the + // client saw. + apiErr := s3err.GetAPIError(s3err.ErrSlowDown) + h.ops.publishRequest(ctx, acct, apiErr, bucket, key, isPut) + return apiErr } if err := h.svc.FinishPrepare(resp.SessionID, true); err != nil { // The finalization failed. Exactly one publication