Skip to content

feat(server): prune sessions past a configurable retention window - #6888

Merged
otavio merged 2 commits into
masterfrom
perf/session-retention-policy
Aug 18, 2026
Merged

feat(server): prune sessions past a configurable retention window#6888
otavio merged 2 commits into
masterfrom
perf/session-retention-policy

Conversation

@otavio

@otavio otavio commented Aug 11, 2026

Copy link
Copy Markdown
Member

Rebased onto master. This no longer sits on #6883, which now lands after it — the rest of the
perf stack has gone the same way (#6886 already targets master on its own). Two commits:
migration 019, then the retention window itself (the follow-up fix for head-of-line blocking
is squashed into it, since the feature never shipped without it).

Closes shellhub-io/team#201 — that trailer cannot close a team issue from here, so the issue
needs closing by hand on merge.

Why

Nothing pruned sessions or session_events. On the instance this was measured on the oldest
session dated from 2023-06-05, and session_events alone was 4,517 MB across 6.7M rows — 76%
of the entire database
. That is why the database does not fit in a sane shared_buffers, and
it is a data retention gap in its own right (see team#106, team#96).

What

Migration 019 — autovacuum. The default scale factors are proportions of the table, so the
bigger it gets the longer autovacuum waits: at 0.2, session_events gets no vacuum until ~1.3M
rows are dead, and at 0.1 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.
Lowered to 0.05/0.02, plus a one-off ANALYZE — the ALTERs only change when the daemon next
acts, so they do nothing about a backlog that already exists.

Retention cron. A nightly job deletes sessions started longer ago than the window,
cascading into their events.

  • Off by default. The deletion is permanent and unattended, and 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 retention commitment and the volume
    are both known.
  • Batched, and capped at 100k sessions per run. An instance adopting this can have years to
    shed; draining that in one 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.
  • Active sessions are never pruned. closed is not usable for this — a session whose server
    died never gets closed and would otherwise be immortal — so the guard is an anti-join against
    active_sessions, which is what a live session actually holds.
  • Recordings go 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.
    Community has no object storage, so the pruner is a registered seam (like the firewall and
    license evaluators) that the cloud module fills in — see shellhub-io/cloud#2490.

Note for reviewers

Two things arrive here that used to come from #6883, because the rebase moved this PR in front
of it.

TestNonTransactionalMigrations guards the two conditions a statement PostgreSQL refuses inside
a transaction needs, neither of them visible in the SQL: 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. Nothing in this PR needs the exemption — 019 is
transactional throughout — but the guard has to exist before the first migration that does, and
#6883 brings that migration.

The match is anchored to the start of a statement rather than searched for anywhere in the chunk.
That is a prerequisite, not a drive-by: unanchored, VACUUM is also a substring of
autovacuum_vacuum_scale_factor, so the guard would condemn the ALTER TABLE in 019, which is
transactional in every respect. Every keyword in the list is statement-initial, and the check is
per-statement, so it is strictly stronger than a substring search.

#6883 carries the identical file, so whichever lands first the other rebases without a conflict.

One rebase fix worth naming: master has since grown its own fixedClock/pinClock in
storetest/helpers.go, so session_tests.go uses those instead of declaring a second copy.

Testing

  • Store: list/delete pair covered for cutoff, ordering, limit, active-session exclusion and
    event cascade. Mutation-checked — flipping the ORDER BY and removing the anti-join each fail.
  • Service: batching, the per-run cap, the disabled window, and that a failing recording prune
    leaves the rows alone.
  • Migration: reloptions and last_analyze asserted on a migrated database.
  • Re-verified after the rebase onto master: ./api/store/... and ./api/services/... green,
    go build/go vet clean, golangci-lint run ./... reports 0 issues.

@otavio
otavio requested review from a team as code owners August 11, 2026 22:22
@gustavosbarreto
gustavosbarreto force-pushed the perf/enable-pg-stat-statements branch from 2a1352d to a1841fe Compare August 12, 2026 13:42
@otavio
otavio force-pushed the perf/session-retention-policy branch from 7b8205d to 6e33420 Compare August 12, 2026 16:33
@otavio
otavio force-pushed the perf/enable-pg-stat-statements branch from a1841fe to 43dd80a Compare August 12, 2026 21:46
@otavio
otavio force-pushed the perf/session-retention-policy branch from 6e33420 to 1848aeb Compare August 12, 2026 21:46
@otavio
otavio force-pushed the perf/enable-pg-stat-statements branch 2 times, most recently from c9ce2ba to 381eb43 Compare August 13, 2026 11:30
@otavio
otavio changed the base branch from perf/enable-pg-stat-statements to perf/device-heartbeat-hot-updates August 13, 2026 11:59
@otavio
otavio force-pushed the perf/device-heartbeat-hot-updates branch from e5a3a4f to 9aa45a3 Compare August 13, 2026 12:34
@otavio
otavio force-pushed the perf/session-retention-policy branch 2 times, most recently from 3df2e69 to 7f5a963 Compare August 13, 2026 13:28
@otavio
otavio force-pushed the perf/device-heartbeat-hot-updates branch from 59a4fd9 to 08b5b32 Compare August 17, 2026 17:10
@otavio
otavio force-pushed the perf/session-retention-policy branch from 7f5a963 to a0656a4 Compare August 17, 2026 17:10
@otavio
otavio force-pushed the perf/device-heartbeat-hot-updates branch from 08b5b32 to a36cd9e Compare August 17, 2026 20:29
@otavio
otavio force-pushed the perf/session-retention-policy branch from a0656a4 to b78d9ff Compare August 17, 2026 20:29
@luizhf42
luizhf42 force-pushed the perf/session-retention-policy branch from b78d9ff to 3ffe68f Compare August 17, 2026 20:51
@otavio
otavio force-pushed the perf/device-heartbeat-hot-updates branch from a36cd9e to 6ad1d52 Compare August 18, 2026 18:49
@otavio
otavio force-pushed the perf/session-retention-policy branch from 3ffe68f to 7c3d2ee Compare August 18, 2026 18:49
otavio added 2 commits August 18, 2026 17:43
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.
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
@otavio
otavio changed the base branch from perf/device-heartbeat-hot-updates to master August 18, 2026 20:54
@otavio
otavio force-pushed the perf/session-retention-policy branch from 7c3d2ee to f7ca304 Compare August 18, 2026 20:56
@otavio
otavio merged commit c783fc5 into master Aug 18, 2026
39 checks passed
@otavio
otavio deleted the perf/session-retention-policy branch August 18, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant