fix(engine): add retention cron to delete expired UserSession rows#4216
Closed
AlexlaGuardia wants to merge 1 commit into
Closed
fix(engine): add retention cron to delete expired UserSession rows#4216AlexlaGuardia wants to merge 1 commit into
AlexlaGuardia wants to merge 1 commit into
Conversation
The UserSession table grows unboundedly: PR hatchet-dev#3923 stopped Bearer-authed requests from creating phantom sessions, but nothing purges rows that have already expired. The only DELETE is by id (explicit logout), and tenant retention does not cover this table. Add a periodic cleanup to the retention controller, modelled on the existing runCleanupOldWorkers / runDeleteMessageQueueItems jobs: - CleanupExpiredUserSessions sqlc query: batched DELETE of expired rows - UserSessionRepository.DeleteExpired: one batch per call inside a statement-timeout-bounded transaction, returning whether more remain - runCleanupExpiredUserSessions: 24h gocron job (SingletonMode) that drains the backlog under a 30-minute context timeout - index migration on UserSession(expiresAt) so each batch does not seq-scan Fixes hatchet-dev#3913
|
@AlexlaGuardia is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
Collaborator
|
Hey @AlexlaGuardia. Thanks for the contribution here but unfortunately this feature is already covered by #4105 |
Author
|
Makes sense, thanks for checking. #4105 covers it, closing this out. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3913.
Problem
The
UserSessiontable grows unboundedly. On the reporter's self-hosted deployment it reached 2.6M rows (~1 GB) in 30 days, alluserId = NULL, written once and never read again. PR #3923 fixed the root-cause insert (Bearer-authed requests no longer create phantom sessions), but nothing purges the rows that already expired: the onlyDELETEon the table isDeleteUserSessionby id (explicit logout), and tenant retention doesn't cover this table.This is the follow-up @gregfurman asked for in #3913 — "we should also be adding a cron to periodically clean up these expired entries."
Fix
A periodic cleanup in the retention controller, modelled on the existing
runCleanupOldWorkers/runDeleteMessageQueueItemsjobs:CleanupExpiredUserSessionssqlc query —DELETEof expired rows via aLIMITedid IN (SELECT …)subquery, so each call deletes a bounded batch.UserSessionRepository.DeleteExpired(ctx, batchSize)— runs one batch inside a statement-timeout-bounded transaction (PrepareTxWithStatementTimeout, 3 min), returning whether a full batch was deleted. Same shape asworkerRepository.CleanupOldWorkers.runCleanupExpiredUserSessionsretention job — loopsDeleteExpireduntil a partial batch (backlog drained), under a 30-min context timeout. Registered as a 24hgocronjob withSingletonMode, mirroring the worker-retention block.v1_0_118) —CREATE INDEX CONCURRENTLYon"UserSession"("expiresAt"). Without it each batch sequential-scans the table, which matters precisely because the backlog can be in the millions. AddedCONCURRENTLY+-- +goose no transactionper the existing precedent (v1_0_47).Notes / decisions
queueRetention/dataRetentionprecedent (always-on global cleanups) rather thanworkerRetention(which has anEnableWorkerRetentionflag only because it defaults off). Deleting expired sessions is universally safe — they're unusable by definition — so there's no operator reason to disable it. AWithSessionRetentionopt exists if you'd prefer it gated behind config; say the word and I'll wireEnableSessionRetentionthroughserver.go/loader.go/run.goto match the worker knob.CleanupOldWorkers.Testing
go build+go vetclean on the changed packages. The retention jobs are integration-level (no existing unit tests forretention/or the session repo), so I didn't add unit tests to match the existing convention — happy to add an integration test that seeds expired rows and asserts the drain if you'd like one before merge.AI usage disclosure
Per CONTRIBUTING.md#ai-usage: implemented with Claude Code (Opus). It traced the issue through the retention controller and repository layers, matched the existing
CleanupOldWorkerscleanup pattern, regenerated the sqlc binding, and added the index migration. I reviewed every change against the repo's conventions before opening. Same human-directed workflow and disclosure as #4180 and #4214.