From 322cf069c23d4d9517827dcaa07e4ede8e1df583 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 11 Aug 2026 19:21:38 -0300 Subject: [PATCH 1/2] perf(postgres): lower the autovacuum scale factors on the session tables The default scale factors are proportions of the table, so the bigger a table gets the longer autovacuum waits. On the instance this was measured on, session_events had reached 6.7M rows and 4.5 GB, where the default 0.2 vacuum factor means no vacuum until ~1.3M rows are dead and the default 0.1 analyze factor means no analyze until ~670k changes. It had gone 45 days without one, leaving the planner estimating against stale statistics on the largest table in the database. The proportion is the wrong shape for these two tables specifically: they are the largest and the fastest growing, which is the combination the default punishes. The ALTERs only change when the daemon next acts, so the migration also runs the ANALYZE once to clear the backlog the old factor already allowed. TestNonTransactionalMigrations arrives with it. PostgreSQL refuses some statements inside a transaction block, and the two conditions such a statement needs are invisible in the SQL itself: bun decides transactionality from the ".tx." filename suffix, and the pool runs in pgx simple-protocol mode, where a multi-statement Exec is itself an implicit transaction block. Getting either wrong fails at boot rather than in review. Nothing here needs the exemption -- 019 is transactional throughout -- but the guard has to exist before the first migration that does. The match is anchored to the start of a statement rather than searched for anywhere in the chunk. Unanchored, "VACUUM" is also a substring of autovacuum_vacuum_scale_factor, so the guard would condemn the ALTER TABLE above, which is transactional in every respect. --- .../api/store/pg/migration_autovacuum_test.go | 56 ++++++ ..._session_autovacuum_thresholds.tx.down.sql | 14 ++ ...et_session_autovacuum_thresholds.tx.up.sql | 48 ++++++ .../store/pg/migrations/migrations_test.go | 162 ++++++++++++++++++ 4 files changed, 280 insertions(+) create mode 100644 server/api/store/pg/migration_autovacuum_test.go create mode 100644 server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.down.sql create mode 100644 server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.up.sql diff --git a/server/api/store/pg/migration_autovacuum_test.go b/server/api/store/pg/migration_autovacuum_test.go new file mode 100644 index 00000000000..2d6df024e22 --- /dev/null +++ b/server/api/store/pg/migration_autovacuum_test.go @@ -0,0 +1,56 @@ +package pg_test + +import ( + "context" + "testing" + + "github.com/shellhub-io/shellhub/server/api/store/storetest/pgprovider" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSessionAutovacuumThresholds covers migration 019. The knobs are per-table storage +// parameters, so the only honest assertion is what pg_class.reloptions holds on a migrated +// database — reading the migration file back would assert nothing but its own text. +func TestSessionAutovacuumThresholds(t *testing.T) { + ctx := context.Background() + + provider, err := pgprovider.NewProvider(ctx) + require.NoError(t, err) + t.Cleanup(func() { provider.Close(t) }) + + for _, table := range []string{"sessions", "session_events"} { + t.Run(table, func(t *testing.T) { + // unnest rather than reading the array whole: it yields one row per option, so + // the assertion matches a setting exactly instead of finding it as a substring + // of another. A table with no storage parameters yields no rows. + var options []string + require.NoError(t, + provider.DB().NewSelect(). + ColumnExpr("unnest(reloptions)"). + TableExpr("pg_class"). + Where("relname = ?", table). + Where("relkind = 'r'"). + Scan(ctx, &options), + ) + + assert.Contains(t, options, "autovacuum_vacuum_scale_factor=0.05") + assert.Contains(t, options, "autovacuum_analyze_scale_factor=0.02") + + // The new analyze factor only decides when the daemon next looks; it does nothing + // about statistics that are already stale, which on the instance this came from + // were 45 days old. The migration refreshes them once itself. + var analyzed int + require.NoError(t, + provider.DB().NewSelect(). + ColumnExpr("count(*)"). + TableExpr("pg_stat_user_tables"). + Where("relname = ?", table). + Where("last_analyze IS NOT NULL"). + Scan(ctx, &analyzed), + ) + + assert.Equal(t, 1, analyzed, "migration must leave %s with fresh statistics", table) + }) + } +} diff --git a/server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.down.sql b/server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.down.sql new file mode 100644 index 00000000000..5f295383735 --- /dev/null +++ b/server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.down.sql @@ -0,0 +1,14 @@ +-- RESET drops the per-table override, returning both tables to whatever the server-level +-- autovacuum_*_scale_factor is set to. It does not restore a previous per-table value, because +-- there was none to restore. +ALTER TABLE sessions RESET ( + autovacuum_vacuum_scale_factor, + autovacuum_analyze_scale_factor +); + +--bun:split + +ALTER TABLE session_events RESET ( + autovacuum_vacuum_scale_factor, + autovacuum_analyze_scale_factor +); diff --git a/server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.up.sql b/server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.up.sql new file mode 100644 index 00000000000..5df8d2c93c6 --- /dev/null +++ b/server/api/store/pg/migrations/019_set_session_autovacuum_thresholds.tx.up.sql @@ -0,0 +1,48 @@ +-- Autovacuum's default scale factors are proportions of the table, so the bigger a table +-- gets the longer it waits: at the default vacuum factor of 0.2, a 6.7M-row session_events +-- does not get vacuumed until ~1.3M rows are dead, and at the default analyze factor of 0.1 +-- its statistics are only refreshed after ~670k changes. Measured on a production instance, +-- that left session_events 45 days stale on a table the planner has to estimate against, and +-- sessions carrying 24k dead tuples ten days after its last vacuum. +-- +-- The proportion is the wrong shape for these two tables specifically: they are the largest +-- in the database and the fastest growing, which is exactly the combination the default +-- punishes. Lowering the factors converts "a fifth of the table" into a bound that stays +-- workable as the table grows — ~335k dead rows before a vacuum and ~134k changes before an +-- analyze at today's size, instead of 1.3M and 670k. +-- +-- These are storage parameters, not a rewrite: the ALTER takes a brief SHARE UPDATE EXCLUSIVE +-- lock, touches only the catalog and cannot block reads or writes. It also does not vacuum or +-- analyze anything by itself — it only changes when the daemon next decides to, which is why +-- the ANALYZE below is needed to clear the backlog the old factor already allowed. +-- +-- Retention (the cron this migration ships alongside) reduces how much dead tuple churn these +-- tables see at all, but does not replace this: a prune is itself a large source of dead +-- tuples, so the thresholds matter more once it is running, not less. +ALTER TABLE sessions SET ( + autovacuum_vacuum_scale_factor = 0.05, + autovacuum_analyze_scale_factor = 0.02 +); + +--bun:split + +ALTER TABLE session_events SET ( + autovacuum_vacuum_scale_factor = 0.05, + autovacuum_analyze_scale_factor = 0.02 +); + +--bun:split + +-- Statistics on the instance this was measured on were 45 days stale, and the lowered factor +-- above does nothing about a backlog that already exists — it only brings the *next* analyze +-- forward. The planner estimates against these tables on every session list, so leaving them +-- stale is the part that actually produces bad plans. +-- +-- Unlike a VACUUM FULL this is safe to run at boot: ANALYZE reads a bounded random +-- sample (default_statistics_target * 300, so ~30k rows) rather than the whole table, takes +-- only SHARE UPDATE EXCLUSIVE, and unlike VACUUM it is allowed inside a transaction block. +ANALYZE sessions; + +--bun:split + +ANALYZE session_events; diff --git a/server/api/store/pg/migrations/migrations_test.go b/server/api/store/pg/migrations/migrations_test.go index 341d7d96c49..e792db87be4 100644 --- a/server/api/store/pg/migrations/migrations_test.go +++ b/server/api/store/pg/migrations/migrations_test.go @@ -26,3 +26,165 @@ func TestNoDuplicateMigrationVersions(t *testing.T) { seen[version] = file } } + +// nonTransactionalStatements are statements PostgreSQL refuses to run inside a transaction +// block. +var nonTransactionalStatements = []string{ + "VACUUM", + "ALTER SYSTEM", + "CREATE INDEX CONCURRENTLY", + "DROP INDEX CONCURRENTLY", + "REINDEX INDEX CONCURRENTLY", + "REINDEX TABLE CONCURRENTLY", + "CREATE DATABASE", + "DROP DATABASE", +} + +// TestNonTransactionalMigrations guards the two conditions such a statement needs, neither of +// which is visible in the SQL itself: bun decides transactionality from the ".tx." filename +// suffix, and the pool runs in pgx simple-protocol mode where a multi-statement Exec is itself +// an implicit transaction block. Getting either wrong fails at boot, not in review. +func TestNonTransactionalMigrations(t *testing.T) { + files, err := fs.Glob(sqlMigrations, "*.sql") + if err != nil { + t.Fatalf("failed to list migrations: %v", err) + } + + for _, file := range files { + raw, err := fs.ReadFile(sqlMigrations, file) + if err != nil { + t.Fatalf("failed to read %q: %v", file, err) + } + + for _, chunk := range strings.Split(string(raw), "--bun:split") { + statement := stripSQLComments(chunk) + + keyword := findNonTransactionalStatement(statement) + if keyword == "" { + continue + } + + if strings.Contains(file, ".tx.") { + t.Errorf("%s: %q cannot run inside a transaction, so the file must not carry the .tx. suffix", file, keyword) + } + + if n := countStatements(statement); n > 1 { + t.Errorf("%s: %q shares its --bun:split chunk with %d other statement(s), which simple-protocol mode batches into an implicit transaction", file, keyword, n-1) + } + } + } +} + +// stripSQLComments removes "--" line comments so the scan below reads statements rather than +// the prose around them. It does not attempt to honour string literals, which no migration +// needs it to. +func stripSQLComments(chunk string) string { + lines := strings.Split(chunk, "\n") + kept := make([]string, 0, len(lines)) + + for _, line := range lines { + if i := strings.Index(line, "--"); i >= 0 { + line = line[:i] + } + + kept = append(kept, line) + } + + return strings.Join(kept, "\n") +} + +// findNonTransactionalStatement returns the offending keyword, or "" when the statement can run +// inside a transaction. +// +// Every keyword above opens a statement, so the match is anchored to the start of one rather +// than searched for anywhere in the chunk. Unanchored, "VACUUM" is also a substring of the +// autovacuum_* storage parameters, which would condemn an ALTER TABLE ... SET that is +// transactional in every respect. +func findNonTransactionalStatement(chunk string) string { + for _, statement := range strings.Split(chunk, ";") { + normalized := strings.Join(strings.Fields(strings.ToUpper(statement)), " ") + + for _, keyword := range nonTransactionalStatements { + if strings.HasPrefix(normalized, keyword) { + return keyword + } + } + } + + return "" +} + +// TestNonTransactionalDetection covers the two things the guard above has to get right: that +// prose merely naming one of these statements does not count, and that a statement sharing its +// chunk with another one does. +func TestNonTransactionalDetection(t *testing.T) { + tests := []struct { + name string + chunk string + keyword string + statements int + }{ + { + name: "vacuum cannot run in a transaction", + chunk: "VACUUM (FULL, ANALYZE) devices;", + keyword: "VACUUM", + statements: 1, + }, + { + name: "prose naming a vacuum is not a statement", + chunk: "-- recover by running VACUUM (FULL, ANALYZE) devices; by hand\nSELECT 1;", + keyword: "", + statements: 1, + }, + { + name: "keyword split across lines is still found", + chunk: "CREATE INDEX CONCURRENTLY\n devices_last_seen ON devices USING btree (last_seen);", + keyword: "CREATE INDEX CONCURRENTLY", + statements: 1, + }, + { + name: "a shared chunk is counted", + chunk: "SET lock_timeout = '60s';\nVACUUM (FULL, ANALYZE) devices;", + keyword: "VACUUM", + statements: 2, + }, + { + name: "ordinary ddl is transactional", + chunk: "DROP INDEX IF EXISTS devices_last_seen;", + keyword: "", + statements: 1, + }, + { + name: "a storage parameter merely spelling a keyword is not that statement", + chunk: "ALTER TABLE sessions SET (autovacuum_vacuum_scale_factor = 0.05);", + keyword: "", + statements: 1, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + statement := stripSQLComments(tc.chunk) + + if keyword := findNonTransactionalStatement(statement); keyword != tc.keyword { + t.Errorf("findNonTransactionalStatement() = %q, want %q", keyword, tc.keyword) + } + + if count := countStatements(statement); count != tc.statements { + t.Errorf("countStatements() = %d, want %d", count, tc.statements) + } + }) + } +} + +func countStatements(statement string) int { + count := 0 + + for _, s := range strings.Split(statement, ";") { + if strings.TrimSpace(s) != "" { + count++ + } + } + + return count +} From f7ca3049abec574907a04fdbac354d7a1cef2260 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Thu, 13 Aug 2026 08:53:20 -0300 Subject: [PATCH 2/2] feat(server): prune sessions past a configurable retention window Nothing pruned sessions or session_events. On the instance this came from, the oldest session dated from 2023-06-05 and session_events alone was 76% of the database, which is both why it does not fit in a sane shared_buffers and a data retention gap in its own right. The window is off by default. The deletion is permanent and unattended, and on editions with recording the events are the recording, so an upgrade must not start discarding history on its own; docker-compose.enterprise.yml sets 180 days, where the commitment and the volume are known. Batched rather than one DELETE, and capped at 100k sessions per run. An instance adopting this can have years to shed, and draining that in a single night is the write storm the batching exists to avoid, on a host already producing 104 GB of WAL a day. The backlog drains over successive nights instead, which also leaves an operator who did not want this time to notice. Sessions still in active_sessions are never pruned. closed is not usable for this, since a session whose server died never gets closed and would otherwise be immortal, whereas a row in active_sessions is what a live session actually holds. Recordings are deleted before rows. A recording is an object keyed from the session UID with nothing in the schema pointing at it, so the row is the only thing that can still name it: deleting rows first would strand every object. Community has no object storage, so the pruner is a registered seam that the cloud module fills in, like the firewall and license evaluators. One unpurgeable object must not stall the rest. DeleteRecordings returns the subset it purged and the caller deletes only those rows, so a session whose recording cannot be removed holds up nothing but itself. Failing the whole batch instead would be permanent rather than transient: SessionListExpired always re-serves the oldest sessions, so a single unreachable object would abort the same batch every night and retention would stop for good, reported only by an ERROR log -- while the objects already purged in that batch kept their rows, reading as recorded but playing back nothing. A batch in which nothing can be deleted ends the run instead of re-listing the same blocked rows until the cap. SessionListExpired carries the recorded flag with each UID so only sessions that own a recording reach object storage; without it an instance that records nothing still pays a storage lookup for every session it deletes, a thousand per batch. The nil-pruner case sits behind pruneRecordings, so the cron loop does not know the seam can be absent, matching how EvaluateFirewall hides its own. Fixes: shellhub-io/team#201 --- .env.enterprise | 4 + docker-compose.enterprise.yml | 1 + server/api/services/service.go | 8 + server/api/services/session-recording.go | 95 +++++++ server/api/services/task.go | 101 +++++++ server/api/services/task_test.go | 291 ++++++++++++++++++++ server/api/store/mocks/mock_store.go | 140 ++++++++++ server/api/store/pg/session.go | 55 ++++ server/api/store/pg/store_test.go | 1 + server/api/store/session.go | 23 ++ server/api/store/storetest/session_tests.go | 190 +++++++++++++ server/api/store/storetest/suite.go | 1 + server/app/server.go | 45 +++ 13 files changed, 955 insertions(+) create mode 100644 server/api/services/session-recording.go diff --git a/.env.enterprise b/.env.enterprise index 81fa188ef45..4f1e87340e7 100644 --- a/.env.enterprise +++ b/.env.enterprise @@ -55,3 +55,7 @@ SHELLHUB_OBJECT_STORAGE_SECRET_KEY=password # NOTICE: Leave empty for license-less development; the API then starts # without a license and gated features stay disabled. SHELLHUB_LICENSE_FILE= + +# How long sessions and their events are kept after the session started (days). +# 0 keeps them indefinitely. +SHELLHUB_SESSION_RETENTION_DAYS=180 diff --git a/docker-compose.enterprise.yml b/docker-compose.enterprise.yml index c3188300f33..e790cbc12c2 100644 --- a/docker-compose.enterprise.yml +++ b/docker-compose.enterprise.yml @@ -27,6 +27,7 @@ services: # GeoIP (MaxMind) database source, read by the enterprise binary - MAXMIND_MIRROR=${SHELLHUB_MAXMIND_MIRROR:-} - MAXMIND_LICENSE=${SHELLHUB_MAXMIND_LICENSE:-} + - SHELLHUB_SESSION_RETENTION_DAYS=${SHELLHUB_SESSION_RETENTION_DAYS} secrets: - api_private_key - api_public_key diff --git a/server/api/services/service.go b/server/api/services/service.go index 5d62bac073c..386d5568f07 100644 --- a/server/api/services/service.go +++ b/server/api/services/service.go @@ -25,6 +25,7 @@ type service struct { billing BillingProvider licenseEvaluator LicenseEvaluator firewallEvaluator FirewallEvaluator + recordingPruner SessionRecordingPruner } type Service interface { @@ -94,6 +95,12 @@ func WithFirewallEvaluator(fe FirewallEvaluator) Option { } } +func WithSessionRecordingPruner(rp SessionRecordingPruner) Option { + return func(service *APIService) { + service.recordingPruner = rp + } +} + func NewService(store store.Store, privKey *rsa.PrivateKey, pubKey *rsa.PublicKey, cache cache.Cache, options ...Option) *APIService { if privKey == nil || pubKey == nil { var err error @@ -114,6 +121,7 @@ func NewService(store store.Store, privKey *rsa.PrivateKey, pubKey *rsa.PublicKe billing: nil, // injected via WithBilling option licenseEvaluator: nil, // injected via WithLicenseEvaluator option firewallEvaluator: nil, // injected via WithFirewallEvaluator option + recordingPruner: nil, // injected via WithSessionRecordingPruner option }, } diff --git a/server/api/services/session-recording.go b/server/api/services/session-recording.go new file mode 100644 index 00000000000..678e38218a4 --- /dev/null +++ b/server/api/services/session-recording.go @@ -0,0 +1,95 @@ +package services + +import ( + "context" + + "github.com/shellhub-io/shellhub/pkg/cache" + "github.com/shellhub-io/shellhub/server/api/store" +) + +// SessionRecordingPrunerFactoryFunc constructs a SessionRecordingPruner from the core store and +// cache. Enterprise packages register a factory via RegisterSessionRecordingPruner in their +// init() functions; it runs during server setup. +type SessionRecordingPrunerFactoryFunc func(ctx context.Context, store store.Store, cache cache.Cache) (SessionRecordingPruner, error) + +var sessionRecordingPrunerFactory SessionRecordingPrunerFactoryFunc + +// RegisterSessionRecordingPruner registers the factory that creates the recording pruner. +// It must be called before the server's Setup() runs. +func RegisterSessionRecordingPruner(f SessionRecordingPrunerFactoryFunc) { + sessionRecordingPrunerFactory = f +} + +// SessionRecordingPrunerFactory returns the registered factory, or nil in Community Edition +// builds. +func SessionRecordingPrunerFactory() SessionRecordingPrunerFactoryFunc { + return sessionRecordingPrunerFactory +} + +// SessionRecordingPruner discards the stored recordings of sessions that retention is about to +// delete. +// +// A recording is an object, not a row, and nothing in the schema points at it: it is found by +// composing a key from the session's UID. Deleting the session row therefore does not delete the +// recording — it destroys the only thing that could still name it. Everything else in this seam +// follows from that asymmetry. +type SessionRecordingPruner interface { + // DeleteRecordings removes the recordings of the given sessions, whatever seats they had, + // and returns the subset it managed to purge. + // + // Returning a subset rather than failing the batch is what keeps one unreachable object from + // halting retention: the caller deletes the rows it names and leaves the rest, so a session + // whose recording cannot be removed holds up nothing but itself. The error is reserved for a + // failure that makes the whole batch moot, such as a cancelled context. + DeleteRecordings(ctx context.Context, uids []string) ([]string, error) +} + +// pruneRecordings discards the recordings of the recorded sessions in the batch and returns the +// sessions whose rows may now be deleted. +// +// Without a pruner — Community Edition, or an enterprise instance with no object storage — no +// session owns anything outside the database, so the whole batch is deletable as it stands. +func (s *service) pruneRecordings(ctx context.Context, sessions []store.ExpiredSession) ([]string, error) { + uids := make([]string, 0, len(sessions)) + recorded := make([]string, 0, len(sessions)) + + for _, session := range sessions { + uids = append(uids, session.UID) + + if session.Recorded { + recorded = append(recorded, session.UID) + } + } + + if s.recordingPruner == nil || len(recorded) == 0 { + return uids, nil + } + + purged, err := s.recordingPruner.DeleteRecordings(ctx, recorded) + if err != nil { + return nil, err + } + + // Everything that was never recorded, plus the recordings actually purged. A session left + // out here keeps its row, so its object stays reachable for the next run to retry. + deletable := make([]string, 0, len(uids)) + purgedSet := make(map[string]struct{}, len(purged)) + + for _, uid := range purged { + purgedSet[uid] = struct{}{} + } + + for _, session := range sessions { + if !session.Recorded { + deletable = append(deletable, session.UID) + + continue + } + + if _, ok := purgedSet[session.UID]; ok { + deletable = append(deletable, session.UID) + } + } + + return deletable, nil +} diff --git a/server/api/services/task.go b/server/api/services/task.go index 31287d30c48..c4a73fedb18 100644 --- a/server/api/services/task.go +++ b/server/api/services/task.go @@ -21,6 +21,20 @@ const ( CronEphemeralCleanup = worker.CronSpec("*/5 * * * *") CronEnrollmentCallbackCleanup = worker.CronSpec("0 4 * * *") CronSSHApprovalCleanup = worker.CronSpec("*/10 * * * *") + CronSessionCleanup = worker.CronSpec("0 1 * * *") +) + +const ( + // A session cascades into its events, so one batch is already thousands of rows. + sessionCleanupBatchSize = 1000 + + // Together with the batch size this caps a run at 100k sessions. An instance adopting + // retention for the first time can have years to shed, and draining all of it in one night + // is the write storm the batching exists to avoid. + sessionCleanupMaxBatches = 100 + + // Leaves room between batches for live traffic and for autovacuum to follow behind. + sessionCleanupBatchPause = 200 * time.Millisecond ) func (s *service) DeviceCleanup() worker.CronHandler { @@ -57,6 +71,93 @@ func (s *service) SSHApprovalCleanup() worker.CronHandler { } } +// SessionCleanup enforces the instance's session retention window: sessions that started longer +// ago than retention are deleted, taking their events and recordings with them. +// +// A retention that is not positive means "keep forever" and prunes nothing. The guard matters +// more than it looks: read as a window, a zero would put the cutoff at now and delete every +// session on the instance. +func (s *service) SessionCleanup(retention time.Duration) worker.CronHandler { + return func(ctx context.Context) error { + return s.sessionCleanup(ctx, retention, sessionCleanupBatchPause) + } +} + +// sessionCleanup takes the pause as an argument so a test can drive the batching loop without +// waiting it out. +func (s *service) sessionCleanup(ctx context.Context, retention, pause time.Duration) error { + if retention <= 0 { + return nil + } + + cutoff := clock.Now().Add(-retention) + + total := int64(0) + batches := 0 + + for batches < sessionCleanupMaxBatches { + sessions, err := s.store.SessionListExpired(ctx, cutoff, sessionCleanupBatchSize) + if err != nil { + log.WithError(err).WithField("deleted", total).Error("failed to list expired sessions") + + return err + } + + if len(sessions) == 0 { + break + } + + // Recordings first, rows second, because the row is the only thing that can still name + // the object. Sessions whose recording could not be purged are left out and keep their + // rows, so the next run finds them again. + deletable, err := s.pruneRecordings(ctx, sessions) + if err != nil { + log.WithError(err).WithField("deleted", total).Error("failed to prune recordings of expired sessions") + + return err + } + + // The batch is not empty but nothing in it can be deleted, so every session in it is + // blocked on its recording. Retrying inside this run would list the same rows again and + // spin until the cap; leave it for the next one, by which time the storage may answer. + if len(deletable) == 0 { + log.WithFields(log.Fields{"deleted": total, "blocked": len(sessions)}). + Warn("no expired session in the batch could be deleted; ending the run") + + break + } + + deleted, err := s.store.SessionDeleteMany(ctx, deletable) + if err != nil { + log.WithError(err).WithField("deleted", total).Error("failed to prune expired sessions") + + return err + } + + total += deleted + batches++ + + // A batch that came back short means the store ran out of sessions older than the + // cutoff, so there is nothing left for this run to do. + if len(sessions) < sessionCleanupBatchSize { + break + } + + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(pause): + } + } + + if total > 0 { + log.WithFields(log.Fields{"deleted": total, "cutoff": cutoff, "capped": batches == sessionCleanupMaxBatches}). + Info("pruned sessions past the retention window") + } + + return nil +} + // EnrollmentCallbackCleanup prunes single-use callback redemption records once older than the maximum // token TTL, past which the token has expired and can no longer gate a replay. The table only gains a // row per resolved deferred webhook, so this keeps its growth bounded. diff --git a/server/api/services/task_test.go b/server/api/services/task_test.go index 3820f2c91c1..6c7eb3acbf8 100644 --- a/server/api/services/task_test.go +++ b/server/api/services/task_test.go @@ -3,6 +3,7 @@ package services import ( "context" "errors" + "fmt" "testing" "time" @@ -14,6 +15,7 @@ import ( "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/store" storemock "github.com/shellhub-io/shellhub/server/api/store/mocks" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -433,3 +435,292 @@ func TestService_NamespaceDeviceCountSync(t *testing.T) { }) } } + +func TestService_SessionCleanup(t *testing.T) { + now := time.Date(2026, 8, 11, 12, 0, 0, 0, time.UTC) + retention := 180 * 24 * time.Hour + cutoff := now.Add(-retention) + + storeMock := storemock.NewMockStore(t) + clockMock := clockmock.NewMockClock(t) + + prevClock := clock.DefaultBackend + t.Cleanup(func() { clock.DefaultBackend = prevClock }) + clock.DefaultBackend = clockMock + clockMock.On("Now").Return(now).Maybe() + + // expired builds a batch of n sessions, none of them recorded. + expired := func(n int) []store.ExpiredSession { + batch := make([]store.ExpiredSession, n) + for i := range batch { + batch[i] = store.ExpiredSession{UID: fmt.Sprintf("session-%d", i)} + } + + return batch + } + + uids := func(sessions []store.ExpiredSession) []string { + out := make([]string, len(sessions)) + for i, session := range sessions { + out[i] = session.UID + } + + return out + } + + full := expired(sessionCleanupBatchSize) + + cases := []struct { + description string + retention time.Duration + requiredMocks func(context.Context, *mockSessionRecordingPruner) + withPruner bool + expected error + }{ + { + description: "does not prune when retention is not positive", + retention: 0, + requiredMocks: func(_ context.Context, _ *mockSessionRecordingPruner) { + // No store call is set up: a non-positive window must never be read as a cutoff + // of now, which would delete every session there is. + }, + expected: nil, + }, + { + description: "fails when listing fails", + retention: retention, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(nil, errors.New("database error")). + Once() + }, + expected: errors.New("database error"), + }, + { + description: "stops when nothing is expired", + retention: retention, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return([]store.ExpiredSession{}, nil). + Once() + }, + expected: nil, + }, + { + description: "fails when deleting fails", + retention: retention, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(expired(3), nil). + Once() + storeMock. + On("SessionDeleteMany", ctx, uids(expired(3))). + Return(int64(0), errors.New("database error")). + Once() + }, + expected: errors.New("database error"), + }, + { + description: "stops after a partial batch", + retention: retention, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + batch := expired(sessionCleanupBatchSize - 1) + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(batch, nil). + Once() + storeMock. + On("SessionDeleteMany", ctx, uids(batch)). + Return(int64(len(batch)), nil). + Once() + }, + expected: nil, + }, + { + description: "keeps batching while each batch comes back full", + retention: retention, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(full, nil). + Once() + storeMock. + On("SessionDeleteMany", ctx, uids(full)). + Return(int64(sessionCleanupBatchSize), nil). + Once() + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return([]store.ExpiredSession{}, nil). + Once() + }, + expected: nil, + }, + { + description: "stops at the per-run cap and leaves the rest for the next run", + retention: retention, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(full, nil). + Times(sessionCleanupMaxBatches) + storeMock. + On("SessionDeleteMany", ctx, uids(full)). + Return(int64(sessionCleanupBatchSize), nil). + Times(sessionCleanupMaxBatches) + }, + expected: nil, + }, + { + description: "does not reach for the bucket when nothing in the batch was recorded", + retention: retention, + withPruner: true, + requiredMocks: func(ctx context.Context, _ *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(expired(3), nil). + Once() + storeMock. + On("SessionDeleteMany", ctx, uids(expired(3))). + Return(int64(3), nil). + Once() + // No DeleteRecordings: an instance that records nothing must not pay a storage + // lookup per session it deletes. + }, + expected: nil, + }, + { + description: "purges a recorded session's recording before deleting its row", + retention: retention, + withPruner: true, + requiredMocks: func(ctx context.Context, pruner *mockSessionRecordingPruner) { + batch := []store.ExpiredSession{ + {UID: "plain"}, + {UID: "recorded", Recorded: true}, + } + + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(batch, nil). + Once() + + purged := false + pruner. + On("DeleteRecordings", ctx, []string{"recorded"}). + Run(func(_ mock.Arguments) { purged = true }). + Return([]string{"recorded"}, nil). + Once() + storeMock. + On("SessionDeleteMany", ctx, []string{"plain", "recorded"}). + Run(func(_ mock.Arguments) { + assert.True(t, purged, "the row must not be deleted before its recording") + }). + Return(int64(2), nil). + Once() + }, + expected: nil, + }, + { + description: "keeps the row of a session whose recording could not be purged", + retention: retention, + withPruner: true, + requiredMocks: func(ctx context.Context, pruner *mockSessionRecordingPruner) { + batch := []store.ExpiredSession{ + {UID: "ok", Recorded: true}, + {UID: "stuck", Recorded: true}, + } + + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return(batch, nil). + Once() + pruner. + On("DeleteRecordings", ctx, []string{"ok", "stuck"}). + Return([]string{"ok"}, nil). + Once() + // "stuck" keeps its row, which is what leaves its object reachable next run. + storeMock. + On("SessionDeleteMany", ctx, []string{"ok"}). + Return(int64(1), nil). + Once() + }, + expected: nil, + }, + { + description: "ends the run when no session in the batch can be deleted", + retention: retention, + withPruner: true, + requiredMocks: func(ctx context.Context, pruner *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return([]store.ExpiredSession{{UID: "stuck", Recorded: true}}, nil). + Once() + pruner. + On("DeleteRecordings", ctx, []string{"stuck"}). + Return([]string{}, nil). + Once() + // No SessionDeleteMany, and only one listing: retrying inside the run would + // re-list the same blocked rows and spin until the cap. + }, + expected: nil, + }, + { + description: "fails when the pruner reports the batch is moot", + retention: retention, + withPruner: true, + requiredMocks: func(ctx context.Context, pruner *mockSessionRecordingPruner) { + storeMock. + On("SessionListExpired", ctx, cutoff, sessionCleanupBatchSize). + Return([]store.ExpiredSession{{UID: "a", Recorded: true}}, nil). + Once() + pruner. + On("DeleteRecordings", ctx, []string{"a"}). + Return(nil, context.Canceled). + Once() + }, + expected: context.Canceled, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(tt *testing.T) { + ctx := context.Background() + + pruner := new(mockSessionRecordingPruner) + tt.Cleanup(func() { pruner.AssertExpectations(tt) }) + tc.requiredMocks(ctx, pruner) + + opts := []Option{} + if tc.withPruner { + opts = append(opts, WithSessionRecordingPruner(pruner)) + } + + s := NewService(storeMock, privateKey, publicKey, cache.NewNullCache(), opts...) + require.Equal(tt, tc.expected, s.sessionCleanup(ctx, tc.retention, 0)) + }) + } + + // The cases above drive the loop directly so they do not pay the batch pause. This one goes + // through the cron handler itself, which is what the composition root registers. + t.Run("the cron handler prunes nothing when retention is not positive", func(tt *testing.T) { + s := NewService(storeMock, privateKey, publicKey, cache.NewNullCache()) + require.NoError(tt, s.SessionCleanup(0)(context.Background())) + }) +} + +// mockSessionRecordingPruner stands in for SessionRecordingPruner. It lives here rather than in +// the generated mocks package for the same reason as mockLicenseEvaluator: services/mocks +// imports services, so an internal test importing it would form a cycle. +type mockSessionRecordingPruner struct { + mock.Mock +} + +func (m *mockSessionRecordingPruner) DeleteRecordings(ctx context.Context, uids []string) ([]string, error) { + args := m.Called(ctx, uids) + + purged, _ := args.Get(0).([]string) + + return purged, args.Error(1) +} diff --git a/server/api/store/mocks/mock_store.go b/server/api/store/mocks/mock_store.go index 36e543d3cd0..d5a035f8a6d 100644 --- a/server/api/store/mocks/mock_store.go +++ b/server/api/store/mocks/mock_store.go @@ -6016,6 +6016,72 @@ func (_c *MockStore_SessionCreate_Call) RunAndReturn(run func(ctx context.Contex return _c } +// SessionDeleteMany provides a mock function for the type MockStore +func (_mock *MockStore) SessionDeleteMany(ctx context.Context, uids []string) (int64, error) { + ret := _mock.Called(ctx, uids) + + if len(ret) == 0 { + panic("no return value specified for SessionDeleteMany") + } + + var r0 int64 + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, []string) (int64, error)); ok { + return returnFunc(ctx, uids) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, []string) int64); ok { + r0 = returnFunc(ctx, uids) + } else { + r0 = ret.Get(0).(int64) + } + if returnFunc, ok := ret.Get(1).(func(context.Context, []string) error); ok { + r1 = returnFunc(ctx, uids) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// MockStore_SessionDeleteMany_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SessionDeleteMany' +type MockStore_SessionDeleteMany_Call struct { + *mock.Call +} + +// SessionDeleteMany is a helper method to define mock.On call +// - ctx context.Context +// - uids []string +func (_e *MockStore_Expecter) SessionDeleteMany(ctx any, uids any) *MockStore_SessionDeleteMany_Call { + return &MockStore_SessionDeleteMany_Call{Call: _e.mock.On("SessionDeleteMany", ctx, uids)} +} + +func (_c *MockStore_SessionDeleteMany_Call) Run(run func(ctx context.Context, uids []string)) *MockStore_SessionDeleteMany_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 []string + if args[1] != nil { + arg1 = args[1].([]string) + } + run( + arg0, + arg1, + ) + }) + return _c +} + +func (_c *MockStore_SessionDeleteMany_Call) Return(n int64, err error) *MockStore_SessionDeleteMany_Call { + _c.Call.Return(n, err) + return _c +} + +func (_c *MockStore_SessionDeleteMany_Call) RunAndReturn(run func(ctx context.Context, uids []string) (int64, error)) *MockStore_SessionDeleteMany_Call { + _c.Call.Return(run) + return _c +} + // SessionEventsCreate provides a mock function for the type MockStore func (_mock *MockStore) SessionEventsCreate(ctx context.Context, event *models.SessionEvent) error { ret := _mock.Called(ctx, event) @@ -6389,6 +6455,80 @@ func (_c *MockStore_SessionList_Call) RunAndReturn(run func(ctx context.Context, return _c } +// SessionListExpired provides a mock function for the type MockStore +func (_mock *MockStore) SessionListExpired(ctx context.Context, before time.Time, limit int) ([]store.ExpiredSession, error) { + ret := _mock.Called(ctx, before, limit) + + if len(ret) == 0 { + panic("no return value specified for SessionListExpired") + } + + var r0 []store.ExpiredSession + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, time.Time, int) ([]store.ExpiredSession, error)); ok { + return returnFunc(ctx, before, limit) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, time.Time, int) []store.ExpiredSession); ok { + r0 = returnFunc(ctx, before, limit) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]store.ExpiredSession) + } + } + if returnFunc, ok := ret.Get(1).(func(context.Context, time.Time, int) error); ok { + r1 = returnFunc(ctx, before, limit) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// MockStore_SessionListExpired_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SessionListExpired' +type MockStore_SessionListExpired_Call struct { + *mock.Call +} + +// SessionListExpired is a helper method to define mock.On call +// - ctx context.Context +// - before time.Time +// - limit int +func (_e *MockStore_Expecter) SessionListExpired(ctx any, before any, limit any) *MockStore_SessionListExpired_Call { + return &MockStore_SessionListExpired_Call{Call: _e.mock.On("SessionListExpired", ctx, before, limit)} +} + +func (_c *MockStore_SessionListExpired_Call) Run(run func(ctx context.Context, before time.Time, limit int)) *MockStore_SessionListExpired_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 time.Time + if args[1] != nil { + arg1 = args[1].(time.Time) + } + var arg2 int + if args[2] != nil { + arg2 = args[2].(int) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *MockStore_SessionListExpired_Call) Return(expiredSessions []store.ExpiredSession, err error) *MockStore_SessionListExpired_Call { + _c.Call.Return(expiredSessions, err) + return _c +} + +func (_c *MockStore_SessionListExpired_Call) RunAndReturn(run func(ctx context.Context, before time.Time, limit int) ([]store.ExpiredSession, error)) *MockStore_SessionListExpired_Call { + _c.Call.Return(run) + return _c +} + // SessionResolve provides a mock function for the type MockStore func (_mock *MockStore) SessionResolve(ctx context.Context, sc scope.Scope, resolver store.SessionResolver, value string, opts ...store.QueryOption) (*models.Session, error) { var tmpRet mock.Arguments diff --git a/server/api/store/pg/session.go b/server/api/store/pg/session.go index a08f93d3ca1..2e0b40a0321 100644 --- a/server/api/store/pg/session.go +++ b/server/api/store/pg/session.go @@ -2,6 +2,7 @@ package pg import ( "context" + "time" "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/clock" @@ -346,6 +347,60 @@ func (pg *Pg) SessionUpdateDeviceUID(ctx context.Context, oldUID models.UID, new return nil } +func (pg *Pg) SessionListExpired(ctx context.Context, before time.Time, limit int) ([]store.ExpiredSession, error) { + if limit <= 0 { + return []store.ExpiredSession{}, nil + } + + db := pg.GetConnection(ctx) + + // Ordering by started_at walks sessions_started_at_idx and takes the oldest rows first, so + // successive batches drain the backlog from its far end instead of re-reading it. + // + // The anti-join is the safety rail: closed is not reliable for this (a session whose server + // died never gets closed and would otherwise be immortal), whereas a row in active_sessions + // is what a live session actually holds. Deleting one would cascade that row away underneath + // the connection still using it. + uids := make([]string, 0, limit) + recorded := make([]bool, 0, limit) + + if err := db.NewSelect(). + Model((*entity.Session)(nil)). + Column("id", "recorded"). + Where("started_at < ?", before). + Where("NOT EXISTS (SELECT 1 FROM active_sessions WHERE active_sessions.session_id = session.id)"). + Order("started_at ASC"). + Limit(limit). + Scan(ctx, &uids, &recorded); err != nil { + return nil, fromSQLError(err) + } + + sessions := make([]store.ExpiredSession, len(uids)) + for i, uid := range uids { + sessions[i] = store.ExpiredSession{UID: uid, Recorded: recorded[i]} + } + + return sessions, nil +} + +func (pg *Pg) SessionDeleteMany(ctx context.Context, uids []string) (int64, error) { + if len(uids) == 0 { + return 0, nil + } + + db := pg.GetConnection(ctx) + + res, err := db.NewDelete(). + Model((*entity.Session)(nil)). + Where("id IN (?)", bun.List(uids)). + Exec(ctx) + if err != nil { + return 0, fromSQLError(err) + } + + return res.RowsAffected() +} + // SessionSelectQuery applies the standard session SELECT decorations: relations, // computed columns (active, event_types, event_seats), and the active_sessions JOIN. // The caller provides the base query with the desired model (core or cloud entity). diff --git a/server/api/store/pg/store_test.go b/server/api/store/pg/store_test.go index 7fc9dc9927f..ff8352aeb7c 100644 --- a/server/api/store/pg/store_test.go +++ b/server/api/store/pg/store_test.go @@ -71,6 +71,7 @@ func TestPgStore(t *testing.T) { suite.TestSessionEventsCreate(t) suite.TestSessionEventsList(t) suite.TestSessionEventsDelete(t) + suite.TestSessionCleanup(t) }) runSubSuite(t, "TagStore", func(suite *storetest.Suite, t *testing.T) { diff --git a/server/api/store/session.go b/server/api/store/session.go index 3e7b9732a2b..27aa4a25398 100644 --- a/server/api/store/session.go +++ b/server/api/store/session.go @@ -2,6 +2,7 @@ package store import ( "context" + "time" "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/models" @@ -13,6 +14,15 @@ const ( SessionUIDResolver SessionResolver = iota + 1 ) +// ExpiredSession is a session that has outlived the retention window, reduced to what deleting +// it needs to know. Recorded travels with the UID because it decides whether the session owns a +// recording at all: without it, an instance that records nothing still pays a storage lookup per +// session it deletes. +type ExpiredSession struct { + UID string + Recorded bool +} + type SessionStore interface { // SessionList retrieves a list of sessions based on the provided filters and pagination settings. // It returns the list of sessions, the total count of matching documents, and an error if any. @@ -48,4 +58,17 @@ type SessionStore interface { // SessionUpdateDeviceUID updates device UID references across sessions. It returns an error if any. SessionUpdateDeviceUID(ctx context.Context, oldUID models.UID, newUID models.UID) error + + // SessionListExpired returns up to limit sessions started before the given time, oldest + // first. A session that is still active is never returned, however old it is, and a limit + // that is not positive returns nothing. + // + // Listing is separate from deleting so a caller can act on what a session owns outside the + // database while the row that names it still exists. + SessionListExpired(ctx context.Context, before time.Time, limit int) ([]ExpiredSession, error) + + // SessionDeleteMany deletes the given sessions, cascading into their events. It returns the + // number deleted, which may be lower than the number asked for if a session went away in + // between. An empty slice is a no-op. + SessionDeleteMany(ctx context.Context, uids []string) (int64, error) } diff --git a/server/api/store/storetest/session_tests.go b/server/api/store/storetest/session_tests.go index 037e12be473..e9678fd8e0a 100644 --- a/server/api/store/storetest/session_tests.go +++ b/server/api/store/storetest/session_tests.go @@ -890,3 +890,193 @@ func (s *Suite) TestSessionEventsDelete(t *testing.T) { assert.Len(t, events2, 1) }) } + +// TestSessionCleanup tests the SessionListExpired/SessionDeleteMany pair across all +// implementations. They are exercised together because retention only ever uses them as a pair: +// list a batch, act on it, delete it. +// +// The cases below age sessions through pinClock rather than by setting StartedAt, because +// SessionCreate stamps started_at from the clock and ignores whatever the caller passed. Pinning +// the clock is the only way to build the age distribution retention is about. +func (s *Suite) TestSessionCleanup(t *testing.T) { + ctx := context.Background() + st := s.provider.Store() + + now := time.Date(2026, 8, 11, 12, 0, 0, 0, time.UTC) + cutoff := now.AddDate(0, 0, -180) + + // listUIDs returns every surviving session's UID. + listUIDs := func(t *testing.T) []string { + t.Helper() + + sessions, _, err := st.SessionList(ctx, scope.NewUnbounded(reasonTestQueryMechanics), + st.Options().Match(&query.Filters{}), + st.Options().Paginate(&query.Paginator{Page: -1, PerPage: -1})) + require.NoError(t, err) + + uids := make([]string, 0, len(sessions)) + for _, session := range sessions { + uids = append(uids, session.UID) + } + + return uids + } + + // prune runs the pair the way retention does. + prune := func(t *testing.T, limit int) int64 { + t.Helper() + + expired, err := st.SessionListExpired(ctx, cutoff, limit) + require.NoError(t, err) + + uids := make([]string, len(expired)) + for i, session := range expired { + uids[i] = session.UID + } + + deleted, err := st.SessionDeleteMany(ctx, uids) + require.NoError(t, err) + + return deleted + } + + t.Run("finds nothing when every session is newer than the cutoff", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, 1)) + uid := s.CreateSession(t, WithSessionActive(false)) + clk.now = now + + assert.Equal(t, int64(0), prune(t, 100)) + assert.Equal(t, []string{string(uid)}, listUIDs(t)) + }) + + t.Run("deletes only the sessions started before the cutoff", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -10)) + device := s.CreateDevice(t) + s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = cutoff.AddDate(0, 0, 10) + kept := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = now + + assert.Equal(t, int64(2), prune(t, 100)) + assert.Equal(t, []string{string(kept)}, listUIDs(t)) + }) + + t.Run("lists the oldest first and stops at the limit", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -30)) + device := s.CreateDevice(t) + oldest := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = cutoff.AddDate(0, 0, -20) + middle := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = cutoff.AddDate(0, 0, -10) + newest := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = now + + expired, err := st.SessionListExpired(ctx, cutoff, 2) + require.NoError(t, err) + assert.Equal(t, []store.ExpiredSession{ + {UID: string(oldest), Recorded: false}, + {UID: string(middle), Recorded: false}, + }, expired) + + deleted, err := st.SessionDeleteMany(ctx, []string{string(oldest), string(middle)}) + require.NoError(t, err) + assert.Equal(t, int64(2), deleted) + assert.Equal(t, []string{string(newest)}, listUIDs(t)) + }) + + t.Run("reports whether each expired session was recorded", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -30)) + device := s.CreateDevice(t) + plain := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = cutoff.AddDate(0, 0, -20) + recorded := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + require.NoError(t, st.SessionUpdate(ctx, &models.Session{UID: string(recorded), Recorded: true})) + + clk.now = now + + expired, err := st.SessionListExpired(ctx, cutoff, 100) + require.NoError(t, err) + assert.Equal(t, []store.ExpiredSession{ + {UID: string(plain), Recorded: false}, + {UID: string(recorded), Recorded: true}, + }, expired) + }) + + t.Run("leaves an active session in place however old it is", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -30)) + device := s.CreateDevice(t) + active := s.CreateSession(t, WithSessionDevice(device), WithSessionActive(true)) + s.CreateSession(t, WithSessionDevice(device), WithSessionActive(false)) + + clk.now = now + + assert.Equal(t, int64(1), prune(t, 100)) + assert.Equal(t, []string{string(active)}, listUIDs(t)) + }) + + t.Run("takes the session's events with it", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -30)) + uid := s.CreateSession(t, WithSessionActive(false)) + + require.NoError(t, st.SessionEventsCreate(ctx, &models.SessionEvent{ + Session: string(uid), + Type: models.SessionEventTypePtyOutput, + Timestamp: clk.now, + Data: map[string]interface{}{"output": "test output"}, + Seat: 1, + })) + + clk.now = now + + assert.Equal(t, int64(1), prune(t, 100)) + + _, count, err := st.SessionEventsList(ctx, uid, 1, models.SessionEventTypePtyOutput) + require.NoError(t, err) + assert.Equal(t, 0, count) + }) + + t.Run("lists nothing when the limit is not positive", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -30)) + uid := s.CreateSession(t, WithSessionActive(false)) + clk.now = now + + expired, err := st.SessionListExpired(ctx, cutoff, 0) + require.NoError(t, err) + assert.Empty(t, expired) + assert.Equal(t, []string{string(uid)}, listUIDs(t)) + }) + + t.Run("deleting none is a no-op", func(t *testing.T) { + require.NoError(t, s.provider.CleanDatabase(t)) + + clk := pinClock(t, cutoff.AddDate(0, 0, -30)) + uid := s.CreateSession(t, WithSessionActive(false)) + clk.now = now + + deleted, err := st.SessionDeleteMany(ctx, []string{}) + require.NoError(t, err) + assert.Equal(t, int64(0), deleted) + assert.Equal(t, []string{string(uid)}, listUIDs(t)) + }) +} diff --git a/server/api/store/storetest/suite.go b/server/api/store/storetest/suite.go index 44a1816448e..6912a9aabf7 100644 --- a/server/api/store/storetest/suite.go +++ b/server/api/store/storetest/suite.go @@ -63,6 +63,7 @@ func (s *Suite) Run(t *testing.T) { s.TestSessionEventsCreate(t) s.TestSessionEventsList(t) s.TestSessionEventsDelete(t) + s.TestSessionCleanup(t) }) t.Run("TagStore", func(t *testing.T) { diff --git a/server/app/server.go b/server/app/server.go index 9f45e9f6e21..fbf6f3e9c49 100644 --- a/server/app/server.go +++ b/server/app/server.go @@ -70,6 +70,15 @@ type Env struct { // AutoSSL reports whether the console is served over HTTPS; it selects the // scheme of that approval URL. AutoSSL bool `env:"SHELLHUB_AUTO_SSL,default=false"` + + // SessionRetentionDays is how long a session and its events are kept after the session + // started; 0, the default, keeps them indefinitely. + // + // The default is off because the deletion is permanent and unattended — session events are + // the recording. Choosing a window is a deployment decision, made where the compliance + // commitment and the volume are both known, so it is the deployment that sets this rather + // than the binary assuming one. docker-compose.enterprise.yml does exactly that. + SessionRetentionDays int `env:"SHELLHUB_SESSION_RETENTION_DAYS,default=0"` } // sshEnv is parsed with the SSH_ prefix, keeping the names the ssh service used. @@ -186,6 +195,13 @@ func (s *Server) Setup(ctx context.Context) error { servicesOptions = append(servicesOptions, feOpts...) + rpOpts, err := s.sessionRecordingPrunerOption(ctx, store, cache) + if err != nil { + return err + } + + servicesOptions = append(servicesOptions, rpOpts...) + routerOptions, err := s.routerOptions() if err != nil { return err @@ -211,6 +227,13 @@ func (s *Server) Setup(ctx context.Context) error { s.worker.HandleCron(services.CronEnrollmentCallbackCleanup, service.EnrollmentCallbackCleanup(), asynq.Unique()) s.worker.HandleCron(services.CronSSHApprovalCleanup, service.SSHApprovalCleanup(), asynq.Unique()) + if retention := time.Duration(s.env.SessionRetentionDays) * 24 * time.Hour; retention > 0 { + s.worker.HandleCron(services.CronSessionCleanup, service.SessionCleanup(retention), asynq.Unique()) + log.WithField("days", s.env.SessionRetentionDays).Info("session retention enabled") + } else { + log.Warn("session retention disabled; sessions and their events are kept indefinitely") + } + // Apply any worker extensions registered by cloud/enterprise packages. routes.ApplyWorkerExtensions(s.worker, store, cache) @@ -411,6 +434,28 @@ func (s *Server) licenseEvaluatorOption(ctx context.Context, st store.Store, c c return nil, nil } +// sessionRecordingPrunerOption initialises the recording pruner when an enterprise package +// registered a factory. Its factory also returns nil on an enterprise instance with no object +// storage configured, where there are no recordings to prune. Same nil guard, same reason as +// licenseEvaluatorOption. +func (s *Server) sessionRecordingPrunerOption(ctx context.Context, st store.Store, c cache.Cache) ([]services.Option, error) { + factory := services.SessionRecordingPrunerFactory() + if factory == nil { + return nil, nil + } + + rp, err := factory(ctx, st, c) + if err != nil { + return nil, errors.Join(errors.New("init session recording pruner"), err) + } + + if rp != nil { + return []services.Option{services.WithSessionRecordingPruner(rp)}, nil + } + + return nil, nil +} + // firewallEvaluatorOption initialises the firewall evaluator when an enterprise package // registered a factory. The nil guard carries the same weight as in // licenseEvaluatorOption: injecting a typed nil would panic on first use.