Backport 2409 (db session cleanup) to 0.10.x release - #2403
Backport 2409 (db session cleanup) to 0.10.x release#2403supreme-gg-gg wants to merge 3 commits into
Conversation
github-actions
Bot
added
enhancement
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
81d7cc5 to
9f86c76
Compare
github-actions
Bot
added
enhancement
| if err := mgr.Add(httpserver.NewMemoryCleanupRunnable(dbClient, 0)); err != nil { | ||
| setupLog.Error(err, "unable to set up memory cleanup runnable") | ||
| // DB TTL cleanup (memory + sessions) runs only on the leader to avoid duplicate deletes. | ||
| if err := mgr.Add(httpserver.NewDbCleanupRunnable(dbClient, 1*time.Minute, cfg.Database.SessionRetentionDays)); err != nil { |
There was a problem hiding this comment.
NewDbCleanupRunnable mentions the default is every 24 hours, but this runs every minute by default and is hardcoded that way. Should we make the frequency configurable?
There was a problem hiding this comment.
Oops, I changed it to every minute for testing but forgot to change it back... I also thought about making this configurable. However, for now I don't find it too useful and once every day would be a reasonable choice for most users. We will need more complicated logic like check cleanup period < TTL, handle different time units (minutes, hours, days) etc and it doesn't bring much benefit.
|
|
||
| -- DeleteExpiredSessionsBatch hard-deletes up to batch_size idle sessions whose | ||
| -- updated_at is older than retention_days, plus cascaded conversation state. | ||
| -- name: DeleteExpiredSessionsBatch :one |
There was a problem hiding this comment.
There is a potential for a race condition with this. The expiry CTE snapshots updated_at, but the final delete only rechecks id/user_id. So you can have an event update the session, followed by cleanup deleting the session, leaving the new event orphaned.
There was a problem hiding this comment.
I've changed the CTE to use a FOR UPDATE locking clause, it also skips sessions locked by other transactions to avoid waiting (since a session actively being updated will not expire), good catch, thanks!
| err := c.withTx(ctx, func(q *dbgen.Queries) error { | ||
| var err error | ||
| n, err = q.DeleteExpiredSessionsBatch(ctx, dbgen.DeleteExpiredSessionsBatchParams{ | ||
| RetentionDays: int32(retentionDays), |
There was a problem hiding this comment.
Is there a reason to convert the int to an int32 here? Should we just take in an int32? Probably not a problem based on the expected value ranges, but an int could be truncated by this conversion.
There was a problem hiding this comment.
flag.IntVar takes in an *int and sqlc generates int32 for integer type, so we will need this conversion somewhere unfortunately
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
supreme-gg-gg
changed the title
This targets the 0.10.x release. To use, e.g. set
database.posetgres.sessionRetentionDays: 30The following tables are eligible for cleanup: session, shares, events (ADK), push notifications (A2A), tasks (A2A), checkpoints (LangGraph), memory and flow states (CrewAI)
This shares the same cleanup runner as long term memory.