diff --git a/README.md b/README.md index 5541e5e23..2cfa33e6f 100644 --- a/README.md +++ b/README.md @@ -220,7 +220,7 @@ printf 'hello\n' > /tmp/elastickv-mount/docs/hello.txt cat /tmp/elastickv-mount/docs/hello.txt ``` -The root owner and mode flags apply when the filesystem root is first created; the defaults are UID/GID `0` and mode `0755`, and the mount enforces `default_permissions`, so set them to the user that will write to the mount. `--filesystemClientID` defaults to `--raftId` and identifies open-handle leases. `--filesystemCapacity` and `--filesystemMaxFiles` set values reported by `statfs`; they do not enforce write quotas. The server mounts through `fusermount3` (package `fuse3`) after its network listeners are up. Stopping the process with a signal leaves the mount attached in a `Transport endpoint is not connected` state; run `fusermount3 -uz ` before restarting. +The root owner and mode flags apply when the filesystem root is first created; the defaults are UID/GID `0` and mode `0755`, and the mount enforces `default_permissions`, so set them to the user that will write to the mount. `--filesystemClientID` defaults to `--raftId` and identifies open-handle leases. `--filesystemCapacity` and `--filesystemMaxFiles` set values reported by `statfs`; they do not enforce write quotas. The server mounts through `fusermount3` (package `fuse3`) after its network listeners are up. The server unmounts the filesystem when it stops, including on `SIGTERM`/`SIGINT`, and a restart after `SIGKILL` or a crash detaches the dead mount it finds at the mount point before mounting again. See the [FUSE filesystem guide](docs/filesystem.md) for details. This FUSE implementation supports regular files and directories, but not cross-directory rename, hard links, symbolic links, extended attributes, or full POSIX locking. Concurrent modifications from different mounts are resolved optimistically: the loser gets `EAGAIN`. Most operations can simply be retried, but `close` must not be, and an `O_APPEND` write that other mounts may also be appending to must not be either; the guide's Concurrency section has the per-operation rules. See the [FUSE filesystem guide](docs/filesystem.md) for prerequisites, multi-node setup, semantics, monitoring, and troubleshooting, and the [filesystem design](docs/design/2026_02_24_implemented_filesystem_on_elastickv.md) for the supported operations and placement model. diff --git a/adapter/background_reservation_shutdown_test.go b/adapter/background_reservation_shutdown_test.go new file mode 100644 index 000000000..bf092f6d5 --- /dev/null +++ b/adapter/background_reservation_shutdown_test.go @@ -0,0 +1,42 @@ +package adapter + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestBackgroundReservationStartsLiveAfterShutdownBound(t *testing.T) { + w := newBackgroundWorkers() + r, ok := w.reserve() + require.True(t, ok) + done := make(chan struct{}) + go func() { w.stop("test", time.Millisecond, time.Second); close(done) }() + <-w.ctx.Done() + ran := make(chan error, 1) + r.start(time.Second, func(ctx context.Context) { ran <- ctx.Err() }) + require.NoError(t, <-ran) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("stop did not join reserved cleanup") + } +} + +func TestBackgroundReservationCannotStartAfterStopReturns(t *testing.T) { + w := newBackgroundWorkers() + r, ok := w.reserve() + require.True(t, ok) + w.stop("test", time.Millisecond, time.Millisecond) + ran := make(chan struct{}, 1) + r.start(time.Second, func(context.Context) { ran <- struct{}{} }) + r.release() + require.Zero(t, w.gate.remaining()) + select { + case <-ran: + t.Fatal("cleanup started after shutdown returned") + case <-time.After(time.Millisecond): + } +} diff --git a/adapter/background_workers.go b/adapter/background_workers.go new file mode 100644 index 000000000..c34d9c027 --- /dev/null +++ b/adapter/background_workers.go @@ -0,0 +1,150 @@ +package adapter + +import ( + "context" + "log/slog" + "sync" + "time" +) + +// backgroundWorkers runs the jobs a request starts but does not wait for, +// such as the cleanup of a deleted table's old generation. Each job is +// counted in an inflightGate so the server's Stop can join it, bounded, +// before the stores close underneath it. Stop cancels jobs already started +// once the bound has passed; reservations that commit later get a live +// timeout and stay counted in the final join. Reservations cannot start +// cleanup after that final join ends. This bounded drain does not provide +// durable recovery of cleanup for mutations that outlive shutdown. +// A nil *backgroundWorkers +// runs every job detached under context.Background() and has nothing to +// stop, which keeps zero-value servers in tests working. +type backgroundWorkers struct { + gate *inflightGate + ctx context.Context + cancel context.CancelFunc + mu sync.Mutex + running map[*backgroundReservation]context.CancelFunc + closed bool +} + +func newBackgroundWorkers() *backgroundWorkers { + ctx, cancel := context.WithCancel(context.Background()) + return &backgroundWorkers{gate: newInflightGate(), ctx: ctx, cancel: cancel, running: make(map[*backgroundReservation]context.CancelFunc)} +} + +// backgroundReservation is a job slot claimed before the commit that makes +// the job necessary, so the job can still start after that commit even when +// stop began in between: stop waits for the slot as for a running job. The +// holder either starts the job through it or releases it. +type backgroundReservation struct { + mu sync.Mutex + w *backgroundWorkers + used bool +} + +// reserve claims a job slot, or reports false once stop has begun, in which +// case the caller must not commit the change the job would follow up on. +func (w *backgroundWorkers) reserve() (*backgroundReservation, bool) { + if w != nil && !w.gate.enter() { + return nil, false + } + return &backgroundReservation{w: w}, true +} + +// start runs run on its own goroutine under a context bounded by timeout, +// handing it the reserved slot. A reservation starts at most one job. +func (r *backgroundReservation) start(timeout time.Duration, run func(ctx context.Context)) { + r.mu.Lock() + defer r.mu.Unlock() + if r.used { + return + } + r.used = true + ctx, cancel := context.WithTimeout(context.Background(), timeout) + if r.w != nil { + r.w.mu.Lock() + if r.w.closed { + r.w.mu.Unlock() + cancel() + r.w.gate.exit() + return + } + r.w.running[r] = cancel + r.w.mu.Unlock() + } + go func() { + defer cancel() + if r.w != nil { + defer r.w.gate.exit() + defer func() { + r.w.mu.Lock() + delete(r.w.running, r) + r.w.mu.Unlock() + }() + } + run(ctx) + }() +} + +// release gives back a slot no job was started through; it is a no-op once +// start ran or after an earlier release. +func (r *backgroundReservation) release() { + r.mu.Lock() + defer r.mu.Unlock() + if r.used { + return + } + r.used = true + if r.w != nil { + r.w.gate.exit() + } +} + +// stop refuses new reservations, waits up to timeout for the reserved slots +// and the running jobs, then cancels the context of any jobs that remain and +// waits up to joinTimeout for them to return, so none is still dispatching +// when the stores close. A call made while an earlier one is still waiting +// waits until the same deadlines (see inflightGate); name labels the +// warnings it logs. +func (w *backgroundWorkers) stop(name string, timeout, joinTimeout time.Duration) { + if w == nil { + return + } + defer w.closeReservations() + finished := w.gate.drain(timeout) + w.cancelRunning() + if finished { + return + } + slog.Warn(name+": background jobs still running after the shutdown bound, cancelling them", + slog.Int64("in_flight", w.gate.remaining()), + slog.Duration("timeout", timeout), + ) + if !w.gate.join(joinTimeout) { + slog.Warn(name+": cancelled background jobs did not return, continuing shutdown", + slog.Int64("in_flight", w.gate.remaining()), + slog.Duration("timeout", joinTimeout), + ) + } +} + +func (w *backgroundWorkers) closeReservations() { + w.mu.Lock() + defer w.mu.Unlock() + w.closed = true + for _, cancel := range w.running { + cancel() + } +} + +func (w *backgroundWorkers) cancelRunning() { + w.mu.Lock() + defer w.mu.Unlock() + w.cancel() + // Reservations still owned by a mutation are not jobs yet. If that + // mutation commits later, its cleanup gets a live, independently bounded + // context and remains counted in the join window. + for _, cancel := range w.running { + cancel() + } +} diff --git a/adapter/dynamodb.go b/adapter/dynamodb.go index 25ec19659..7b106efe9 100644 --- a/adapter/dynamodb.go +++ b/adapter/dynamodb.go @@ -122,6 +122,7 @@ type DynamoDBServer struct { store store.MVCCStore coordinator kv.Coordinator httpServer *http.Server + httpConns *httpConnTracker targetHandlers map[string]func(http.ResponseWriter, *http.Request) readTracker *kv.ActiveTimestampTracker requestObserver monitoring.DynamoDBRequestObserver @@ -138,6 +139,13 @@ type DynamoDBServer struct { // Set ELASTICKV_DYNAMODB_ONEPHASE_DEDUP=0 or pass // WithDynamoOnePhaseTxnDedup(false) for an operator rollback. onePhaseTxnDedup bool + + // tableCleanups runs the cleanup of a deleted table's old generation + // (see launchDeletedTableCleanup) so Stop can join it, bounded, before + // the stores close underneath it. Its slot is reserved before the change + // that orphans the generation commits, whichever surface (public + // listener, admin HTTP or gRPC forwarder) the change came through. + tableCleanups *backgroundWorkers } // WithDynamoDBRequestObserver enables Prometheus-compatible request metrics. @@ -264,6 +272,7 @@ func NewDynamoDBServer(listen net.Listener, st store.MVCCStore, coordinate kv.Co store: st, coordinator: kv.WithKeyVizLabel(coordinate, keyviz.LabelDynamo), onePhaseTxnDedup: os.Getenv("ELASTICKV_DYNAMODB_ONEPHASE_DEDUP") != "0", + tableCleanups: newBackgroundWorkers(), } for i := range d.itemUpdateLocks { d.itemUpdateLocks[i] = make(chan struct{}, 1) @@ -287,6 +296,7 @@ func NewDynamoDBServer(listen net.Listener, st store.MVCCStore, coordinate kv.Co mux := http.NewServeMux() mux.HandleFunc("/", d.handle) d.httpServer = &http.Server{Handler: mux, ReadHeaderTimeout: time.Second} + d.httpConns = trackHTTPConns(d.httpServer) for _, opt := range opts { if opt != nil { opt(d) @@ -308,12 +318,36 @@ func (d *DynamoDBServer) Run() error { return nil } +// Stop drains the HTTP listener (bounded, see shutdownHTTPServer), then +// waits, under one more bound, for the deleted-table cleanups already +// started or reserved by a delete still committing, cancelling and briefly +// joining any still running, so none is dispatching when the stores close +// after Stop. From then on a change that would need a cleanup is refused +// with ErrDynamoServerStopping. func (d *DynamoDBServer) Stop() { if d.httpServer != nil { - _ = d.httpServer.Shutdown(context.Background()) + shutdownHTTPServer(d.httpServer, d.httpConns, "dynamodb") } + d.tableCleanups.stop("dynamodb server", httpShutdownTimeout, dynamoTableCleanupJoinTimeout) } +// ErrDynamoServerStopping is returned by a table delete, or a legacy-table +// migration step, that would orphan a table generation once Stop has begun: +// the generation's cleanup can no longer start, so the change is refused +// before it commits, and the table is left as it was. The public listener +// answers it with 503 ServiceUnavailable, which clients retry. It is also +// marked as ErrAdminNotLeader, so the admin bridge answers it with the +// not-leader response and the admin client retries against another node. +var ErrDynamoServerStopping = errors.Mark( + newDynamoAPIError(http.StatusServiceUnavailable, dynamoErrServiceUnavailable, "server is stopping"), + ErrAdminNotLeader, +) + +// dynamoTableCleanupJoinTimeout bounds how long Stop waits, after +// cancelling the table cleanups that outlived the shutdown bound, for them +// to return. A variable so tests can shorten it. +var dynamoTableCleanupJoinTimeout = 2 * time.Second + // proxyToLeader forwards the HTTP request to the current DynamoDB leader when // this node is not the Raft leader. Returns true if the request was handled // (either proxied or an error response was written), false if the request diff --git a/adapter/dynamodb_migration.go b/adapter/dynamodb_migration.go index e7b51b1f7..7c6f5bb8e 100644 --- a/adapter/dynamodb_migration.go +++ b/adapter/dynamodb_migration.go @@ -318,6 +318,13 @@ func (d *DynamoDBServer) finalizeLegacyTableMigration(ctx context.Context, schem if err != nil { return errors.WithStack(err) } + // As for a table delete, the old generation's cleanup slot is claimed + // before the schema switch can commit. + cleanup, ok := d.tableCleanups.reserve() + if !ok { + return errors.WithStack(ErrDynamoServerStopping) + } + defer cleanup.release() readTimestamp, err := d.beginTxnReadTimestamp(ctx, "dynamodb finalize migration: begin read timestamp", dynamoTableMetaKey(schema.TableName)) if err != nil { return errors.WithStack(err) @@ -334,6 +341,6 @@ func (d *DynamoDBServer) finalizeLegacyTableMigration(ctx context.Context, schem if _, err := kv.DispatchWithReadTimestamp(dispatchCtx, d.coordinator, req); err != nil { return errors.WithStack(err) } - d.launchDeletedTableCleanup(schema.TableName, oldGeneration) + d.launchDeletedTableCleanup(cleanup, schema.TableName, oldGeneration) return nil } diff --git a/adapter/dynamodb_schema.go b/adapter/dynamodb_schema.go index d3565477a..96d12c120 100644 --- a/adapter/dynamodb_schema.go +++ b/adapter/dynamodb_schema.go @@ -295,6 +295,14 @@ func decodeDeleteTableInput(bodyReader io.Reader) (deleteTableInput, error) { } func (d *DynamoDBServer) deleteTableWithRetry(ctx context.Context, tableName string) error { + // Claim the cleanup's slot before the metadata delete can commit: once + // Stop has begun the cleanup could no longer start, and a committed + // delete without it would leave the generation's items behind. + cleanup, ok := d.tableCleanups.reserve() + if !ok { + return errors.WithStack(ErrDynamoServerStopping) + } + defer cleanup.release() backoff := transactRetryInitialBackoff deadline := time.Now().Add(transactRetryMaxDuration) for range transactRetryMaxAttempts { @@ -324,7 +332,7 @@ func (d *DynamoDBServer) deleteTableWithRetry(ctx context.Context, tableName str return errors.WithStack(err) } } else { - d.launchDeletedTableCleanup(tableName, schema.Generation) + d.launchDeletedTableCleanup(cleanup, tableName, schema.Generation) return nil } @@ -336,10 +344,11 @@ func (d *DynamoDBServer) deleteTableWithRetry(ctx context.Context, tableName str return newDynamoAPIError(http.StatusInternalServerError, dynamoErrInternal, "delete table retry attempts exhausted") } -func (d *DynamoDBServer) launchDeletedTableCleanup(tableName string, generation uint64) { - go func() { - ctx, cancel := context.WithTimeout(context.Background(), tableCleanupAsyncTimeout) - defer cancel() +// launchDeletedTableCleanup removes a deleted generation's items, seals and +// index entries in the background, through the slot the caller reserved +// before committing the change that orphaned them. +func (d *DynamoDBServer) launchDeletedTableCleanup(cleanup *backgroundReservation, tableName string, generation uint64) { + cleanup.start(tableCleanupAsyncTimeout, func(ctx context.Context) { if err := d.cleanupDeletedTableGeneration(ctx, tableName, generation); err != nil { slog.Error("dynamodb delete table cleanup failed", "table", tableName, @@ -347,7 +356,7 @@ func (d *DynamoDBServer) launchDeletedTableCleanup(tableName string, generation "error", err, ) } - }() + }) } func (d *DynamoDBServer) cleanupDeletedTableGeneration(ctx context.Context, tableName string, generation uint64) error { diff --git a/adapter/dynamodb_table_cleanup_shutdown_test.go b/adapter/dynamodb_table_cleanup_shutdown_test.go new file mode 100644 index 000000000..2fabf1525 --- /dev/null +++ b/adapter/dynamodb_table_cleanup_shutdown_test.go @@ -0,0 +1,154 @@ +package adapter + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/bootjp/elastickv/store" + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +// TestDynamoDBServerStopWaitsForTableCleanup pins that the cleanup a +// DeleteTable (or a legacy-table migration) starts after its request +// returned keeps Stop, and with it the stores closed after Stop, until it +// finished, inside the graceful window. +func TestDynamoDBServerStopWaitsForTableCleanup(t *testing.T) { + shortenHTTPShutdownTimeout(t, 5*time.Second) + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + d := NewDynamoDBServer(nil, st, coord) + launchReservedTableCleanup(t, d, "orders", 1) + waitDispatchEntered(t, coord) + + stopDone := make(chan struct{}) + go func() { + d.Stop() + close(stopDone) + }() + select { + case <-stopDone: + t.Fatal("Stop returned while the table cleanup was still dispatching") + case <-time.After(100 * time.Millisecond): + } + close(coord.release) + select { + case <-stopDone: + case <-time.After(5 * time.Second): + t.Fatal("Stop did not return once the table cleanup finished") + } +} + +// A table cleanup still running when the bound expires is cancelled and +// joined: Stop returns only once it is out of its dispatch. +func TestDynamoDBServerStopCancelsAndJoinsTableCleanup(t *testing.T) { + shortenHTTPShutdownTimeout(t, 100*time.Millisecond) + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + coord.unwind = 300 * time.Millisecond + t.Cleanup(func() { close(coord.release) }) + d := NewDynamoDBServer(nil, st, coord) + launchReservedTableCleanup(t, d, "orders", 1) + waitDispatchEntered(t, coord) + + d.Stop() + require.True(t, coord.returned.Load(), "Stop returned while the cancelled cleanup was still in Dispatch") + require.Zero(t, d.tableCleanups.gate.remaining()) + + // No cleanup slot can be reserved once Stop has begun. + _, ok := d.tableCleanups.reserve() + require.False(t, ok, "a cleanup slot must not be reserved after Stop") +} + +func launchReservedTableCleanup(t *testing.T, d *DynamoDBServer, tableName string, generation uint64) { + t.Helper() + cleanup, ok := d.tableCleanups.reserve() + require.True(t, ok) + d.launchDeletedTableCleanup(cleanup, tableName, generation) +} + +func waitDispatchEntered(t *testing.T, coord *cleanupBlockingCoordinator) { + t.Helper() + select { + case <-coord.entered: + case <-time.After(5 * time.Second): + t.Fatal("the cleanup never dispatched") + } +} + +// TestDynamoDBAdminDeleteTableRefusedOnceStopBegan pins that a table delete +// arriving through the admin surface (which Stop's listener drain does not +// cover) after Stop began is refused before it commits: committing it +// would leave the deleted generation's items and index entries behind, +// since its cleanup can no longer start. +func TestDynamoDBAdminDeleteTableRefusedOnceStopBegan(t *testing.T) { + t.Parallel() + nodes, _, _ := createNode(t, 1) + defer shutdown(nodes) + srv := nodes[0].dynamoServer + ctx := context.Background() + _, err := srv.AdminCreateTable(ctx, fullAdminPrincipal, AdminCreateTableInput{ + TableName: "orders", + PartitionKey: AdminAttribute{Name: "id", Type: "S"}, + }) + require.NoError(t, err) + + srv.Stop() + err = srv.AdminDeleteTable(ctx, fullAdminPrincipal, "orders") + require.ErrorIs(t, err, ErrDynamoServerStopping) + _, exists, err := srv.AdminDescribeTable(ctx, "orders") + require.NoError(t, err) + require.True(t, exists, "a delete refused for the shutdown must not commit") +} + +// A slot reserved before stop holds it until the job started through it +// returns, or until it is released unused; a reservation is refused once +// stop has begun. +func TestBackgroundWorkersReservationHoldsStop(t *testing.T) { + w := newBackgroundWorkers() + started, ok := w.reserve() + require.True(t, ok) + unused, ok := w.reserve() + require.True(t, ok) + + stopDone := make(chan struct{}) + go func() { + w.stop("test", 5*time.Second, time.Second) + close(stopDone) + }() + require.Eventually(t, func() bool { + r, ok := w.reserve() + if ok { + r.release() + } + return !ok + }, 5*time.Second, time.Millisecond, "a reservation must be refused once stop began") + + finish := make(chan struct{}) + started.start(time.Minute, func(context.Context) { <-finish }) + unused.release() + unused.release() + select { + case <-stopDone: + t.Fatal("stop returned while a reserved job was still running") + case <-time.After(100 * time.Millisecond): + } + close(finish) + select { + case <-stopDone: + case <-time.After(5 * time.Second): + t.Fatal("stop did not return once the reserved job finished") + } +} + +// The public listener answers the stopping refusal with a retryable 503 +// ServiceUnavailable, not with the 500 an unknown error gets. +func TestDynamoDBStoppingErrorIsServiceUnavailable(t *testing.T) { + rec := httptest.NewRecorder() + writeDynamoErrorFromErr(rec, errors.WithStack(ErrDynamoServerStopping)) + require.Equal(t, http.StatusServiceUnavailable, rec.Code) + require.Equal(t, dynamoErrServiceUnavailable, rec.Header().Get("x-amzn-ErrorType")) +} diff --git a/adapter/http_shutdown.go b/adapter/http_shutdown.go new file mode 100644 index 000000000..6359b00c2 --- /dev/null +++ b/adapter/http_shutdown.go @@ -0,0 +1,95 @@ +package adapter + +import ( + "context" + "log/slog" + "net" + "net/http" + "sync" + "time" + + "github.com/bootjp/elastickv/monitoring" +) + +// httpShutdownTimeout bounds how long an HTTP adapter's Stop waits for +// in-flight requests to finish before closing the remaining connections. A +// client that stalls mid-request (for example an S3 upload that never sends +// its body) would otherwise hold http.Server.Shutdown open indefinitely, +// which in the server process means the shutdown errgroup never drains and +// the WAL/store cleanup behind it never runs. A variable so tests can +// shorten it. +var httpShutdownTimeout = 10 * time.Second + +// httpHandlerJoinTimeout bounds how long an HTTP adapter's Stop waits, +// once the shutdown bound forced the remaining connections closed, for the +// handlers still running on them. A variable so tests can shorten it. +var httpHandlerJoinTimeout = 2 * time.Second + +// httpConnTracker counts the connections an http.Server serves, from the +// accept until the goroutine serving one has returned from its handler. +// http.Server.Close closes the connections but does not wait for their +// handlers, so without it Stop would let the caller close the stores under +// a handler still unwinding out of a store call or a Raft dispatch. The +// count follows ConnState, which net/http reports as StateClosed (or +// StateHijacked) only after the handler returned. All methods are safe on +// a nil tracker, which has nothing to wait for. +type httpConnTracker struct { + gate *inflightGate + conns sync.Map // net.Conn -> struct{}, the connections counted in gate +} + +// trackHTTPConns installs the tracker as srv's ConnState hook. +func trackHTTPConns(srv *http.Server) *httpConnTracker { + t := &httpConnTracker{gate: newInflightGate()} + srv.ConnState = t.connState + return t +} + +func (t *httpConnTracker) connState(conn net.Conn, state http.ConnState) { + switch state { + case http.StateNew: + // Serve has returned before the join starts, so the gate never + // refuses here; a refused connection is simply not counted. + if t.gate.enter() { + t.conns.Store(conn, struct{}{}) + } + case http.StateClosed, http.StateHijacked: + if _, ok := t.conns.LoadAndDelete(conn); ok { + t.gate.exit() + } + case http.StateActive, http.StateIdle: + } +} + +// join waits up to timeout for every counted connection to be done, and +// reports whether they were. Concurrent and later calls share the deadline +// the first one set (see inflightGate.drain). +func (t *httpConnTracker) join(timeout time.Duration) bool { + if t == nil { + return true + } + return t.gate.drain(timeout) +} + +// shutdownHTTPServer drains srv gracefully for at most httpShutdownTimeout +// and then forces the remaining connections closed +// (monitoring.ShutdownHTTPServer), then waits, for at most +// httpHandlerJoinTimeout, for the handlers still running on the closed +// connections (conns). It is safe to call more than once: a second call +// finds no listeners and no connections and returns once the first call's +// wait is over. Stop has no error to return, so a failure is logged. +func shutdownHTTPServer(srv *http.Server, conns *httpConnTracker, name string) { + if err := monitoring.ShutdownHTTPServer(context.Background(), srv, name, httpShutdownTimeout); err != nil { + slog.Warn("http adapter shutdown failed", + slog.String("adapter", name), + slog.Any("err", err), + ) + } + if !conns.join(httpHandlerJoinTimeout) { + slog.Warn("http adapter handlers still running on closed connections, continuing shutdown", + slog.String("adapter", name), + slog.Int64("in_flight", conns.gate.remaining()), + slog.Duration("timeout", httpHandlerJoinTimeout), + ) + } +} diff --git a/adapter/http_shutdown_test.go b/adapter/http_shutdown_test.go new file mode 100644 index 000000000..f690db87d --- /dev/null +++ b/adapter/http_shutdown_test.go @@ -0,0 +1,457 @@ +package adapter + +import ( + "context" + "io" + "net" + "net/http" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/bootjp/elastickv/store" + "github.com/stretchr/testify/require" +) + +// stalledHandler never finishes a request on its own: it only returns once +// the server tears the connection down. That is the shape of a client that +// stalls mid-upload, which http.Server.Shutdown would wait on forever. +func stalledHandler(started chan<- struct{}) http.Handler { + return http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case started <- struct{}{}: + default: + } + <-r.Context().Done() + }) +} + +// httpAdapterUnderTest builds one HTTP adapter around a listener and +// exposes the pieces the bounded-stop tests need: the handler slot to +// stall, Run, and Stop. +type httpAdapterUnderTest struct { + name string + build func(t *testing.T, lis net.Listener) (srv *http.Server, run func() error, stop func()) +} + +func httpAdaptersUnderTest() []httpAdapterUnderTest { + return []httpAdapterUnderTest{ + { + name: "dynamodb", + build: func(_ *testing.T, lis net.Listener) (*http.Server, func() error, func()) { + s := NewDynamoDBServer(lis, nil, nil) + return s.httpServer, s.Run, s.Stop + }, + }, + { + name: "s3", + build: func(_ *testing.T, lis net.Listener) (*http.Server, func() error, func()) { + s := NewS3Server(lis, lis.Addr().String(), nil, nil, nil) + return s.httpServer, s.Run, s.Stop + }, + }, + { + name: "sqs", + build: func(_ *testing.T, lis net.Listener) (*http.Server, func() error, func()) { + s := NewSQSServer(lis, nil, nil) + return s.httpServer, s.Run, s.Stop + }, + }, + } +} + +func shortenHTTPShutdownTimeout(t *testing.T, d time.Duration) { + t.Helper() + old := httpShutdownTimeout + httpShutdownTimeout = d + t.Cleanup(func() { httpShutdownTimeout = old }) +} + +func listenLoopback(t *testing.T) net.Listener { + t.Helper() + lis, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + return lis +} + +// TestHTTPAdapterStopBoundsStalledRequest pins that Stop on each HTTP +// adapter returns within the shutdown bound even while a request is still +// in flight, and that the stalled connection is closed rather than left to +// finish on its own. Without the bound, a SIGTERM would wait on the client +// for as long as it stays connected while repeat signals are swallowed, so +// the WAL/store cleanup behind the errgroup would never run. +func TestHTTPAdapterStopBoundsStalledRequest(t *testing.T) { + shortenHTTPShutdownTimeout(t, 200*time.Millisecond) + for _, tc := range httpAdaptersUnderTest() { + t.Run(tc.name, func(t *testing.T) { + lis := listenLoopback(t) + srv, run, stop := tc.build(t, lis) + started := make(chan struct{}, 1) + srv.Handler = stalledHandler(started) + runDone := make(chan error, 1) + go func() { runDone <- run() }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+lis.Addr().String()+"/", http.NoBody) + require.NoError(t, err) + client := &http.Client{Transport: &http.Transport{DisableKeepAlives: true}} + reqDone := make(chan error, 1) + go func() { + resp, err := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + reqDone <- err + }() + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("handler did not start for the stalled request") + } + + start := time.Now() + stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "graceful shutdown must be given its bound first") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a client that never finishes its request") + + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } + select { + case err := <-reqDone: + require.Error(t, err, "the stalled request must be cut off, not completed") + case <-time.After(5 * time.Second): + t.Fatal("the stalled request was not closed by Stop") + } + }) + } +} + +// With nothing in flight each adapter's Stop drains within the graceful +// phase and returns long before the bound. +func TestHTTPAdapterStopReturnsPromptlyWhenIdle(t *testing.T) { + shortenHTTPShutdownTimeout(t, 5*time.Second) + for _, tc := range httpAdaptersUnderTest() { + t.Run(tc.name, func(t *testing.T) { + lis := listenLoopback(t) + _, run, stop := tc.build(t, lis) + runDone := make(chan error, 1) + go func() { runDone <- run() }() + time.Sleep(20 * time.Millisecond) + + start := time.Now() + stop() + require.Less(t, time.Since(start), 2*time.Second) + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } + }) + } +} + +// Stop is called twice on the normal path (once from the context waiter, +// once from the deferred call after Run returns); the second call must be a +// prompt no-op even after the first one had to force connections closed. +func TestHTTPAdapterStopIsIdempotentAfterForcedClose(t *testing.T) { + shortenHTTPShutdownTimeout(t, 100*time.Millisecond) + lis := listenLoopback(t) + srv, run, stop := httpAdaptersUnderTest()[0].build(t, lis) + started := make(chan struct{}, 1) + srv.Handler = stalledHandler(started) + runDone := make(chan error, 1) + go func() { runDone <- run() }() + + conn, err := (&net.Dialer{}).DialContext(context.Background(), "tcp", lis.Addr().String()) + require.NoError(t, err) + defer func() { _ = conn.Close() }() + _, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n")) + require.NoError(t, err) + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("handler did not start") + } + + stop() + start := time.Now() + stop() + require.Less(t, time.Since(start), 50*time.Millisecond, "second Stop must not wait the bound again") + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } +} + +// closeRecordingBlobCluster records when Stop closes the blob peer cluster. +type closeRecordingBlobCluster struct { + *fakeS3BlobCluster + closed atomic.Bool +} + +func (c *closeRecordingBlobCluster) Close() error { + c.closed.Store(true) + return nil +} + +// stopInBackground runs Stop on its own goroutine and returns a channel +// closed once it returned. +func stopInBackground(s *S3Server) <-chan struct{} { + done := make(chan struct{}) + go func() { + s.Stop() + close(done) + }() + return done +} + +// requireStopStillDraining checks, partway into Stop's graceful window, +// that Stop has not returned and the blob cluster is still open. +func requireStopStillDraining(t *testing.T, stopDone <-chan struct{}, cluster *closeRecordingBlobCluster) { + t.Helper() + time.Sleep(100 * time.Millisecond) + select { + case <-stopDone: + t.Fatal("Stop returned before its graceful window was over") + default: + } + require.False(t, cluster.closed.Load(), "the blob cluster must stay open during the graceful window") +} + +func waitStopped(t *testing.T, stopDone <-chan struct{}) { + t.Helper() + select { + case <-stopDone: + case <-time.After(5 * time.Second): + t.Fatal("Stop did not return") + } +} + +// TestS3ServerStopDrainsHTTPBeforeClosingBlobCluster pins the order in +// Stop: while an S3 request is still running inside the graceful window +// the blob cluster stays open (its peer replication or fetch would +// otherwise fail at once), and the cluster is closed only once the HTTP +// drain has finished or forced the remaining connections closed. The +// check runs while Stop is still inside the window, not from the handler, +// because a forced Close does not wait for the handler to return. +func TestS3ServerStopDrainsHTTPBeforeClosingBlobCluster(t *testing.T) { + shortenHTTPShutdownTimeout(t, 300*time.Millisecond) + lis := listenLoopback(t) + s := NewS3Server(lis, lis.Addr().String(), nil, nil, nil) + cluster := &closeRecordingBlobCluster{fakeS3BlobCluster: newFakeS3BlobCluster()} + s.blobCluster = cluster + started := make(chan struct{}, 1) + s.httpServer.Handler = stalledHandler(started) + runDone := make(chan error, 1) + go func() { runDone <- s.Run() }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+lis.Addr().String()+"/", http.NoBody) + require.NoError(t, err) + client := &http.Client{Transport: &http.Transport{DisableKeepAlives: true}} + go func() { + resp, _ := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + }() + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("handler did not start") + } + + stopDone := stopInBackground(s) + requireStopStillDraining(t, stopDone, cluster) + waitStopped(t, stopDone) + require.True(t, cluster.closed.Load(), "Stop must close the blob cluster once the HTTP drain is over") + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } +} + +// TestS3ServerStopWaitsForAdminObjectReadBeforeClosingBlobCluster pins +// that the admin listener's S3 object bridge, which calls into the same +// server outside its HTTP listener, keeps the blob cluster while an object +// operation is still running: with the S3 listener idle its drain is over +// at once, and closing the cluster then would fail the admin download (or +// upload) that is still fetching (or replicating) chunk blobs. Once Stop +// began, a new admin object operation is refused instead of running +// against a cluster that is about to close. +func TestS3ServerStopWaitsForAdminObjectReadBeforeClosingBlobCluster(t *testing.T) { + shortenHTTPShutdownTimeout(t, 5*time.Second) + st := store.NewMVCCStore() + s := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + createBucketForAdminTest(t, s, "downloads") + require.NoError(t, s.AdminPutObject(context.Background(), + fullAdminBucketsPrincipal(), "downloads", "report.txt", + strings.NewReader("hello, world"), "text/plain")) + cluster := &closeRecordingBlobCluster{fakeS3BlobCluster: newFakeS3BlobCluster()} + s.blobCluster = cluster + + body, _, err := s.AdminGetObject(context.Background(), + fullAdminBucketsPrincipal(), "downloads", "report.txt") + require.NoError(t, err) + + stopDone := stopInBackground(s) + requireStopStillDraining(t, stopDone, cluster) + got, err := io.ReadAll(body) + require.NoError(t, err, "the admin download keeps reading during the graceful window") + require.Equal(t, "hello, world", string(got)) + require.NoError(t, body.Close()) + waitStopped(t, stopDone) + require.True(t, cluster.closed.Load(), "the cluster is closed once the admin read finished") + + err = s.AdminPutObject(context.Background(), + fullAdminBucketsPrincipal(), "downloads", "late.txt", + strings.NewReader("late"), "text/plain") + require.ErrorIs(t, err, ErrAdminS3Stopping) + _, _, err = s.AdminGetObject(context.Background(), + fullAdminBucketsPrincipal(), "downloads", "report.txt") + require.ErrorIs(t, err, ErrAdminS3Stopping) +} + +// An admin object read whose body is never closed holds Stop for the +// bound only, then the cluster is closed anyway. +func TestS3ServerStopGivesUpOnUnclosedAdminReadAfterBound(t *testing.T) { + shortenHTTPShutdownTimeout(t, 200*time.Millisecond) + st := store.NewMVCCStore() + s := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + createBucketForAdminTest(t, s, "downloads") + require.NoError(t, s.AdminPutObject(context.Background(), + fullAdminBucketsPrincipal(), "downloads", "report.txt", + strings.NewReader("x"), "text/plain")) + cluster := &closeRecordingBlobCluster{fakeS3BlobCluster: newFakeS3BlobCluster()} + s.blobCluster = cluster + body, _, err := s.AdminGetObject(context.Background(), + fullAdminBucketsPrincipal(), "downloads", "report.txt") + require.NoError(t, err) + t.Cleanup(func() { _ = body.Close() }) + + start := time.Now() + s.Stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "the admin read must be given its bound") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a body nobody closes") + require.True(t, cluster.closed.Load()) +} + +func shortenHTTPHandlerJoinTimeout(t *testing.T, d time.Duration) { + t.Helper() + old := httpHandlerJoinTimeout + httpHandlerJoinTimeout = d + t.Cleanup(func() { httpHandlerJoinTimeout = old }) +} + +// slowUnwindingHandler stands for a handler still inside a store call when +// the server forces its connection closed: it notices the cancellation but +// takes unwind longer to return, or never returns while release +// stays open. +func slowUnwindingHandler(started chan<- struct{}, unwind time.Duration, release <-chan struct{}, finished *atomic.Bool) http.Handler { + return http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case started <- struct{}{}: + default: + } + <-r.Context().Done() + time.Sleep(unwind) + <-release + finished.Store(true) + }) +} + +// startStalledRequest sends one request the handler will not finish and +// waits until the handler runs. +func startStalledRequest(t *testing.T, lis net.Listener, started <-chan struct{}) { + t.Helper() + conn, err := (&net.Dialer{}).DialContext(context.Background(), "tcp", lis.Addr().String()) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + _, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n")) + require.NoError(t, err) + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("handler did not start") + } +} + +// TestHTTPAdapterStopJoinsHandlerAfterForcedClose pins that Stop does not +// return while a handler is still running on a connection the bound forced +// closed: http.Server.Close does not wait for handlers, so the caller would +// otherwise close the stores under a handler still unwinding out of a store +// call. +func TestHTTPAdapterStopJoinsHandlerAfterForcedClose(t *testing.T) { + shortenHTTPShutdownTimeout(t, 100*time.Millisecond) + shortenHTTPHandlerJoinTimeout(t, 5*time.Second) + for _, tc := range httpAdaptersUnderTest() { + t.Run(tc.name, func(t *testing.T) { + lis := listenLoopback(t) + srv, run, stop := tc.build(t, lis) + started := make(chan struct{}, 1) + release := make(chan struct{}) + close(release) + var finished atomic.Bool + srv.Handler = slowUnwindingHandler(started, 300*time.Millisecond, release, &finished) + runDone := make(chan error, 1) + go func() { runDone <- run() }() + startStalledRequest(t, lis, started) + + stop() + require.True(t, finished.Load(), "Stop returned while the handler on the closed connection was still running") + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } + }) + } +} + +// A handler that never returns holds Stop for the join bound only. +func TestHTTPAdapterStopBoundsJoinOfHandlerIgnoringClose(t *testing.T) { + shortenHTTPShutdownTimeout(t, 100*time.Millisecond) + shortenHTTPHandlerJoinTimeout(t, 400*time.Millisecond) + lis := listenLoopback(t) + srv, run, stop := httpAdaptersUnderTest()[0].build(t, lis) + started := make(chan struct{}, 1) + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + var finished atomic.Bool + srv.Handler = slowUnwindingHandler(started, 0, release, &finished) + runDone := make(chan error, 1) + go func() { runDone <- run() }() + startStalledRequest(t, lis, started) + + start := time.Now() + stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 400*time.Millisecond, "the handler must be given the join bound") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a handler that never returns") + require.False(t, finished.Load()) + + start = time.Now() + stop() + require.Less(t, time.Since(start), 50*time.Millisecond, "second Stop must not wait the join bound again") + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } +} diff --git a/adapter/inflight_gate.go b/adapter/inflight_gate.go new file mode 100644 index 000000000..cf738747a --- /dev/null +++ b/adapter/inflight_gate.go @@ -0,0 +1,132 @@ +package adapter + +import ( + "sync" + "sync/atomic" + "time" +) + +// inflightGate counts the operations being executed so a Stop can wait, +// bounded, for them to finish, and refuses new ones once the drain began. +// A few atomic operations per operation keep it off the hot path's locks. +// The Redis listener counts its commands with it; the S3 server counts the +// admin object operations that use its blob peer cluster, and its +// background cleanups. All methods are +// safe on a nil gate, which admits everything and has nothing to drain. +type inflightGate struct { + n atomic.Int64 + draining atomic.Bool + idle chan struct{} + idleOnce sync.Once + + // drainOnce and joinOnce fix, on the first drain and the first join, + // the deadline every later or concurrent call waits until. Stop runs + // from two places at once (the context waiter and the deferred call + // after Run returns), and whichever returns first lets the caller close + // the resources the operations use, so neither may return earlier than + // the other, nor spend a fresh bound. + drainOnce sync.Once + drainUntil time.Time + joinOnce sync.Once + joinUntil time.Time +} + +func newInflightGate() *inflightGate { + return &inflightGate{idle: make(chan struct{})} +} + +// enter registers an operation about to run and reports whether it may. +// Once drain has started it refuses: an operation entering after the count +// reached zero would run against resources the caller is closing. (redcon, +// for example, dispatches every command of an already-parsed pipeline even +// after Stop closed the connection; that client's connection is already +// gone, so a refused command never had a reply coming.) +// +// The increment comes before the draining check, and drain sets draining +// before it reads the count, so with sequentially consistent atomics either +// drain sees this operation or this operation sees the drain. +func (i *inflightGate) enter() bool { + if i == nil { + return true + } + i.n.Add(1) + if i.draining.Load() { + i.exit() + return false + } + return true +} + +func (i *inflightGate) exit() { + if i != nil && i.n.Add(-1) == 0 && i.draining.Load() { + i.signalIdle() + } +} + +func (i *inflightGate) signalIdle() { + i.idleOnce.Do(func() { close(i.idle) }) +} + +// remaining is the number of operations still being executed. +func (i *inflightGate) remaining() int64 { + if i == nil { + return 0 + } + return i.n.Load() +} + +// drain refuses new operations, waits until timeout after the first drain +// for every in-flight operation to finish, and reports whether they did. +// A call made while an earlier one is still waiting waits until the same +// deadline; one made after it reports at once. +func (i *inflightGate) drain(timeout time.Duration) bool { + if i == nil { + return true + } + i.drainOnce.Do(func() { + i.draining.Store(true) + if i.n.Load() == 0 { + i.signalIdle() + } + i.drainUntil = time.Now().Add(timeout) + }) + return i.waitIdleUntil(i.drainUntil) +} + +// join waits, after a drain that gave up, for the operations still running +// to finish, and reports whether they did. It is meant for a caller that +// has meanwhile told them to stop (by cancelling their context) and must +// not release their resources while they unwind. Like drain, every call +// waits until the deadline the first one set, timeout after it. +func (i *inflightGate) join(timeout time.Duration) bool { + if i == nil || i.n.Load() == 0 { + return true + } + if !i.draining.Load() { + return false + } + i.joinOnce.Do(func() { i.joinUntil = time.Now().Add(timeout) }) + return i.waitIdleUntil(i.joinUntil) +} + +// waitIdleUntil waits, once the drain has begun, until no operation is in +// flight or deadline passes, and reports whether the gate went idle. +func (i *inflightGate) waitIdleUntil(deadline time.Time) bool { + wait := time.Until(deadline) + if wait <= 0 { + select { + case <-i.idle: + return true + default: + return false + } + } + timer := time.NewTimer(wait) + defer timer.Stop() + select { + case <-i.idle: + return true + case <-timer.C: + return i.n.Load() == 0 + } +} diff --git a/adapter/inflight_gate_test.go b/adapter/inflight_gate_test.go new file mode 100644 index 000000000..30a53e033 --- /dev/null +++ b/adapter/inflight_gate_test.go @@ -0,0 +1,106 @@ +package adapter + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestInflightGateRefusesEntryOnceDraining(t *testing.T) { + gate := newInflightGate() + require.True(t, gate.enter()) + gate.exit() + require.True(t, gate.drain(time.Second)) + require.False(t, gate.enter(), "no operation may start once the drain began") + require.Zero(t, gate.remaining()) + + var nilGate *inflightGate + require.True(t, nilGate.enter()) + nilGate.exit() + require.True(t, nilGate.drain(time.Second)) +} + +// An operation that entered before the drain keeps it waiting until it +// exits; only the first drain waits. +func TestInflightGateDrainWaitsForRunningOperation(t *testing.T) { + gate := newInflightGate() + require.True(t, gate.enter()) + go func() { + time.Sleep(100 * time.Millisecond) + gate.exit() + }() + start := time.Now() + require.True(t, gate.drain(5*time.Second)) + require.GreaterOrEqual(t, time.Since(start), 80*time.Millisecond) + + stuck := newInflightGate() + require.True(t, stuck.enter()) + start = time.Now() + require.False(t, stuck.drain(100*time.Millisecond)) + require.GreaterOrEqual(t, time.Since(start), 80*time.Millisecond) + start = time.Now() + require.False(t, stuck.drain(5*time.Second), "a second drain reports without waiting again") + require.Less(t, time.Since(start), 50*time.Millisecond) +} + +// join waits, once, for an operation that outlived the drain to exit. +func TestInflightGateJoinWaitsAfterDrainGaveUp(t *testing.T) { + gate := newInflightGate() + require.True(t, gate.enter()) + require.False(t, gate.drain(10*time.Millisecond)) + go func() { + time.Sleep(100 * time.Millisecond) + gate.exit() + }() + start := time.Now() + require.True(t, gate.join(5*time.Second)) + require.GreaterOrEqual(t, time.Since(start), 80*time.Millisecond) + require.True(t, gate.join(5*time.Second), "an idle gate joins at once") + + stuck := newInflightGate() + require.True(t, stuck.enter()) + require.False(t, stuck.drain(10*time.Millisecond)) + start = time.Now() + require.False(t, stuck.join(100*time.Millisecond)) + require.GreaterOrEqual(t, time.Since(start), 80*time.Millisecond) + start = time.Now() + require.False(t, stuck.join(5*time.Second), "a second join reports without waiting again") + require.Less(t, time.Since(start), 50*time.Millisecond) + + var nilGate *inflightGate + require.True(t, nilGate.join(time.Second)) +} + +// Stop runs from two places at once (the context waiter and the deferred +// call after Run returns), so a drain that starts while another is still +// waiting must wait for the same deadline rather than spend a fresh bound. +func TestInflightGateConcurrentDrainsShareOneDeadline(t *testing.T) { + gate := newInflightGate() + require.True(t, gate.enter()) + firstDrain := make(chan bool, 1) + go func() { firstDrain <- gate.drain(300 * time.Millisecond) }() + time.Sleep(100 * time.Millisecond) + start := time.Now() + require.False(t, gate.drain(5*time.Second), "the operation is still running at the shared deadline") + require.Less(t, time.Since(start), time.Second, "a concurrent drain must not spend a fresh bound") + require.False(t, <-firstDrain) +} + +// Likewise a join that starts while another is still waiting must wait +// for the operation too: returning at once lets that Stop caller close the +// stores while the operation unwinds. +func TestInflightGateConcurrentJoinsShareOneDeadline(t *testing.T) { + gate := newInflightGate() + require.True(t, gate.enter()) + require.False(t, gate.drain(10*time.Millisecond)) + firstJoin := make(chan bool, 1) + go func() { firstJoin <- gate.join(5 * time.Second) }() + time.Sleep(100 * time.Millisecond) + go func() { + time.Sleep(200 * time.Millisecond) + gate.exit() + }() + require.True(t, gate.join(5*time.Second), "a concurrent join must wait for the operation, not return at once") + require.True(t, <-firstJoin) +} diff --git a/adapter/redis.go b/adapter/redis.go index 6ac3aac97..5d0bee2e9 100644 --- a/adapter/redis.go +++ b/adapter/redis.go @@ -246,6 +246,10 @@ type RedisServer struct { onePhaseTxnDedup bool route map[string]func(conn redcon.Conn, cmd redcon.Command) + + // inflight counts the commands being executed so Stop can wait for them, + // bounded, after closing the listener; see inflightGate. + inflight *inflightGate } type RedisServerOption func(*RedisServer) @@ -557,6 +561,7 @@ func NewRedisServer(listen net.Listener, redisAddr string, store store.MVCCStore onePhaseTxnDedup: os.Getenv("ELASTICKV_REDIS_ONEPHASE_DEDUP") != "0", baseCtx: baseCtx, baseCancel: baseCancel, + inflight: newInflightGate(), streamWaiters: newKeyWaiterRegistry(), zsetWaiters: newKeyWaiterRegistry(), } @@ -824,6 +829,11 @@ func (r *RedisServer) RegisterLuaPoolMetrics(registerer prometheus.Registerer) e func (r *RedisServer) Run() error { err := redcon.Serve(r.listen, func(conn redcon.Conn, cmd redcon.Command) { + if !r.inflight.enter() { + conn.WriteError("ERR server is shutting down") + return + } + defer r.inflight.exit() needsTiming := r.requestObserver != nil || r.traceCommands var start time.Time if needsTiming { @@ -1005,6 +1015,16 @@ func (r *RedisServer) Stop() { ) } } + // redcon closes the connections when Serve returns but does not join + // the handlers; give the commands still running their bounded window + // before the caller tears down the stores they use. + if !r.inflight.drain(redisShutdownTimeout) { + slog.Warn("redis server: commands still running after the shutdown bound, continuing", + slog.String("addr", r.redisAddr), + slog.Int64("in_flight", r.inflight.remaining()), + slog.Duration("timeout", redisShutdownTimeout), + ) + } } func (r *RedisServer) publishLocal(channel, message []byte) int64 { diff --git a/adapter/redis_shutdown.go b/adapter/redis_shutdown.go new file mode 100644 index 000000000..3185fcc84 --- /dev/null +++ b/adapter/redis_shutdown.go @@ -0,0 +1,11 @@ +package adapter + +import "time" + +// redisShutdownTimeout bounds how long Stop waits for the commands still +// being executed once the listener and connections are closed. redcon ends +// Serve as soon as the listener closes and does not join its handler +// goroutines, so without this wait the errgroup task in main would finish, +// and the stores be closed, underneath a handler still dispatching (a SETEX +// waiting on Raft, for example). A variable so tests can shorten it. +var redisShutdownTimeout = 10 * time.Second diff --git a/adapter/redis_shutdown_test.go b/adapter/redis_shutdown_test.go new file mode 100644 index 000000000..1efc140be --- /dev/null +++ b/adapter/redis_shutdown_test.go @@ -0,0 +1,181 @@ +package adapter + +import ( + "context" + "net" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" + "github.com/tidwall/redcon" +) + +// blockingRedisCommand is a test-only command whose handler reports that it +// started and then waits until the test releases it, the shape of a command +// waiting on Raft (a SETEX with the leader unavailable). +const blockingRedisCommand = "BLOCKTEST" + +// startBlockingRedisServer serves a RedisServer on a loopback listener with +// blockingRedisCommand installed and returns what the tests need to drive +// it: the server, the channel the handler reports on, the channel that +// releases the handler, and Run's result. +func startBlockingRedisServer(t *testing.T) (server *RedisServer, started <-chan struct{}, release chan struct{}, runDone <-chan error) { + t.Helper() + lis := listenLoopback(t) + server = NewRedisServer(lis, lis.Addr().String(), nil, nil, nil, nil) + startedCh := make(chan struct{}, 1) + release = make(chan struct{}) + server.route[blockingRedisCommand] = func(conn redcon.Conn, _ redcon.Command) { + select { + case startedCh <- struct{}{}: + default: + } + <-release + conn.WriteString("OK") + } + done := make(chan error, 1) + go func() { done <- server.Run() }() + return server, startedCh, release, done +} + +// sendBlockingRedisCommand opens a client connection and issues the +// blocking command on it; the connection stays open until the test ends. +func sendBlockingRedisCommand(t *testing.T, server *RedisServer, started <-chan struct{}) { + t.Helper() + conn, err := (&net.Dialer{}).DialContext(context.Background(), "tcp", server.listen.Addr().String()) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + _, err = conn.Write([]byte("*1\r\n$9\r\n" + blockingRedisCommand + "\r\n")) + require.NoError(t, err) + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("the blocking command did not start") + } +} + +func shortenRedisShutdownTimeout(t *testing.T, d time.Duration) { + t.Helper() + old := redisShutdownTimeout + redisShutdownTimeout = d + t.Cleanup(func() { redisShutdownTimeout = old }) +} + +// TestRedisServerStopWaitsForInflightCommandWithinBound pins that Stop gives +// a command still being executed its bounded window instead of returning as +// soon as the listener is closed: redcon does not join its handlers, so +// without the wait the errgroup task in main would finish and the stores be +// closed underneath the handler. A command that never finishes is given up +// on after the bound, and the second Stop (the deferred one after Run +// returns) does not spend the bound again. +func TestRedisServerStopWaitsForInflightCommandWithinBound(t *testing.T) { + shortenRedisShutdownTimeout(t, 200*time.Millisecond) + server, started, release, runDone := startBlockingRedisServer(t) + t.Cleanup(func() { close(release) }) + sendBlockingRedisCommand(t, server, started) + + start := time.Now() + server.Stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "Stop must give the running command its bound") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a command that never finishes") + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } + + start = time.Now() + server.Stop() + require.Less(t, time.Since(start), 50*time.Millisecond, "second Stop must not wait the bound again") +} + +// TestRedisServerStopReturnsOnceInflightCommandFinishes: a command that does +// finish inside the window ends the wait at once; Stop neither returns before +// it nor waits out the whole bound. +func TestRedisServerStopReturnsOnceInflightCommandFinishes(t *testing.T) { + shortenRedisShutdownTimeout(t, 5*time.Second) + server, started, release, runDone := startBlockingRedisServer(t) + sendBlockingRedisCommand(t, server, started) + go func() { + time.Sleep(100 * time.Millisecond) + close(release) + }() + + start := time.Now() + server.Stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 80*time.Millisecond, "Stop must wait for the running command") + require.Less(t, elapsed, 2*time.Second, "Stop must return as soon as the command finished, not after the bound") + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } +} + +// With nothing in flight Stop returns at once. +func TestRedisServerStopReturnsPromptlyWhenIdle(t *testing.T) { + shortenRedisShutdownTimeout(t, 5*time.Second) + server, _, release, runDone := startBlockingRedisServer(t) + close(release) + time.Sleep(20 * time.Millisecond) + + start := time.Now() + server.Stop() + require.Less(t, time.Since(start), time.Second) + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } +} + +// countingRedisCommand is a test-only command that only records that it +// ran. +const countingRedisCommand = "COUNTTEST" + +// TestRedisServerStopRejectsPipelinedCommandAfterDrain pins that a command +// redcon hands over after the drain began is not executed. redcon parses a +// whole pipeline before it looks at the connection again, so the command +// behind a long-running one is dispatched even though Stop already closed +// the connection; once the running command finished, Stop has declared the +// drain complete, and the late command would run against stores the +// caller is about to close. +func TestRedisServerStopRejectsPipelinedCommandAfterDrain(t *testing.T) { + shortenRedisShutdownTimeout(t, 5*time.Second) + server, started, release, runDone := startBlockingRedisServer(t) + var lateRuns atomic.Int32 + server.route[countingRedisCommand] = func(conn redcon.Conn, _ redcon.Command) { + lateRuns.Add(1) + conn.WriteString("OK") + } + conn, err := (&net.Dialer{}).DialContext(context.Background(), "tcp", server.listen.Addr().String()) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + _, err = conn.Write([]byte("*1\r\n$9\r\n" + blockingRedisCommand + "\r\n*1\r\n$9\r\n" + countingRedisCommand + "\r\n")) + require.NoError(t, err) + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("the blocking command did not start") + } + + go func() { + time.Sleep(100 * time.Millisecond) + close(release) + }() + server.Stop() + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } + time.Sleep(200 * time.Millisecond) + require.Zero(t, lateRuns.Load(), "a command handed over after the drain began must not run") +} diff --git a/adapter/s3.go b/adapter/s3.go index 6020843c0..9f2a783df 100644 --- a/adapter/s3.go +++ b/adapter/s3.go @@ -108,6 +108,7 @@ type S3Server struct { staticCreds map[string]string readTracker *kv.ActiveTimestampTracker httpServer *http.Server + httpConns *httpConnTracker cleanupSem chan struct{} putAdmission *s3PutAdmission putAdmissionObserver S3PutAdmissionObserver @@ -120,6 +121,19 @@ type S3Server struct { blobMinReplicas int blobPushBlocked func() bool blobBackfiller *S3BlobBackfiller + // adminBlobUsers counts the admin object operations that may use + // blobCluster from outside the HTTP listener (see enterAdminBlobUse), + // so Stop can wait for them before closing the cluster. + adminBlobUsers *inflightGate + // cleanupWorkers counts the background cleanups (see startCleanup) so + // Stop can join them, bounded, before the blob cluster and the stores + // close underneath them. + cleanupWorkers *inflightGate + reservedCleanups *backgroundWorkers + // cleanupCtx is the parent of every background cleanup's context; + // cancelCleanups ends it once Stop has waited its bound. + cleanupCtx context.Context + cancelCleanups context.CancelFunc } type s3BucketMeta struct { @@ -362,7 +376,11 @@ func NewS3Server(listen net.Listener, s3Addr string, st store.MVCCStore, coordin cleanupSem: make(chan struct{}, s3ManifestCleanupWorkers), putAdmission: newS3PutAdmissionFromEnv(), blobOffloadEnabled: S3BlobOffloadEnabledFromEnv(), + adminBlobUsers: newInflightGate(), + cleanupWorkers: newInflightGate(), + reservedCleanups: newBackgroundWorkers(), } + s.cleanupCtx, s.cancelCleanups = context.WithCancel(context.Background()) if localStores, ok := st.(S3BlobLocalStoreResolver); ok { s.blobLocalStores = localStores } @@ -384,6 +402,7 @@ func NewS3Server(listen net.Listener, s3Addr string, st store.MVCCStore, coordin mux := http.NewServeMux() mux.HandleFunc("/", s.handle) s.httpServer = &http.Server{Handler: mux, ReadHeaderTimeout: time.Second} + s.httpConns = trackHTTPConns(s.httpServer) return s } @@ -406,15 +425,39 @@ func (s *S3Server) Run() error { return nil } +// Stop ends the background backfill, drains the HTTP listener (bounded, +// see shutdownHTTPServer), then waits, under one more bound, for the admin +// object operations still using the blob peer cluster (see +// enterAdminBlobUse) and for the background cleanups those requests +// started (see startCleanup). Only then does it close the cluster: an +// in-flight upload or read that replicates or fetches chunk blobs through +// it keeps its peers for the whole graceful window instead of failing at +// once with a closed cluster, and a cleanup finishes its deletes before +// the stores are closed after Stop. The admin operations are counted +// separately because they reach this server through the admin listener or +// the gRPC admin forwarder, which drain on their own; an idle S3 listener +// finishes its drain at once. func (s *S3Server) Stop() { - if s != nil && s.blobBackfiller != nil { + if s == nil { + return + } + if s.blobBackfiller != nil { s.blobBackfiller.Stop() } - if s != nil && s.blobCluster != nil { - _ = s.blobCluster.Close() + if s.httpServer != nil { + shutdownHTTPServer(s.httpServer, s.httpConns, "s3") } - if s != nil && s.httpServer != nil { - _ = s.httpServer.Shutdown(context.Background()) + deadline := time.Now().Add(httpShutdownTimeout) + if !s.adminBlobUsers.drain(time.Until(deadline)) { + slog.Warn("s3 server: admin object operations still running after the shutdown bound, closing the blob cluster", + slog.Int64("in_flight", s.adminBlobUsers.remaining()), + slog.Duration("timeout", httpShutdownTimeout), + ) + } + s.reservedCleanups.stop("s3 reserved cleanup", time.Until(deadline), s3CleanupJoinTimeout) + s.drainCleanups(time.Until(deadline)) + if s.blobCluster != nil { + _ = s.blobCluster.Close() } } @@ -1481,18 +1524,9 @@ func (s *S3Server) listParts(w http.ResponseWriter, r *http.Request, bucket stri } func (s *S3Server) cleanupUploadPartsAsync(bucket string, generation uint64, objectKey string, uploadID string) { - select { - case s.cleanupSem <- struct{}{}: - default: - // Semaphore saturated; skip to avoid unbounded goroutine accumulation. - return - } - go func() { - defer func() { <-s.cleanupSem }() - ctx, cancel := context.WithTimeout(context.Background(), s3ManifestCleanupTimeout) - defer cancel() + s.startCleanup(func(ctx context.Context) { s.cleanupUploadParts(ctx, bucket, generation, objectKey, uploadID) - }() + }) } func (s *S3Server) cleanupUploadParts(ctx context.Context, bucket string, generation uint64, objectKey string, uploadID string) { @@ -1515,16 +1549,7 @@ func (s *S3Server) cleanupPartBlobsAsync( chunkRefVersion uint64, offloaded bool, ) { - select { - case s.cleanupSem <- struct{}{}: - default: - // Semaphore saturated; skip to avoid unbounded goroutine accumulation. - return - } - go func() { - defer func() { <-s.cleanupSem }() - ctx, cancel := context.WithTimeout(context.Background(), s3ManifestCleanupTimeout) - defer cancel() + s.startCleanup(func(ctx context.Context) { pending := make([]*kv.Elem[kv.OP], 0, s3MetaBatchOps) flush := func() { if len(pending) == 0 { @@ -1555,20 +1580,11 @@ func (s *S3Server) cleanupPartBlobsAsync( } } flush() - }() + }) } func (s *S3Server) cleanupUploadDataAsync(bucket string, generation uint64, objectKey string, uploadID string) { - select { - case s.cleanupSem <- struct{}{}: - default: - // Semaphore saturated; skip to avoid unbounded goroutine accumulation. - return - } - go func() { - defer func() { <-s.cleanupSem }() - ctx, cancel := context.WithTimeout(context.Background(), s3ManifestCleanupTimeout) - defer cancel() + s.startCleanup(func(ctx context.Context) { // Delete part descriptors. s.cleanupUploadParts(ctx, bucket, generation, objectKey, uploadID) // Delete legacy blob chunks and offloaded chunk references. Content- @@ -1577,7 +1593,7 @@ func (s *S3Server) cleanupUploadDataAsync(bucket string, generation uint64, obje s.deleteByPrefix(ctx, blobPrefix, bucket, generation, objectKey, uploadID) chunkRefPrefix := s3keys.ChunkRefPrefixForUpload(bucket, generation, objectKey, uploadID) s.deleteByPrefix(ctx, chunkRefPrefix, bucket, generation, objectKey, uploadID) - }() + }) } func (s *S3Server) deleteByPrefix(ctx context.Context, prefix []byte, bucket string, generation uint64, objectKey string, uploadID string) { @@ -1755,18 +1771,9 @@ func (s *S3Server) cleanupManifestBlobsAsync(bucket string, generation uint64, o if manifest == nil { return } - select { - case s.cleanupSem <- struct{}{}: - default: - // Semaphore saturated; skip to avoid unbounded goroutine accumulation. - return - } - go func() { - defer func() { <-s.cleanupSem }() - ctx, cancel := context.WithTimeout(context.Background(), s3ManifestCleanupTimeout) - defer cancel() + s.startCleanup(func(ctx context.Context) { s.cleanupManifestBlobs(ctx, bucket, generation, objectKey, manifest) - }() + }) } func (s *S3Server) cleanupManifestBlobs(ctx context.Context, bucket string, generation uint64, objectKey string, manifest *s3ObjectManifest) { diff --git a/adapter/s3_admin_delete_shutdown_test.go b/adapter/s3_admin_delete_shutdown_test.go new file mode 100644 index 000000000..e389fca7b --- /dev/null +++ b/adapter/s3_admin_delete_shutdown_test.go @@ -0,0 +1,112 @@ +package adapter + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/bootjp/elastickv/kv" + "github.com/bootjp/elastickv/store" + "github.com/stretchr/testify/require" +) + +// firstDispatchHoldingCoordinator holds the first Dispatch after arm is +// called until release is closed, and counts the dispatches that follow +// it, the way an admin delete transaction waits on Raft before its +// background cleanup dispatches the blob deletes. +type firstDispatchHoldingCoordinator struct { + *localAdapterCoordinator + armed atomic.Bool + held atomic.Bool + after atomic.Int64 + entered chan struct{} + release chan struct{} + once sync.Once +} + +func newFirstDispatchHoldingCoordinator(st store.MVCCStore) *firstDispatchHoldingCoordinator { + return &firstDispatchHoldingCoordinator{ + localAdapterCoordinator: newLocalAdapterCoordinator(st), + entered: make(chan struct{}), + release: make(chan struct{}), + } +} + +func (c *firstDispatchHoldingCoordinator) Dispatch(ctx context.Context, req *kv.OperationGroup[kv.OP]) (*kv.CoordinateResponse, error) { + if c.armed.Load() { + if c.held.CompareAndSwap(false, true) { + c.once.Do(func() { close(c.entered) }) + <-c.release + } else { + c.after.Add(1) + } + } + return c.localAdapterCoordinator.Dispatch(ctx, req) +} + +// An admin delete already running when Stop begins must keep Stop, and +// with it the cleanup gate, open until it finished, so the blob cleanup it +// schedules after committing still runs instead of being refused and +// leaving the deleted object's blob keys behind. A delete that arrives +// once Stop has begun is refused. +func TestS3ServerStopWaitsForAdminDeleteAndItsCleanup(t *testing.T) { + shortenHTTPShutdownTimeout(t, 5*time.Second) + st := store.NewMVCCStore() + coord := newFirstDispatchHoldingCoordinator(st) + s := NewS3Server(nil, "", st, coord, nil) + putObjectForAdminTest(t, s, "deletable", "k1", "hello") + cluster := &closeRecordingBlobCluster{fakeS3BlobCluster: newFakeS3BlobCluster()} + s.blobCluster = cluster + + coord.armed.Store(true) + deleteDone := make(chan error, 1) + go func() { + deleteDone <- s.AdminDeleteObject(context.Background(), fullAdminBucketsPrincipal(), "deletable", "k1") + }() + select { + case <-coord.entered: + case <-time.After(5 * time.Second): + t.Fatal("the admin delete never dispatched its transaction") + } + + stopDone := stopInBackground(s) + requireStopStillDraining(t, stopDone, cluster) + close(coord.release) + require.NoError(t, <-deleteDone) + waitStopped(t, stopDone) + require.Positive(t, coord.after.Load(), "the delete's blob cleanup must run, not be refused by the stopping server") + require.True(t, cluster.closed.Load()) + + err := s.AdminDeleteObject(context.Background(), fullAdminBucketsPrincipal(), "deletable", "k1") + require.ErrorIs(t, err, ErrAdminS3Stopping) +} + +func TestS3ServerTimedOutAdminDeleteRetainsCleanupReservation(t *testing.T) { + shortenHTTPShutdownTimeout(t, 10*time.Millisecond) + st := store.NewMVCCStore() + coord := newFirstDispatchHoldingCoordinator(st) + s := NewS3Server(nil, "", st, coord, nil) + putObjectForAdminTest(t, s, "deletable", "k1", "hello") + coord.armed.Store(true) + deleteDone := make(chan error, 1) + go func() { + deleteDone <- s.AdminDeleteObject(context.Background(), fullAdminBucketsPrincipal(), "deletable", "k1") + }() + select { + case <-coord.entered: + case <-time.After(time.Second): + t.Fatal("delete did not dispatch") + } + stopDone := stopInBackground(s) + select { + case <-s.reservedCleanups.ctx.Done(): + case <-time.After(time.Second): + t.Fatal("Stop did not reach reserved cleanup cancellation") + } + close(coord.release) + require.NoError(t, <-deleteDone) + waitStopped(t, stopDone) + require.Positive(t, coord.after.Load(), "late committed delete must clean its manifest blobs") +} diff --git a/adapter/s3_admin_objects.go b/adapter/s3_admin_objects.go index ca843a121..ead2b921f 100644 --- a/adapter/s3_admin_objects.go +++ b/adapter/s3_admin_objects.go @@ -77,6 +77,7 @@ var ErrAdminInvalidContinuationToken = errors.New("s3 admin: invalid continuatio // - ErrAdminNotLeader — follower // - ErrAdminBucketNotFound — bucket absent // - ErrAdminInvalidObjectKey — empty key +// - ErrAdminS3Stopping — Stop has begun func (s *S3Server) AdminDeleteObject(ctx context.Context, principal AdminPrincipal, bucket, key string) error { if !principal.Role.canWrite() { return ErrAdminForbidden @@ -90,10 +91,22 @@ func (s *S3Server) AdminDeleteObject(ctx context.Context, principal AdminPrincip if err := validateS3BucketName(bucket); err != nil { return errors.Wrapf(ErrAdminInvalidBucketName, "%s", err.Error()) } + // Counted like the other admin object operations so Stop waits for + // the delete before it closes the cleanup gate: the blob cleanup the + // delete schedules once it has committed must not be refused. + if err := s.enterAdminBlobUse(); err != nil { + return err + } + defer s.exitAdminBlobUse() + cleanup, err := s.reserveAdminCleanup() + if err != nil { + return err + } + defer cleanup.release() var cleanupManifest *s3ObjectManifest var generation uint64 - err := s.retryS3Mutation(ctx, func() error { + err = s.retryS3Mutation(ctx, func() error { manifest, gen, err := s.adminDeleteObjectTxn(ctx, bucket, key) if err != nil { return err @@ -106,7 +119,9 @@ func (s *S3Server) AdminDeleteObject(ctx context.Context, principal AdminPrincip return err //nolint:wrapcheck // sentinels propagate as-is; wrapped values already carry context. } if cleanupManifest != nil { - s.cleanupManifestBlobsAsync(bucket, generation, key, cleanupManifest) + cleanup.start(func(cleanupCtx context.Context) { + s.cleanupManifestBlobs(cleanupCtx, bucket, generation, key, cleanupManifest) + }) } return nil } @@ -185,6 +200,10 @@ func (s *S3Server) AdminPutObject(ctx context.Context, principal AdminPrincipal, if !s.isVerifiedS3Leader(ctx) { return ErrAdminNotLeader } + if err := s.enterAdminBlobUse(); err != nil { + return err + } + defer s.exitAdminBlobUse() if key == "" { return ErrAdminInvalidObjectKey } @@ -212,6 +231,11 @@ func (s *S3Server) AdminPutObject(ctx context.Context, principal AdminPrincipal, // the admin sentinels. It makes a single attempt: the chunk fan-out has no // idempotency token, so a retry would leak the first attempt's chunks. func (s *S3Server) adminPutObject(ctx context.Context, in *s3PutObjectInput, bucket, key string) error { + cleanup, err := s.reserveAdminCleanup() + if err != nil { + return err + } + defer cleanup.release() state, err := s.prepareS3PutObject(ctx, in, bucket, key) if err != nil { var respErr *s3ResponseError @@ -237,7 +261,9 @@ func (s *S3Server) adminPutObject(ctx context.Context, in *s3PutObjectInput, buc return errors.WithStack(err) } if state.previous != nil { - s.cleanupManifestBlobsAsync(bucket, state.meta.Generation, key, state.previous) + cleanup.start(func(cleanupCtx context.Context) { + s.cleanupManifestBlobs(cleanupCtx, bucket, state.meta.Generation, key, state.previous) + }) } return nil } @@ -293,12 +319,20 @@ func (s *S3Server) AdminGetObject(ctx context.Context, principal AdminPrincipal, return nil, AdminObject{}, errors.Wrapf(ErrAdminInvalidBucketName, "%s", err.Error()) } + // The body fetches chunks (offloaded ones through the blob peer + // cluster) until the caller closes it, so the registration is + // released by the body's Close, not on return. + if err := s.enterAdminBlobUse(); err != nil { + return nil, AdminObject{}, err + } manifest, generation, readTS, readPin, err := s.adminLoadObjectForRead(ctx, bucket, key) if err != nil { + s.exitAdminBlobUse() return nil, AdminObject{}, err } body := newS3AdminObjectReader(ctx, s, bucket, generation, key, manifest, readTS, readPin) + body.release = s.exitAdminBlobUse adminMeta := AdminObject{ Key: key, @@ -358,8 +392,10 @@ func (s *S3Server) adminLoadObjectForRead(ctx context.Context, bucket, key strin // // Lifecycle: holds a read-tracker pin (the readTS the manifest was // loaded at) so concurrent MVCC GC cannot reclaim chunks mid-read. -// The pin is released by Close. Calling Close more than once is -// safe (the underlying token's Release is idempotent on nil). +// The pin, and the server's admin blob-use registration that keeps +// Stop from closing the blob peer cluster under the read, are released +// by Close. Calling Close more than once is safe (both are cleared on +// the first call). type s3AdminObjectReader struct { ctx context.Context server *S3Server @@ -369,6 +405,9 @@ type s3AdminObjectReader struct { manifest *s3ObjectManifest readTS uint64 pin *kv.ActiveTimestampToken + // release ends the server's admin blob-use registration (see + // S3Server.enterAdminBlobUse); called once, by Close. + release func() // Cursor state. partIdx int // index into manifest.Parts @@ -470,6 +509,10 @@ func (r *s3AdminObjectReader) Close() error { r.pin.Release() r.pin = nil } + if r.release != nil { + r.release() + r.release = nil + } return nil } diff --git a/adapter/s3_cleanup_reservation.go b/adapter/s3_cleanup_reservation.go new file mode 100644 index 000000000..69289899e --- /dev/null +++ b/adapter/s3_cleanup_reservation.go @@ -0,0 +1,38 @@ +package adapter + +import ( + "context" +) + +// An admin mutation claims its cleanup slot before committing, so a timed +// out admin drain cannot close cleanup admission underneath that mutation. +type s3CleanupReservation struct { + s *S3Server + reservation *backgroundReservation +} + +func (s *S3Server) reserveAdminCleanup() (*s3CleanupReservation, error) { + reservation, ok := s.reservedCleanups.reserve() + if !ok { + return nil, ErrAdminS3Stopping + } + return &s3CleanupReservation{s: s, reservation: reservation}, nil +} + +func (r *s3CleanupReservation) release() { + r.reservation.release() +} + +func (r *s3CleanupReservation) start(run func(context.Context)) { + r.reservation.start(s3ManifestCleanupTimeout, func(ctx context.Context) { + // Unlike opportunistic cleanups this slot predates shutdown and is + // required by an admitted commit. Wait boundedly for the semaphore. + select { + case r.s.cleanupSem <- struct{}{}: + defer func() { <-r.s.cleanupSem }() + case <-ctx.Done(): + return + } + run(ctx) + }) +} diff --git a/adapter/s3_cleanup_shutdown_test.go b/adapter/s3_cleanup_shutdown_test.go new file mode 100644 index 000000000..a9388a621 --- /dev/null +++ b/adapter/s3_cleanup_shutdown_test.go @@ -0,0 +1,182 @@ +package adapter + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/bootjp/elastickv/kv" + "github.com/bootjp/elastickv/store" + "github.com/stretchr/testify/require" +) + +// cleanupBlockingCoordinator holds every Dispatch until release is closed +// or the request's context ends, the way a cleanup batch waits on Raft. +// unwind delays the return after a cancellation, the way a proposal takes a +// moment to give up; ignoreCancel keeps waiting for release regardless. +type cleanupBlockingCoordinator struct { + *localAdapterCoordinator + unwind time.Duration + ignoreCancel bool + returned atomic.Bool + dispatches atomic.Int64 + entered chan struct{} + enterOnce sync.Once + release chan struct{} + cancelled chan struct{} + cancelOnce sync.Once +} + +func newCleanupBlockingCoordinator(st store.MVCCStore) *cleanupBlockingCoordinator { + return &cleanupBlockingCoordinator{ + localAdapterCoordinator: newLocalAdapterCoordinator(st), + entered: make(chan struct{}), + release: make(chan struct{}), + cancelled: make(chan struct{}), + } +} + +func (c *cleanupBlockingCoordinator) Dispatch(ctx context.Context, req *kv.OperationGroup[kv.OP]) (*kv.CoordinateResponse, error) { + c.dispatches.Add(1) + defer c.returned.Store(true) + c.enterOnce.Do(func() { close(c.entered) }) + done := ctx.Done() + if c.ignoreCancel { + done = nil + } + select { + case <-c.release: + case <-done: + c.cancelOnce.Do(func() { close(c.cancelled) }) + time.Sleep(c.unwind) + return nil, ctx.Err() //nolint:wrapcheck // test double reports the cancellation as is. + } + return c.localAdapterCoordinator.Dispatch(ctx, req) +} + +// startBlockedManifestCleanup starts the background cleanup an overwrite +// or delete triggers and waits until its delete batch is in Dispatch. +func startBlockedManifestCleanup(t *testing.T, s *S3Server, coord *cleanupBlockingCoordinator) { + t.Helper() + s.cleanupManifestBlobsAsync("bucket", 1, "object", &s3ObjectManifest{ + UploadID: "upload", + Parts: []s3ObjectPart{{PartNo: 1, ChunkSizes: []uint64{1}}}, + }) + select { + case <-coord.entered: + case <-time.After(5 * time.Second): + t.Fatal("the cleanup never dispatched its delete batch") + } +} + +// TestS3ServerStopWaitsForCleanupBeforeClosingBlobCluster pins that a +// background cleanup started by an overwrite, delete or failed upload +// keeps Stop, and with it the blob cluster and the stores closed after +// Stop, until it finished, inside the graceful window. +func TestS3ServerStopWaitsForCleanupBeforeClosingBlobCluster(t *testing.T) { + shortenHTTPShutdownTimeout(t, 5*time.Second) + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + s := NewS3Server(nil, "", st, coord, nil) + cluster := &closeRecordingBlobCluster{fakeS3BlobCluster: newFakeS3BlobCluster()} + s.blobCluster = cluster + startBlockedManifestCleanup(t, s, coord) + + stopDone := stopInBackground(s) + requireStopStillDraining(t, stopDone, cluster) + close(coord.release) + waitStopped(t, stopDone) + require.True(t, cluster.closed.Load(), "the cluster is closed once the cleanup finished") +} + +// A cleanup still running when the bound expires holds Stop for the bound +// only; its context is then cancelled so it stops dispatching against +// stores that are about to close. +func TestS3ServerStopCancelsCleanupAfterBound(t *testing.T) { + shortenHTTPShutdownTimeout(t, 200*time.Millisecond) + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + t.Cleanup(func() { close(coord.release) }) + s := NewS3Server(nil, "", st, coord, nil) + startBlockedManifestCleanup(t, s, coord) + + start := time.Now() + s.Stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "the cleanup must be given its bound") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a cleanup past the bound") + select { + case <-coord.cancelled: + case <-time.After(5 * time.Second): + t.Fatal("the cleanup's context was not cancelled after the bound") + } +} + +// Once Stop has begun, a new background cleanup is not started. +func TestS3ServerStopRefusesLateCleanup(t *testing.T) { + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + close(coord.release) + s := NewS3Server(nil, "", st, coord, nil) + s.Stop() + + s.cleanupManifestBlobsAsync("bucket", 1, "object", &s3ObjectManifest{ + UploadID: "upload", + Parts: []s3ObjectPart{{PartNo: 1, ChunkSizes: []uint64{1}}}, + }) + time.Sleep(100 * time.Millisecond) + require.Zero(t, coord.dispatches.Load(), "a cleanup must not start after Stop") +} + +// A cleanup cancelled at the bound still has to unwind out of its dispatch. +// Stop must wait for that, briefly, before the blob cluster and the stores +// close underneath it. +func TestS3ServerStopJoinsCleanupAfterCancellingIt(t *testing.T) { + shortenHTTPShutdownTimeout(t, 100*time.Millisecond) + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + coord.unwind = 300 * time.Millisecond + t.Cleanup(func() { close(coord.release) }) + s := NewS3Server(nil, "", st, coord, nil) + cluster := &closeRecordingBlobCluster{fakeS3BlobCluster: newFakeS3BlobCluster()} + s.blobCluster = cluster + startBlockedManifestCleanup(t, s, coord) + + s.Stop() + require.True(t, coord.returned.Load(), "Stop returned while the cancelled cleanup was still in Dispatch") + require.Zero(t, s.cleanupWorkers.remaining(), "Stop returned before the cancelled cleanup exited") + require.True(t, cluster.closed.Load()) +} + +// A cleanup that does not observe its cancellation holds Stop for the +// bound and the join only. +func TestS3ServerStopBoundsJoinOfCleanupIgnoringCancellation(t *testing.T) { + shortenHTTPShutdownTimeout(t, 100*time.Millisecond) + shortenS3CleanupJoinTimeout(t, 200*time.Millisecond) + st := store.NewMVCCStore() + coord := newCleanupBlockingCoordinator(st) + coord.ignoreCancel = true + t.Cleanup(func() { close(coord.release) }) + s := NewS3Server(nil, "", st, coord, nil) + startBlockedManifestCleanup(t, s, coord) + + start := time.Now() + s.Stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 250*time.Millisecond, "the cancelled cleanup must be given its join") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a stuck cleanup past the join") + + // A second Stop does not spend the bound or the join again. + start = time.Now() + s.Stop() + require.Less(t, time.Since(start), 100*time.Millisecond) +} + +func shortenS3CleanupJoinTimeout(t *testing.T, d time.Duration) { + t.Helper() + old := s3CleanupJoinTimeout + s3CleanupJoinTimeout = d + t.Cleanup(func() { s3CleanupJoinTimeout = old }) +} diff --git a/adapter/s3_shutdown.go b/adapter/s3_shutdown.go new file mode 100644 index 000000000..a7cb113cb --- /dev/null +++ b/adapter/s3_shutdown.go @@ -0,0 +1,99 @@ +package adapter + +import ( + "context" + "log/slog" + "time" + + "github.com/cockroachdb/errors" +) + +// ErrAdminS3Stopping is returned by the admin object operations +// (AdminPutObject, AdminGetObject, AdminDeleteObject) once Stop has begun: +// the blob peer cluster and the cleanup gate are about to close, so the +// operation is refused rather than started against them. It is marked as +// ErrAdminNotLeader, so the admin bridge answers it with the not-leader +// response and the client retries against another node. +var ErrAdminS3Stopping = errors.Mark(errors.New("s3 admin: server is stopping"), ErrAdminNotLeader) + +// enterAdminBlobUse registers an admin object operation that may replicate +// or fetch chunk blobs through the blob peer cluster, or schedule a +// background cleanup. Stop waits, bounded, for every registered operation +// before it drains the cleanups and closes the cluster: the admin +// listener's object bridge (and the gRPC admin forwarder) call into this +// server outside its HTTP listener, so draining that listener does not +// cover them. Each successful call must be paired with exitAdminBlobUse. +func (s *S3Server) enterAdminBlobUse() error { + if s == nil || s.adminBlobUsers.enter() { + return nil + } + return errors.WithStack(ErrAdminS3Stopping) +} + +func (s *S3Server) exitAdminBlobUse() { + if s != nil { + s.adminBlobUsers.exit() + } +} + +// startCleanup runs one background cleanup (stale manifest blobs, upload +// parts, part blobs or upload data) on its own goroutine, under the +// cleanup semaphore and with s3ManifestCleanupTimeout. The cleanup is +// counted so Stop can join it before the blob cluster and the stores close, +// and its context derives from the server's cleanup context so Stop can +// cancel it once the bound has passed. A cleanup is skipped when the +// semaphore is saturated, as before, and once Stop has begun. +func (s *S3Server) startCleanup(run func(ctx context.Context)) { + select { + case s.cleanupSem <- struct{}{}: + default: + // Semaphore saturated; skip to avoid unbounded goroutine accumulation. + return + } + if !s.cleanupWorkers.enter() { + <-s.cleanupSem + slog.Warn("s3 server: background cleanup skipped, the server is stopping") + return + } + parent := s.cleanupCtx + if parent == nil { + parent = context.Background() + } + go func() { + defer func() { <-s.cleanupSem }() + defer s.cleanupWorkers.exit() + ctx, cancel := context.WithTimeout(parent, s3ManifestCleanupTimeout) + defer cancel() + run(ctx) + }() +} + +// s3CleanupJoinTimeout bounds how long Stop waits, after cancelling the +// background cleanups that outlived the shutdown bound, for them to unwind +// out of their dispatch or scan. A variable so tests can shorten it. +var s3CleanupJoinTimeout = 2 * time.Second + +// drainCleanups waits up to timeout for the background cleanups still +// running, then cancels the context of any that remain and waits, under +// s3CleanupJoinTimeout, for them to return, so none is still dispatching +// or scanning when the blob cluster and the stores close. New cleanups are +// refused from the moment the wait begins. +func (s *S3Server) drainCleanups(timeout time.Duration) { + finished := s.cleanupWorkers.drain(timeout) + if s.cancelCleanups != nil { + s.cancelCleanups() + } + if finished { + return + } + slog.Warn("s3 server: background cleanups still running after the shutdown bound, cancelling them", + slog.Int64("in_flight", s.cleanupWorkers.remaining()), + slog.Duration("timeout", timeout), + ) + if !s.cleanupWorkers.join(s3CleanupJoinTimeout) { + slog.Warn("s3 server: cancelled background cleanups did not return, closing the blob cluster", + slog.Int64("in_flight", s.cleanupWorkers.remaining()), + slog.Duration("timeout", s3CleanupJoinTimeout), + ) + } +} diff --git a/adapter/sqs.go b/adapter/sqs.go index 32300f063..df374d667 100644 --- a/adapter/sqs.go +++ b/adapter/sqs.go @@ -153,6 +153,7 @@ type SQSServer struct { store store.MVCCStore coordinator kv.Coordinator httpServer *http.Server + httpConns *httpConnTracker targetHandlers map[string]func(http.ResponseWriter, *http.Request) leaderSQS map[string]string region string @@ -215,6 +216,9 @@ type SQSServer struct { // nil on non-monitored fixtures; observeThrottleDecision is // nil-safe so the request path pays one branch when unwired. throttleObserver SQSThrottleObserver + // workers tracks the goroutines Run starts (retention reaper, + // throttle bucket sweep) so Stop can join them before returning. + workers sqsWorkers // adminObserver records the §3.6 admin purge / peek counters. // nil on non-monitored fixtures; the increment helpers are // nil-safe so an unwired server pays one branch. @@ -488,6 +492,7 @@ func NewSQSServer(listen net.Listener, st store.MVCCStore, coordinate kv.Coordin mux := http.NewServeMux() mux.HandleFunc("/", s.handle) s.httpServer = &http.Server{Handler: mux, ReadHeaderTimeout: time.Second} + s.httpConns = trackHTTPConns(s.httpServer) for _, opt := range opts { if opt != nil { opt(s) @@ -500,8 +505,9 @@ func (s *SQSServer) Run() error { s.startReaper(s.reaperCtx) // Throttle bucket idle-evict runs on a background ticker so the // request hot path never pays the O(N) sweep cost. Cleaned up by - // the same reaperCtx cancellation that stops the message reaper. - go s.throttle.runSweepLoop(s.reaperCtx) + // the same reaperCtx cancellation that stops the message reaper, + // and joined by Stop like the reaper. + s.workers.start(func() { s.throttle.runSweepLoop(s.reaperCtx) }) if s.listen == nil { // Listenless mode: the SQS adapter was constructed without a // public HTTP listener (--sqsAddress empty). Admin endpoints @@ -528,9 +534,19 @@ func (s *SQSServer) Stop() { // http.Server.Shutdown returns immediately when Serve was // never called (listenless mode — no public listener was // constructed), so this is a no-op for admin-only - // deployments. The branch is still gated on httpServer - // being non-nil because Shutdown panics on a nil receiver. - _ = s.httpServer.Shutdown(context.Background()) + // deployments. With a listener, shutdownHTTPServer drains + // in-flight requests for at most httpShutdownTimeout and then + // closes whatever is still connected. + shutdownHTTPServer(s.httpServer, s.httpConns, "sqs") + } + // The reaper and the bucket sweep observe the cancelled reaperCtx, + // but a reaper pass can still be inside a store scan or a Raft + // dispatch; give it its bounded window before the caller closes the + // stores. In listenless mode nothing above waited at all. + if !s.workers.stop(sqsWorkerStopTimeout) { + slog.Warn("sqs server: background workers still running after the shutdown bound, continuing", + slog.Duration("timeout", sqsWorkerStopTimeout), + ) } } diff --git a/adapter/sqs_reaper.go b/adapter/sqs_reaper.go index 2877dda1c..13c4931a1 100644 --- a/adapter/sqs_reaper.go +++ b/adapter/sqs_reaper.go @@ -34,12 +34,12 @@ const ( // startReaper kicks off the retention sweeper on the leader. It is // safe to call multiple times; the first call wins and subsequent // calls are no-ops. Stop() cancels the context so the goroutine -// returns promptly. +// returns promptly, and joins it (bounded) before returning. func (s *SQSServer) startReaper(ctx context.Context) { if s == nil || s.coordinator == nil || s.store == nil { return } - go s.runReaper(ctx) + s.workers.start(func() { s.runReaper(ctx) }) } func (s *SQSServer) runReaper(ctx context.Context) { diff --git a/adapter/sqs_shutdown.go b/adapter/sqs_shutdown.go new file mode 100644 index 000000000..a10616662 --- /dev/null +++ b/adapter/sqs_shutdown.go @@ -0,0 +1,81 @@ +package adapter + +import ( + "sync" + "time" +) + +// sqsWorkerStopTimeout bounds how long Stop waits for the background +// workers Run started (the retention reaper and the throttle bucket +// sweep) to return after their context was cancelled. A reaper pass can +// be inside a store scan or a Raft dispatch when the shutdown starts, and +// in a listenless (admin-only) deployment nothing else holds the errgroup +// task open, so without the join the deferred cleanup could close the +// stores underneath it. A variable so tests can shorten it. +var sqsWorkerStopTimeout = 10 * time.Second + +// sqsWorkers tracks the SQS server's background goroutines so Stop can +// join them, bounded, and refuses new ones once Stop has begun (a Run that +// races a Stop must not start a worker the join has already missed). +type sqsWorkers struct { + mu sync.Mutex + stopping bool + wg sync.WaitGroup + + // stopOnce fixes, on the first stop, the deadline every later or + // concurrent stop waits until, and starts the one goroutine that waits + // for the workers (done closes once they all returned). + stopOnce sync.Once + stopUntil time.Time + done chan struct{} +} + +// start runs fn in a tracked goroutine and reports whether it did; it +// refuses once stop has been called. +func (w *sqsWorkers) start(fn func()) bool { + w.mu.Lock() + defer w.mu.Unlock() + if w.stopping { + return false + } + w.wg.Go(fn) + return true +} + +// stop refuses new workers and waits, until timeout after the first stop, +// for the running ones, reporting whether they all returned. Stop runs +// twice on the normal path, from the context waiter and deferred after +// Run returns, possibly at the same time: a call made while an earlier one +// is still waiting waits until the same deadline (whichever returns first +// lets its caller close the stores, so neither may return early), and one +// made after it reports at once without spending the bound again. +func (w *sqsWorkers) stop(timeout time.Duration) bool { + w.stopOnce.Do(func() { + w.mu.Lock() + w.stopping = true + w.mu.Unlock() + w.stopUntil = time.Now().Add(timeout) + w.done = make(chan struct{}) + go func() { + w.wg.Wait() + close(w.done) + }() + }) + wait := time.Until(w.stopUntil) + if wait <= 0 { + select { + case <-w.done: + return true + default: + return false + } + } + timer := time.NewTimer(wait) + defer timer.Stop() + select { + case <-w.done: + return true + case <-timer.C: + return false + } +} diff --git a/adapter/sqs_shutdown_test.go b/adapter/sqs_shutdown_test.go new file mode 100644 index 000000000..634e8cecc --- /dev/null +++ b/adapter/sqs_shutdown_test.go @@ -0,0 +1,116 @@ +package adapter + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func shortenSQSWorkerStopTimeout(t *testing.T, d time.Duration) { + t.Helper() + old := sqsWorkerStopTimeout + sqsWorkerStopTimeout = d + t.Cleanup(func() { sqsWorkerStopTimeout = old }) +} + +// TestSQSServerStopJoinsBackgroundWorkers pins that Stop does not return +// while a background worker Run started (the retention reaper inside a +// store scan or a Raft dispatch) is still running: in a listenless +// deployment the HTTP shutdown returns at once, Run unblocks on the same +// cancellation, and the errgroup would otherwise let the deferred cleanup +// close the stores underneath the worker. The join is bounded, and only +// the first Stop waits. +func TestSQSServerStopJoinsBackgroundWorkers(t *testing.T) { + shortenSQSWorkerStopTimeout(t, 5*time.Second) + s := NewSQSServer(nil, nil, nil) + release := make(chan struct{}) + finished := make(chan struct{}) + require.True(t, s.workers.start(func() { + <-s.reaperCtx.Done() + <-release + close(finished) + })) + go func() { + time.Sleep(100 * time.Millisecond) + close(release) + }() + + start := time.Now() + s.Stop() + require.GreaterOrEqual(t, time.Since(start), 80*time.Millisecond, "Stop must wait for the running worker") + select { + case <-finished: + default: + t.Fatal("Stop returned before the worker finished") + } + require.False(t, s.workers.start(func() {}), "no worker may start once Stop began") +} + +func TestSQSServerStopGivesUpOnStuckWorkerAfterBound(t *testing.T) { + shortenSQSWorkerStopTimeout(t, 150*time.Millisecond) + s := NewSQSServer(nil, nil, nil) + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + require.True(t, s.workers.start(func() { <-release })) + + start := time.Now() + s.Stop() + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 100*time.Millisecond, "the worker must be given its bound") + require.Less(t, elapsed, 3*time.Second, "Stop must not wait on a worker that never finishes") + + start = time.Now() + s.Stop() + require.Less(t, time.Since(start), 50*time.Millisecond, "second Stop must not wait the bound again") +} + +// A listenless Run (admin-only deployment) returns once Stop cancelled it, +// with its sweep worker joined. +func TestSQSServerListenlessRunReturnsAfterStop(t *testing.T) { + shortenSQSWorkerStopTimeout(t, 5*time.Second) + s := NewSQSServer(nil, nil, nil) + runDone := make(chan error, 1) + go func() { runDone <- s.Run() }() + time.Sleep(20 * time.Millisecond) + s.Stop() + select { + case err := <-runDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Run did not return after Stop") + } +} + +// Stop runs from two places at once (the context waiter and the deferred +// call after Run returns). A Stop that starts while the first is still +// joining the workers must wait for them as well: returning at once lets +// its caller close the stores under a worker that is still running. +func TestSQSServerConcurrentStopsShareTheWorkerJoin(t *testing.T) { + shortenSQSWorkerStopTimeout(t, 5*time.Second) + s := NewSQSServer(nil, nil, nil) + release := make(chan struct{}) + finished := make(chan struct{}) + require.True(t, s.workers.start(func() { + <-release + close(finished) + })) + firstStop := make(chan struct{}) + go func() { + s.Stop() + close(firstStop) + }() + time.Sleep(100 * time.Millisecond) + go func() { + time.Sleep(200 * time.Millisecond) + close(release) + }() + + s.Stop() + select { + case <-finished: + default: + t.Fatal("a concurrent Stop returned before the worker finished") + } + <-firstStop +} diff --git a/docs/filesystem.md b/docs/filesystem.md index 1aaea34db..c1d5373a4 100644 --- a/docs/filesystem.md +++ b/docs/filesystem.md @@ -189,22 +189,32 @@ When `--filesystemMount` is set, the server: group, on a multi-group cluster it can span several groups. Concurrent root creation from several nodes is resolved by write-conflict retries; the losers find the root the first node created. -3. Replays filesystem recovery state: safely aborted create/delete intents are +3. Checks the mount point for a FUSE mount left behind by a previous server + that died without unmounting (the mounts table lists a `fuse.elastickv` + entry, the type the server mounts with, for the path and `stat` fails + with `ENOTCONN`). Such a dead mount is detached with `fusermount3 -u -z` + (or `fusermount`, or a lazy `umount` when neither helper exists) and + logged as `detached stale filesystem FUSE mount left by a previous + process`. A live mount, or a filesystem of any other type at the path, + FUSE or not, is never touched. When `/proc/self/mounts` cannot be read + (no `/proc` in a minimal container), the check is skipped with a warning + and the mount proceeds as before. +4. Replays filesystem recovery state: safely aborted create/delete intents are cleared and unfinished file moves are resumed, in batches of 256, until nothing is left. The whole replay must finish within 5 minutes; if it does not, the mount fails and the server exits. Recovery is resumable, so restarting the server continues where the previous attempt stopped. -4. Mounts the directory with `fusermount3`, waits for the kernel to confirm +5. Mounts the directory with `fusermount3`, waits for the kernel to confirm the mount, and logs `filesystem FUSE mounted` with the mount point and client ID. -5. Starts the placement-metrics collector and the lease reaper. +6. Starts the placement-metrics collector and the lease reaper. Because the listeners come up before the mount, a readiness check on the gRPC or Redis port is not enough for clients that use the mount. Check the mount point itself, and issue an I/O request against it rather than only -consulting the mount table, so that a mount left behind by a previous server -process (see [Stopping and restarting](#stopping-and-restarting)) does not -pass the check: +consulting the mount table: a mount left behind by a previous server process +(see [Stopping and restarting](#stopping-and-restarting)) stays in the mount +table until step 3 detaches it, and the listeners are already up by then: ```bash until mountpoint -q /mnt/elastickv && ls /mnt/elastickv >/dev/null 2>&1; do @@ -217,25 +227,69 @@ filesystem it was asked to expose. ## Stopping and restarting -The server has no signal handler: `SIGTERM`, `SIGINT`, and `SIGKILL` all end -the process immediately, and the kernel keeps the now-dead mount attached. -Until it is detached, every access to the mount point fails with -`Transport endpoint is not connected`, and a restarted server exits with -`fusermount3: failed to access mountpoint ...: Transport endpoint is not -connected`. The orderly unmount built into the server only runs when the -server shuts itself down (a failed component or the memory watchdog). - -Stop and restart a mounted node like this: +`SIGTERM` and `SIGINT` stop the server gracefully: the first signal is logged +as `shutdown signal received, stopping`, the FUSE mount is unmounted, the +listeners drain, and the WAL is synced before the process exits with status +0. That status is for a clean shutdown, one whose only errors are the +cancellation itself; a component that fails while stopping still makes the +process exit with an error after the same cleanup. Repeated +`SIGTERM`/`SIGINT` while that shutdown is running are logged and ignored so +a supervisor's second stop attempt cannot cut the cleanup short. +This is the same path the server takes when a component fails or the memory +watchdog trips (which still exits with status 2). If the unmount fails because +a client still holds files on the mount open (`EBUSY`), the server detaches +the mount lazily instead: new lookups see the plain directory at once, and +the server stops serving, so a request still in flight, or one made through +a handle that is still open, returns an error instead of running against +stores that are about to close. The server waits at most 10 seconds in total +for the unmount and for the kernel to release the connection before +finishing its shutdown, so a stop never hangs on a busy mount or on a +request stuck in the backend. That lazy detach, like the +startup check, only ever acts on a mount the mounts table lists with the +server's own type (`fuse.elastickv`): if the kernel mount is already gone by +then, whatever filesystem the mount point sits on, another FUSE filesystem +included, is left alone. The gRPC listeners likewise drain +in-flight calls for at most 10 seconds before the remaining long-lived +streams (catalog watches, Raft transport streams) are closed, so a stop +never waits on a peer that stays connected. The DynamoDB, S3, and SQS HTTP +listeners follow the same rule: in-flight requests get at most 10 seconds to +finish, then the remaining connections are closed, so a client that stalls +mid-request (for example an S3 upload that never sends its body) cannot hold +the shutdown open either. The Redis listener closes its connections at once +and then waits at most 10 seconds for the commands that were already +executing (for example a write waiting on Raft) before the stores behind them +are closed. + +A stop requested while the server is still opening its Raft/Pebble stores at +startup (for example during a long snapshot restore) takes effect once the +open finishes; repeated signals are ignored then too. If the open never +returns, for instance because the data filesystem has stopped answering, only +`SIGKILL` ends the process. + +`SIGKILL`, an OOM kill, or a crash cannot run that cleanup, and the kernel +keeps the now-dead mount attached. Until it is detached, every access to the +mount point fails with `Transport endpoint is not connected`. A restarted +server detaches such a dead mount itself before mounting (see step 3 of the +[startup sequence](#startup-sequence)) and logs `detached stale filesystem +FUSE mount left by a previous process`, so an ordinary restart needs no +manual step: ```bash -kill "$(pidof elastickv)" # or your supervisor's stop action -fusermount3 -uz /mnt/elastickv # detach the stale mount (umount -l also works) +kill "$(pidof elastickv)" # SIGTERM: unmounts, syncs, exits 0 # start the server again with the same flags ``` -Put the `fusermount3 -uz` step in the service's `ExecStopPost` (systemd) or -the equivalent hook of your supervisor so an automatic restart does not loop -on the stale mount. Detaching the mount loses nothing that a completed +To free the mount point without restarting the server (for example before +reusing the directory for something else), detach it by hand: + +```bash +fusermount3 -uz /mnt/elastickv # umount -l also works +``` + +The same command in a supervisor's post-stop hook (`ExecStopPost` under +systemd) is still a reasonable belt-and-braces measure for hosts where the +server runs as a user that cannot detach the mount, but it is no longer +required for restarts. Detaching the mount loses nothing that a completed system call wrote: every `write(2)` that returned is committed in the store, and open handles on the dead mount are released by the lease reaper. The one exception is `mmap`: stores into a shared mapping sit in the kernel page @@ -426,8 +480,11 @@ filesystem backup adapter exists, there are two options: | Server exits with `mount filesystem frontend: ...` naming `fusermount3` or `fusermount` as not found in `$PATH` | `fuse3` is not installed or not on `PATH`. Install it, or run the server outside the distroless image. | | Server exits with `initialize filesystem root: ... leader not found` | The node could not reach a Raft leader at startup. Start or repair the cluster first; for per-node joins, mount on the bootstrap node first and on peers after they joined. | | `mkdir: cannot create directory: Permission denied` right after the first mount | The root directory is owned by UID 0 with mode `0755`. Start with `--filesystemRootUID` / `--filesystemRootGID` set to the writing user (only effective before the root exists). If the root already exists, repair it as described under [Single node](#single-node): restart one server as root and `chown` the mount point from a root shell; a root shell cannot reach a mount made by another user, and that user cannot `chown` a root-owned directory. | -| `Transport endpoint is not connected` on the mount point | The server process stopped (any signal ends it without unmounting). Run `fusermount3 -uz ` (or `umount -l`), then restart the server. | -| Restart exits with `fusermount3: failed to access mountpoint ...: Transport endpoint is not connected` | The previous mount is still attached. Detach it with `fusermount3 -uz` first; see [Stopping and restarting](#stopping-and-restarting). | +| `Transport endpoint is not connected` on the mount point | The server process died without unmounting (`SIGKILL`, OOM kill, or a crash; `SIGTERM`/`SIGINT` unmount cleanly). Restart the server: it detaches the dead mount before mounting. To free the directory without a restart, run `fusermount3 -uz ` (or `umount -l`). | +| Restart exits with `detach stale filesystem FUSE mount: ...` | The server found a dead FUSE mount at the path but could not detach it: neither `fusermount3` nor `fusermount` is on `PATH` and the process lacks `CAP_SYS_ADMIN` for a direct lazy unmount, or the helper failed (its output is in the error). Detach it as root with `fusermount3 -uz` / `umount -l`, then restart; see [Stopping and restarting](#stopping-and-restarting). | +| Restart exits with `detach stale filesystem FUSE mount: ... stat FUSE mount at ...: no answer within 10s` | A FUSE server is still attached to the mount point but not answering (a hung previous process). Find and stop that process (`fuser -m ` or `ps`), then restart. | +| Restart exits with `detach stale filesystem FUSE mount: ... resolve FUSE mount point ...: no answer within 10s` | A parent directory of the mount point sits on a filesystem that is not answering (a wedged FUSE or network mount above the mount point), so the server could not even resolve the path. Repair or detach that outer filesystem, or move the mount point off it, then restart. | +| Restart exits with `fusermount3: failed to access mountpoint ...: Transport endpoint is not connected` | The dead mount at the path is not one the server will touch: the mounts table does not list it as `fuse.elastickv` (for example a `fuseblk`, a non-FUSE filesystem, or another FUSE filesystem such as `fuse.sshfs` stacked over the mount point). Inspect `/proc/self/mounts` and detach it by hand. | | `Resource temporarily unavailable` (`EAGAIN`) | Another mount modified the same file or directory at the same time, or the file is being moved between groups. Retry the call, with two exceptions: never retry `close`, and do not retry an `O_APPEND` write that other mounts may also be appending to. See [Concurrency](#semantics-worth-knowing) for what a retry reports when the first attempt did apply. | | `Input/output error` (`EIO`) | A backend error that is not a write conflict, for example a lost connection to the leader. Check the server log; retry once the cluster is healthy. | | `ENOENT` opening a file that exists | The file was replaced by `rename` from another mount between lookup and open. Retry. | @@ -443,7 +500,13 @@ filesystem backup adapter exists, there are two options: runs only the latter). The kernel-mount end-to-end tests in `internal/filesystem/fuseadapter` and in `main` mount a real FUSE filesystem; they skip when `/dev/fuse` or `fusermount3` is unavailable, and - `ELASTICKV_REQUIRE_FUSE=1` turns that skip into a failure. + `ELASTICKV_REQUIRE_FUSE=1` turns that skip into a failure. Among them, + `TestShutdownSignalUnmountsFilesystem` (package `main`) sends the test + process a real `SIGTERM` and checks that the mount is gone afterwards, and + `TestMountDetachesStaleMountLeftByKilledServer` (`fuseadapter`) kills a + helper process that holds a mount, then mounts over the dangling mount it + left behind. When the tests run as root they mount directly through + `mount(2)` and do not need `fusermount3`. - The Jepsen linearizable-register workload drives real mounts on every node of a cluster (`jepsen/src/elastickv/filesystem_workload.clj`). See "Running Jepsen tests" in the README; `scripts/run-jepsen-local.sh` runs it diff --git a/internal/filesystem/fuseadapter/abandoned_mount_test.go b/internal/filesystem/fuseadapter/abandoned_mount_test.go new file mode 100644 index 000000000..d11d524e8 --- /dev/null +++ b/internal/filesystem/fuseadapter/abandoned_mount_test.go @@ -0,0 +1,113 @@ +//go:build linux + +package fuseadapter + +import ( + "os" + "testing" + "time" + + "github.com/bootjp/elastickv/internal/filesystem/fstest" + "github.com/hanwen/go-fuse/v2/fuse" + "github.com/stretchr/testify/require" +) + +// TestReleaseAbandonedMountReturns mounts for real and hands the server to +// releaseAbandonedMount the way newServerContext does after a cancellation. +// go-fuse's Unmount waits for an event loop that only Serve runs, so a +// release that skipped Serve would never return; the mount point must also +// be a plain directory again afterwards, and a request in flight on the +// late mount must have been cancelled. +func TestReleaseAbandonedMountReturns(t *testing.T) { + if _, err := os.Stat("/dev/fuse"); err != nil { + if os.Getenv("ELASTICKV_REQUIRE_FUSE") != "" { + t.Fatalf("no /dev/fuse: %v", err) + } + t.Skipf("no /dev/fuse: %v", err) + } + svc, err := fstest.NewMemoryService() + require.NoError(t, err) + adapter := New(svc, []byte("abandoned")) + t.Cleanup(adapter.Close) + mountPoint := t.TempDir() + opts := defaultMountOptions(&fuse.MountOptions{DirectMount: os.Geteuid() == 0}) + fs := NewRawFileSystem(adapter) + server, err := fuse.NewServer(fs, mountPoint, opts) + if err != nil { + if os.Getenv("ELASTICKV_REQUIRE_FUSE") != "" { + t.Fatalf("FUSE mount unavailable: %v", err) + } + t.Skipf("FUSE mount unavailable: %v", err) + } + results := make(chan newServerResult, 1) + results <- newServerResult{server: server} + // A request a client sent to the late mount before it is released. + inFlight, done := fs.requestContext(nil) + defer done() + + released := make(chan struct{}) + go func() { + releaseAbandonedMount(results, fs, mountPoint, fuseTypeFor(opts.Name)) + close(released) + }() + select { + case <-released: + case <-time.After(15 * time.Second): + t.Fatal("releaseAbandonedMount did not return; Unmount is waiting for a serve loop that never ran") + } + // The release cancels the requests in flight, so a handler blocked in + // the backend returns before the stores behind it are closed. + require.Error(t, inFlight.Err(), "the release must cancel the requests in flight on the late mount") + + entries, err := readMountTable(mountTablePath) + require.NoError(t, err) + for _, entry := range entries { + require.NotEqual(t, mountPoint, entry.mountPoint, "abandoned mount is still attached") + } +} + +// TestMountContextTypeFollowsMountName mounts for real with an FsName that +// differs from Name. go-fuse records the mount as "fuse."+Name (the +// subtype; FsName only fills the source column), so the type the server +// remembers for its stale-mount check and lazy detach must be built from +// Name: built from FsName it would never match the server's own mount. +func TestMountContextTypeFollowsMountName(t *testing.T) { + if _, err := os.Stat("/dev/fuse"); err != nil { + if os.Getenv("ELASTICKV_REQUIRE_FUSE") != "" { + t.Fatalf("no /dev/fuse: %v", err) + } + t.Skipf("no /dev/fuse: %v", err) + } + svc, err := fstest.NewMemoryService() + require.NoError(t, err) + uid, gid := uint32(os.Getuid()), uint32(os.Getgid()) //nolint:gosec // test process ids fit uint32. + require.NoError(t, svc.InitializeRoot(t.Context(), 0o755, uid, gid)) + mountPoint := t.TempDir() + server, err := MountContext(t.Context(), mountPoint, New(svc, []byte("subtype")), &fuse.MountOptions{ + DirectMount: os.Geteuid() == 0, + FsName: "elastickv-source", + Name: "elastickv-subtype", + }) + if err != nil { + if os.Getenv("ELASTICKV_REQUIRE_FUSE") != "" { + t.Fatalf("FUSE mount unavailable: %v", err) + } + t.Skipf("FUSE mount unavailable: %v", err) + } + serveDone := make(chan struct{}) + go func() { + defer close(serveDone) + server.Serve() + }() + t.Cleanup(func() { + require.NoError(t, server.Unmount()) + <-serveDone + }) + require.NoError(t, server.WaitMount()) + + top, found, err := topmostMount(t.Context(), mountTablePath, mountPoint) + require.NoError(t, err) + require.True(t, found, "the mount must be listed") + require.Equal(t, "fuse.elastickv-subtype", top.fsType, "go-fuse records the subtype as the type") + require.Equal(t, top.fsType, server.fsType, "the server must remember the type the kernel lists") +} diff --git a/internal/filesystem/fuseadapter/detach_helper_lookup_test.go b/internal/filesystem/fuseadapter/detach_helper_lookup_test.go new file mode 100644 index 000000000..72ed5bada --- /dev/null +++ b/internal/filesystem/fuseadapter/detach_helper_lookup_test.go @@ -0,0 +1,72 @@ +package fuseadapter + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// TestDetachFUSEMountBoundsHelperLookup stands in a helper lookup that never +// returns, as exec.LookPath does when a PATH directory sits on a wedged FUSE +// or network filesystem. The lookup must be bounded like the helper itself, +// so the kernel fallback is still attempted inside the caller's budget (here +// on a plain directory, so it fails with its own error, not the deadline's). +func TestDetachFUSEMountBoundsHelperLookup(t *testing.T) { + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + block := make(chan struct{}) + t.Cleanup(func() { close(block) }) + old := lookupMountHelperFn + lookupMountHelperFn = func(string) (string, error) { + <-block + return "", nil + } + t.Cleanup(func() { lookupMountHelperFn = old }) + + budget := 2 * time.Second + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + start := time.Now() + method, err := detachFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName)) + elapsed := time.Since(start) + require.Error(t, err) + require.Empty(t, method) + require.NotErrorIs(t, err, context.DeadlineExceeded, "the hung lookups must not pre-empt the kernel fallback") + require.ErrorContains(t, err, lazyUnmountMethod) + require.Less(t, elapsed, budget, "the fallback must run inside the caller's budget") +} + +// TestDetachFUSEMountRechecksAfterHelperLookup covers our mount going away +// while the first helper is still being looked up, which exposes a foreign +// FUSE mount at the path. fusermount3 -u -z acts on whatever FUSE +// filesystem the mount point names, so the mounts table must be checked +// again after every lookup, right before the helper runs. +func TestDetachFUSEMountRechecksAfterHelperLookup(t *testing.T) { + mountPoint := t.TempDir() + table := useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + helperDir := t.TempDir() + marker := filepath.Join(helperDir, "helper-ran") + helper := filepath.Join(helperDir, "fusermount3") + require.NoError(t, os.WriteFile(helper, []byte("#!/bin/sh\n: > "+marker+"\nexit 0\n"), 0o700)) //nolint:gosec // the helper stand-in must be executable. + old := lookupMountHelperFn + lookupMountHelperFn = func(string) (string, error) { + // The plain unmount finishes while the lookup runs and a foreign + // FUSE filesystem is now on top at the path. + // It runs on the lookup goroutine, so it reports with Errorf. + if err := os.WriteFile(table, []byte(mountTableLine("sshfs#host:/", mountPoint, "fuse.sshfs")), 0o600); err != nil { + t.Errorf("rewrite mounts table: %v", err) + } + return helper, nil + } + t.Cleanup(func() { lookupMountHelperFn = old }) + + method, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + require.ErrorIs(t, err, errNoFUSEMount) + require.Empty(t, method) + require.NotContains(t, err.Error(), lazyUnmountMethod) + require.NoFileExists(t, marker, "no helper may run against the exposed foreign mount") +} diff --git a/internal/filesystem/fuseadapter/detach_helper_recheck_test.go b/internal/filesystem/fuseadapter/detach_helper_recheck_test.go new file mode 100644 index 000000000..0dea6a003 --- /dev/null +++ b/internal/filesystem/fuseadapter/detach_helper_recheck_test.go @@ -0,0 +1,36 @@ +package fuseadapter + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestDetachFUSEMountRechecksBeforeSecondHelper covers fusermount3 detaching +// our mount and still reporting a failure (a descendant holding its output +// pipe past WaitDelay, for example), which exposes a foreign FUSE mount at +// the path. The legacy fusermount helper accepts any FUSE filesystem, so it +// must not run once the mounts table no longer shows our mount on top. +func TestDetachFUSEMountRechecksBeforeSecondHelper(t *testing.T) { + mountPoint := t.TempDir() + table := useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + helperDir := t.TempDir() + replacement := filepath.Join(helperDir, "table.after") + require.NoError(t, os.WriteFile(replacement, []byte(mountTableLine("sshfs#host:/", mountPoint, "fuse.sshfs")), 0o600)) + first := "#!/bin/sh\n/bin/cp " + replacement + " " + table + "\nexit 1\n" + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "fusermount3"), []byte(first), 0o700)) //nolint:gosec // the helper stand-in must be executable. + marker := filepath.Join(helperDir, "second-ran") + second := "#!/bin/sh\n: > " + marker + "\nexit 0\n" + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "fusermount"), []byte(second), 0o700)) //nolint:gosec // the helper stand-in must be executable. + onlyMountHelpersIn(t, helperDir) + + method, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + require.ErrorIs(t, err, errNoFUSEMount) + require.ErrorContains(t, err, "fusermount3 -u -z", "the first helper's failure stays in the chain") + require.NotContains(t, err.Error(), lazyUnmountMethod) + require.Empty(t, method) + require.NoFileExists(t, marker, "the second helper must not run against the exposed foreign mount") +} diff --git a/internal/filesystem/fuseadapter/helper_process_test.go b/internal/filesystem/fuseadapter/helper_process_test.go new file mode 100644 index 000000000..68a4701b7 --- /dev/null +++ b/internal/filesystem/fuseadapter/helper_process_test.go @@ -0,0 +1,36 @@ +//go:build linux || darwin || freebsd + +package fuseadapter + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestUnmountHelperCancellationKillsLateDescendant(t *testing.T) { + dir := t.TempDir() + helper, ready, late := filepath.Join(dir, "helper"), filepath.Join(dir, "ready"), filepath.Join(dir, "late") + script := fmt.Sprintf("#!/bin/sh\n( /bin/sleep 1; printf late > '%s' ) &\nprintf ready > '%s'\nwait\n", late, ready) + require.NoError(t, os.WriteFile(helper, []byte(script), 0o700)) //nolint:gosec // Executable helper fixture, isolated in the test's temporary directory. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := make(chan error, 1) + go func() { done <- runUnmountHelper(ctx, helper, dir, false) }() + require.Eventually(t, func() bool { _, err := os.Stat(ready); return err == nil }, time.Second, time.Millisecond) + cancel() + select { + case err := <-done: + require.Error(t, err) + case <-time.After(time.Second): + t.Fatal("unmount helper was not reaped after cancellation") + } + time.Sleep(1100 * time.Millisecond) + _, err := os.Stat(late) + require.ErrorIs(t, err, os.ErrNotExist, "an old helper descendant must not act after a replacement mount can appear") +} diff --git a/internal/filesystem/fuseadapter/helper_process_unix.go b/internal/filesystem/fuseadapter/helper_process_unix.go new file mode 100644 index 000000000..536563996 --- /dev/null +++ b/internal/filesystem/fuseadapter/helper_process_unix.go @@ -0,0 +1,26 @@ +//go:build linux || darwin || freebsd + +package fuseadapter + +import ( + "os/exec" + "syscall" + + "github.com/cockroachdb/errors" +) + +func isolateMountHelper(cmd *exec.Cmd) func() { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + kill := func() error { + if cmd.Process == nil { + return nil + } + err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + if errors.Is(err, syscall.ESRCH) { + return nil + } + return errors.WithStack(err) + } + cmd.Cancel = kill + return func() { _ = kill() } +} diff --git a/internal/filesystem/fuseadapter/mount_context_test.go b/internal/filesystem/fuseadapter/mount_context_test.go new file mode 100644 index 000000000..08a6b3dbf --- /dev/null +++ b/internal/filesystem/fuseadapter/mount_context_test.go @@ -0,0 +1,28 @@ +package fuseadapter_test + +import ( + "context" + "testing" + + "github.com/bootjp/elastickv/internal/filesystem/fstest" + "github.com/bootjp/elastickv/internal/filesystem/fuseadapter" + "github.com/stretchr/testify/require" +) + +// TestMountContextAbortsWhenCancelled pins that a shutdown requested while +// the filesystem is still starting stops MountContext before any pre-mount +// work (stale-mount detach, intent recovery) instead of running it to +// completion. No kernel mount is attempted, so no FUSE is needed. +func TestMountContextAbortsWhenCancelled(t *testing.T) { + svc, err := fstest.NewMemoryService() + require.NoError(t, err) + adapter := fuseadapter.New(svc, []byte("cancelled")) + t.Cleanup(adapter.Close) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + server, err := fuseadapter.MountContext(ctx, t.TempDir(), adapter, nil) + require.ErrorIs(t, err, context.Canceled) + require.ErrorContains(t, err, "aborted before start") + require.Nil(t, server) +} diff --git a/internal/filesystem/fuseadapter/mount_e2e_test.go b/internal/filesystem/fuseadapter/mount_e2e_test.go index 177e89bb8..41481f139 100644 --- a/internal/filesystem/fuseadapter/mount_e2e_test.go +++ b/internal/filesystem/fuseadapter/mount_e2e_test.go @@ -1,3 +1,9 @@ +//go:build linux + +// The kernel-mount tests drive a real /dev/fuse mount and read Linux statfs +// fields, so they only build on Linux; elsewhere the package's portable +// tests still run. + package fuseadapter_test import ( @@ -5,6 +11,7 @@ import ( "path/filepath" "sort" "strconv" + "sync" "syscall" "testing" "time" @@ -26,6 +33,28 @@ const ( // FUSE mount. Environments without a usable /dev/fuse skip unless // ELASTICKV_REQUIRE_FUSE is set, in which case a mount failure is fatal. func mountMemoryFilesystem(t *testing.T) string { + t.Helper() + mountPoint := t.TempDir() + mountMemoryFilesystemAt(t, mountPoint) + return mountPoint +} + +// mountMemoryFilesystemAt mounts a fresh in-memory filesystem at an existing +// directory and unmounts it when the test ends. +func mountMemoryFilesystemAt(t *testing.T, mountPoint string) { + t.Helper() + mountMemoryFilesystemServer(t, mountPoint) +} + +// e2eMount is a kernel mount owned by a test. unmount is idempotent so a +// test may unmount early and let the cleanup run harmlessly afterwards. +type e2eMount struct { + server *fuseadapter.Server + serveDone <-chan struct{} + unmount func() error +} + +func mountMemoryFilesystemServer(t *testing.T, mountPoint string) *e2eMount { t.Helper() if _, err := os.Stat("/dev/fuse"); err != nil { skipOrFail(t, "no /dev/fuse: %v", err) @@ -35,7 +64,6 @@ func mountMemoryFilesystem(t *testing.T) string { uid, gid := uint32(os.Getuid()), uint32(os.Getgid()) //nolint:gosec // test process ids fit uint32. require.NoError(t, svc.InitializeRoot(t.Context(), e2eRootMode, uid, gid)) - mountPoint := t.TempDir() adapter := fuseadapter.New(svc, []byte("e2e-mount")) server, err := fuseadapter.Mount(mountPoint, adapter, &fuse.MountOptions{DirectMount: os.Geteuid() == 0}) if err != nil { @@ -52,15 +80,16 @@ func mountMemoryFilesystem(t *testing.T) string { <-serveDone skipOrFail(t, "FUSE mount did not come up: %v", err) } + mount := &e2eMount{server: server, serveDone: serveDone, unmount: sync.OnceValue(server.Unmount)} t.Cleanup(func() { - require.NoError(t, server.Unmount()) + require.NoError(t, mount.unmount()) select { case <-serveDone: case <-time.After(10 * time.Second): t.Error("FUSE server did not stop after unmount") } }) - return mountPoint + return mount } func skipOrFail(t *testing.T, format string, args ...any) { diff --git a/internal/filesystem/fuseadapter/mount_helper_cancel_test.go b/internal/filesystem/fuseadapter/mount_helper_cancel_test.go new file mode 100644 index 000000000..2461a0de4 --- /dev/null +++ b/internal/filesystem/fuseadapter/mount_helper_cancel_test.go @@ -0,0 +1,47 @@ +//go:build linux + +package fuseadapter_test + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/bootjp/elastickv/internal/filesystem/fstest" + "github.com/bootjp/elastickv/internal/filesystem/fuseadapter" + "github.com/hanwen/go-fuse/v2/fuse" + "github.com/stretchr/testify/require" +) + +// TestMountContextAbortsWhileMountHelperHangs puts a fusermount3 stand-in +// on PATH and checks that cancellation joins late cleanup within its budget. +// go-fuse itself waits on the helper with no context, so startup must retain +// ownership of that cleanup without waiting forever for the helper. +func TestMountContextAbortsWhileMountHelperHangs(t *testing.T) { + helperDir := t.TempDir() + // The stand-in holds go-fuse's communication socket open and never sends + // a descriptor; it exits on its own so no process outlives the test by + // much. + helper := filepath.Join(helperDir, "fusermount3") + require.NoError(t, os.WriteFile(helper, []byte("#!/bin/sh\nexec /bin/sleep 3\n"), 0o700)) //nolint:gosec // the helper stand-in must be executable. + t.Setenv("PATH", helperDir) + + svc, err := fstest.NewMemoryService() + require.NoError(t, err) + adapter := fuseadapter.New(svc, []byte("hung-helper")) + t.Cleanup(adapter.Close) + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + // DirectMount off: force the fusermount helper path even when root. + server, err := fuseadapter.MountContext(ctx, t.TempDir(), adapter, &fuse.MountOptions{DirectMount: false}) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.ErrorContains(t, err, "waiting for the mount helper") + require.Nil(t, server) + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "the helper must really have been waited on until the deadline") + require.Less(t, elapsed, 12*time.Second, "startup cancellation must bound its joined late-mount cleanup") +} diff --git a/internal/filesystem/fuseadapter/release_deadline_test.go b/internal/filesystem/fuseadapter/release_deadline_test.go new file mode 100644 index 000000000..ddd542976 --- /dev/null +++ b/internal/filesystem/fuseadapter/release_deadline_test.go @@ -0,0 +1,84 @@ +package fuseadapter + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// shortenAbandonedRelease sets the two parts of the window an abandoned +// mount's release runs in for the duration of a test. +func shortenAbandonedRelease(t *testing.T, unmount, detach time.Duration) { + t.Helper() + oldUnmount, oldDetach := abandonedUnmountTimeout, abandonedDetachTimeout + abandonedUnmountTimeout, abandonedDetachTimeout = unmount, detach + t.Cleanup(func() { abandonedUnmountTimeout, abandonedDetachTimeout = oldUnmount, oldDetach }) +} + +// TestReleaseAfterReadinessSharesTheUnmountDeadline: the wait for the +// readiness check and the plain unmount that follows it share one deadline, +// so a readiness check that returns late leaves the unmount only what is +// left of it rather than a fresh bound of its own. +func TestReleaseAfterReadinessSharesTheUnmountDeadline(t *testing.T) { + shortenAbandonedRelease(t, 2*time.Second, 2*time.Second) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + marker := writeRecordingMountHelper(t) + ready := make(chan error, 1) + time.AfterFunc(1600*time.Millisecond, func() { ready <- nil }) + start := time.Now() + releaseAfterReadiness(ready, func(ctx context.Context) error { + <-ctx.Done() + return ctx.Err() + }, mountPoint, fuseTypeFor(filesystemName)) + elapsed := time.Since(start) + require.FileExists(t, marker, "the hung unmount must be followed by the lazy detach") + require.Less(t, elapsed, 3*time.Second, "the unmount got a fresh bound after the readiness wait") +} + +// TestReleaseAfterReadinessBoundsTheLazyDetach: the lazy detach runs by the +// end of the same window, so a detach helper that hangs cannot stretch the +// release past it. +func TestReleaseAfterReadinessBoundsTheLazyDetach(t *testing.T) { + shortenAbandonedRelease(t, 100*time.Millisecond, 2*time.Second) + helperDir := t.TempDir() + helper := filepath.Join(helperDir, "fusermount3") + require.NoError(t, os.WriteFile(helper, []byte("#!/bin/sh\nexec /bin/sleep 30\n"), 0o700)) //nolint:gosec // the helper stand-in must be executable. + onlyMountHelpersIn(t, helperDir) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + + unmounted := make(chan struct{}, 1) + start := time.Now() + releaseAfterReadiness(make(chan error), func(context.Context) error { + unmounted <- struct{}{} + return nil + }, mountPoint, fuseTypeFor(filesystemName)) + require.Less(t, time.Since(start), 3*time.Second, "the lazy detach ran past the release window") + require.Empty(t, unmounted, "the unmount must be skipped while the readiness check is stuck") +} + +// TestReleaseAbandonedSkipsUnmountPastItsDeadline: when no time is left for +// the plain unmount (a readiness check that returned right at the +// deadline), go-fuse's unbounded Unmount is not started at all, so it +// cannot run on alongside the lazy detach and act, by path, on whatever +// the detach exposed; the lazy detach still runs. +func TestReleaseAbandonedSkipsUnmountPastItsDeadline(t *testing.T) { + shortenAbandonedRelease(t, time.Second, 2*time.Second) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + marker := writeRecordingMountHelper(t) + unmounted := make(chan struct{}, 1) + + releaseAbandonedBy(time.Now().Add(-time.Millisecond), func(context.Context) error { + unmounted <- struct{}{} + return nil + }, mountPoint, fuseTypeFor(filesystemName)) + require.FileExists(t, marker, "the lazy detach must still run") + time.Sleep(50 * time.Millisecond) + require.Empty(t, unmounted, "the plain unmount must not start once its deadline has passed") +} diff --git a/internal/filesystem/fuseadapter/server.go b/internal/filesystem/fuseadapter/server.go index 0b7093837..eabe17b8a 100644 --- a/internal/filesystem/fuseadapter/server.go +++ b/internal/filesystem/fuseadapter/server.go @@ -56,19 +56,28 @@ type RawFileSystem struct { attrTTL time.Duration entryTTL time.Duration filesystem string + + // requests is the parent of every request's context. cancelRequests + // ends it, which makes in-flight backend calls return so the kernel + // gets an answer for each request it already handed to this server. + requests context.Context + cancelRequests context.CancelFunc } // NewRawFileSystem creates the request bridge without mounting it. Tests and // embedders can exercise this surface directly; Mount performs the kernel bind. func NewRawFileSystem(adapter *Adapter) *RawFileSystem { + requests, cancelRequests := context.WithCancel(context.Background()) return &RawFileSystem{ - RawFileSystem: fuse.NewDefaultRawFileSystem(), - adapter: adapter, - nextDirFH: rootDirHandle, - dirHandles: make(map[uint64]*directoryHandle), - attrTTL: defaultAttrTTL, - entryTTL: defaultEntryTTL, - filesystem: filesystemName, + RawFileSystem: fuse.NewDefaultRawFileSystem(), + adapter: adapter, + nextDirFH: rootDirHandle, + dirHandles: make(map[uint64]*directoryHandle), + attrTTL: defaultAttrTTL, + entryTTL: defaultEntryTTL, + filesystem: filesystemName, + requests: requests, + cancelRequests: cancelRequests, } } @@ -81,7 +90,7 @@ func (r *RawFileSystem) String() string { } func (r *RawFileSystem) Lookup(cancel <-chan struct{}, header *fuse.InHeader, name string, out *fuse.EntryOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() stat, errno := r.adapter.Lookup(ctx, header.NodeId, []byte(name)) if errno != 0 { @@ -92,7 +101,7 @@ func (r *RawFileSystem) Lookup(cancel <-chan struct{}, header *fuse.InHeader, na } func (r *RawFileSystem) GetAttr(cancel <-chan struct{}, input *fuse.GetAttrIn, out *fuse.AttrOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() stat, errno := r.adapter.GetAttr(ctx, input.NodeId) if errno != 0 { @@ -104,7 +113,7 @@ func (r *RawFileSystem) GetAttr(cancel <-chan struct{}, input *fuse.GetAttrIn, o } func (r *RawFileSystem) SetAttr(cancel <-chan struct{}, input *fuse.SetAttrIn, out *fuse.AttrOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() mask, attrs := fuseSetAttr(input) stat, errno := r.adapter.SetAttr(ctx, input.NodeId, mask, attrs) @@ -120,7 +129,7 @@ func (r *RawFileSystem) Mknod(cancel <-chan struct{}, input *fuse.MknodIn, name if input.Mode&syscall.S_IFMT != syscall.S_IFREG { return fuse.Status(syscall.EOPNOTSUPP) } - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() result, errno := r.adapter.Mknod(ctx, input.NodeId, []byte(name), filesystem.CreateOptions{ Mode: input.Mode & permissionModeMask, @@ -135,7 +144,7 @@ func (r *RawFileSystem) Mknod(cancel <-chan struct{}, input *fuse.MknodIn, name } func (r *RawFileSystem) Mkdir(cancel <-chan struct{}, input *fuse.MkdirIn, name string, out *fuse.EntryOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() result, errno := r.adapter.Mkdir(ctx, input.NodeId, []byte(name), filesystem.CreateOptions{ Mode: input.Mode & permissionModeMask, @@ -150,13 +159,13 @@ func (r *RawFileSystem) Mkdir(cancel <-chan struct{}, input *fuse.MkdirIn, name } func (r *RawFileSystem) Unlink(cancel <-chan struct{}, header *fuse.InHeader, name string) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() return fuse.Status(r.adapter.Unlink(ctx, header.NodeId, []byte(name))) } func (r *RawFileSystem) Rmdir(cancel <-chan struct{}, header *fuse.InHeader, name string) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() return fuse.Status(r.adapter.Rmdir(ctx, header.NodeId, []byte(name))) } @@ -165,7 +174,7 @@ func (r *RawFileSystem) Rename(cancel <-chan struct{}, input *fuse.RenameIn, old if input.Flags != 0 { return fuse.Status(syscall.EOPNOTSUPP) } - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() return fuse.Status(r.adapter.Rename(ctx, input.NodeId, []byte(oldName), input.Newdir, []byte(newName))) } @@ -183,14 +192,14 @@ func (r *RawFileSystem) Readlink(<-chan struct{}, *fuse.InHeader) ([]byte, fuse. } func (r *RawFileSystem) Access(cancel <-chan struct{}, input *fuse.AccessIn) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() _, errno := r.adapter.GetAttr(ctx, input.NodeId) return fuse.Status(errno) } func (r *RawFileSystem) Create(cancel <-chan struct{}, input *fuse.CreateIn, name string, out *fuse.CreateOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() result, errno := r.adapter.Create(ctx, input.NodeId, []byte(name), filesystem.CreateOptions{ Mode: input.Mode & permissionModeMask, @@ -206,7 +215,7 @@ func (r *RawFileSystem) Create(cancel <-chan struct{}, input *fuse.CreateIn, nam } func (r *RawFileSystem) Open(cancel <-chan struct{}, input *fuse.OpenIn, out *fuse.OpenOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() fh, errno := r.adapter.Open(ctx, input.NodeId) if errno == 0 { @@ -216,7 +225,7 @@ func (r *RawFileSystem) Open(cancel <-chan struct{}, input *fuse.OpenIn, out *fu } func (r *RawFileSystem) Read(cancel <-chan struct{}, input *fuse.ReadIn, _ []byte) (fuse.ReadResult, fuse.Status) { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() data, errno := r.adapter.Read(ctx, input.NodeId, input.Fh, input.Offset, uint64(input.Size)) if errno != 0 { @@ -226,7 +235,7 @@ func (r *RawFileSystem) Read(cancel <-chan struct{}, input *fuse.ReadIn, _ []byt } func (r *RawFileSystem) Write(cancel <-chan struct{}, input *fuse.WriteIn, data []byte) (uint32, fuse.Status) { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() n, errno := r.adapter.Write(ctx, input.NodeId, input.Fh, input.Offset, data) if errno != 0 { @@ -239,7 +248,7 @@ func (r *RawFileSystem) Write(cancel <-chan struct{}, input *fuse.WriteIn, data } func (r *RawFileSystem) Release(cancel <-chan struct{}, input *fuse.ReleaseIn) { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() if errno := r.adapter.Release(ctx, input.NodeId, input.Fh); errno != 0 { slog.WarnContext(ctx, "filesystem FUSE release failed", "inode", input.NodeId, "fh", input.Fh, "errno", errno) @@ -247,20 +256,20 @@ func (r *RawFileSystem) Release(cancel <-chan struct{}, input *fuse.ReleaseIn) { } func (r *RawFileSystem) Flush(cancel <-chan struct{}, input *fuse.FlushIn) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() return fuse.Status(r.adapter.Flush(ctx, input.NodeId, input.Fh)) } func (r *RawFileSystem) Fsync(cancel <-chan struct{}, input *fuse.FsyncIn) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() datasync := input.FsyncFlags&1 != 0 return fuse.Status(r.adapter.Fsync(ctx, input.NodeId, input.Fh, datasync)) } func (r *RawFileSystem) OpenDir(cancel <-chan struct{}, input *fuse.OpenIn, out *fuse.OpenOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() stat, errno := r.adapter.GetAttr(ctx, input.NodeId) if errno != 0 { @@ -274,7 +283,7 @@ func (r *RawFileSystem) OpenDir(cancel <-chan struct{}, input *fuse.OpenIn, out } func (r *RawFileSystem) ReadDir(cancel <-chan struct{}, input *fuse.ReadIn, out *fuse.DirEntryList) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() handle, ok := r.directoryHandle(input.Fh) if !ok { @@ -341,7 +350,7 @@ func (r *RawFileSystem) FsyncDir(<-chan struct{}, *fuse.FsyncIn) fuse.Status { } func (r *RawFileSystem) StatFs(cancel <-chan struct{}, input *fuse.InHeader, out *fuse.StatfsOut) fuse.Status { - ctx, done := requestContext(cancel) + ctx, done := r.requestContext(cancel) defer done() stat, errno := r.adapter.StatFS(ctx, input.NodeId) if errno != 0 { @@ -393,8 +402,31 @@ func (r *RawFileSystem) directoryHandle(fh uint64) (*directoryHandle, bool) { return handle, ok } +// requestContext is the context of one FUSE request: it ends when the +// kernel interrupts the request (cancel) or when the server cancels all of +// its requests (cancelInFlight). +func (r *RawFileSystem) requestContext(cancel <-chan struct{}) (context.Context, context.CancelFunc) { + parent := context.Background() + if r != nil && r.requests != nil { + parent = r.requests + } + return requestContextWithin(parent, cancel) +} + +// cancelInFlight ends the context of every request in flight and of any +// that arrives later. A nil RawFileSystem is a no-op. +func (r *RawFileSystem) cancelInFlight() { + if r != nil && r.cancelRequests != nil { + r.cancelRequests() + } +} + func requestContext(cancel <-chan struct{}) (context.Context, context.CancelFunc) { - ctx, done := context.WithCancel(context.Background()) + return requestContextWithin(context.Background(), cancel) +} + +func requestContextWithin(parent context.Context, cancel <-chan struct{}) (context.Context, context.CancelFunc) { + ctx, done := context.WithCancel(parent) if cancel == nil { return ctx, done } @@ -489,33 +521,198 @@ func saturatingAdd(left uint64, right uint64) uint64 { // Server owns an active kernel mount and its adapter session. type Server struct { - inner *fuse.Server - adapter *Adapter + inner *fuse.Server + adapter *Adapter + mountPoint string + // fsType is the mounts-table type of this mount (`fuse.`, the + // subtype go-fuse mounts with); the lazy detach refuses to act on + // anything else at the mount point. + fsType string + // fs is the request bridge the kernel mount serves. + fs *RawFileSystem + // startCtx is the context MountContext ran under. It bounds only + // WaitMount: go-fuse's readiness check opens a file on the new mount, + // the kernel's permission check on the root sends a getattr through to + // the backend, and that read can block while no leader is available. + startCtx context.Context + // released is closed once WaitMount gave up on a cancelled startup and + // released the mount; Unmount and Serve then return at once. + released chan struct{} + releaseOnce sync.Once } // Mount attaches adapter to mountPoint using go-fuse. The returned server is -// ready for Serve; callers should call Unmount during shutdown. +// ready for Serve; callers should call Unmount during shutdown. It is +// MountContext with a background context; servers that can be told to stop +// during startup should use MountContext. +// +// A dangling FUSE mount left at mountPoint by a server that died without +// unmounting (SIGKILL, a crash) is detached first, because fusermount3 +// refuses to mount over a mount point whose transport is not connected. Only +// a mount the mounts table reports with this server's own type +// (`fuse.`) and whose stat fails with ENOTCONN is touched; a live +// mount or a filesystem of any other type at the path is left alone. func Mount(mountPoint string, adapter *Adapter, options *fuse.MountOptions) (*Server, error) { + return MountContext(context.Background(), mountPoint, adapter, options) +} + +// MountContext is Mount whose pre-mount work (the stale-mount detach and the +// intent recovery) stops when ctx is cancelled, so a shutdown requested +// during startup does not wait on a hung fusermount helper, an unavailable +// leader, or a long recovery backlog. The returned Server's WaitMount stops +// on ctx too, releasing the mount; otherwise the kernel mount and the +// Server are not bound to ctx, and Unmount ends them. +func MountContext(ctx context.Context, mountPoint string, adapter *Adapter, options *fuse.MountOptions) (*Server, error) { if adapter == nil || adapter.core == nil { return nil, errors.New("filesystem FUSE adapter is required") } if strings.TrimSpace(mountPoint) == "" { return nil, errors.New("filesystem FUSE mount point is required") } + if err := ctx.Err(); err != nil { + return nil, errors.Wrap(err, "filesystem FUSE mount aborted before start") + } opts := defaultMountOptions(options) + if _, err := detachStaleMount(ctx, mountPoint, opts.Name, slog.Default()); err != nil { + return nil, errors.Wrap(err, "detach stale filesystem FUSE mount") + } if recoverer, ok := adapter.core.(intentRecoverer); ok { - recoveryCtx, cancel := context.WithTimeout(context.Background(), startupRecoveryTimeout) + recoveryCtx, cancel := context.WithTimeout(ctx, startupRecoveryTimeout) recoveryErr := recoverIntentsBeforeMount(recoveryCtx, recoverer) cancel() if recoveryErr != nil { return nil, errors.Wrap(recoveryErr, "recover filesystem intents before FUSE mount") } } - inner, err := fuse.NewServer(NewRawFileSystem(adapter), mountPoint, opts) + fs := NewRawFileSystem(adapter) + inner, err := newServerContext(ctx, fs, mountPoint, opts) if err != nil { - return nil, errors.Wrap(err, "mount filesystem FUSE server") + return nil, err + } + return &Server{ + inner: inner, + adapter: adapter, + mountPoint: mountPoint, + fsType: fuseTypeFor(opts.Name), + fs: fs, + startCtx: ctx, + released: make(chan struct{}), + }, nil +} + +type newServerResult struct { + server *fuse.Server + err error +} + +// newServerContext runs fuse.NewServer in its own goroutine so a cancelled +// ctx can unwind startup: go-fuse waits for the fusermount helper with no +// context of its own, and a hung helper would otherwise block the mount, the +// errgroup drain and the WAL sync behind it forever. A mount that completes +// after the caller gave up is unmounted again so no kernel mount leaks. +func newServerContext(ctx context.Context, fs *RawFileSystem, mountPoint string, opts *fuse.MountOptions) (*fuse.Server, error) { + results := make(chan newServerResult, 1) + go func() { + server, err := fuse.NewServer(fs, mountPoint, opts) + results <- newServerResult{server: server, err: err} + }() + select { + case res := <-results: + if res.err != nil { + return nil, errors.Wrap(res.err, "mount filesystem FUSE server") + } + return res.server, nil + case <-ctx.Done(): + fs.cancelInFlight() + if !joinAbandonedMount(func() { + releaseAbandonedMount(results, fs, mountPoint, fuseTypeFor(opts.Name)) + }, abandonedUnmountTimeout+abandonedDetachTimeout) { + slog.ErrorContext(ctx, "filesystem late mount cleanup exceeded shutdown budget", "mount_point", mountPoint) + } + return nil, errors.Wrap(ctx.Err(), "filesystem FUSE mount cancelled while waiting for the mount helper") + } +} + +func joinAbandonedMount(release func(), timeout time.Duration) bool { + done := make(chan struct{}) + go func() { + defer close(done) + release() + }() + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case <-done: + return true + case <-timer.C: + return false + } +} + +// abandonedUnmountTimeout and abandonedDetachTimeout split the one window a +// mount whose caller gave up on it is released in. The context-bound plain +// unmount, together with the wait for a cancelled +// readiness check before it, must finish within abandonedUnmountTimeout of +// the start of the release; the lazy detach that may follow must finish +// within abandonedDetachTimeout after that deadline. A release therefore +// never takes longer than their sum, a stop requested during startup +// included. Variables so tests can shorten them. +var ( + abandonedUnmountTimeout = 7 * time.Second + abandonedDetachTimeout = 3 * time.Second +) + +// releaseAbandonedMount tears down a mount that completed after MountContext +// had already given up on it. It first cancels every request the late mount +// serves, now and later (fs is its request bridge), so a client request that +// reached the backend before the release returns instead of running on +// while the caller's unwinding closes the stores. go-fuse's NewServer +// registers an event loop that only Serve can drain, so Serve must run +// while the context-bound unmount releases the kernel connection. +func releaseAbandonedMount(results <-chan newServerResult, fs *RawFileSystem, mountPoint, fsType string) { + res := <-results + if res.server == nil { + return + } + fs.cancelInFlight() + go res.server.Serve() + releaseAbandoned(func(ctx context.Context) error { return unmountFUSEMount(ctx, mountPoint, fsType) }, mountPoint, fsType) +} + +// releaseAbandoned releases a mount in a release window that starts now +// (see releaseAbandonedBy). +func releaseAbandoned(unmount func(context.Context) error, mountPoint, fsType string) { + releaseAbandonedBy(time.Now().Add(abandonedUnmountTimeout), unmount, mountPoint, fsType) +} + +// releaseAbandonedBy runs unmount until unmountBy. When it fails or does +// not return in time, the mount is detached lazily instead, by +// abandonedDetachTimeout after unmountBy, so no kernel mount is left +// behind; the lazy detach itself refuses a path that no longer holds a +// mount of fsType, so an unmount that did succeed late is not followed by a +// detach of whatever sits underneath. When unmountBy has already passed, +// unmount is not started at all. The callback must reap its context-bound +// helper before returning so nothing can later act on a replacement mount. +func releaseAbandonedBy(unmountBy time.Time, unmount func(context.Context) error, mountPoint, fsType string) { + ctx, cancel := context.WithDeadline(context.Background(), unmountBy) + if ctx.Err() != nil { + slog.Warn("filesystem FUSE unmount of an abandoned mount has no time left, detaching lazily", + "mount_point", mountPoint, + "timeout", abandonedUnmountTimeout, + ) + } else if err := unmount(ctx); err == nil { + cancel() + return + } else { + slog.Warn("filesystem FUSE unmount of an abandoned mount failed, detaching lazily", "mount_point", mountPoint, "err", err) + } + // The context-bound callback reaps its helper before returning. + cancel() + ctx, cancel = context.WithDeadline(context.Background(), unmountBy.Add(abandonedDetachTimeout)) + defer cancel() + if _, err := detachFUSEMount(ctx, mountPoint, fsType); err != nil && !errors.Is(err, errNoFUSEMount) { + slog.Warn("filesystem FUSE lazy detach of an abandoned mount failed", "mount_point", mountPoint, "err", err) } - return &Server{inner: inner, adapter: adapter}, nil } func recoverIntentsBeforeMount(ctx context.Context, recoverer intentRecoverer) error { @@ -559,17 +756,105 @@ func containsMountOption(options []string, want string) bool { return false } +// Serve runs the request loop until the kernel mount goes away. After +// WaitMount released the mount of a cancelled startup it returns once that +// release has finished, even when a backend request that ignores its +// context still holds one of go-fuse's loops. func (s *Server) Serve() { - if s != nil && s.inner != nil { + if s == nil || s.inner == nil { + return + } + if s.released == nil { s.inner.Serve() + return + } + done := make(chan struct{}) + go func() { + defer close(done) + s.inner.Serve() + }() + select { + case <-done: + case <-s.released: } } +// WaitMount waits until the kernel mount answers requests. When the context +// MountContext ran under is cancelled first, it releases the mount (see +// release) and returns the cancellation, so a shutdown requested while the +// readiness check is stuck in a backend request is not held up by it. +// Serve must be running, as for go-fuse's own WaitMount. func (s *Server) WaitMount() error { if s == nil || s.inner == nil { return nil } - return errors.WithStack(s.inner.WaitMount()) + ready := make(chan error, 1) + inner := s.inner + go func() { ready <- inner.WaitMount() }() + var stop <-chan struct{} + if s.startCtx != nil { + stop = s.startCtx.Done() + } + select { + case err := <-ready: + return errors.WithStack(err) + case <-stop: + s.release(ready) + return errors.Wrap(s.startCtx.Err(), "filesystem FUSE mount cancelled while waiting for it to become ready") + } +} + +// release tears down a mount whose startup was cancelled. It first cancels +// every request in flight: the kernel caller of a request already handed to +// this server waits for the answer uninterruptibly (here go-fuse's readiness +// check in this very process), so the handler must return before anything +// else can. It then waits for the readiness check (ready) and unmounts, +// falling back to the guarded lazy detach, inside one release window (see +// releaseAfterReadiness). The Server is then marked released. +func (s *Server) release(ready <-chan error) { + s.releaseOnce.Do(func() { + s.fs.cancelInFlight() + releaseAfterReadiness(ready, s.UnmountContext, s.mountPoint, s.fsType) + if s.released != nil { + close(s.released) + } + }) +} + +// releaseAfterReadiness waits for the cancelled readiness check (ready) and +// then releases the mount with releaseAbandonedBy, all inside one release +// window: the readiness wait and the plain unmount share its unmount +// deadline, so a check that returns late leaves the unmount only what is +// left of it, and the lazy detach runs by its end. A check still running at +// the unmount deadline means a backend request is ignoring its context, and +// the ordinary unmount is skipped for the lazy detach alone. +func releaseAfterReadiness(ready <-chan error, unmount func(context.Context) error, mountPoint, fsType string) { + unmountBy := time.Now().Add(abandonedUnmountTimeout) + timer := time.NewTimer(abandonedUnmountTimeout) + select { + case <-ready: + case <-timer.C: + unmount = func(context.Context) error { return errReadinessStuck } + } + timer.Stop() + releaseAbandonedBy(unmountBy, unmount, mountPoint, fsType) +} + +// errReadinessStuck reports that go-fuse's readiness check did not return +// after its backend requests were cancelled, so the plain unmount is skipped. +var errReadinessStuck = errors.New("filesystem FUSE readiness check did not return after cancellation") + +// isReleased reports whether WaitMount already released the mount. +func (s *Server) isReleased() bool { + if s.released == nil { + return false + } + select { + case <-s.released: + return true + default: + return false + } } func (s *Server) Wait() { @@ -585,8 +870,39 @@ func (s *Server) Unmount() error { if s.adapter != nil { s.adapter.Close() } - if s.inner == nil { + if s.inner == nil || s.isReleased() { return nil } return errors.WithStack(s.inner.Unmount()) } + +// UnmountLazy detaches the mount from the namespace even while clients still +// hold files on it open (`fusermount -u -z`, or a lazy unmount(2)): new +// lookups see the plain directory at once, and the kernel releases the FUSE +// connection, ending Serve, once the last open handle is closed. It is the +// fallback for an Unmount that failed with EBUSY or did not return during +// shutdown, so it first stops serving: every request in flight, and every +// one that arrives later, has its context cancelled. A handler blocked in +// the backend (the reason the Unmount hung) therefore returns before the +// stores behind it are closed, and requests through a handle that is still +// open get an error from then on. The detach itself acts only on a mount of +// this server's own type still present at the mount point: when the kernel +// mount is already gone (Unmount removed it and is merely waiting for its +// event loop), or a filesystem of any other type is exposed there, nothing +// is touched and nil is returned. A nil or never-mounted Server is a no-op +// apart from the cancellation. +func (s *Server) UnmountLazy(ctx context.Context) error { + if s == nil { + return nil + } + s.fs.cancelInFlight() + if s.inner == nil || s.mountPoint == "" { + return nil + } + _, err := detachFUSEMount(ctx, s.mountPoint, s.fsType) + if errors.Is(err, errNoFUSEMount) { + slog.InfoContext(ctx, "filesystem FUSE mount is already gone, nothing to detach", "mount_point", s.mountPoint) + return nil + } + return err +} diff --git a/internal/filesystem/fuseadapter/stale_mount.go b/internal/filesystem/fuseadapter/stale_mount.go new file mode 100644 index 000000000..0b647adaa --- /dev/null +++ b/internal/filesystem/fuseadapter/stale_mount.go @@ -0,0 +1,674 @@ +package fuseadapter + +import ( + "bufio" + "context" + "log/slog" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "time" + + "github.com/cockroachdb/errors" +) + +const ( + // mountTablePath lists the mounts visible to this process. Entries are + // whitespace separated with octal escapes for the characters that would + // break the format (space, tab, newline, backslash). + mountTablePath = "/proc/self/mounts" + // mountTableMinFields is the number of leading columns this code reads + // from a mounts entry (device, mount point, type); shorter rows are + // malformed and skipped. + mountTableMinFields = 3 + // mountTableOctalDigits is the width of a `\ooo` escape in the table. + mountTableOctalDigits = 3 + // mountTableOctalBase is the radix of a `\ooo` escape in the table. + mountTableOctalBase = 8 + // mountTableOctalBits is the bit size of the byte a `\ooo` escape encodes. + mountTableOctalBits = 8 + // fuseTypePrefix starts the type go-fuse mounts report in the mounts + // table (`fuse.`); fuseTypeFor builds the full type. Only a + // mount of this server's own type is ever detached: a bare `fuse` or a + // foreign `fuse.` mount at the path belongs to someone else. + fuseTypePrefix = "fuse." + // detachCommandTimeout bounds one fusermount invocation. A dangling + // mount does not need the server, so detaching it never blocks on I/O, + // but a hung suid helper must not stall startup forever. + detachCommandTimeout = 10 * time.Second + // detachHelperBudgetShare is the fraction (1/n) of the caller's remaining + // deadline one helper invocation may use in total, including the + // WaitDelay below, so a hung helper cannot spend the whole budget and + // starve the privileged unmount(2) fallback. + detachHelperBudgetShare = 2 + // detachHelperWaitDelay bounds how long a killed helper's output pipes + // are waited for (exec.Cmd.WaitDelay). It is reserved out of the + // helper's share: os/exec waits for the pipes after the context has + // killed the helper, on top of the context timeout. + detachHelperWaitDelay = 500 * time.Millisecond + // maxStaleMountDetaches bounds the detach loop. Each iteration removes + // the topmost stale mount at the path; several only stack up when a + // server was restarted over a dangling mount more than once. + maxStaleMountDetaches = 8 + // maxMountPointSymlinkHops bounds symlink resolution of the mount point + // itself, matching the kernel's MAXSYMLINKS. + maxMountPointSymlinkHops = 40 + // staleProbeTimeout bounds the liveness stat on a FUSE mount. A dead + // mount answers ENOTCONN at once and a healthy server answers quickly; + // only a server that is alive but wedged makes stat block, and that mount + // is not ours to detach, so startup fails with a clear error instead of + // hanging in fusermount's own stat afterwards. + staleProbeTimeout = 10 * time.Second +) + +// mountPointResolveTimeout bounds the symlink resolution of the mount +// point's parent directories, which stats each path component and can +// block when a parent sits on an unresponsive FUSE or network filesystem. +// A variable so tests can shorten it. +var mountPointResolveTimeout = staleProbeTimeout + +// mountProbeError reports that an inspection of the mount point could not +// reach a verdict: the liveness stat on a FUSE mount, or the resolution of +// the mount point's parent directories, was cancelled (shutdown requested) +// or timed out (a live but unresponsive FUSE server still holds the mount +// point, or a parent directory sits on an unresponsive filesystem). op names +// the step that gave up. +type mountProbeError struct { + op string + path string + err error +} + +const ( + probeOpStat = "stat FUSE mount at" + probeOpResolve = "resolve FUSE mount point" +) + +func (e *mountProbeError) Error() string { + op := e.op + if op == "" { + op = probeOpStat + } + return op + " " + e.path + ": " + e.err.Error() +} + +func (e *mountProbeError) Unwrap() error { return e.err } + +// mountTableEntry is one row of /proc/self/mounts. +type mountTableEntry struct { + mountPoint string + fsType string +} + +// mountTableUnavailableError reports that the mounts table could not be +// read at all (no /proc in a minimal container or chroot). It is not a +// stale-mount verdict: the caller skips the check and lets the real mount +// report any conflict, so /proc does not become a new hard prerequisite. +type mountTableUnavailableError struct { + path string + err error +} + +func (e *mountTableUnavailableError) Error() string { + return "open mount table " + e.path + ": " + e.err.Error() +} + +func (e *mountTableUnavailableError) Unwrap() error { return e.err } + +// staleMountInspector locates dangling FUSE mounts at a path. The fields are +// swapped in tests so the decision logic can be exercised without a kernel +// mount; production code uses defaultStaleMountInspector. +type staleMountInspector struct { + mountTable string + stat func(string) error + probeTimeout time.Duration + // fsType is the mounts-table type of the mounts this server makes; + // only a dead mount of that type is detached. Empty means the default + // mount name's type. + fsType string +} + +func defaultStaleMountInspector(mountName string) *staleMountInspector { + return &staleMountInspector{ + mountTable: mountTablePath, + stat: statMountPoint, + probeTimeout: staleProbeTimeout, + fsType: fuseTypeFor(mountName), + } +} + +// expectedType is the mounts-table type a mount must have to be ours. +func (i *staleMountInspector) expectedType() string { + if i.fsType == "" { + return fuseTypeFor(filesystemName) + } + return i.fsType +} + +func statMountPoint(path string) error { + var st syscall.Stat_t + return errors.WithStack(syscall.Stat(path, &st)) +} + +// detachStaleMount removes a dangling FUSE mount left at mountPoint by a +// server that died without unmounting (SIGKILL, a crash, or a stop before +// the process installed signal handling). mountName is the Name (subtype) +// this server mounts with, so only a mount of the same `fuse.` type +// is a candidate. It reports how many mounts were detached. A path that holds +// no mount, a live mount (its server still answers stat), or a filesystem +// of any other type, FUSE or not, is left untouched: the mount table +// decides the type, never the error from stat alone. +func detachStaleMount(ctx context.Context, mountPoint, mountName string, logger *slog.Logger) (int, error) { + inspector := defaultStaleMountInspector(mountName) + return inspector.detachAll(ctx, mountPoint, logger, func(ctx context.Context, path string) (string, error) { + return detachFUSEMount(ctx, path, inspector.fsType) + }) +} + +func (i *staleMountInspector) detachAll( + ctx context.Context, + mountPoint string, + logger *slog.Logger, + detach func(context.Context, string) (string, error), +) (int, error) { + if logger == nil { + logger = slog.Default() + } + detached := 0 + for { + entry, stale, err := i.staleFUSEMount(ctx, mountPoint) + var unavailable *mountTableUnavailableError + if errors.As(err, &unavailable) { + logger.Warn("mount table unavailable, skipping stale FUSE mount check", + "mount_point", mountPoint, + "err", err, + ) + return detached, nil + } + if err != nil { + return detached, err + } + if !stale { + return detached, nil + } + if detached >= maxStaleMountDetaches { + // Re-inspected after the last allowed detach, so exactly + // maxStaleMountDetaches stacked mounts succeed; only a mount + // that is still there is an error. + return detached, errors.Errorf("more than %d stale FUSE mounts at %s", maxStaleMountDetaches, mountPoint) + } + method, err := detach(ctx, entry.mountPoint) + if errors.Is(err, errNoFUSEMount) { + // The mount went away between the inspection and the detach + // (another process, or the kernel finishing an unmount); + // inspect again rather than count or report it. + continue + } + if err != nil { + return detached, errors.Wrapf(err, "detach stale FUSE mount at %s", entry.mountPoint) + } + detached++ + logger.Warn("detached stale filesystem FUSE mount left by a previous process", + "mount_point", entry.mountPoint, + "fs_type", entry.fsType, + "method", method, + ) + } +} + +// topmostMount returns the last mounts-table entry for mountPoint (the one +// the kernel currently exposes there), read from tablePath. found is false +// when nothing is mounted at the path. Resolving the mount point's +// spellings is bounded by ctx and mountPointResolveTimeout; when it cannot +// finish the error is a *mountProbeError. +func topmostMount(ctx context.Context, tablePath, mountPoint string) (top mountTableEntry, found bool, err error) { + candidates, err := resolveMountPointCandidates(ctx, mountPoint) + if err != nil { + return mountTableEntry{}, false, err + } + entries, err := readMountTable(tablePath) + if err != nil { + return mountTableEntry{}, false, err + } + for _, entry := range entries { + if candidates[entry.mountPoint] { + top = entry + found = true + } + } + return top, found, nil +} + +// errNoFUSEMount reports that the path currently holds no FUSE mount of this +// server's type: either nothing is mounted there or the topmost filesystem +// is of another type, FUSE or not. A detach stops at that point so it can +// never remove a foreign filesystem. +var errNoFUSEMount = errors.New("no FUSE mount of this server's type at the path") + +// detachMountTable is the mounts table detachFUSEMount checks before it +// touches anything. A variable so tests can point it at a table they wrote. +var detachMountTable = mountTablePath + +// requireFUSEMount fails unless the topmost mount at mountPoint is a FUSE +// mount of type fsType: errNoFUSEMount when the path holds nothing or a +// filesystem of any other type, a wrapped *mountTableUnavailableError when +// that cannot be verified. +func requireFUSEMount(ctx context.Context, mountPoint, fsType string) error { + top, found, err := topmostMount(ctx, detachMountTable, mountPoint) + if err != nil { + return errors.Wrapf(err, "verify FUSE mount at %s", mountPoint) + } + if !found || top.fsType != fsType { + return errors.Wrapf(errNoFUSEMount, "%s", mountPoint) + } + return nil +} + +// staleFUSEMount reports whether the topmost mount at mountPoint is a FUSE +// mount of this server's type whose server is gone. The topmost entry +// decides: any other filesystem mounted over a dead mount is not touched +// either. The liveness stat is bounded by ctx and probeTimeout; when it +// cannot reach a verdict the error is a *mountProbeError. +func (i *staleMountInspector) staleFUSEMount(ctx context.Context, mountPoint string) (mountTableEntry, bool, error) { + top, found, err := topmostMount(ctx, i.mountTable, mountPoint) + if err != nil { + return mountTableEntry{}, false, err + } + if !found || top.fsType != i.expectedType() { + return mountTableEntry{}, false, nil + } + statErr := i.probe(ctx, top.mountPoint) + var probeErr *mountProbeError + if errors.As(statErr, &probeErr) { + return mountTableEntry{}, false, statErr + } + if statErr == nil { + return mountTableEntry{}, false, nil + } + if !errors.Is(statErr, syscall.ENOTCONN) { + // A FUSE mount whose server is alive but refuses stat (for + // example a permission error from default_permissions) is not + // stale. Only the broken-transport errno proves the server died. + return mountTableEntry{}, false, nil + } + return top, true, nil +} + +// probe runs the liveness stat on a mount point the table reports as FUSE. +// Nothing can interrupt a stat blocked on a wedged FUSE server, so the call +// runs in its own goroutine and only the wait is bounded, by ctx and by +// probeTimeout; on cancellation or timeout the goroutine is left to finish +// whenever the kernel lets it and a *mountProbeError is returned instead of +// a verdict. +func (i *staleMountInspector) probe(ctx context.Context, path string) error { + timeout := i.probeTimeout + if timeout <= 0 { + timeout = staleProbeTimeout + } + result := make(chan error, 1) + go func() { result <- i.stat(path) }() + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case err := <-result: + return err + case <-ctx.Done(): + return errors.WithStack(&mountProbeError{op: probeOpStat, path: path, err: ctx.Err()}) + case <-timer.C: + return errors.WithStack(&mountProbeError{ + op: probeOpStat, + path: path, + err: errors.Newf("no answer within %s, a live but unresponsive FUSE server may still hold the mount point", timeout), + }) + } +} + +// mountPointCandidatesFn resolves a mount point's spellings; a variable so +// tests can stand in a resolution that blocks, as it does when a parent +// directory sits on an unresponsive filesystem. +var mountPointCandidatesFn = mountPointCandidates + +// resolveMountPointCandidates runs mountPointCandidates with a bound. The +// parent canonicalization stats each path component, and nothing can +// interrupt a stat blocked on a wedged FUSE or network filesystem, so the +// call runs in its own goroutine and only the wait is bounded, by ctx and +// by mountPointResolveTimeout; on cancellation or timeout the goroutine is +// left to finish whenever the kernel lets it and a *mountProbeError is +// returned instead of a verdict. +func resolveMountPointCandidates(ctx context.Context, mountPoint string) (map[string]bool, error) { + type resolution struct { + candidates map[string]bool + err error + } + results := make(chan resolution, 1) + resolve := mountPointCandidatesFn + go func() { + candidates, err := resolve(mountPoint) + results <- resolution{candidates: candidates, err: err} + }() + timer := time.NewTimer(mountPointResolveTimeout) + defer timer.Stop() + select { + case res := <-results: + return res.candidates, res.err + case <-ctx.Done(): + return nil, errors.WithStack(&mountProbeError{op: probeOpResolve, path: mountPoint, err: ctx.Err()}) + case <-timer.C: + return nil, errors.WithStack(&mountProbeError{ + op: probeOpResolve, + path: mountPoint, + err: errors.Newf("no answer within %s, a parent directory may sit on an unresponsive filesystem", mountPointResolveTimeout), + }) + } +} + +// mountPointCandidates returns the spellings of mountPoint that may appear +// in the mounts table: the cleaned absolute path, the path with a symlinked +// final component followed (the kernel mounts on the target and records the +// target), and each of those with a symlinked parent directory resolved. The +// mounted directory itself is never stat'ed because stat on a dangling mount +// fails. +func mountPointCandidates(mountPoint string) (map[string]bool, error) { + abs, err := filepath.Abs(mountPoint) + if err != nil { + return nil, errors.Wrapf(err, "resolve FUSE mount point %q", mountPoint) + } + candidates := map[string]bool{} + for _, path := range []string{abs, resolveMountPointSymlinks(abs)} { + candidates[path] = true + candidates[filepath.Join(canonicalParent(path), filepath.Base(path))] = true + } + return candidates, nil +} + +// resolveMountPointSymlinks follows a symlink used as the mount point itself +// using only Readlink. Readlink on a non-symlink fails with EINVAL from the +// dentry type alone, so unlike Lstat it never sends a getattr through a +// FUSE mount covering the path: a dead mount cannot fail it and a live but +// wedged server cannot block it (that case is left to the bounded probe). +// A relative link target is interpreted from the canonical parent +// directory, as the kernel's pathname resolution does, so a link under a +// symlinked parent lands where the kernel mounted. It returns path unchanged +// when it is not a symlink or cannot be read. +func resolveMountPointSymlinks(path string) string { + for hop := 0; hop < maxMountPointSymlinkHops; hop++ { + target, err := os.Readlink(path) + if err != nil { + return path + } + if !filepath.IsAbs(target) { + target = filepath.Join(canonicalParent(path), target) + } + path = filepath.Clean(target) + } + return path +} + +// canonicalParent resolves symlinks in the directory holding path without +// ever stat'ing path itself (which may be a dead mount). It falls back to the +// lexical parent when resolution fails. +func canonicalParent(path string) string { + parent := filepath.Dir(path) + if resolved, err := filepath.EvalSymlinks(parent); err == nil { + return resolved + } + return parent +} + +// fuseTypeFor returns the mounts-table type go-fuse records for a mount +// made with MountOptions.Name: go-fuse passes Name as the subtype +// (`fuse.`), while FsName only fills the source column. +func fuseTypeFor(mountName string) string { + return fuseTypePrefix + mountName +} + +func readMountTable(path string) ([]mountTableEntry, error) { + file, err := os.Open(path) + if err != nil { + return nil, errors.WithStack(&mountTableUnavailableError{path: path, err: err}) + } + defer file.Close() + var entries []mountTableEntry + scanner := bufio.NewScanner(file) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) < mountTableMinFields { + continue + } + entries = append(entries, mountTableEntry{ + mountPoint: unescapeMountTableField(fields[1]), + fsType: fields[2], + }) + } + if err := scanner.Err(); err != nil { + return nil, errors.Wrapf(err, "read mount table %s", path) + } + return entries, nil +} + +// unescapeMountTableField decodes the `\ooo` octal escapes the kernel uses +// for space, tab, newline, and backslash in mount table columns. +func unescapeMountTableField(field string) string { + if !strings.Contains(field, `\`) { + return field + } + var out strings.Builder + for index := 0; index < len(field); index++ { + if field[index] != '\\' || index+mountTableOctalDigits >= len(field) { + out.WriteByte(field[index]) + continue + } + code, err := strconv.ParseUint(field[index+1:index+1+mountTableOctalDigits], mountTableOctalBase, mountTableOctalBits) + if err != nil { + out.WriteByte(field[index]) + continue + } + out.WriteByte(byte(code)) + index += mountTableOctalDigits + } + return out.String() +} + +// mountHelperFallbackDir mirrors go-fuse's helper lookup: when PATH does not +// hold fusermount3/fusermount, /bin is tried, so a service started with a +// restricted PATH detaches with the same helper go-fuse mounted with. A +// variable so tests can point it at a directory they control. +var mountHelperFallbackDir = "/bin" + +// lookupMountHelperFn locates a helper; a variable so tests can stand in a +// lookup that blocks, as exec.LookPath does when a PATH directory sits on an +// unresponsive filesystem. +var lookupMountHelperFn = lookupMountHelper + +// lookupMountHelperContext runs the helper lookup under the same share of +// ctx's deadline a helper invocation gets. exec.LookPath stats each PATH +// directory and nothing can interrupt a stat blocked on a wedged FUSE or +// network filesystem, so the lookup runs in its own goroutine and only the +// wait is bounded; a lookup that does not answer in time counts as the +// helper not being found, which keeps the kernel fallback reachable. +func lookupMountHelperContext(ctx context.Context, name string) (string, error) { + type lookupResult struct { + path string + err error + } + results := make(chan lookupResult, 1) + lookup := lookupMountHelperFn + go func() { + path, err := lookup(name) + results <- lookupResult{path: path, err: err} + }() + timer := time.NewTimer(detachHelperTimeout(ctx)) + defer timer.Stop() + select { + case res := <-results: + return res.path, res.err + case <-ctx.Done(): + return "", errors.Wrapf(ctx.Err(), "locate mount helper %s", name) + case <-timer.C: + return "", errors.Errorf("locate mount helper %s: no answer within its share of the detach budget", name) + } +} + +// lookupMountHelper finds a fusermount helper the way go-fuse does: PATH +// first, then mountHelperFallbackDir. +func lookupMountHelper(name string) (string, error) { + if path, err := exec.LookPath(name); err == nil { + return path, nil + } + path, err := exec.LookPath(filepath.Join(mountHelperFallbackDir, name)) + if err != nil { + return "", errors.Wrapf(err, "locate mount helper %s", name) + } + return path, nil +} + +// detachHelperTimeout bounds one helper invocation's context: +// detachCommandTimeout, or a share of whatever is left of the caller's +// deadline, so that when the helper hangs the privileged unmount(2) +// fallback still runs inside the caller's budget instead of being skipped +// behind an expired context. The share covers the whole invocation: the +// pipe wait that os/exec adds after killing the helper (WaitDelay) is +// taken out of it, so a helper that leaves a child holding its output +// pipes still returns within the share. A share no larger than that wait +// yields a zero or negative timeout, which makes exec skip the helper +// without starting it. +func detachHelperTimeout(ctx context.Context) time.Duration { + timeout := detachCommandTimeout + if deadline, ok := ctx.Deadline(); ok { + if share := time.Until(deadline) / detachHelperBudgetShare; share < timeout { + timeout = share + } + } + return timeout - detachHelperWaitDelay +} + +// detachFUSEMount lazily detaches the FUSE mount of type fsType at +// mountPoint through the same helpers go-fuse's own unmount uses: the suid fusermount3 (or legacy +// fusermount) binary with `-u -z`, which works for unprivileged users, and +// otherwise a direct lazy unmount(2), the equivalent of `umount -l`, which +// needs CAP_SYS_ADMIN (lazyUnmount, Linux only). It returns a description of +// the mechanism used. A cancelled ctx is reported as the cancellation, not +// as the EPERM the privileged fallback would produce for an unprivileged +// server, so a shutdown requested mid-detach still exits cleanly. +// +// It only ever acts on a mount of this server's own type. The mounts table +// is checked before the helpers run and again right before the kernel +// fallback: the helpers refuse a non-FUSE mount on their own but accept any +// FUSE filesystem, unmount(2) accepts anything, and the path may no longer +// hold our mount by then (go-fuse's own Unmount can have removed it while +// still blocked on its event loop; another process may have detached it). +// Such a path yields errNoFUSEMount and nothing is touched, so a filesystem +// mounted underneath the mount point, another FUSE filesystem included, is +// never detached. +func detachFUSEMount(ctx context.Context, mountPoint, fsType string) (string, error) { + if err := requireFUSEMount(ctx, mountPoint, fsType); err != nil { + return "", err + } + method, helperErr := detachWithHelpers(ctx, mountPoint, fsType) + if method != "" { + return method, nil + } + if errors.Is(helperErr, errNoFUSEMount) { + return "", helperErr + } + return detachWithKernel(ctx, mountPoint, fsType, helperErr) +} + +// detachMountHelpers are the helpers go-fuse's own unmount uses, in order. +var detachMountHelpers = []string{"fusermount3", "fusermount"} + +// detachWithHelpers runs `fusermount3 -u -z`, then `fusermount -u -z`, each +// under its share of the deadline, as is the lookup of each helper. It +// returns the mechanism on success; +// otherwise the last helper's failure, or nil when no helper was found. +// Right before each helper runs, after its lookup, the mounts table is +// checked again: the mount may have gone away while the lookup ran (the +// plain unmount finishing late) or a failed helper may still have detached +// it (it reports an error when, for example, a descendant keeps its output +// pipe open past WaitDelay), and a helper acts on whatever FUSE filesystem +// the mount point names, so it must not run against one that is now +// exposed. Such a path yields errNoFUSEMount with any earlier helper +// failure kept in the chain. +func detachWithHelpers(ctx context.Context, mountPoint, fsType string) (string, error) { + var helperErr error + for _, helper := range detachMountHelpers { + bin, err := lookupMountHelperContext(ctx, helper) + if err != nil { + continue + } + if err := requireFUSEMount(ctx, mountPoint, fsType); err != nil { + return "", afterHelper(err, helperErr) + } + if err := runDetachHelper(ctx, bin, mountPoint); err != nil { + helperErr = errors.Wrapf(err, "%s -u -z %s", helper, mountPoint) + continue + } + return helper + " -u -z", nil + } + return "", helperErr +} + +// runDetachHelper runs one helper binary with `-u -z mountPoint` under its +// share of ctx's deadline, reporting its output alongside a failure. +func runDetachHelper(ctx context.Context, bin, mountPoint string) error { + return runUnmountHelper(ctx, bin, mountPoint, true) +} + +func runUnmountHelper(ctx context.Context, bin, mountPoint string, lazy bool) error { + args := []string{"-u"} + if lazy { + args = append(args, "-z") + } + args = append(args, mountPoint) + return runMountHelper(ctx, bin, args) +} + +func runMountHelper(ctx context.Context, bin string, args []string) error { + cmdCtx, cancel := context.WithTimeout(ctx, detachHelperTimeout(ctx)) + defer cancel() + //nolint:gosec // bin comes from LookPath over two fixed helper names and mountPoint is the operator's configured path. + cmd := exec.CommandContext(cmdCtx, bin, args...) + finish := isolateMountHelper(cmd) + defer finish() + // After the context kills the helper, do not wait on its output pipes + // for longer than this: a child the helper left behind could otherwise + // hold them open past the budget. + cmd.WaitDelay = detachHelperWaitDelay + output, err := cmd.CombinedOutput() + if err != nil { + return errors.Wrap(err, strings.TrimSpace(string(output))) + } + return nil +} + +// detachWithKernel is the privileged fallback after the helpers: it stops +// on a cancelled context, re-verifies that the path still holds a FUSE +// mount of type fsType, and only then calls unmount(2) with MNT_DETACH. +// helperErr, when set, is kept in the error chain so the log shows why the +// helpers did not do the job. +func detachWithKernel(ctx context.Context, mountPoint, fsType string, helperErr error) (string, error) { + if err := ctx.Err(); err != nil { + return "", afterHelper(errors.Wrap(err, "detach cancelled"), helperErr) + } + // Re-check right before the privileged fallback: a helper may have + // taken long enough for the mount to disappear, and unmount(2) would + // otherwise detach whatever filesystem is now exposed at the path. + if err := requireFUSEMount(ctx, mountPoint, fsType); err != nil { + return "", afterHelper(err, helperErr) + } + if err := lazyUnmount(mountPoint); err != nil { + return "", afterHelper(errors.Wrapf(err, "%s %s", lazyUnmountMethod, mountPoint), helperErr) + } + return lazyUnmountMethod, nil +} + +// afterHelper records the helpers' failure alongside err. +func afterHelper(err, helperErr error) error { + if helperErr == nil { + return err + } + return errors.Wrapf(err, "after %s", helperErr.Error()) +} diff --git a/internal/filesystem/fuseadapter/stale_mount_e2e_test.go b/internal/filesystem/fuseadapter/stale_mount_e2e_test.go new file mode 100644 index 000000000..e185bb06b --- /dev/null +++ b/internal/filesystem/fuseadapter/stale_mount_e2e_test.go @@ -0,0 +1,171 @@ +//go:build linux + +package fuseadapter_test + +import ( + "bufio" + "bytes" + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/bootjp/elastickv/internal/filesystem" + "github.com/bootjp/elastickv/internal/filesystem/fstest" + "github.com/bootjp/elastickv/internal/filesystem/fuseadapter" + "github.com/cockroachdb/errors" + "github.com/hanwen/go-fuse/v2/fuse" + "github.com/stretchr/testify/require" +) + +const ( + // staleMountHelperEnv names the mount point a helper copy of this test + // binary mounts and then holds until it is killed. + staleMountHelperEnv = "ELASTICKV_FUSE_STALE_MOUNT_HELPER" + staleMountHelperOK = "mounted" + // staleMountHelperLifetime bounds how long an orphaned helper keeps its + // mount if the parent test dies before killing it. + staleMountHelperLifetime = 2 * time.Minute + staleMountWait = 30 * time.Second + staleMountPoll = 50 * time.Millisecond +) + +// TestHelperProcessHoldsMount is the entry point of the helper process used +// by the stale-mount tests. It is a no-op in the normal test run and only +// does work when staleMountHelperEnv is set: it mounts an in-memory +// filesystem at that path, reports readiness on stdout, and blocks until the +// parent kills it, which leaves a dangling FUSE mount behind. +func TestHelperProcessHoldsMount(t *testing.T) { + mountPoint := os.Getenv(staleMountHelperEnv) + if mountPoint == "" { + t.Skip("helper entry point; only runs in the spawned helper process") + } + svc, err := fstest.NewMemoryService(filesystem.WithChunkSize(e2eChunkSize)) + require.NoError(t, err) + uid, gid := uint32(os.Getuid()), uint32(os.Getgid()) //nolint:gosec // test process ids fit uint32. + require.NoError(t, svc.InitializeRoot(t.Context(), e2eRootMode, uid, gid)) + adapter := fuseadapter.New(svc, []byte("stale-helper")) + server, err := fuseadapter.Mount(mountPoint, adapter, &fuse.MountOptions{DirectMount: os.Geteuid() == 0}) + require.NoError(t, err) + go server.Serve() + require.NoError(t, server.WaitMount()) + fmt.Println(staleMountHelperOK) + // Hold the mount; the parent SIGKILLs us. Exit without unmounting if the + // parent never does, so a lost parent cannot leak the helper forever. + time.Sleep(staleMountHelperLifetime) + os.Exit(1) +} + +// spawnDanglingMount mounts a FUSE filesystem from a helper process at a +// fresh directory, kills the helper the way a crashed or SIGKILLed server +// dies, and returns the path once the kernel reports the mount as dead +// (ENOTCONN, "Transport endpoint is not connected"). +func spawnDanglingMount(t *testing.T) string { + t.Helper() + if _, err := os.Stat("/dev/fuse"); err != nil { + skipOrFail(t, "no /dev/fuse: %v", err) + } + mountPoint := t.TempDir() + cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=^TestHelperProcessHoldsMount$", "-test.count=1") //nolint:gosec // re-executes this test binary. + cmd.Env = append(os.Environ(), staleMountHelperEnv+"="+mountPoint) + var stderr bytes.Buffer + cmd.Stderr = &stderr + stdout, err := cmd.StdoutPipe() + require.NoError(t, err) + require.NoError(t, cmd.Start()) + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + // Whatever the test left behind, never leak a dangling mount into + // the next test or the host. + _ = syscall.Unmount(mountPoint, syscall.MNT_DETACH) + }) + + ready := make(chan error, 1) + go func() { + reader := bufio.NewReader(stdout) + for { + line, err := reader.ReadString('\n') + if strings.TrimSpace(line) == staleMountHelperOK { + ready <- nil + return + } + if err != nil { + ready <- errors.Wrapf(err, "helper exited before mounting: %s", stderr.String()) + return + } + } + }() + select { + case err := <-ready: + if err != nil { + skipOrFail(t, "FUSE mount unavailable in helper: %v", err) + } + case <-time.After(staleMountWait): + t.Fatalf("helper did not report its mount in time; stderr: %s", stderr.String()) + } + + require.NoError(t, cmd.Process.Kill()) + _ = cmd.Wait() + require.Eventually(t, func() bool { + var st syscall.Stat_t + return errors.Is(syscall.Stat(mountPoint, &st), syscall.ENOTCONN) + }, staleMountWait, staleMountPoll, "killed helper did not leave a dangling mount") + return mountPoint +} + +// TestMountDetachesStaleMountLeftByKilledServer reproduces the restart +// failure from the operations guide: a server killed without unmounting +// leaves `Transport endpoint is not connected` at the mount point and the +// next Mount used to fail. Mount now detaches the dead mount and serves. +func TestMountDetachesStaleMountLeftByKilledServer(t *testing.T) { + mountPoint := spawnDanglingMount(t) + + mountMemoryFilesystemAt(t, mountPoint) + + path := filepath.Join(mountPoint, "after-restart") + require.NoError(t, os.WriteFile(path, []byte("recovered"), 0o600)) + got, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, []byte("recovered"), got) +} + +// TestKernelMountUnmountLeavesPlainDirectory guards the Mount-time detach +// against false positives: a mount point that was unmounted cleanly holds no +// FUSE entry, so remounting there must not try to detach anything and the +// directory must be usable as an ordinary directory in between. +func TestKernelMountUnmountLeavesPlainDirectory(t *testing.T) { + if _, err := os.Stat("/dev/fuse"); err != nil { + skipOrFail(t, "no /dev/fuse: %v", err) + } + mountPoint := t.TempDir() + mount := mountMemoryFilesystemServer(t, mountPoint) + require.NoError(t, os.WriteFile(filepath.Join(mountPoint, "probe"), []byte("x"), 0o600)) + require.NoError(t, mount.unmount()) + select { + case <-mount.serveDone: + case <-time.After(staleMountWait): + t.Fatal("FUSE server did not stop after unmount") + } + + entries, err := os.ReadDir(mountPoint) + require.NoError(t, err) + require.Empty(t, entries) + + // The shutdown fallback after the kernel mount is already gone must be + // a no-op: nothing to detach, no error, the plain directory untouched. + require.NoError(t, mount.server.UnmountLazy(context.Background())) + entries, err = os.ReadDir(mountPoint) + require.NoError(t, err) + require.Empty(t, entries) + + mountMemoryFilesystemAt(t, mountPoint) + entries, err = os.ReadDir(mountPoint) + require.NoError(t, err) + require.Empty(t, entries, "fresh in-memory filesystem starts empty") +} diff --git a/internal/filesystem/fuseadapter/stale_mount_guard_linux_test.go b/internal/filesystem/fuseadapter/stale_mount_guard_linux_test.go new file mode 100644 index 000000000..5118db48a --- /dev/null +++ b/internal/filesystem/fuseadapter/stale_mount_guard_linux_test.go @@ -0,0 +1,43 @@ +//go:build linux + +package fuseadapter + +import ( + "context" + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestDetachFUSEMountLeavesForeignMountAlone mounts a tmpfs where a FUSE +// mount might have been and runs the detach against it with the real mounts +// table: the kernel fallback must refuse, and the tmpfs must still be +// mounted afterwards. This is the shutdown case Codex raised: go-fuse's +// Unmount can remove the FUSE mount and then block on its event loop, so the +// path-based lazy detach that follows would otherwise MNT_DETACH whatever +// filesystem the mount point sits on, such as the root of a data volume. +func TestDetachFUSEMountLeavesForeignMountAlone(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("mounting a tmpfs needs root") + } + mountPoint := t.TempDir() + if err := syscall.Mount("tmpfs", mountPoint, "tmpfs", 0, ""); err != nil { + t.Skipf("cannot mount a tmpfs here: %v", err) + } + t.Cleanup(func() { _ = syscall.Unmount(mountPoint, syscall.MNT_DETACH) }) + require.NoError(t, os.WriteFile(filepath.Join(mountPoint, "keep"), []byte("x"), 0o600)) + onlyMountHelpersIn(t, t.TempDir()) // no fusermount helpers: straight to the fallback decision + + method, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + require.ErrorIs(t, err, errNoFUSEMount) + require.Empty(t, method) + + top, found, err := topmostMount(context.Background(), mountTablePath, mountPoint) + require.NoError(t, err) + require.True(t, found, "the tmpfs must still be mounted") + require.Equal(t, "tmpfs", top.fsType) + require.FileExists(t, filepath.Join(mountPoint, "keep")) +} diff --git a/internal/filesystem/fuseadapter/stale_mount_linux.go b/internal/filesystem/fuseadapter/stale_mount_linux.go new file mode 100644 index 000000000..83f7c0ae1 --- /dev/null +++ b/internal/filesystem/fuseadapter/stale_mount_linux.go @@ -0,0 +1,19 @@ +//go:build linux + +package fuseadapter + +import ( + "syscall" + + "github.com/cockroachdb/errors" +) + +// lazyUnmountMethod names the privileged fallback in detach logs. +const lazyUnmountMethod = "umount(MNT_DETACH)" + +// lazyUnmount is the privileged fallback behind fusermount: a lazy +// unmount(2), the equivalent of `umount -l`, which needs CAP_SYS_ADMIN. +// MNT_DETACH is Linux-only, which is why this lives behind a build tag. +func lazyUnmount(mountPoint string) error { + return errors.WithStack(syscall.Unmount(mountPoint, syscall.MNT_DETACH)) +} diff --git a/internal/filesystem/fuseadapter/stale_mount_other.go b/internal/filesystem/fuseadapter/stale_mount_other.go new file mode 100644 index 000000000..41a6d23c8 --- /dev/null +++ b/internal/filesystem/fuseadapter/stale_mount_other.go @@ -0,0 +1,16 @@ +//go:build !linux + +package fuseadapter + +import "github.com/cockroachdb/errors" + +// lazyUnmountMethod names the privileged fallback in detach logs. +const lazyUnmountMethod = "lazy unmount" + +// lazyUnmount has no portable implementation: MNT_DETACH is Linux-only. +// Stale-mount detection itself already depends on /proc/self/mounts and is +// skipped where that table is missing, so on other platforms the fusermount +// helpers are the only detach mechanism and this fallback reports that. +func lazyUnmount(string) error { + return errors.New("lazy unmount is only implemented on Linux") +} diff --git a/internal/filesystem/fuseadapter/stale_mount_test.go b/internal/filesystem/fuseadapter/stale_mount_test.go new file mode 100644 index 000000000..8a11c8bf6 --- /dev/null +++ b/internal/filesystem/fuseadapter/stale_mount_test.go @@ -0,0 +1,789 @@ +package fuseadapter + +import ( + "context" + "log/slog" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +// mountTableLine renders one /proc/self/mounts row for mountPoint, escaping +// the characters the kernel escapes. +func mountTableLine(device, mountPoint, fsType string) string { + escaped := strings.NewReplacer(`\`, `\134`, " ", `\040`, "\t", `\011`, "\n", `\012`).Replace(mountPoint) + return device + " " + escaped + " " + fsType + " rw,relatime 0 0\n" +} + +func writeMountTable(t *testing.T, lines ...string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "mounts") + require.NoError(t, os.WriteFile(path, []byte(strings.Join(lines, "")), 0o600)) + return path +} + +func statReturning(err error) func(string) error { + return func(string) error { return err } +} + +func TestStaleFUSEMountDetection(t *testing.T) { + base := t.TempDir() + mountPoint := filepath.Join(base, "mount point") + require.NoError(t, os.Mkdir(mountPoint, 0o755)) + other := filesystemNameForTest(base, "elsewhere") + tests := []struct { + name string + table []string + stat func(string) error + wantStale bool + wantType string + }{ + { + name: "dead go-fuse mount", + table: []string{mountTableLine("elastickv", mountPoint, "fuse.elastickv")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: true, + wantType: "fuse.elastickv", + }, + { + name: "dead bare fuse mount is not this server's", + table: []string{mountTableLine("fuse", mountPoint, "fuse")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: false, + }, + { + name: "dead foreign FUSE filesystem is not this server's", + table: []string{mountTableLine("sshfs#host:/", mountPoint, "fuse.sshfs")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: false, + }, + { + name: "live fuse mount answers stat", + table: []string{mountTableLine("elastickv", mountPoint, "fuse.elastickv")}, + stat: statReturning(nil), + wantStale: false, + }, + { + name: "fuse mount with a non-transport stat error", + table: []string{mountTableLine("elastickv", mountPoint, "fuse.elastickv")}, + stat: statReturning(syscall.EACCES), + wantStale: false, + }, + { + name: "non-FUSE filesystem is never touched", + table: []string{mountTableLine("/dev/sda1", mountPoint, "ext4")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: false, + }, + { + name: "fuseblk is not a go-fuse mount", + table: []string{mountTableLine("/dev/sdb", mountPoint, "fuseblk")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: false, + }, + { + name: "no entry for the path", + table: []string{mountTableLine("elastickv", other, "fuse.elastickv")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: false, + }, + { + name: "non-FUSE mount stacked over a dead fuse mount", + table: []string{ + mountTableLine("elastickv", mountPoint, "fuse.elastickv"), + mountTableLine("tmpfs", mountPoint, "tmpfs"), + }, + stat: statReturning(syscall.ENOTCONN), + wantStale: false, + }, + { + name: "dead fuse mount stacked over another filesystem", + table: []string{ + mountTableLine("tmpfs", mountPoint, "tmpfs"), + mountTableLine("elastickv", mountPoint, "fuse.elastickv"), + }, + stat: statReturning(syscall.ENOTCONN), + wantStale: true, + wantType: "fuse.elastickv", + }, + { + name: "malformed rows are skipped", + table: []string{"garbage\n", mountTableLine("elastickv", mountPoint, "fuse.elastickv")}, + stat: statReturning(syscall.ENOTCONN), + wantStale: true, + wantType: "fuse.elastickv", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inspector := &staleMountInspector{mountTable: writeMountTable(t, tt.table...), stat: tt.stat} + entry, stale, err := inspector.staleFUSEMount(context.Background(), mountPoint) + require.NoError(t, err) + require.Equal(t, tt.wantStale, stale) + if tt.wantStale { + require.Equal(t, mountPoint, entry.mountPoint) + require.Equal(t, tt.wantType, entry.fsType) + } + }) + } +} + +func filesystemNameForTest(base, name string) string { + return filepath.Join(base, name) +} + +func TestStaleFUSEMountMatchesSymlinkedParent(t *testing.T) { + base := t.TempDir() + real := filepath.Join(base, "real") + require.NoError(t, os.Mkdir(real, 0o755)) + link := filepath.Join(base, "link") + require.NoError(t, os.Symlink(real, link)) + resolvedBase, err := filepath.EvalSymlinks(real) + require.NoError(t, err) + + table := writeMountTable(t, mountTableLine("elastickv", filepath.Join(resolvedBase, "mnt"), "fuse.elastickv")) + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + entry, stale, err := inspector.staleFUSEMount(context.Background(), filepath.Join(link, "mnt")) + require.NoError(t, err) + require.True(t, stale) + require.Equal(t, filepath.Join(resolvedBase, "mnt"), entry.mountPoint) +} + +func TestStaleFUSEMountMatchesSymlinkedMountPoint(t *testing.T) { + base := t.TempDir() + real := filepath.Join(base, "real-mount") + require.NoError(t, os.Mkdir(real, 0o755)) + link := filepath.Join(base, "mount-link") + require.NoError(t, os.Symlink("real-mount", link)) + resolvedReal, err := filepath.EvalSymlinks(real) + require.NoError(t, err) + + // The kernel mounts on the symlink's target and records the target. + table := writeMountTable(t, mountTableLine("elastickv", resolvedReal, "fuse.elastickv")) + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + entry, stale, err := inspector.staleFUSEMount(context.Background(), link) + require.NoError(t, err) + require.True(t, stale) + require.Equal(t, resolvedReal, entry.mountPoint) +} + +// TestStaleFUSEMountMatchesRelativeSymlinkUnderSymlinkedParent covers +// /base/link/mp with link -> /base/real/deep and mp -> ../target: the kernel +// resolves the parent first and mounts on /base/real/target, so a relative +// link target must be joined to the canonical parent, not the typed one. +func TestStaleFUSEMountMatchesRelativeSymlinkUnderSymlinkedParent(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) + deep := filepath.Join(base, "real", "deep") + target := filepath.Join(base, "real", "target") + require.NoError(t, os.MkdirAll(deep, 0o755)) + require.NoError(t, os.MkdirAll(target, 0o755)) + require.NoError(t, os.Symlink(deep, filepath.Join(base, "link"))) + require.NoError(t, os.Symlink("../target", filepath.Join(deep, "mp"))) + typed := filepath.Join(base, "link", "mp") + + require.Equal(t, target, resolveMountPointSymlinks(typed)) + + table := writeMountTable(t, mountTableLine("elastickv", target, "fuse.elastickv")) + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + entry, stale, err := inspector.staleFUSEMount(context.Background(), typed) + require.NoError(t, err) + require.True(t, stale) + require.Equal(t, target, entry.mountPoint) +} + +func TestResolveMountPointSymlinksStopsAtNonSymlink(t *testing.T) { + base := t.TempDir() + real := filepath.Join(base, "real") + require.NoError(t, os.Mkdir(real, 0o755)) + first := filepath.Join(base, "first") + second := filepath.Join(base, "second") + require.NoError(t, os.Symlink(real, second)) + require.NoError(t, os.Symlink("second", first)) + + require.Equal(t, real, resolveMountPointSymlinks(first)) + require.Equal(t, real, resolveMountPointSymlinks(real)) + missing := filepath.Join(base, "missing") + require.Equal(t, missing, resolveMountPointSymlinks(missing)) +} + +func TestStaleFUSEMountReportsUnreadableTable(t *testing.T) { + inspector := &staleMountInspector{ + mountTable: filepath.Join(t.TempDir(), "missing"), + stat: statReturning(syscall.ENOTCONN), + } + _, _, err := inspector.staleFUSEMount(context.Background(), t.TempDir()) + var unavailable *mountTableUnavailableError + require.ErrorAs(t, err, &unavailable) + require.ErrorContains(t, err, "open mount table") +} + +func TestDetachAllSkipsCheckWithoutMountTable(t *testing.T) { + // A chroot or minimal container may have /dev/fuse but no /proc; the + // stale check must not turn that into a new mount prerequisite. + inspector := &staleMountInspector{ + mountTable: filepath.Join(t.TempDir(), "missing"), + stat: statReturning(syscall.ENOTCONN), + } + detach := func(context.Context, string) (string, error) { + t.Fatal("detach must not run without a mount table") + return "", nil + } + + detached, err := inspector.detachAll(context.Background(), t.TempDir(), slog.Default(), detach) + require.NoError(t, err) + require.Zero(t, detached) +} + +func TestDetachAllDetachesUntilClean(t *testing.T) { + mountPoint := t.TempDir() + stale := mountTableLine("elastickv", mountPoint, "fuse.elastickv") + // One table per state: two stacked dead mounts, one, none. Each fake + // detach advances the inspector to the next table, as the kernel would + // drop the topmost row after a real unmount. + tables := []string{ + writeMountTable(t, stale, stale), + writeMountTable(t, stale), + writeMountTable(t), + } + inspector := &staleMountInspector{mountTable: tables[0], stat: statReturning(syscall.ENOTCONN)} + var detachedPaths []string + detach := func(_ context.Context, path string) (string, error) { + detachedPaths = append(detachedPaths, path) + inspector.mountTable = tables[len(detachedPaths)] + return "fake", nil + } + + detached, err := inspector.detachAll(context.Background(), mountPoint, slog.Default(), detach) + require.NoError(t, err) + require.Equal(t, 2, detached) + require.Equal(t, []string{mountPoint, mountPoint}, detachedPaths) +} + +func TestDetachAllLeavesHealthyPathAlone(t *testing.T) { + mountPoint := t.TempDir() + table := writeMountTable(t, mountTableLine("/dev/sda1", mountPoint, "ext4")) + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + detach := func(context.Context, string) (string, error) { + t.Fatal("detach must not run for a non-FUSE mount") + return "", nil + } + + detached, err := inspector.detachAll(context.Background(), mountPoint, nil, detach) + require.NoError(t, err) + require.Zero(t, detached) +} + +func TestDetachAllWrapsDetachFailure(t *testing.T) { + mountPoint := t.TempDir() + table := writeMountTable(t, mountTableLine("elastickv", mountPoint, "fuse.elastickv")) + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + boom := errors.New("fusermount3 exploded") + detach := func(context.Context, string) (string, error) { return "", boom } + + detached, err := inspector.detachAll(context.Background(), mountPoint, slog.Default(), detach) + require.ErrorIs(t, err, boom) + require.ErrorContains(t, err, "detach stale FUSE mount at "+mountPoint) + require.Zero(t, detached) +} + +func TestDetachAllClearsExactlyTheMaximum(t *testing.T) { + mountPoint := t.TempDir() + stale := mountTableLine("elastickv", mountPoint, "fuse.elastickv") + // tables[k] holds maxStaleMountDetaches-k stacked dead mounts; the last + // one is clean. Exactly the maximum must succeed, not report "more than". + tables := make([]string, 0, maxStaleMountDetaches+1) + for remaining := maxStaleMountDetaches; remaining >= 0; remaining-- { + lines := make([]string, remaining) + for index := range lines { + lines[index] = stale + } + tables = append(tables, writeMountTable(t, lines...)) + } + inspector := &staleMountInspector{mountTable: tables[0], stat: statReturning(syscall.ENOTCONN)} + calls := 0 + detach := func(context.Context, string) (string, error) { + calls++ + inspector.mountTable = tables[calls] + return "fake", nil + } + + detached, err := inspector.detachAll(context.Background(), mountPoint, slog.Default(), detach) + require.NoError(t, err) + require.Equal(t, maxStaleMountDetaches, detached) + require.Equal(t, maxStaleMountDetaches, calls) +} + +func TestDetachAllBoundsTheLoop(t *testing.T) { + mountPoint := t.TempDir() + table := writeMountTable(t, mountTableLine("elastickv", mountPoint, "fuse.elastickv")) + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + calls := 0 + // The fake detach "succeeds" without changing the table, as a helper + // that lies about the unmount would. + detach := func(context.Context, string) (string, error) { + calls++ + return "noop", nil + } + + detached, err := inspector.detachAll(context.Background(), mountPoint, slog.Default(), detach) + require.ErrorContains(t, err, "more than") + require.Equal(t, maxStaleMountDetaches, detached) + require.Equal(t, maxStaleMountDetaches, calls) +} + +// TestDetachFUSEMountReportsCancellation pins that a shutdown requested while +// a detach is in flight surfaces as context.Canceled rather than as the +// EPERM the privileged fallback would return for an unprivileged server: +// onlyContextCanceled in main relies on that to exit 0 after a signal. The +// path is a plain directory, so nothing is unmounted whether or not a +// fusermount helper exists on this machine. +// blockingStat imitates stat on a FUSE mount whose server is alive but +// wedged: the syscall never returns until release is closed. +func blockingStat(release <-chan struct{}) func(string) error { + return func(string) error { + <-release + return nil + } +} + +func TestStaleFUSEMountProbeStopsOnCancellation(t *testing.T) { + mountPoint := t.TempDir() + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + inspector := &staleMountInspector{ + mountTable: writeMountTable(t, mountTableLine("elastickv", mountPoint, "fuse.elastickv")), + stat: blockingStat(release), + probeTimeout: time.Minute, + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, stale, err := inspector.staleFUSEMount(ctx, mountPoint) + require.ErrorIs(t, err, context.Canceled) + var probeErr *mountProbeError + require.ErrorAs(t, err, &probeErr) + require.False(t, stale) +} + +func TestStaleFUSEMountProbeTimesOutOnWedgedServer(t *testing.T) { + mountPoint := t.TempDir() + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + inspector := &staleMountInspector{ + mountTable: writeMountTable(t, mountTableLine("elastickv", mountPoint, "fuse.elastickv")), + stat: blockingStat(release), + probeTimeout: 20 * time.Millisecond, + } + + _, stale, err := inspector.staleFUSEMount(context.Background(), mountPoint) + require.ErrorContains(t, err, "unresponsive FUSE server") + var probeErr *mountProbeError + require.ErrorAs(t, err, &probeErr) + require.False(t, stale) + + // The verdict error must stop the detach loop rather than be read as + // "not stale and proceed". + detach := func(context.Context, string) (string, error) { + t.Fatal("detach must not run when the probe reached no verdict") + return "", nil + } + detached, err := inspector.detachAll(context.Background(), mountPoint, slog.Default(), detach) + require.ErrorAs(t, err, &probeErr) + require.Zero(t, detached) +} + +// useFakeDetachMountTable points the detach path's mounts-table check at a +// table listing mountPoint as a FUSE mount, so detachFUSEMount on a plain +// test directory gets past the verification to the mechanism under test. +func useFakeDetachMountTable(t *testing.T, lines ...string) string { + t.Helper() + table := writeMountTable(t, lines...) + old := detachMountTable + detachMountTable = table + t.Cleanup(func() { detachMountTable = old }) + return table +} + +func fuseTableLine(mountPoint string) string { + return mountTableLine("elastickv", mountPoint, "fuse.elastickv") +} + +// writeFakeMountHelper drops an executable stand-in for a fusermount binary +// into dir and returns its path. +func writeFakeMountHelper(t *testing.T, dir, name string) string { + t.Helper() + path := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(path, []byte("#!/bin/sh\nexit 0\n"), 0o700)) //nolint:gosec // the helper stand-in must be executable. + return path +} + +// onlyMountHelpersIn makes dir the only place lookupMountHelper finds a +// fusermount helper: PATH and the go-fuse style fallback directory both +// point at it, so a real /bin/fusermount or /bin/fusermount3 on the host +// (GitHub's Ubuntu runners ship /bin/fusermount) cannot run after the +// stand-ins and change what the test observes. +func onlyMountHelpersIn(t *testing.T, dir string) { + t.Helper() + t.Setenv("PATH", dir) + old := mountHelperFallbackDir + mountHelperFallbackDir = dir + t.Cleanup(func() { mountHelperFallbackDir = old }) +} + +func TestLookupMountHelperFallsBackLikeGoFuse(t *testing.T) { + t.Setenv("PATH", t.TempDir()) // empty PATH: nothing found there + fallback := t.TempDir() + oldFallback := mountHelperFallbackDir + mountHelperFallbackDir = fallback + t.Cleanup(func() { mountHelperFallbackDir = oldFallback }) + + _, err := lookupMountHelper("fusermount3") + require.Error(t, err, "nothing on PATH or in the fallback dir") + + want := writeFakeMountHelper(t, fallback, "fusermount3") + got, err := lookupMountHelper("fusermount3") + require.NoError(t, err) + require.Equal(t, want, got) + + // PATH still wins when it has the helper. + onPath := t.TempDir() + t.Setenv("PATH", onPath) + wantOnPath := writeFakeMountHelper(t, onPath, "fusermount3") + got, err = lookupMountHelper("fusermount3") + require.NoError(t, err) + require.Equal(t, wantOnPath, got) +} + +// TestDetachFUSEMountReachesKernelFallbackWhenHelperHangs puts a fusermount3 +// stand-in that never finishes on PATH and gives the detach a short deadline. +// The helper must be cut off before the deadline so the privileged +// unmount(2) fallback is still attempted (here on a plain directory, so it +// fails with its own error rather than the deadline's). +func TestDetachFUSEMountReachesKernelFallbackWhenHelperHangs(t *testing.T) { + helperDir := t.TempDir() + helper := filepath.Join(helperDir, "fusermount3") + require.NoError(t, os.WriteFile(helper, []byte("#!/bin/sh\nexec /bin/sleep 5\n"), 0o700)) //nolint:gosec // the helper stand-in must be executable. + onlyMountHelpersIn(t, helperDir) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + // Half of this budget is the helper's share; the reserved pipe wait + // leaves it 500 ms of real running time before it is killed. + budget := 2 * time.Second + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + start := time.Now() + method, err := detachFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName)) + t.Logf("detach error: %v", err) + require.Error(t, err) + require.Empty(t, method) + require.NotErrorIs(t, err, context.DeadlineExceeded, "the deadline must not pre-empt the kernel fallback") + require.ErrorContains(t, err, lazyUnmountMethod) + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, budget/2-detachHelperWaitDelay-50*time.Millisecond, "the helper must have been given its share of the deadline") + require.Less(t, elapsed, budget, "the fallback must run inside the caller's budget") +} + +func TestDetachHelperTimeoutSharesRemainingDeadline(t *testing.T) { + require.Equal(t, detachCommandTimeout-detachHelperWaitDelay, detachHelperTimeout(context.Background())) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + timeout := detachHelperTimeout(ctx) + require.Greater(t, timeout, time.Duration(0)) + require.LessOrEqual(t, timeout, 2*time.Second/detachHelperBudgetShare-detachHelperWaitDelay, + "the pipe wait after the kill must fit inside the helper's share") + + // A share that the pipe wait alone would use up leaves nothing for the + // helper itself: the timeout is not positive, so exec skips it. + tight, cancelTight := context.WithTimeout(context.Background(), 2*detachHelperWaitDelay) + defer cancelTight() + require.LessOrEqual(t, detachHelperTimeout(tight), time.Duration(0)) + + expired, cancelExpired := context.WithTimeout(context.Background(), time.Nanosecond) + defer cancelExpired() + time.Sleep(time.Millisecond) + require.LessOrEqual(t, detachHelperTimeout(expired), time.Duration(0)) +} + +// TestDetachFUSEMountReachesKernelFallbackWhenHelpersHoldPipes is the case +// the share must survive: both helpers are present, each hangs, and each +// leaves a background child holding its output pipes open, so every +// invocation costs its context timeout plus the WaitDelay pipe wait. With +// the pipe wait reserved out of the share, the first helper uses at most +// half of the budget, the second gets a share the pipe wait would exhaust +// and is skipped, and the kernel fallback still runs before the deadline. +// Before the reservation the two helpers overran the budget and the +// fallback was skipped behind the expired context. +func TestDetachFUSEMountReachesKernelFallbackWhenHelpersHoldPipes(t *testing.T) { + helperDir := t.TempDir() + const script = "#!/bin/sh\n/bin/sleep 5 &\nexec /bin/sleep 5\n" + for _, name := range []string{"fusermount3", "fusermount"} { + require.NoError(t, os.WriteFile(filepath.Join(helperDir, name), []byte(script), 0o700)) //nolint:gosec // the helper stand-in must be executable. + } + onlyMountHelpersIn(t, helperDir) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + budget := 4 * detachHelperWaitDelay + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + start := time.Now() + method, err := detachFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName)) + t.Logf("detach error: %v", err) + require.Error(t, err) + require.Empty(t, method) + require.NotErrorIs(t, err, context.DeadlineExceeded, "the helpers' pipe waits must not pre-empt the kernel fallback") + require.ErrorContains(t, err, lazyUnmountMethod) + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, budget/2-detachHelperWaitDelay-50*time.Millisecond, "the first helper must have been given its execution share") + require.Less(t, elapsed, budget, "the fallback must run inside the caller's budget") +} + +func TestDetachFUSEMountReportsCancellation(t *testing.T) { + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + method, err := detachFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName)) + require.ErrorIs(t, err, context.Canceled) + require.Empty(t, method) +} + +// TestDetachFUSEMountRefusesPathWithoutFUSEMount pins that the detach never +// acts on a path the mounts table does not list as a FUSE mount: nothing +// mounted, another filesystem on top, or a table it cannot read. No helper +// is run and the kernel fallback is not attempted, so a filesystem that +// sits where the FUSE mount used to be (go-fuse's own Unmount having +// already removed it while still waiting for its event loop) is left alone. +func TestDetachFUSEMountRefusesPathWithoutFUSEMount(t *testing.T) { + mountPoint := t.TempDir() + helperDir := t.TempDir() + marker := filepath.Join(helperDir, "ran") + script := "#!/bin/sh\n: > " + marker + "\nexit 0\n" + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "fusermount3"), []byte(script), 0o700)) //nolint:gosec // the helper stand-in must be executable. + onlyMountHelpersIn(t, helperDir) + + tests := []struct { + name string + table []string + }{ + {name: "nothing mounted at the path", table: nil}, + {name: "another filesystem on top", table: []string{mountTableLine("/dev/sdb1", mountPoint, "ext4")}}, + {name: "another FUSE filesystem exposed at the path", table: []string{mountTableLine("sshfs#host:/", mountPoint, "fuse.sshfs")}}, + {name: "a bare fuse mount at the path", table: []string{mountTableLine("fuse", mountPoint, "fuse")}}, + {name: "foreign FUSE filesystem over a dead mount of ours", table: []string{fuseTableLine(mountPoint), mountTableLine("sshfs#host:/", mountPoint, "fuse.sshfs")}}, + {name: "foreign filesystem over a dead fuse mount", table: []string{fuseTableLine(mountPoint), mountTableLine("tmpfs", mountPoint, "tmpfs")}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + useFakeDetachMountTable(t, tt.table...) + method, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + require.ErrorIs(t, err, errNoFUSEMount) + require.NotContains(t, err.Error(), lazyUnmountMethod, "the kernel fallback must not be attempted") + require.Empty(t, method) + require.NoFileExists(t, marker, "no helper may run against a path without a FUSE mount") + }) + } + + t.Run("mount table unavailable", func(t *testing.T) { + old := detachMountTable + detachMountTable = filepath.Join(t.TempDir(), "missing") + t.Cleanup(func() { detachMountTable = old }) + _, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + var unavailable *mountTableUnavailableError + require.ErrorAs(t, err, &unavailable) + require.NotContains(t, err.Error(), lazyUnmountMethod) + require.NoFileExists(t, marker) + }) +} + +// TestDetachFUSEMountRechecksBeforeKernelFallback covers the mount vanishing +// while a helper runs: the stand-in rewrites the table to show another +// filesystem at the path and fails, and the kernel fallback must then be +// refused instead of detaching that filesystem. +func TestDetachFUSEMountRechecksBeforeKernelFallback(t *testing.T) { + mountPoint := t.TempDir() + table := useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + helperDir := t.TempDir() + // The helper replaces the table with one that lists ext4 at the path, + // as if the FUSE mount had been removed underneath it, then fails so the + // code reaches the fallback decision. + replacement := filepath.Join(helperDir, "table.after") + require.NoError(t, os.WriteFile(replacement, []byte(mountTableLine("/dev/sdb1", mountPoint, "ext4")), 0o600)) + script := "#!/bin/sh\n/bin/cp " + replacement + " " + table + "\nexit 1\n" + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "fusermount3"), []byte(script), 0o700)) //nolint:gosec // the helper stand-in must be executable. + onlyMountHelpersIn(t, helperDir) + + method, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + require.ErrorIs(t, err, errNoFUSEMount) + require.ErrorContains(t, err, "fusermount3 -u -z", "the helper's failure stays in the chain") + require.NotContains(t, err.Error(), lazyUnmountMethod, "the kernel fallback must not run once the FUSE mount is gone") + require.Empty(t, method) +} + +// TestDetachAllReinspectsWhenMountVanishes: a stale mount that disappears +// between the inspection and the detach is neither counted nor an error. +func TestDetachAllReinspectsWhenMountVanishes(t *testing.T) { + mountPoint := t.TempDir() + tables := []string{ + writeMountTable(t, fuseTableLine(mountPoint)), + writeMountTable(t), + } + inspector := &staleMountInspector{mountTable: tables[0], stat: statReturning(syscall.ENOTCONN)} + calls := 0 + detach := func(context.Context, string) (string, error) { + calls++ + inspector.mountTable = tables[1] + return "", errors.Wrap(errNoFUSEMount, mountPoint) + } + + detached, err := inspector.detachAll(context.Background(), mountPoint, slog.Default(), detach) + require.NoError(t, err) + require.Zero(t, detached) + require.Equal(t, 1, calls) +} + +func TestUnescapeMountTableField(t *testing.T) { + tests := []struct { + in, want string + }{ + {in: "/mnt/plain", want: "/mnt/plain"}, + {in: `/mnt/with\040space`, want: "/mnt/with space"}, + {in: `/mnt/tab\011here`, want: "/mnt/tab\there"}, + {in: `/mnt/back\134slash`, want: `/mnt/back\slash`}, + {in: `/mnt/trailing\04`, want: `/mnt/trailing\04`}, + {in: `/mnt/not\0zzoctal`, want: `/mnt/not\0zzoctal`}, + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + require.Equal(t, tt.want, unescapeMountTableField(tt.in)) + }) + } +} + +// TestFUSETypeFor pins that the type a mount must carry to be detached is +// derived from the mount Name (the subtype) the server mounts with, so a +// mount made with a custom Name is recognised and every other subtype is +// not. TestMountContextTypeFollowsMountName checks the same against a real +// kernel mount. +func TestFUSETypeFor(t *testing.T) { + require.Equal(t, "fuse.elastickv", fuseTypeFor(filesystemName)) + require.Equal(t, "fuse.custom", fuseTypeFor("custom")) + require.Equal(t, "fuse.elastickv", (&staleMountInspector{}).expectedType()) + + mountPoint := t.TempDir() + inspector := &staleMountInspector{ + mountTable: writeMountTable(t, mountTableLine("custom", mountPoint, "fuse.custom")), + stat: statReturning(syscall.ENOTCONN), + fsType: fuseTypeFor("custom"), + } + _, stale, err := inspector.staleFUSEMount(context.Background(), mountPoint) + require.NoError(t, err) + require.True(t, stale, "a dead mount of the configured Name is ours") + inspector.mountTable = writeMountTable(t, fuseTableLine(mountPoint)) + _, stale, err = inspector.staleFUSEMount(context.Background(), mountPoint) + require.NoError(t, err) + require.False(t, stale, "the default Name is foreign to a server mounting with another one") +} + +// TestTopmostMountStopsWhenResolutionBlocks stands in a mount-point +// resolution that never returns, as EvalSymlinks does when a parent +// directory sits on a wedged FUSE or network filesystem, and pins that the +// inspection and the detach both give up on cancellation or after the +// resolution bound instead of holding the shutdown. +func TestTopmostMountStopsWhenResolutionBlocks(t *testing.T) { + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + oldResolve := mountPointCandidatesFn + mountPointCandidatesFn = func(string) (map[string]bool, error) { + <-release + return nil, nil + } + t.Cleanup(func() { mountPointCandidatesFn = oldResolve }) + mountPoint := t.TempDir() + table := writeMountTable(t, fuseTableLine(mountPoint)) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, _, err := topmostMount(ctx, table, mountPoint) + var resolveErr *mountProbeError + require.ErrorAs(t, err, &resolveErr) + require.ErrorIs(t, err, context.Canceled) + require.ErrorContains(t, err, "resolve FUSE mount point") + + oldTimeout := mountPointResolveTimeout + mountPointResolveTimeout = 20 * time.Millisecond + t.Cleanup(func() { mountPointResolveTimeout = oldTimeout }) + start := time.Now() + _, _, err = topmostMount(context.Background(), table, mountPoint) + require.ErrorAs(t, err, &resolveErr) + require.ErrorContains(t, err, "unresponsive filesystem") + require.Less(t, time.Since(start), 2*time.Second) + + // Neither caller reads the failure as "nothing there": the startup + // inspection reports it and the detach refuses to go on. + inspector := &staleMountInspector{mountTable: table, stat: statReturning(syscall.ENOTCONN)} + _, stale, err := inspector.staleFUSEMount(context.Background(), mountPoint) + require.ErrorAs(t, err, &resolveErr) + require.False(t, stale) + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + method, err := detachFUSEMount(context.Background(), mountPoint, fuseTypeFor(filesystemName)) + require.ErrorAs(t, err, &resolveErr) + require.Empty(t, method) +} + +// writeRecordingMountHelper puts a fusermount3 stand-in on PATH that records +// having run by creating the returned marker file. +func writeRecordingMountHelper(t *testing.T) string { + t.Helper() + helperDir := t.TempDir() + marker := filepath.Join(helperDir, "ran") + script := "#!/bin/sh\n: > " + marker + "\nexit 0\n" + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "fusermount3"), []byte(script), 0o700)) //nolint:gosec // the helper stand-in must be executable. + onlyMountHelpersIn(t, helperDir) + return marker +} + +// TestReleaseAbandonedDetachesWhenUnmountHangs: an abandoned mount whose +// plain unmount never returns (a hung helper, or an event loop stuck in a +// backend request) is detached lazily once the unmount overran its bound, +// instead of leaving the kernel mount attached to a dead server. +func TestReleaseAbandonedDetachesWhenUnmountHangs(t *testing.T) { + old := abandonedUnmountTimeout + abandonedUnmountTimeout = 50 * time.Millisecond + t.Cleanup(func() { abandonedUnmountTimeout = old }) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + marker := writeRecordingMountHelper(t) + start := time.Now() + releaseAbandoned(func(ctx context.Context) error { + <-ctx.Done() + return ctx.Err() + }, mountPoint, fuseTypeFor(filesystemName)) + require.Less(t, time.Since(start), 5*time.Second, "the release must not wait on the hung unmount") + require.FileExists(t, marker, "the lazy detach must run once the unmount overran its bound") +} + +// TestReleaseAbandonedFallsBackOnlyWhenUnmountFails: a clean unmount ends +// the release without any detach; a failed one is followed by the lazy +// detach. +func TestReleaseAbandonedFallsBackOnlyWhenUnmountFails(t *testing.T) { + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + marker := writeRecordingMountHelper(t) + + releaseAbandoned(func(context.Context) error { return nil }, mountPoint, fuseTypeFor(filesystemName)) + require.NoFileExists(t, marker, "a clean unmount needs no detach") + + releaseAbandoned(func(context.Context) error { return errors.New("device or resource busy") }, mountPoint, fuseTypeFor(filesystemName)) + require.FileExists(t, marker, "a failed unmount falls back to the lazy detach") +} diff --git a/internal/filesystem/fuseadapter/unmount_context.go b/internal/filesystem/fuseadapter/unmount_context.go new file mode 100644 index 000000000..2c0cfcd29 --- /dev/null +++ b/internal/filesystem/fuseadapter/unmount_context.go @@ -0,0 +1,67 @@ +package fuseadapter + +import ( + "context" + + "github.com/cockroachdb/errors" +) + +// UnmountContext bounds and reaps the path-based helper before returning. +// Unlike go-fuse's Unmount, cancellation never leaves a helper that could +// later unmount a new filesystem at the same path. +func (s *Server) UnmountContext(ctx context.Context) error { + if s == nil { + return nil + } + if s.adapter != nil { + s.adapter.Close() + } + if s.inner == nil || s.isReleased() || s.mountPoint == "" { + return nil + } + // Ordinary unmount can succeed before backend handlers leave Serve. + // Cancel their common parent before the lifecycle can close stores. + defer s.fs.cancelInFlight() + return unmountFUSEMount(ctx, s.mountPoint, s.fsType) +} + +func unmountFUSEMount(ctx context.Context, mountPoint, fsType string) error { + if err := verifyOrdinaryUnmount(ctx, mountPoint, fsType); err != nil { + if errors.Is(err, errNoFUSEMount) { + return nil + } + return errors.WithStack(err) + } + var helperErr error + for _, helper := range []string{"fusermount3", "fusermount", "umount"} { + bin, err := lookupMountHelperContext(ctx, helper) + if err != nil { + helperErr = err + continue + } + if err := verifyOrdinaryUnmount(ctx, mountPoint, fsType); err != nil { + return errors.WithStack(err) + } + args := []string{mountPoint} + if helper != "umount" { + args = append([]string{"-u"}, args...) + } + if err := runMountHelper(ctx, bin, args); err != nil { + helperErr = err + continue + } + return nil + } + return errors.Wrap(helperErr, "ordinary FUSE unmount helper failed") +} + +func verifyOrdinaryUnmount(ctx context.Context, mountPoint, fsType string) error { + err := requireFUSEMount(ctx, mountPoint, fsType) + var unavailable *mountTableUnavailableError + if errors.As(err, &unavailable) { + // Startup permits this platform/container configuration. Only ordinary + // unmount is allowed without the table; lazy detach stays type-guarded. + return nil + } + return errors.WithStack(err) +} diff --git a/internal/filesystem/fuseadapter/unmount_context_test.go b/internal/filesystem/fuseadapter/unmount_context_test.go new file mode 100644 index 000000000..5a0939030 --- /dev/null +++ b/internal/filesystem/fuseadapter/unmount_context_test.go @@ -0,0 +1,52 @@ +package fuseadapter + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestOrdinaryUnmountWithoutMountTableUsesBoundedHelper(t *testing.T) { + helperDir := t.TempDir() + marker := filepath.Join(helperDir, "called") + t.Setenv("ELASTICKV_ORDINARY_MARKER", marker) + script := "#!/bin/sh\nprintf '%s' \"$*\" > \"$ELASTICKV_ORDINARY_MARKER\"\n" + //nolint:gosec // Executable helper fixture, isolated in the test's temporary directory. + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "umount"), []byte(script), 0o700)) + onlyMountHelpersIn(t, helperDir) + table := useFakeDetachMountTable(t, "") + require.NoError(t, os.Remove(table)) + mountPoint := t.TempDir() + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + require.NoError(t, unmountFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName))) + data, err := os.ReadFile(marker) + require.NoError(t, err) + require.Equal(t, mountPoint, string(data), "ordinary umount receives no force/lazy flags") + _, err = detachFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName)) + require.Error(t, err, "lazy detach must still refuse an unverifiable mount") +} + +func TestAbandonedReleaseReapsOrdinaryHelperBeforeDetach(t *testing.T) { + shortenAbandonedRelease(t, 2*time.Second, 3*time.Second) + helperDir := t.TempDir() + detached, late := filepath.Join(helperDir, "detached"), filepath.Join(helperDir, "late") + t.Setenv("ELASTICKV_DETACHED_MARKER", detached) + t.Setenv("ELASTICKV_LATE_MARKER", late) + script := "#!/bin/sh\nif [ \"$2\" = '-z' ]; then : > \"$ELASTICKV_DETACHED_MARKER\"; exit 0; fi\n( /bin/sleep 1; : > \"$ELASTICKV_LATE_MARKER\" ) &\nexec /bin/sleep 5\n" + //nolint:gosec // Executable helper fixture, isolated in the test's temporary directory. + require.NoError(t, os.WriteFile(filepath.Join(helperDir, "fusermount3"), []byte(script), 0o700)) + onlyMountHelpersIn(t, helperDir) + mountPoint := t.TempDir() + useFakeDetachMountTable(t, fuseTableLine(mountPoint)) + releaseAbandoned(func(ctx context.Context) error { + return unmountFUSEMount(ctx, mountPoint, fuseTypeFor(filesystemName)) + }, mountPoint, fuseTypeFor(filesystemName)) + require.FileExists(t, detached) + time.Sleep(1100 * time.Millisecond) + require.NoFileExists(t, late, "ordinary helper descendants must not survive the lazy detach") +} diff --git a/internal/filesystem/fuseadapter/unmount_lazy_cancel_test.go b/internal/filesystem/fuseadapter/unmount_lazy_cancel_test.go new file mode 100644 index 000000000..92d00e042 --- /dev/null +++ b/internal/filesystem/fuseadapter/unmount_lazy_cancel_test.go @@ -0,0 +1,33 @@ +package fuseadapter + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// TestUnmountLazyCancelsInFlightRequests pins that the shutdown fallback +// stops serving: a handler blocked in a context-aware backend call (the +// reason the plain unmount hung or a client kept the mount busy) has its +// request context cancelled, so it returns before the shard stores and the +// Raft runtimes behind it are closed, and a request arriving afterwards +// starts already cancelled. +func TestUnmountLazyCancelsInFlightRequests(t *testing.T) { + fs := NewRawFileSystem(nil) + server := &Server{fs: fs} + inFlight, done := fs.requestContext(nil) + defer done() + + require.NoError(t, server.UnmountLazy(context.Background())) + select { + case <-inFlight.Done(): + case <-time.After(5 * time.Second): + t.Fatal("the in-flight request was not cancelled by the lazy detach") + } + + late, lateDone := fs.requestContext(nil) + defer lateDone() + require.Error(t, late.Err(), "a request arriving after the lazy detach must start cancelled") +} diff --git a/internal/filesystem/fuseadapter/unmount_shutdown_join_test.go b/internal/filesystem/fuseadapter/unmount_shutdown_join_test.go new file mode 100644 index 000000000..cb9018574 --- /dev/null +++ b/internal/filesystem/fuseadapter/unmount_shutdown_join_test.go @@ -0,0 +1,43 @@ +package fuseadapter + +import ( + "context" + "testing" + "time" + + "github.com/hanwen/go-fuse/v2/fuse" + "github.com/stretchr/testify/require" +) + +func TestOrdinaryUnmountCancelsRequestsAfterMountRemoved(t *testing.T) { + raw := NewRawFileSystem(nil) + request, cancel := raw.requestContext(nil) + defer cancel() + useFakeDetachMountTable(t, "") + server := &Server{inner: &fuse.Server{}, fs: raw, mountPoint: t.TempDir(), fsType: fuseTypeFor(filesystemName)} + require.NoError(t, server.UnmountContext(t.Context())) + require.ErrorIs(t, request.Err(), context.Canceled, "successful unmount must cancel handlers even before Serve returns") +} + +func TestJoinAbandonedMountWaitsForCleanup(t *testing.T) { + entered, release := make(chan struct{}), make(chan struct{}) + done := make(chan bool, 1) + go func() { + done <- joinAbandonedMount(func() { close(entered); <-release }, time.Second) + }() + <-entered + select { + case <-done: + t.Fatal("startup returned before its late mount cleanup") + case <-time.After(time.Millisecond): + } + close(release) + require.True(t, <-done) +} + +func TestJoinAbandonedMountHasBound(t *testing.T) { + release, finished := make(chan struct{}), make(chan struct{}) + require.False(t, joinAbandonedMount(func() { <-release; close(finished) }, time.Millisecond)) + close(release) + <-finished +} diff --git a/internal/filesystem/fuseadapter/wait_mount_cancel_test.go b/internal/filesystem/fuseadapter/wait_mount_cancel_test.go new file mode 100644 index 000000000..d2fe0e951 --- /dev/null +++ b/internal/filesystem/fuseadapter/wait_mount_cancel_test.go @@ -0,0 +1,141 @@ +//go:build linux + +package fuseadapter + +import ( + "context" + "os" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/bootjp/elastickv/internal/filesystem" + "github.com/bootjp/elastickv/internal/filesystem/fstest" + "github.com/hanwen/go-fuse/v2/fuse" + "github.com/stretchr/testify/require" +) + +// getAttrBlockingCore passes everything through to the wrapped backend +// except GetAttr, which once armed blocks until release or until its request +// context ends, the way a backend read does while no leader is available. +type getAttrBlockingCore struct { + Core + armed atomic.Bool + entered chan struct{} + release chan struct{} + once sync.Once +} + +func (c *getAttrBlockingCore) GetAttr(ctx context.Context, inode uint64) (filesystem.Stat, error) { + if c.armed.Load() { + c.once.Do(func() { close(c.entered) }) + select { + case <-c.release: + case <-ctx.Done(): + return filesystem.Stat{}, ctx.Err() //nolint:wrapcheck // test double reports the cancellation as is. + } + } + return c.Core.GetAttr(ctx, inode) //nolint:wrapcheck // pass-through test double. +} + +// TestWaitMountStopsWhenCancelledWhileReadinessBlocks mounts for real and +// blocks the backend getattr that go-fuse's readiness check (the poll hack) +// triggers through the kernel's permission check on the root. Cancelling the +// MountContext context must end WaitMount, cancel the in-flight backend +// request so the kernel gets its answer (the caller of a request already +// sent to the server waits for it uninterruptibly, and here that caller is +// this very process), release the mount within its bound, and let Unmount +// and the serve loop return, so a SIGTERM during that window still reaches +// the bounded shutdown and the WAL sync. +func TestWaitMountStopsWhenCancelledWhileReadinessBlocks(t *testing.T) { + old := abandonedUnmountTimeout + abandonedUnmountTimeout = 200 * time.Millisecond + t.Cleanup(func() { abandonedUnmountTimeout = old }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + m := mountWithBlockedReadiness(t, ctx) + + cancel() + select { + case err := <-m.waitDone: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(15 * time.Second): + t.Fatal("WaitMount did not return after its context was cancelled") + } + unmountDone := make(chan error, 1) + go func() { unmountDone <- m.server.Unmount() }() + select { + case err := <-unmountDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Unmount blocked on the mount WaitMount had already released") + } + select { + case <-m.serveDone: + case <-time.After(5 * time.Second): + t.Fatal("Serve did not return after the mount was released") + } + top, found, err := topmostMount(context.Background(), mountTablePath, m.mountPoint) + require.NoError(t, err) + require.False(t, found && top.fsType == m.server.fsType, "the released mount is still attached") +} + +// blockedReadinessMount is a kernel mount whose WaitMount is stuck in the +// backend getattr of go-fuse's readiness check. +type blockedReadinessMount struct { + server *Server + mountPoint string + serveDone <-chan struct{} + waitDone <-chan error +} + +// mountWithBlockedReadiness mounts under ctx, arms the getattr block, starts +// Serve and WaitMount, and returns once the readiness check is blocked in +// the backend with WaitMount still waiting. +func mountWithBlockedReadiness(t *testing.T, ctx context.Context) blockedReadinessMount { + t.Helper() + if _, err := os.Stat("/dev/fuse"); err != nil { + if os.Getenv("ELASTICKV_REQUIRE_FUSE") != "" { + t.Fatalf("no /dev/fuse: %v", err) + } + t.Skipf("no /dev/fuse: %v", err) + } + svc, err := fstest.NewMemoryService() + require.NoError(t, err) + uid, gid := uint32(os.Getuid()), uint32(os.Getgid()) //nolint:gosec // test process ids fit uint32. + require.NoError(t, svc.InitializeRoot(t.Context(), 0o755, uid, gid)) + // The mount point is created before the release is registered so that + // the release runs first at cleanup: removing the directory stats it, + // and that getattr must not block. + mountPoint := t.TempDir() + core := &getAttrBlockingCore{Core: svc, entered: make(chan struct{}), release: make(chan struct{})} + t.Cleanup(func() { close(core.release) }) + server, err := MountContext(ctx, mountPoint, New(core, []byte("readiness")), &fuse.MountOptions{DirectMount: os.Geteuid() == 0}) + if err != nil { + if os.Getenv("ELASTICKV_REQUIRE_FUSE") != "" { + t.Fatalf("FUSE mount unavailable: %v", err) + } + t.Skipf("FUSE mount unavailable: %v", err) + } + core.armed.Store(true) + serveDone := make(chan struct{}) + go func() { + defer close(serveDone) + server.Serve() + }() + waitDone := make(chan error, 1) + go func() { waitDone <- server.WaitMount() }() + + select { + case <-core.entered: + case <-time.After(10 * time.Second): + t.Fatal("the readiness check never reached the backend getattr") + } + select { + case err := <-waitDone: + t.Fatalf("WaitMount returned (%v) while the backend getattr was blocked", err) + case <-time.After(100 * time.Millisecond): + } + return blockedReadinessMount{server: server, mountPoint: mountPoint, serveDone: serveDone, waitDone: waitDone} +} diff --git a/main.go b/main.go index f4f9387c4..0b144fbc7 100644 --- a/main.go +++ b/main.go @@ -10,12 +10,14 @@ import ( "net" "net/http" "os" + "os/signal" "path/filepath" "slices" "strconv" "strings" "sync" "sync/atomic" + "syscall" "time" "github.com/bootjp/elastickv/adapter" @@ -38,6 +40,7 @@ import ( pb "github.com/bootjp/elastickv/proto" "github.com/bootjp/elastickv/store" "github.com/cockroachdb/errors" + "github.com/hanwen/go-fuse/v2/fuse" "golang.org/x/sync/errgroup" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -285,6 +288,21 @@ var memoryPressureExit atomic.Bool // so log scraping can tell them apart. const exitCodeMemoryPressure = 2 +// shutdownSignals are the signals that start a graceful stop. Without a +// handler Go's default disposition ends the process immediately, which +// skips the CleanupStack: the WAL is not synced and a FUSE mount stays +// attached to a dead server (`Transport endpoint is not connected`). +var shutdownSignals = []os.Signal{syscall.SIGTERM, syscall.SIGINT} + +// shutdownSignalHandlerJoinTimeout bounds how long stopping the signal +// handler waits for its goroutine, which a blocked log write can hold. +const shutdownSignalHandlerJoinTimeout = time.Second + +// signalShutdown is set by the signal handler once a shutdown signal has +// cancelled the root context, so main can tell a requested stop from a +// failure when the errgroup surfaces context.Canceled while unwinding. +var signalShutdown atomic.Bool + // memoryShutdownThresholdEnvVar configures the heap-inuse ceiling at // which memwatch triggers a graceful shutdown. Empty or "0" disables the // watchdog (the default; existing operators see no behaviour change). @@ -319,11 +337,49 @@ func main() { } os.Exit(exitCodeMemoryPressure) } - if err != nil { + if err := ignoreCanceledAfterSignal(err); err != nil { log.Fatalf("%v", err) } } +// ignoreCanceledAfterSignal maps the context.Canceled an errgroup member may +// surface while unwinding after SIGTERM/SIGINT to a clean exit. The +// cancellation is the shutdown the operator asked for, not a failure; any +// other error (or a cancellation nobody requested) is reported unchanged. +// A joined error (waitErrgroupAfterStartupFailure combines the startup error +// with the drain error) is only clean when every branch is a cancellation. +func ignoreCanceledAfterSignal(err error) error { + if err == nil || !signalShutdown.Load() || !onlyContextCanceled(err) { + return err + } + slog.Info("graceful shutdown after signal completed") + return nil +} + +// onlyContextCanceled reports whether err carries nothing but +// context.Canceled: a plain or wrapped cancellation, or a joined error whose +// every branch is one. errors.Is alone would accept a join that also holds a +// real failure, hiding it behind exit status 0. +func onlyContextCanceled(err error) bool { + if err == nil { + return false + } + var joined interface{ Unwrap() []error } + if errors.As(err, &joined) { + branches := joined.Unwrap() + if len(branches) == 0 { + return false + } + for _, branch := range branches { + if !onlyContextCanceled(branch) { + return false + } + } + return true + } + return errors.Is(err, context.Canceled) +} + // memwatchConfigFromEnv resolves the memwatch Config from environment // variables. It returns (cfg, true) when the watcher should run, or // (_, false) when the operator has not opted in (the default). Errors in @@ -475,6 +531,19 @@ func run() error { readTracker := kv.NewActiveTimestampTracker(kv.WithActiveTimestampTrackerMaxBackupPins(*backupMaxActivePins)) s3BlobBackfiller := adapter.NewS3BlobBackfiller(cfg.s3BlobBackfillConfig) + cleanup := internalutil.CleanupStack{} + defer cleanup.Run() + // The root context and the SIGTERM/SIGINT handler are installed before + // the Raft/Pebble runtimes are opened, so a stop requested during a slow + // open or snapshot restore is recorded (ctx is cancelled) instead of + // killing the process mid-open through the default disposition; startup + // then unwinds through the normal cancelled-context path once the + // runtimes exist. The handler is registered before the store/runtime + // closers so it is still swallowing repeat signals while CleanupStack + // syncs and closes them. + ctx, cancel := context.WithCancel(context.Background()) + cleanup.Add(installShutdownSignalHandler(cancel)) + // Stage 6D-6c: buildShardGroupsWithEncryptionWiring assembles the // storage-envelope write-path wiring (cipher + deterministic nonce // factory + the process-shared StateCache) before opening any @@ -529,11 +598,7 @@ func run() error { // shards share the same env and therefore the same label). recordFSMApplySyncMode(metricsRegistry, runtimes) - cleanup := internalutil.CleanupStack{} - defer cleanup.Run() cleanup.Add(readTracker.Close) - - ctx, cancel := context.WithCancel(context.Background()) shardStore := kv.NewShardStore(cfg.engine, shardGroups) cleanup.Add(func() { _ = shardStore.Close() @@ -604,7 +669,11 @@ func run() error { tsoWiring: tsoWiring, }) if err != nil { - return err + // Members already run on eg (TSO reloader, raft lifecycle + // watchers, the registration gate); drain them before the deferred + // CleanupStack closes the stores they still use. A SIGTERM during + // the registration retry surfaces here as context.Canceled. + return waitErrgroupAfterStartupFailure(cancel, eg, err) } defaultRuntime := distStartup.defaultRuntime distServer := distStartup.distServer @@ -853,6 +922,9 @@ type filesystemStartConfig struct { rootMode uint32 rootUID uint32 rootGID uint32 + // mountOptions overrides go-fuse's mount options. Production leaves it + // nil (fusermount3 path); tests set DirectMount where no helper exists. + mountOptions *fuse.MountOptions } func filesystemStartConfigFromFlags() (filesystemStartConfig, error) { @@ -912,7 +984,7 @@ func startFilesystemIfEnabled( ); err != nil { return errors.Wrap(err, "initialize filesystem root") } - server, serveDone, err := mountFilesystemService(service, config) + server, serveDone, err := mountFilesystemService(ctx, service, config) if err != nil { return err } @@ -923,12 +995,17 @@ func startFilesystemIfEnabled( return nil } +// mountFilesystemService mounts service at the configured mount point. ctx +// bounds only the pre-mount work (stale-mount detach, intent recovery) so a +// shutdown signal during startup is not held up by it; the mount itself is +// ended by the lifecycle installed afterwards. func mountFilesystemService( + ctx context.Context, service *filesystem.Service, config filesystemStartConfig, ) (*fuseadapter.Server, <-chan struct{}, error) { frontend := fuseadapter.New(service, []byte(config.clientID)) - server, err := fuseadapter.Mount(config.mountPoint, frontend, nil) + server, err := fuseadapter.MountContext(ctx, config.mountPoint, frontend, config.mountOptions) if err != nil { frontend.Close() return nil, nil, errors.Wrap(err, "mount filesystem frontend") @@ -946,27 +1023,59 @@ func mountFilesystemService( return server, serveDone, nil } +// filesystemMountServer is the part of *fuseadapter.Server the shutdown +// lifecycle drives; tests substitute a fake to exercise the failure paths. +type filesystemMountServer interface { + Unmount() error + UnmountContext(context.Context) error + UnmountLazy(context.Context) error +} + +// filesystemServeStopTimeout bounds how long a signal shutdown waits for the +// FUSE serve loop after the unmount. After a plain unmount failed (EBUSY: a +// client still holds files open) the mount is detached lazily and the loop +// only ends once those handles close, which may never happen; the shutdown +// proceeds after this wait so a SIGTERM cannot hang the process until +// SIGKILL. A variable so tests can shorten it. +var filesystemServeStopTimeout = 10 * time.Second + func installFilesystemServerLifecycle( ctx context.Context, eg *errgroup.Group, cleanup *internalutil.CleanupStack, - server *fuseadapter.Server, + server filesystemMountServer, serveDone <-chan struct{}, mountPoint string, ) { - unmount := sync.OnceValue(server.Unmount) + unmountCtx, cancelUnmount := context.WithCancel(context.Background()) + unmounter := newFilesystemUnmounter(func() error { return server.UnmountContext(unmountCtx) }) + unmounter.cancel = cancelUnmount cleanup.Add(func() { - if err := unmount(); err != nil { - slog.Warn("filesystem FUSE unmount failed", "mount_point", mountPoint, "err", err) + // Normally the lifecycle goroutine has already run the shutdown by + // the time CleanupStack runs (eg.Wait precedes it); then there is + // nothing left to do, and re-waiting on an unmount that is stuck in + // a backend request would hang the WAL sync and store close that + // follow. Only an unmount nobody started yet is run here, bounded. + if unmounter.handled.Load() { + return + } + select { + case <-unmounter.begin(): + if err := unmounter.result(); err != nil { + slog.Warn("filesystem FUSE unmount failed", "mount_point", mountPoint, "err", err) + } + case <-time.After(filesystemServeStopTimeout): + cancelUnmount() + slog.Error("filesystem FUSE unmount did not return in time during cleanup, continuing", + "mount_point", mountPoint, + "timeout", filesystemServeStopTimeout, + ) } }) eg.Go(func() error { select { case <-ctx.Done(): - if err := unmount(); err != nil { - slog.WarnContext(ctx, "filesystem FUSE unmount failed", "mount_point", mountPoint, "err", err) - } - <-serveDone + stopFilesystemServer(ctx, unmounter, server, serveDone, mountPoint) return nil case <-serveDone: select { @@ -981,6 +1090,125 @@ func installFilesystemServerLifecycle( }) } +// stopFilesystemServer unmounts during a shutdown within one bounded budget +// (filesystemServeStopTimeout). The plain unmount runs in its own goroutine +// because a plain unmount can block or fail with EBUSY while a client holds +// files open. Its helper is context-bound and must be killed and reaped +// before the lazy detach can start. The mount is then detached lazily, which +// also cancels the requests still in flight so a handler blocked in the +// backend returns before the stores close, and the wait for the serve loop +// is bounded, so the rest of the shutdown (WAL sync, store close) still runs +// and the process exits. Exiting closes the /dev/fuse descriptor, which +// makes the kernel drop whatever the lazy detach left behind. +func stopFilesystemServer( + ctx context.Context, + unmounter *filesystemUnmounter, + server filesystemMountServer, + serveDone <-chan struct{}, + mountPoint string, +) { + defer unmounter.handled.Store(true) + // ctx is already cancelled; the whole shutdown shares one budget. + stopCtx, cancel := context.WithTimeout(context.Background(), filesystemServeStopTimeout) + defer cancel() + // The plain unmount may use most of the budget, but a slice is kept so + // the lazy detach still runs inside it when the unmount hangs. + unmountWait := time.NewTimer(filesystemServeStopTimeout - filesystemServeStopTimeout/filesystemLazyDetachShare) + defer unmountWait.Stop() + select { + case <-unmounter.begin(): + if err := unmounter.result(); err != nil { + slog.WarnContext(ctx, "filesystem FUSE unmount failed, detaching lazily", "mount_point", mountPoint, "err", err) + lazyDetachFilesystem(ctx, stopCtx, server, mountPoint) + } + case <-unmountWait.C: + slog.ErrorContext(ctx, "filesystem FUSE unmount did not return in time (a request may be stuck in the backend), detaching lazily and continuing shutdown", + "mount_point", mountPoint, + "timeout", filesystemServeStopTimeout, + ) + // Fall through: the reserved remainder of the budget still waits for + // the serve loop, so a request that finishes in time is not cut off + // by the store close that follows. + // Killing a context-aware unmount must also reap its helper before + // another path-based unmount can target a replacement filesystem. + if !unmounter.cancelAndJoin(stopCtx) { + slog.ErrorContext(ctx, "filesystem unmount helper did not exit; refusing concurrent lazy detach", "mount_point", mountPoint) + return + } + lazyDetachFilesystem(ctx, stopCtx, server, mountPoint) + } + select { + case <-serveDone: + case <-stopCtx.Done(): + slog.ErrorContext(ctx, "filesystem FUSE serve loop did not stop in time, continuing shutdown", + "mount_point", mountPoint, + "timeout", filesystemServeStopTimeout, + ) + } +} + +// filesystemUnmounter runs the context-bound unmount at most once, in the background, +// and lets any number of waiters observe its completion without a later +// caller ever blocking on an earlier call that is stuck (go-fuse's Unmount +// waits for an event loop that may be held by a hung backend request; +// sync.OnceValue would make the second caller wait for the first). +type filesystemUnmounter struct { + run func() error + cancel context.CancelFunc + started atomic.Bool + done chan struct{} + err error + // handled is set once the lifecycle's shutdown has run to completion + // (unmounted, or gave up after its budget), so the CleanupStack entry + // knows there is nothing left to wait for. + handled atomic.Bool +} + +func (u *filesystemUnmounter) cancelAndJoin(ctx context.Context) bool { + if u.cancel == nil { + return true + } + u.cancel() + select { + case <-u.done: + return true + case <-ctx.Done(): + return false + } +} + +func newFilesystemUnmounter(run func() error) *filesystemUnmounter { + return &filesystemUnmounter{run: run, done: make(chan struct{})} +} + +// begin starts the unmount the first time it is called and returns a channel +// that is closed when it finishes; later calls return the same channel. +func (u *filesystemUnmounter) begin() <-chan struct{} { + if u.started.CompareAndSwap(false, true) { + go func() { + defer close(u.done) + u.err = u.run() + }() + } + return u.done +} + +// result is the unmount's error; valid once the channel from begin is closed. +func (u *filesystemUnmounter) result() error { return u.err } + +// filesystemLazyDetachShare is the fraction (1/n) of filesystemServeStopTimeout +// held back from the plain-unmount wait so the lazy detach fallback still +// runs inside the one advertised budget when the unmount hangs. +const filesystemLazyDetachShare = 5 + +// lazyDetachFilesystem runs the lazy detach fallback within the shutdown's +// remaining budget (stopCtx); logCtx only correlates the log line. +func lazyDetachFilesystem(logCtx, stopCtx context.Context, server filesystemMountServer, mountPoint string) { + if err := server.UnmountLazy(stopCtx); err != nil { + slog.WarnContext(logCtx, "filesystem FUSE lazy detach failed", "mount_point", mountPoint, "err", err) + } +} + func startFilesystemPlacementCollector( ctx context.Context, eg *errgroup.Group, @@ -2210,14 +2438,26 @@ func startServersAfterStartupRotation(waitRotateOnStartup startupRotationWaiter, return err } runner.startAdminHTTP() - return startFilesystemIfEnabled( + return startFilesystemAfterServing(in) +} + +// startFilesystemAfterServing mounts the optional FUSE filesystem once the +// public services are up. On failure the public services are already serving +// on eg, so they are drained before the deferred CleanupStack closes the +// stores they use; a shutdown signal during the FUSE pre-mount work surfaces +// here as context.Canceled and must not skip that drain either. +func startFilesystemAfterServing(in serversInput) error { + if err := startFilesystemIfEnabled( in.ctx, in.eg, in.cleanup, in.shardStore, in.coordinate, in.metricsRegistry.FileSystemObserver(), - ) + ); err != nil { + return waitErrgroupAfterStartupFailure(in.cancel, in.eg, err) + } + return nil } func startupRoleStoreFromFlags(enabled bool) admin.RoleStore { @@ -3109,6 +3349,86 @@ func loadS3BlobPeerTokenFile(path string) (string, error) { return tok, nil } +// grpcGracefulStopTimeout bounds how long a gRPC server's GracefulStop may +// wait for in-flight RPCs during shutdown. Long-lived streams (catalog +// watches, the Raft transport's SendStream) only end when their client goes +// away, so without a bound a SIGTERM would wait on them for as long as a +// peer stays connected while repeat signals are swallowed. After the bound, +// Stop closes the remaining streams. A variable so tests can shorten it. +var grpcGracefulStopTimeout = 10 * time.Second + +// stopGRPCServerBounded drains srv for at most grpcGracefulStopTimeout and +// then forces Stop, which also makes the pending GracefulStop return. +func stopGRPCServerBounded(ctx context.Context, srv *grpc.Server, name string) { + done := make(chan struct{}) + go func() { + srv.GracefulStop() + close(done) + }() + timer := time.NewTimer(grpcGracefulStopTimeout) + defer timer.Stop() + select { + case <-done: + case <-timer.C: + slog.WarnContext(ctx, "gRPC graceful stop did not finish in time, closing remaining streams", + "server", name, + "timeout", grpcGracefulStopTimeout, + ) + srv.Stop() + <-done + } +} + +// installShutdownSignalHandler routes SIGTERM/SIGINT (or the given signals) +// into cancel so the process unwinds through the same path the memory +// watchdog uses: the errgroup drains, the FUSE mount is unmounted, and the +// CleanupStack syncs the WAL. The first signal cancels; later ones are +// logged and otherwise ignored so a second SIGTERM from a supervisor cannot +// cut the shutdown short (SIGKILL still can, which no handler prevents). +// The returned function releases the registration and must be called once +// the cleanup that needs protection has run. +func installShutdownSignalHandler(cancel context.CancelFunc, signals ...os.Signal) func() { + if len(signals) == 0 { + signals = shutdownSignals + } + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, signals...) + stopCh := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case sig := <-sigCh: + if signalShutdown.CompareAndSwap(false, true) { + // Cancel before logging: a log write that blocks (a + // backpressured stderr, a synchronous shipper) must not + // hold the shutdown back, and later signals are ignored. + cancel() + slog.Info("shutdown signal received, stopping", "signal", sig.String()) + continue + } + slog.Warn("shutdown already in progress, ignoring repeated signal", "signal", sig.String()) + case <-stopCh: + return + } + } + }() + return sync.OnceFunc(func() { + signal.Stop(sigCh) + close(stopCh) + // Wait for the goroutine, but bounded: it may be stuck in a log + // write that never completes, and this runs at the very end of the + // shutdown, which must not hang on it. + timer := time.NewTimer(shutdownSignalHandlerJoinTimeout) + defer timer.Stop() + select { + case <-done: + case <-timer.C: + } + }) +} + // startMemoryWatchdog optionally starts the memwatch goroutine. The // watcher is off by default; it is enabled only when the operator sets // ELASTICKV_MEMORY_SHUTDOWN_THRESHOLD_MB. On threshold crossing the @@ -3435,7 +3755,7 @@ func startRaftServers( go func() { select { case <-ctx.Done(): - srv.GracefulStop() + stopGRPCServerBounded(ctx, srv, rt.spec.address) _ = lis.Close() closeService() case <-stop: diff --git a/main_admin.go b/main_admin.go index 0004842e5..2fc529c5a 100644 --- a/main_admin.go +++ b/main_admin.go @@ -17,6 +17,7 @@ import ( "github.com/bootjp/elastickv/internal/raftengine" "github.com/bootjp/elastickv/keyviz" "github.com/bootjp/elastickv/kv" + "github.com/bootjp/elastickv/monitoring" "github.com/cockroachdb/errors" "golang.org/x/sync/errgroup" ) @@ -33,7 +34,6 @@ const ( adminReadHeaderTimeout = 5 * time.Second adminWriteTimeout = 10 * time.Second adminIdleTimeout = 30 * time.Second - adminShutdownTimeout = 5 * time.Second ) // adminBuildVersion is surfaced by both GET /admin/api/v1/cluster and the @@ -1219,6 +1219,11 @@ func newAdminHTTPServer(server *admin.Server) *http.Server { } } +// adminShutdownTimeout bounds how long the admin listener waits for +// in-flight requests on shutdown before the remaining connections are +// closed (monitoring.ShutdownHTTPServer). A variable so tests can shorten it. +var adminShutdownTimeout = 5 * time.Second + func registerAdminLifecycle( ctx context.Context, eg *errgroup.Group, @@ -1229,15 +1234,12 @@ func registerAdminLifecycle( version string, ) { addr := adminCfg.Listen + handlers := trackAdminHandlers(httpSrv) eg.Go(func() error { <-ctx.Done() slog.Info("shutting down admin listener", "address", addr, "reason", ctx.Err()) - shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), adminShutdownTimeout) - defer cancel() - err := httpSrv.Shutdown(shutdownCtx) - if err == nil || errors.Is(err, http.ErrServerClosed) || errors.Is(err, net.ErrClosed) { - return nil - } + err := monitoring.ShutdownHTTPServer(ctx, httpSrv, "admin", adminShutdownTimeout) + handlers.join(adminHandlerJoinTimeout) return errors.WithStack(err) }) eg.Go(func() error { diff --git a/main_admin_dynamo_stopping_test.go b/main_admin_dynamo_stopping_test.go new file mode 100644 index 000000000..3828e58d5 --- /dev/null +++ b/main_admin_dynamo_stopping_test.go @@ -0,0 +1,20 @@ +package main + +import ( + "testing" + + "github.com/bootjp/elastickv/adapter" + "github.com/bootjp/elastickv/internal/admin" + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" +) + +// An admin table or item request refused because the DynamoDB server +// already began stopping is answered like a follower's (503 with +// Retry-After), so the client retries against another node instead of +// seeing a 500. +func TestTranslateAdminDynamoErrorsMapStoppingToNotLeader(t *testing.T) { + err := errors.Wrap(adapter.ErrDynamoServerStopping, "admin delete table") + require.ErrorIs(t, translateAdminTablesError(err), admin.ErrTablesNotLeader) + require.ErrorIs(t, translateAdminItemsError(err), admin.ErrItemsNotLeader) +} diff --git a/main_admin_lifecycle_test.go b/main_admin_lifecycle_test.go new file mode 100644 index 000000000..0aa1be5d8 --- /dev/null +++ b/main_admin_lifecycle_test.go @@ -0,0 +1,92 @@ +package main + +import ( + "context" + "net" + "net/http" + "testing" + "time" + + "github.com/bootjp/elastickv/adapter" + "github.com/bootjp/elastickv/internal/admin" + "github.com/cockroachdb/errors" + "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" +) + +// TestRegisterAdminLifecycleReturnsCleanAfterForcedClose pins that an admin +// request outliving the drain window ends the admin tasks with a forced +// close and a nil error. Previously the shutdown task returned +// context.DeadlineExceeded, which is not a pure cancellation, so a SIGTERM +// during a slow admin request made run() report a failure and the process +// exit 1 instead of the documented status 0. +func TestRegisterAdminLifecycleReturnsCleanAfterForcedClose(t *testing.T) { + old := adminShutdownTimeout + adminShutdownTimeout = 200 * time.Millisecond + t.Cleanup(func() { adminShutdownTimeout = old }) + + lis, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + started := make(chan struct{}, 1) + httpSrv := &http.Server{ + Handler: http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case started <- struct{}{}: + default: + } + <-r.Context().Done() + }), + ReadHeaderTimeout: time.Second, + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + eg, egCtx := errgroup.WithContext(ctx) + registerAdminLifecycle(egCtx, eg, httpSrv, lis, &admin.Config{Listen: lis.Addr().String()}, false, "test") + + reqCtx, reqCancel := context.WithCancel(context.Background()) + defer reqCancel() + req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, "http://"+lis.Addr().String()+"/", http.NoBody) + require.NoError(t, err) + client := &http.Client{Transport: &http.Transport{DisableKeepAlives: true}} + reqDone := make(chan error, 1) + go func() { + resp, err := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + reqDone <- err + }() + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("admin handler did not start for the stalled request") + } + + start := time.Now() + cancel() + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err, "a forced close after the admin drain window must not fail the shutdown") + case <-time.After(5 * time.Second): + t.Fatal("the admin tasks did not drain after cancellation") + } + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "the drain window must be given first") + require.Less(t, elapsed, 3*time.Second, "the shutdown must not wait on the stalled request") + select { + case err := <-reqDone: + require.Error(t, err, "the stalled admin request must be cut off") + case <-time.After(5 * time.Second): + t.Fatal("the stalled admin request was not closed") + } +} + +// An admin object request that reaches an S3 server which already began +// stopping is answered like a follower's (503 with Retry-After), so the +// client retries against another node instead of seeing a 500. +func TestTranslateAdminObjectsErrorMapsStoppingToNotLeader(t *testing.T) { + err := translateAdminObjectsError(errors.Wrap(adapter.ErrAdminS3Stopping, "admin put")) + require.ErrorIs(t, err, admin.ErrObjectsNotLeader) +} diff --git a/main_admin_shutdown.go b/main_admin_shutdown.go new file mode 100644 index 000000000..5bb0da391 --- /dev/null +++ b/main_admin_shutdown.go @@ -0,0 +1,71 @@ +package main + +import ( + "log/slog" + "net/http" + "sync" + "time" +) + +var adminHandlerJoinTimeout = 2 * time.Second + +type adminHandlerDrain struct { + mu sync.Mutex + active int + stopping bool + done chan struct{} +} + +func trackAdminHandlers(server *http.Server) *adminHandlerDrain { + d := &adminHandlerDrain{done: make(chan struct{})} + handler := server.Handler + if handler == nil { + handler = http.DefaultServeMux + } + server.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !d.enter() { + http.Error(w, "server is stopping", http.StatusServiceUnavailable) + return + } + defer d.exit() + handler.ServeHTTP(w, r) + }) + return d +} + +func (d *adminHandlerDrain) enter() bool { + d.mu.Lock() + defer d.mu.Unlock() + if d.stopping { + return false + } + d.active++ + return true +} + +func (d *adminHandlerDrain) exit() { + d.mu.Lock() + defer d.mu.Unlock() + d.active-- + if d.stopping && d.active == 0 { + close(d.done) + } +} + +func (d *adminHandlerDrain) join(timeout time.Duration) { + d.mu.Lock() + if !d.stopping { + d.stopping = true + if d.active == 0 { + close(d.done) + } + } + d.mu.Unlock() + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case <-d.done: + case <-timer.C: + slog.Warn("admin handlers still running after forced connection close", "timeout", timeout) + } +} diff --git a/main_admin_shutdown_test.go b/main_admin_shutdown_test.go new file mode 100644 index 000000000..330a4811e --- /dev/null +++ b/main_admin_shutdown_test.go @@ -0,0 +1,93 @@ +package main + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/bootjp/elastickv/internal/admin" + "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" +) + +func TestAdminHandlerDrainJoinsHandlerAfterForcedClose(t *testing.T) { + entered, release := make(chan struct{}), make(chan struct{}) + srv := &http.Server{ReadHeaderTimeout: time.Second, Handler: http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + close(entered) + <-release + })} + drain := trackAdminHandlers(srv) + go srv.Handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)) + <-entered + done := make(chan struct{}) + go func() { drain.join(time.Second); close(done) }() + select { + case <-done: + t.Fatal("join returned while the admitted handler was running") + case <-time.After(10 * time.Millisecond): + } + close(release) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("handler did not join") + } + response := httptest.NewRecorder() + srv.Handler.ServeHTTP(response, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)) + require.Equal(t, http.StatusServiceUnavailable, response.Code) +} + +func TestAdminLifecycleWaitsForForceClosedHandler(t *testing.T) { + old := adminShutdownTimeout + adminShutdownTimeout = 10 * time.Millisecond + t.Cleanup(func() { adminShutdownTimeout = old }) + entered, canceled, release := make(chan struct{}), make(chan struct{}), make(chan struct{}) + srv := &http.Server{ReadHeaderTimeout: time.Second, Handler: http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + close(entered) + <-r.Context().Done() + close(canceled) + <-release + })} + listenConfig := &net.ListenConfig{} + listener, err := listenConfig.Listen(t.Context(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + group := &errgroup.Group{} + registerAdminLifecycle(ctx, group, srv, listener, &admin.Config{Listen: listener.Addr().String()}, false, "test") + clientDone := make(chan struct{}) + request, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://"+listener.Addr().String(), nil) + require.NoError(t, err) + go func() { + defer close(clientDone) + client := &http.Client{Timeout: time.Second} + response, requestErr := client.Do(request) + if requestErr == nil { + _ = response.Body.Close() + } + }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("admin handler did not start") + } + cancel() + select { + case <-canceled: + case <-time.After(time.Second): + t.Fatal("admin connection was not forced closed") + } + done := make(chan error, 1) + go func() { done <- group.Wait() }() + select { + case <-done: + t.Fatal("admin lifecycle returned before its handler unwound") + case <-time.After(10 * time.Millisecond): + } + close(release) + require.NoError(t, <-done) + <-clientDone +} diff --git a/main_filesystem_test.go b/main_filesystem_test.go index 98609544b..89bf536eb 100644 --- a/main_filesystem_test.go +++ b/main_filesystem_test.go @@ -173,7 +173,24 @@ func TestMountFilesystemServiceFailsOnMissingMountPoint(t *testing.T) { clientID: "mount-a", } - server, serveDone, err := mountFilesystemService(svc, config) + server, serveDone, err := mountFilesystemService(context.Background(), svc, config) + require.ErrorContains(t, err, "mount filesystem frontend") + require.Nil(t, server) + require.Nil(t, serveDone) +} + +// TestMountFilesystemServiceAbortsOnCancelledContext pins that a shutdown +// requested while the filesystem is still starting stops the pre-mount work +// instead of letting it run to completion (or to the five-minute recovery +// timeout) while the signal handler swallows repeat signals. +func TestMountFilesystemServiceAbortsOnCancelledContext(t *testing.T) { + svc := newFilesystemTestService(t) + config := filesystemStartConfig{mountPoint: t.TempDir(), clientID: "mount-cancel"} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + server, serveDone, err := mountFilesystemService(ctx, svc, config) + require.ErrorIs(t, err, context.Canceled) require.ErrorContains(t, err, "mount filesystem frontend") require.Nil(t, server) require.Nil(t, serveDone) @@ -194,9 +211,9 @@ func TestMountFilesystemServiceServesKernelMount(t *testing.T) { skipOrFailWithoutFuse(t, "no /dev/fuse: %v", err) } svc := newFilesystemTestService(t) - config := filesystemStartConfig{mountPoint: t.TempDir(), clientID: "mount-a"} + config := filesystemStartConfig{mountPoint: t.TempDir(), clientID: "mount-a", mountOptions: testFUSEMountOptions()} - server, serveDone, err := mountFilesystemService(svc, config) + server, serveDone, err := mountFilesystemService(context.Background(), svc, config) if err != nil { skipOrFailWithoutFuse(t, "FUSE mount unavailable: %v", err) } @@ -258,6 +275,231 @@ func TestInstallFilesystemServerLifecycleUnmountsOnContextCancel(t *testing.T) { cleanup.Run() } +// fakeFilesystemMountServer stands in for *fuseadapter.Server so the unmount +// failure paths of the lifecycle can be driven without a kernel mount. +type fakeFilesystemMountServer struct { + unmountErr error + // unmountBlock, when set, makes Unmount block until it is closed, like + // go-fuse waiting for an event loop stuck in a backend request. + unmountBlock <-chan struct{} + lazyErr error + onLazy func() + lazyCalls int +} + +func (f *fakeFilesystemMountServer) Unmount() error { + if f.unmountBlock != nil { + <-f.unmountBlock + } + return f.unmountErr +} + +func (f *fakeFilesystemMountServer) UnmountContext(ctx context.Context) error { + if f.unmountBlock != nil { + select { + case <-f.unmountBlock: + case <-ctx.Done(): + return ctx.Err() + } + } + return f.Unmount() +} + +func (f *fakeFilesystemMountServer) UnmountLazy(context.Context) error { + f.lazyCalls++ + if f.onLazy != nil { + f.onLazy() + } + return f.lazyErr +} + +func shortenFilesystemServeStopTimeout(t *testing.T, d time.Duration) { + t.Helper() + old := filesystemServeStopTimeout + filesystemServeStopTimeout = d + t.Cleanup(func() { filesystemServeStopTimeout = old }) +} + +// A plain unmount that fails (EBUSY: a client holds a file open) must fall +// back to a lazy detach; the serve loop then ends when the kernel releases +// the connection, here simulated by the fake closing serveDone. +func TestInstallFilesystemServerLifecycleDetachesLazilyWhenUnmountFails(t *testing.T) { + shortenFilesystemServeStopTimeout(t, 5*time.Second) + ctx, cancel := context.WithCancel(context.Background()) + eg, egCtx := errgroup.WithContext(ctx) + cleanup := &internalutil.CleanupStack{} + serveDone := make(chan struct{}) + server := &fakeFilesystemMountServer{unmountErr: errors.New("umount: device or resource busy")} + server.onLazy = func() { close(serveDone) } + + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, "/mnt/fs") + cancel() + + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("lifecycle did not stop after the lazy detach released the serve loop") + } + require.Equal(t, 1, server.lazyCalls) + cleanup.Run() +} + +// When even the lazy detach cannot end the serve loop (handles stay open), +// the shutdown must proceed after the bounded wait instead of hanging until +// SIGKILL. +func TestInstallFilesystemServerLifecycleBoundsWaitWhenServeLoopStalls(t *testing.T) { + shortenFilesystemServeStopTimeout(t, 50*time.Millisecond) + ctx, cancel := context.WithCancel(context.Background()) + eg, egCtx := errgroup.WithContext(ctx) + cleanup := &internalutil.CleanupStack{} + serveDone := make(chan struct{}) // never closed + server := &fakeFilesystemMountServer{ + unmountErr: errors.New("umount: device or resource busy"), + lazyErr: errors.New("fusermount: not permitted"), + } + + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, "/mnt/fs") + cancel() + + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("lifecycle hung on a serve loop that never stops") + } + require.Equal(t, 1, server.lazyCalls) + cleanup.Run() +} + +// go-fuse's Unmount waits for its event loops; one stuck in a backend +// request never returns. The shutdown must not wait on it past the budget. +func TestInstallFilesystemServerLifecycleBoundsHungUnmount(t *testing.T) { + shortenFilesystemServeStopTimeout(t, 50*time.Millisecond) + ctx, cancel := context.WithCancel(context.Background()) + eg, egCtx := errgroup.WithContext(ctx) + cleanup := &internalutil.CleanupStack{} + serveDone := make(chan struct{}) // never closed + block := make(chan struct{}) + release := sync.OnceFunc(func() { close(block) }) + t.Cleanup(release) + server := &fakeFilesystemMountServer{unmountBlock: block} + + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, "/mnt/fs") + cancel() + + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("lifecycle hung on an Unmount that never returns") + } + require.Equal(t, 1, server.lazyCalls) + // The CleanupStack entry must not re-wait on the unmount that is still + // stuck: the lifecycle already gave up on it, and the WAL sync and store + // close that follow in the real CleanupStack must still run. + cleanupDone := make(chan struct{}) + go func() { + cleanup.Run() + close(cleanupDone) + }() + select { + case <-cleanupDone: + case <-time.After(5 * time.Second): + t.Fatal("cleanup blocked on the hung unmount") + } + release() +} + +// After a hung unmount the lazy detach must still be followed by the wait +// for the serve loop inside the remaining budget: a backend request that +// finishes in that window ends the loop and the shutdown proceeds at once, +// before the stores are closed underneath it. +func TestInstallFilesystemServerLifecycleWaitsForServeLoopAfterHungUnmount(t *testing.T) { + // A budget large enough that scheduling jitter under -race or a loaded + // CI runner cannot push the observed end of the serve loop past the + // upper bound below. + shortenFilesystemServeStopTimeout(t, 2*time.Second) + ctx, cancel := context.WithCancel(context.Background()) + eg, egCtx := errgroup.WithContext(ctx) + cleanup := &internalutil.CleanupStack{} + serveDone := make(chan struct{}) + block := make(chan struct{}) + release := sync.OnceFunc(func() { close(block) }) + t.Cleanup(release) + server := &fakeFilesystemMountServer{unmountBlock: block} + // The lazy detach lets the loop finish shortly afterwards, inside the + // reserved slice of the budget. + server.onLazy = func() { + go func() { + time.Sleep(20 * time.Millisecond) + close(serveDone) + }() + } + + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, "/mnt/fs") + start := time.Now() + cancel() + + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("lifecycle did not stop") + } + elapsed := time.Since(start) + require.Equal(t, 1, server.lazyCalls) + // Past the 4/5 unmount wait (1.6 s), but well before the full budget: + // the loop ending was observed rather than timed out. + require.GreaterOrEqual(t, elapsed, 1600*time.Millisecond) + require.Less(t, elapsed, 1900*time.Millisecond, "serveDone closing must end the wait before the budget expires") + release() + cleanup.Run() +} + +// The CleanupStack entry is the fallback when the lifecycle goroutine never +// ran the shutdown (ctx not cancelled); a hung unmount must not block it past +// the budget either. +func TestInstallFilesystemServerLifecycleCleanupBoundsHungUnmount(t *testing.T) { + shortenFilesystemServeStopTimeout(t, 50*time.Millisecond) + ctx, cancel := context.WithCancel(context.Background()) + eg, egCtx := errgroup.WithContext(ctx) + cleanup := &internalutil.CleanupStack{} + serveDone := make(chan struct{}) + block := make(chan struct{}) + release := sync.OnceFunc(func() { close(block) }) + t.Cleanup(release) + server := &fakeFilesystemMountServer{unmountBlock: block} + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, "/mnt/fs") + + cleanupDone := make(chan struct{}) + go func() { + cleanup.Run() + close(cleanupDone) + }() + select { + case <-cleanupDone: + case <-time.After(5 * time.Second): + t.Fatal("cleanup blocked on the hung unmount") + } + + // Teardown: let the lifecycle goroutine finish. Cancel before the serve + // loop ends, as a real shutdown does; the other order is the "stopped + // unexpectedly" case the lifecycle reports as an error. + release() + cancel() + close(serveDone) + require.NoError(t, eg.Wait()) +} + func TestInstallFilesystemServerLifecycleTreatsServeExitAfterCancelAsClean(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) eg, egCtx := errgroup.WithContext(ctx) diff --git a/main_grpc_stop_test.go b/main_grpc_stop_test.go new file mode 100644 index 000000000..f5710182f --- /dev/null +++ b/main_grpc_stop_test.go @@ -0,0 +1,172 @@ +package main + +import ( + "context" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" +) + +// TestStopGRPCServerBoundedForcesStopOnLongLivedStream opens a stream whose +// handler only returns when its context ends, the shape of a catalog watch +// or a Raft transport stream. GracefulStop alone would wait on it for as +// long as the client stays connected; the bounded stop must force it closed +// after the timeout and return. +func TestStopGRPCServerBoundedForcesStopOnLongLivedStream(t *testing.T) { + old := grpcGracefulStopTimeout + grpcGracefulStopTimeout = 200 * time.Millisecond + t.Cleanup(func() { grpcGracefulStopTimeout = old }) + + lis, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + started := make(chan struct{}, 1) + srv := grpc.NewServer(grpc.UnknownServiceHandler(func(_ any, stream grpc.ServerStream) error { + select { + case started <- struct{}{}: + default: + } + <-stream.Context().Done() + return stream.Context().Err() + })) + serveDone := make(chan struct{}) + go func() { + defer close(serveDone) + _ = srv.Serve(lis) + }() + + conn, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + stream, err := conn.NewStream(ctx, &grpc.StreamDesc{StreamName: "Watch", ServerStreams: true, ClientStreams: true}, "/elastickv.test.LongLived/Watch") + require.NoError(t, err) + require.NotNil(t, stream) + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("server handler did not start for the long-lived stream") + } + + start := time.Now() + stopGRPCServerBounded(context.Background(), srv, "test") + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "GracefulStop must have been given its bound first") + require.Less(t, elapsed, 3*time.Second, "the stop must not wait on the client that never disconnects") + select { + case <-serveDone: + case <-time.After(5 * time.Second): + t.Fatal("Serve did not return after the forced stop") + } +} + +// A server with nothing in flight stops within GracefulStop, well before +// the bound, and nothing is forced. +func TestStopGRPCServerBoundedReturnsPromptlyWhenIdle(t *testing.T) { + lis, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + srv := grpc.NewServer() + serveDone := make(chan struct{}) + go func() { + defer close(serveDone) + _ = srv.Serve(lis) + }() + + start := time.Now() + stopGRPCServerBounded(context.Background(), srv, "idle") + require.Less(t, time.Since(start), 2*time.Second) + select { + case <-serveDone: + case <-time.After(5 * time.Second): + t.Fatal("Serve did not return after the stop") + } +} + +// newLongLivedStreamServer returns a gRPC server whose only handler holds +// the stream open until its context ends, plus a channel that receives once +// the handler has started and one that is closed when it returns. +func newLongLivedStreamServer() (srv *grpc.Server, started <-chan struct{}, ended <-chan struct{}) { + startedCh := make(chan struct{}, 1) + endedCh := make(chan struct{}) + srv = grpc.NewServer(grpc.UnknownServiceHandler(func(_ any, stream grpc.ServerStream) error { + select { + case startedCh <- struct{}{}: + default: + } + <-stream.Context().Done() + close(endedCh) + return stream.Context().Err() + })) + return srv, startedCh, endedCh +} + +// openLongLivedStream dials addr, opens a stream the server holds open for +// the rest of the test, and waits until the handler reports it started. +func openLongLivedStream(t *testing.T, addr string, started <-chan struct{}) { + t.Helper() + conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + streamCtx, streamCancel := context.WithCancel(context.Background()) + t.Cleanup(streamCancel) + _, err = conn.NewStream(streamCtx, &grpc.StreamDesc{StreamName: "Watch", ServerStreams: true, ClientStreams: true}, "/elastickv.test.LongLived/Watch") + require.NoError(t, err) + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("server handler did not start for the long-lived stream") + } +} + +// TestGRPCServeReturnsOnlyAfterBoundedStop pins the property the per-group +// serve task in startRaftServers relies on: grpc.Server.Serve does not +// return when GracefulStop closes the listener, only once the stop has +// finished, after the in-flight RPCs drained or the bound forced them +// closed. Were Serve to return earlier, the errgroup task would report the +// server as stopped while handlers were still running, and the cleanup +// would close the stores and Raft runtimes underneath them. +func TestGRPCServeReturnsOnlyAfterBoundedStop(t *testing.T) { + old := grpcGracefulStopTimeout + grpcGracefulStopTimeout = 200 * time.Millisecond + t.Cleanup(func() { grpcGracefulStopTimeout = old }) + + lis, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + srv, started, ended := newLongLivedStreamServer() + serveDone := make(chan error, 1) + go func() { serveDone <- srv.Serve(lis) }() + openLongLivedStream(t, lis.Addr().String(), started) + + stopDone := make(chan struct{}) + go func() { + defer close(stopDone) + stopGRPCServerBounded(context.Background(), srv, "test") + }() + // The listener is closed as soon as GracefulStop starts; Serve must still + // be blocked while the drain waits on the stream, well inside the bound. + select { + case err := <-serveDone: + t.Fatalf("Serve returned (%v) while the bounded stop was still draining", err) + case <-time.After(100 * time.Millisecond): + } + select { + case <-stopDone: + case <-time.After(5 * time.Second): + t.Fatal("the bounded stop did not finish") + } + select { + case err := <-serveDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("Serve did not return after the bounded stop") + } + select { + case <-ended: + default: + t.Fatal("Serve returned while the handler was still running") + } +} diff --git a/main_shutdown_signal_log_test.go b/main_shutdown_signal_log_test.go new file mode 100644 index 000000000..21c9389de --- /dev/null +++ b/main_shutdown_signal_log_test.go @@ -0,0 +1,89 @@ +package main + +import ( + "context" + "log/slog" + "os" + "sync" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// blockingLogHandler blocks every record until release is closed, the way +// a write to a backpressured stderr pipe or a synchronous log shipper does. +type blockingLogHandler struct { + release chan struct{} +} + +func (h *blockingLogHandler) Enabled(context.Context, slog.Level) bool { return true } + +func (h *blockingLogHandler) Handle(context.Context, slog.Record) error { + <-h.release + return nil +} + +func (h *blockingLogHandler) WithAttrs([]slog.Attr) slog.Handler { return h } + +func (h *blockingLogHandler) WithGroup(string) slog.Handler { return h } + +// The first signal must start the shutdown before the handler logs it: a +// log write that blocks would otherwise hold the cancellation back, and the +// signals that follow are swallowed on purpose. +func TestInstallShutdownSignalHandlerCancelsBeforeLogging(t *testing.T) { + resetSignalShutdown(t) + handler := &blockingLogHandler{release: make(chan struct{})} + release := sync.OnceFunc(func() { close(handler.release) }) + old := slog.Default() + slog.SetDefault(slog.New(handler)) + ctx, cancel := context.WithCancel(context.Background()) + stop := installShutdownSignalHandler(cancel, syscall.SIGUSR2) + t.Cleanup(func() { + release() + stop() + slog.SetDefault(old) + }) + + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGUSR2)) + select { + case <-ctx.Done(): + case <-time.After(shutdownTestTimeout): + t.Fatal("a blocked log write held back the shutdown") + } +} + +// The cleanup that stops the handler must not wait forever on a goroutine +// stuck in that log write: it runs at the end of the shutdown, after the +// WAL sync, and a hang there would keep the process from exiting. +func TestInstallShutdownSignalHandlerStopDoesNotWaitOnBlockedLog(t *testing.T) { + resetSignalShutdown(t) + handler := &blockingLogHandler{release: make(chan struct{})} + release := sync.OnceFunc(func() { close(handler.release) }) + old := slog.Default() + slog.SetDefault(slog.New(handler)) + ctx, cancel := context.WithCancel(context.Background()) + stop := installShutdownSignalHandler(cancel, syscall.SIGUSR2) + t.Cleanup(func() { + release() + slog.SetDefault(old) + }) + + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGUSR2)) + select { + case <-ctx.Done(): + case <-time.After(shutdownTestTimeout): + t.Fatal("the signal did not cancel the shutdown") + } + stopped := make(chan struct{}) + go func() { + stop() + close(stopped) + }() + select { + case <-stopped: + case <-time.After(shutdownTestTimeout): + t.Fatal("stopping the handler waited on the goroutine stuck in the log write") + } +} diff --git a/main_shutdown_signal_test.go b/main_shutdown_signal_test.go new file mode 100644 index 000000000..75e98cc1c --- /dev/null +++ b/main_shutdown_signal_test.go @@ -0,0 +1,224 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + internalutil "github.com/bootjp/elastickv/internal" + "github.com/cockroachdb/errors" + "github.com/hanwen/go-fuse/v2/fuse" + "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" +) + +const shutdownTestTimeout = 15 * time.Second + +// resetSignalShutdown clears the process-wide sentinel so tests that raise a +// signal start from the same state main does. +func resetSignalShutdown(t *testing.T) { + t.Helper() + signalShutdown.Store(false) + t.Cleanup(func() { signalShutdown.Store(false) }) +} + +// testFUSEMountOptions mounts directly when the test runs as root, which +// lets the kernel-mount tests run in containers without fusermount3. +func testFUSEMountOptions() *fuse.MountOptions { + return &fuse.MountOptions{DirectMount: os.Geteuid() == 0} +} + +func TestInstallShutdownSignalHandlerCancelsOnSignal(t *testing.T) { + resetSignalShutdown(t) + ctx, cancel := context.WithCancel(context.Background()) + stop := installShutdownSignalHandler(cancel, syscall.SIGTERM) + t.Cleanup(stop) + + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGTERM)) + select { + case <-ctx.Done(): + case <-time.After(shutdownTestTimeout): + t.Fatal("SIGTERM did not cancel the root context") + } + require.True(t, signalShutdown.Load()) + + // A repeat signal while the shutdown is in flight must stay with the + // handler: if the registration had been released, the default + // disposition would kill this test binary. + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGTERM)) + time.Sleep(50 * time.Millisecond) + stop() + stop() // idempotent +} + +func TestInstallShutdownSignalHandlerStopReleasesWithoutSignal(t *testing.T) { + resetSignalShutdown(t) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + stop := installShutdownSignalHandler(cancel, syscall.SIGUSR2) + + stop() + require.NoError(t, ctx.Err()) + require.False(t, signalShutdown.Load()) +} + +func TestIgnoreCanceledAfterSignal(t *testing.T) { + resetSignalShutdown(t) + wrappedCancel := errors.Wrap(context.Canceled, "failed to serve") + other := errors.New("listener exploded") + // What waitErrgroupAfterStartupFailure produces when a signal cancels + // startup: the startup error joined with the drain error. + joinedOnlyCanceled := errors.Wrap(errors.Join( + errors.Wrap(context.Canceled, "wait for raft join"), + errors.Wrap(context.Canceled, "shutdown failed after startup error"), + ), "startup failed") + joinedWithFailure := errors.Wrap(errors.Join( + errors.Wrap(context.Canceled, "wait for raft join"), + errors.Wrap(other, "shutdown failed after startup error"), + ), "startup failed") + tests := []struct { + name string + signaled bool + err error + wantNil bool + }{ + {name: "nil error", signaled: true, err: nil, wantNil: true}, + {name: "canceled after signal", signaled: true, err: context.Canceled, wantNil: true}, + {name: "wrapped canceled after signal", signaled: true, err: wrappedCancel, wantNil: true}, + {name: "joined cancellations after signal", signaled: true, err: joinedOnlyCanceled, wantNil: true}, + {name: "joined cancellation and failure after signal", signaled: true, err: joinedWithFailure, wantNil: false}, + {name: "other error after signal", signaled: true, err: other, wantNil: false}, + {name: "canceled without signal", signaled: false, err: context.Canceled, wantNil: false}, + {name: "other error without signal", signaled: false, err: other, wantNil: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + signalShutdown.Store(tt.signaled) + got := ignoreCanceledAfterSignal(tt.err) + if tt.wantNil { + require.NoError(t, got) + return + } + require.ErrorIs(t, got, tt.err) + }) + } + // errors.Is alone would have accepted the mixed join; pin the reason the + // helper exists. + require.True(t, errors.Is(joinedWithFailure, context.Canceled)) + require.False(t, onlyContextCanceled(joinedWithFailure)) + require.True(t, onlyContextCanceled(joinedOnlyCanceled)) +} + +// TestShutdownSignalWithOpenFileDetachesLazily: SIGTERM while a client still +// holds a file open on the mount. The plain unmount fails with EBUSY, so the +// lifecycle must detach lazily and finish the shutdown within the bounded +// wait instead of hanging until SIGKILL; once the handle closes the kernel +// releases the connection and the serve loop ends, leaving a bare directory. +func TestShutdownSignalWithOpenFileDetachesLazily(t *testing.T) { + if _, err := os.Stat("/dev/fuse"); err != nil { + skipOrFailWithoutFuse(t, "no /dev/fuse: %v", err) + } + resetSignalShutdown(t) + shortenFilesystemServeStopTimeout(t, 2*time.Second) + svc := newFilesystemTestService(t) + config := filesystemStartConfig{ + mountPoint: t.TempDir(), + clientID: "mount-busy", + mountOptions: testFUSEMountOptions(), + } + server, serveDone, err := mountFilesystemService(context.Background(), svc, config) + if err != nil { + skipOrFailWithoutFuse(t, "FUSE mount unavailable: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cleanup := &internalutil.CleanupStack{} + t.Cleanup(cleanup.Run) + cleanup.Add(installShutdownSignalHandler(cancel, syscall.SIGTERM)) + cleanup.Add(cancel) + eg, egCtx := errgroup.WithContext(ctx) + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, config.mountPoint) + + held, err := os.OpenFile(filepath.Join(config.mountPoint, "held"), os.O_CREATE|os.O_RDWR, 0o600) + require.NoError(t, err) + t.Cleanup(func() { _ = held.Close() }) + _, err = held.Write([]byte("held")) + require.NoError(t, err) + + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGTERM)) + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err) + case <-time.After(shutdownTestTimeout): + t.Fatal("shutdown hung on a busy FUSE mount") + } + + require.NoError(t, held.Close()) + select { + case <-serveDone: + case <-time.After(shutdownTestTimeout): + t.Fatal("FUSE serve loop did not stop after the last handle closed") + } + entries, err := os.ReadDir(config.mountPoint) + require.NoError(t, err) + require.Empty(t, entries) +} + +// TestShutdownSignalUnmountsFilesystem drives the production wiring end to +// end: the signal handler cancels the root context, the errgroup-derived +// context fires, and the filesystem lifecycle unmounts the kernel mount. +func TestShutdownSignalUnmountsFilesystem(t *testing.T) { + if _, err := os.Stat("/dev/fuse"); err != nil { + skipOrFailWithoutFuse(t, "no /dev/fuse: %v", err) + } + resetSignalShutdown(t) + svc := newFilesystemTestService(t) + config := filesystemStartConfig{ + mountPoint: t.TempDir(), + clientID: "mount-signal", + mountOptions: testFUSEMountOptions(), + } + server, serveDone, err := mountFilesystemService(context.Background(), svc, config) + if err != nil { + skipOrFailWithoutFuse(t, "FUSE mount unavailable: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cleanup := &internalutil.CleanupStack{} + t.Cleanup(cleanup.Run) + cleanup.Add(installShutdownSignalHandler(cancel, syscall.SIGTERM)) + cleanup.Add(cancel) + eg, egCtx := errgroup.WithContext(ctx) + installFilesystemServerLifecycle(egCtx, eg, cleanup, server, serveDone, config.mountPoint) + + probe := filepath.Join(config.mountPoint, "probe") + require.NoError(t, os.WriteFile(probe, []byte("probe"), 0o600)) + + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGTERM)) + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err) + case <-time.After(shutdownTestTimeout): + t.Fatal("filesystem lifecycle did not stop after SIGTERM") + } + require.True(t, signalShutdown.Load()) + select { + case <-serveDone: + case <-time.After(shutdownTestTimeout): + t.Fatal("FUSE serve loop did not stop after SIGTERM") + } + + // The mount is gone: the path is the bare temp directory again, so the + // probe written through the mount is no longer visible and stat works. + require.NoError(t, ignoreCanceledAfterSignal(egCtx.Err())) + entries, err := os.ReadDir(config.mountPoint) + require.NoError(t, err) + require.Empty(t, entries) +} diff --git a/monitoring/http.go b/monitoring/http.go index 515ce891b..d976d1860 100644 --- a/monitoring/http.go +++ b/monitoring/http.go @@ -13,10 +13,12 @@ import ( "github.com/cockroachdb/errors" ) -const ( - metricsReadHeaderTimeout = time.Second - metricsShutdownTimeout = 5 * time.Second -) +const metricsReadHeaderTimeout = time.Second + +// metricsShutdownTimeout bounds how long the metrics and pprof servers wait +// for in-flight requests on shutdown before the remaining connections are +// closed (ShutdownHTTPServer). A variable so tests can shorten it. +var metricsShutdownTimeout = 5 * time.Second // AddressRequiresToken reports whether the address is exposed beyond loopback // and therefore requires bearer-token protection. @@ -76,7 +78,10 @@ func NewMetricsServer(handler http.Handler, bearerToken string) *http.Server { } } -// MetricsShutdownTask returns an errgroup task that stops the metrics server on context cancellation. +// MetricsShutdownTask returns an errgroup task that stops the metrics server +// on context cancellation: a graceful drain bounded by +// metricsShutdownTimeout, then a forced close of whatever is still +// connected, so a scrape that stalls cannot hold the shutdown open. func MetricsShutdownTask(ctx context.Context, server *http.Server, address string) func() error { return func() error { if server == nil { @@ -84,13 +89,7 @@ func MetricsShutdownTask(ctx context.Context, server *http.Server, address strin } <-ctx.Done() slog.Info("Shutting down metrics server", "address", address, "reason", ctx.Err()) - shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), metricsShutdownTimeout) - defer cancel() - err := server.Shutdown(shutdownCtx) - if err == nil || errors.Is(err, http.ErrServerClosed) || errors.Is(err, net.ErrClosed) { - return nil - } - return errors.WithStack(err) + return ShutdownHTTPServer(ctx, server, "metrics", metricsShutdownTimeout) } } diff --git a/monitoring/http_shutdown.go b/monitoring/http_shutdown.go new file mode 100644 index 000000000..e6e9ca3d6 --- /dev/null +++ b/monitoring/http_shutdown.go @@ -0,0 +1,44 @@ +package monitoring + +import ( + "context" + "log/slog" + "net" + "net/http" + "time" + + "github.com/cockroachdb/errors" +) + +// ShutdownHTTPServer drains server gracefully for at most timeout and then +// forces the remaining connections closed with http.Server.Close. A request +// that never finishes on its own (a pprof profile that was asked for 30 +// seconds, a client that stalls mid-body) would otherwise hold Shutdown +// open past the bound, and a graceful stop requested by a signal must +// never wait on it. A forced close after the bound is the expected +// outcome of that bounded shutdown and is reported as success; only a +// Close that itself fails, or a Shutdown failure other than the deadline, +// is returned. A nil server is a no-op. The returned error is wrapped. +func ShutdownHTTPServer(ctx context.Context, server *http.Server, name string, timeout time.Duration) error { + if server == nil { + return nil + } + shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), timeout) + defer cancel() + err := server.Shutdown(shutdownCtx) + switch { + case err == nil, errors.Is(err, http.ErrServerClosed), errors.Is(err, net.ErrClosed): + return nil + case errors.Is(err, context.DeadlineExceeded): + slog.WarnContext(ctx, "http server graceful shutdown did not finish in time, closing remaining connections", + slog.String("server", name), + slog.Duration("timeout", timeout), + ) + if closeErr := server.Close(); closeErr != nil && !errors.Is(closeErr, net.ErrClosed) { + return errors.Wrapf(closeErr, "close %s server after shutdown timeout", name) + } + return nil + default: + return errors.Wrapf(err, "shut down %s server", name) + } +} diff --git a/monitoring/http_shutdown_test.go b/monitoring/http_shutdown_test.go new file mode 100644 index 000000000..703d75661 --- /dev/null +++ b/monitoring/http_shutdown_test.go @@ -0,0 +1,159 @@ +package monitoring + +import ( + "context" + "net" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" +) + +// stalledHandler never finishes on its own: it returns only when the server +// tears the connection down, like a profile request for 30 seconds or a +// scrape whose client stops reading. +func stalledHandler(started chan<- struct{}) http.Handler { + return http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case started <- struct{}{}: + default: + } + <-r.Context().Done() + }) +} + +func listenLoopback(t *testing.T) net.Listener { + t.Helper() + lis, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + return lis +} + +// startStalledRequest opens a request against addr that the stalled handler +// will hold, waits until the handler is running, and returns a channel that +// yields the client's error once the request ends. +func startStalledRequest(t *testing.T, addr string, started <-chan struct{}) <-chan error { + t.Helper() + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+addr+"/", http.NoBody) + require.NoError(t, err) + client := &http.Client{Transport: &http.Transport{DisableKeepAlives: true}} + done := make(chan error, 1) + go func() { + resp, err := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + done <- err + }() + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("handler did not start for the stalled request") + } + return done +} + +func TestShutdownHTTPServerForcesCloseAfterTimeout(t *testing.T) { + lis := listenLoopback(t) + started := make(chan struct{}, 1) + srv := &http.Server{Handler: stalledHandler(started), ReadHeaderTimeout: time.Second} + serveDone := make(chan error, 1) + go func() { serveDone <- srv.Serve(lis) }() + reqDone := startStalledRequest(t, lis.Addr().String(), started) + + start := time.Now() + err := ShutdownHTTPServer(context.Background(), srv, "test", 200*time.Millisecond) + elapsed := time.Since(start) + require.NoError(t, err, "a forced close after the bound is the expected bounded shutdown, not a failure") + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond, "the graceful drain must be given its bound first") + require.Less(t, elapsed, 3*time.Second, "the shutdown must not wait on a request that never finishes") + + select { + case err := <-serveDone: + require.ErrorIs(t, err, http.ErrServerClosed) + case <-time.After(5 * time.Second): + t.Fatal("Serve did not return after the forced close") + } + select { + case err := <-reqDone: + require.Error(t, err, "the stalled request must be cut off") + case <-time.After(5 * time.Second): + t.Fatal("the stalled request was not closed") + } +} + +func TestShutdownHTTPServerReturnsPromptlyWhenIdle(t *testing.T) { + lis := listenLoopback(t) + srv := &http.Server{Handler: http.NotFoundHandler(), ReadHeaderTimeout: time.Second} + serveDone := make(chan error, 1) + go func() { serveDone <- srv.Serve(lis) }() + time.Sleep(20 * time.Millisecond) + + start := time.Now() + require.NoError(t, ShutdownHTTPServer(context.Background(), srv, "idle", 5*time.Second)) + require.Less(t, time.Since(start), 2*time.Second) + select { + case <-serveDone: + case <-time.After(5 * time.Second): + t.Fatal("Serve did not return after the shutdown") + } +} + +func TestShutdownHTTPServerNilIsNoop(t *testing.T) { + require.NoError(t, ShutdownHTTPServer(context.Background(), nil, "nil", time.Second)) +} + +// The errgroup tasks main wires for the metrics and pprof listeners must +// return nil after a bounded, forced shutdown: a context.DeadlineExceeded +// from them would reach main after a SIGTERM, fail the "only cancellation" +// check, and turn the documented status-0 shutdown into a Fatalf. +func TestAuxiliaryShutdownTasksReturnNilAfterForcedClose(t *testing.T) { + old := metricsShutdownTimeout + metricsShutdownTimeout = 200 * time.Millisecond + t.Cleanup(func() { metricsShutdownTimeout = old }) + + cases := []struct { + name string + serve func(*http.Server, net.Listener, string) func() error + shutdown func(context.Context, *http.Server, string) func() error + }{ + {name: "metrics", serve: MetricsServeTask, shutdown: MetricsShutdownTask}, + {name: "pprof", serve: PprofServeTask, shutdown: PprofShutdownTask}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + lis := listenLoopback(t) + started := make(chan struct{}, 1) + srv := &http.Server{Handler: stalledHandler(started), ReadHeaderTimeout: time.Second} + ctx, cancel := context.WithCancel(context.Background()) + eg, egCtx := errgroup.WithContext(ctx) + eg.Go(tc.serve(srv, lis, lis.Addr().String())) + eg.Go(tc.shutdown(egCtx, srv, lis.Addr().String())) + reqDone := startStalledRequest(t, lis.Addr().String(), started) + + start := time.Now() + cancel() + waitDone := make(chan error, 1) + go func() { waitDone <- eg.Wait() }() + select { + case err := <-waitDone: + require.NoError(t, err, "the bounded shutdown must not surface as an errgroup failure") + case <-time.After(5 * time.Second): + t.Fatal("the errgroup did not drain after cancellation") + } + elapsed := time.Since(start) + require.GreaterOrEqual(t, elapsed, 150*time.Millisecond) + require.Less(t, elapsed, 3*time.Second) + select { + case err := <-reqDone: + require.Error(t, err) + case <-time.After(5 * time.Second): + t.Fatal("the stalled request was not closed") + } + }) + } +} diff --git a/monitoring/pprof.go b/monitoring/pprof.go index effd6121b..c7c486f65 100644 --- a/monitoring/pprof.go +++ b/monitoring/pprof.go @@ -32,7 +32,9 @@ func NewPprofServer(bearerToken string) *http.Server { } // PprofShutdownTask returns an errgroup task that stops the pprof server on -// context cancellation. +// context cancellation: a graceful drain bounded by metricsShutdownTimeout, +// then a forced close, so a long profile request (`/debug/pprof/profile? +// seconds=30`) cannot hold the shutdown open or turn it into a failure. func PprofShutdownTask(ctx context.Context, server *http.Server, address string) func() error { return func() error { if server == nil { @@ -40,13 +42,7 @@ func PprofShutdownTask(ctx context.Context, server *http.Server, address string) } <-ctx.Done() slog.Info("Shutting down pprof server", "address", address, "reason", ctx.Err()) - shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), metricsShutdownTimeout) - defer cancel() - err := server.Shutdown(shutdownCtx) - if err == nil || errors.Is(err, http.ErrServerClosed) || errors.Is(err, net.ErrClosed) { - return nil - } - return errors.WithStack(err) + return ShutdownHTTPServer(ctx, server, "pprof", metricsShutdownTimeout) } }