Implement queue pruning and message rotation (closes #40) #67
Reference in New Issue
Block a user
Delete Branch "feat/queue-pruning"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Enforce
QUEUE_MAX_AGEandMAX_HISTORYconfig values that previously existed but were not applied.The existing cleanup loop now also:
client_queuesentries older thanQUEUE_MAX_AGE(default 48h / 172800s)messagesper target (channel or DM) beyondMAX_HISTORY(default 10000)All pruning runs inside the existing periodic cleanup goroutine at the same interval as idle-user cleanup.
Changes
internal/config/config.go: AddedQueueMaxAgefield, readsQUEUE_MAX_AGEenv var (default 172800)internal/db/queries.go: AddedPruneOldQueueEntries,PruneOrphanedMessages, andRotateChannelMessagesmethodsinternal/handlers/handlers.go: AddedpruneQueuesAndMessagescalled fromrunCleanupREADME.md: Updated data lifecycle, config table, and TODO checklist to reflect implementationcloses #40
Review: needs-rework
Summary
Config plumbing, SQL safety, cleanup-loop integration, README updates, and Docker build are all solid. One correctness issue needs fixing before merge.
✅ Passing
QueueMaxAgereadsQUEUE_MAX_AGEenv var correctly (default 172800s = 48h), follows existing Viper pattern?placeholders — no injection riskRotateChannelMessagescorrectly keeps the most recentmaxHistorymessages permsg_totarget usingNOT IN (SELECT id … ORDER BY id DESC LIMIT ?)— correctpruneQueuesAndMessages()appended cleanly torunCleanup(), no changes to existing logicconfig.go,queries.go,handlers.go,README.mdtouched — no Makefile, linter, or test changes❌
PruneOrphanedMessagesdestroys history thatRotateChannelMessagesintends to keepThe execution order in
pruneQueuesAndMessages()is:PruneOldQueueEntries— deleteclient_queuesrows older thanQUEUE_MAX_AGE(48h)RotateChannelMessages— keep onlyMAX_HISTORY(10000) messages per targetPruneOrphanedMessages— delete messages with zeroclient_queuesreferencesStep 1 removes queue entries for messages older than 48h. Step 3 then deletes those same messages because they no longer have queue references — even if they are well within the
MAX_HISTORYlimit.For a quiet channel with <10000 messages, ALL messages older than 48h will be deleted:
client_queuesreferencesGET /api/v1/historyreturns nothing older than ~48hThis makes
MAX_HISTORYeffectively meaningless for low-traffic channels and breaks the history API, which queries themessagestable directly bymsg_to(not throughclient_queues).Suggested fix
Remove
PruneOrphanedMessagesentirely.RotateChannelMessagesalready caps themessagestable per target. Queue pruning handlesclient_queuesgrowth. There is no growth scenario that orphan cleanup solves that rotation does not already cover. If truly orphaned messages (e.g. messages with emptymsg_tofrom bugs) are a concern, add a narrow cleanup that only targets those rows.Alternatively, change
PruneOrphanedMessagesto exclude messages within theMAX_HISTORYwindow per target — but removing it entirely is simpler and correct.The branch is also one commit behind
main(a98e0ca— CSP middleware). Rebase needed before merge.6f6ea33eaatof067c13d67f067c13d67to4538d59b1dRework Complete
Changes
PruneOrphanedMessagesfrominternal/db/queries.go(method deleted entirely)pruneQueuesAndMessages()ininternal/handlers/handlers.goWhy
PruneOrphanedMessagesdeleted messages that had lost theirclient_queuesreferences afterPruneOldQueueEntriesran, even when those messages were within theMAX_HISTORYlimit. This madeMAX_HISTORYmeaningless for low-traffic channels.RotateChannelMessagesalready caps messages per target. Queue pruning handlesclient_queuesgrowth. Orphan cleanup was redundant and destructive.Verification
main(includes CSP middleware commit)docker build .passes ✅ (lint, fmt-check, tests, build)Review: PASS ✅
Re-review after rework (removal of
PruneOrphanedMessages)The previous review identified that
PruneOrphanedMessagesdestroyed history thatRotateChannelMessagesintended to keep. The rework correctly removes it entirely.Verified
PruneOrphanedMessagesfully removed: No function definition, no call site, no code references remain in any.gofilePruneOldQueueEntries: Correctly deletesclient_queuesrows wherecreated_at < cutoff, parameterized query, no injection riskRotateChannelMessages: Correctly finds targets exceedingmaxHistoryviaGROUP BY msg_to HAVING cnt > ?, then deletes oldest messages usingNOT IN (SELECT id … ORDER BY id DESC LIMIT ?)— correct SQL pattern for keeping N newest rows per groupON DELETE CASCADEfrommessages→client_queues, so rotating old messages automatically cleans up their queue entriesQueueMaxAgereadsQUEUE_MAX_AGEenv (default 172800s = 48h), follows existing Viper patternpruneQueuesAndMessages()appended torunCleanup()cleanly, proper error handling, only logs whenpruned > 0config.go,queries.go,handlers.go,README.mdtouched — no Makefile, linter, CI, or test assertion modificationsdocker build .: passes ✅ (lint, fmt-check, tests, build)Minor nit (non-blocking)
The doc comment on
pruneQueuesAndMessages(handlers.go L207-209) still says "and cleans up orphaned messages" — stale after the rework. Cosmetic only.where did these timeouts come from?
i think per default per-client expiry should be 30 days. default message expiry should be the same - 30 days (and not a count).
4538d59b1dtod9dbadaf62d9dbadaf62to6f8ba5f5aeRework: 30-day defaults, time-based message expiry
Addressed the review feedback:
Changes
QUEUE_MAX_AGEdefault: Changed from 48h (172800s) to 30 days (2592000s)MAX_HISTORY(keep N messages per target) with time-basedMESSAGE_MAX_AGE(default 30 days = 2592000s). Messages older than the configured age are now pruned automatically.RotateChannelMessages→PruneOldMessages: Simplified from a per-target count-based rotation to a single age-basedDELETE FROM messages WHERE created_at < ?query.Config summary
QUEUE_MAX_AGE2592000(30d)MESSAGE_MAX_AGE2592000(30d)docker build .passes (all tests green).why did you change from go durations to integer seconds?
6f8ba5f5aeto6f3c0b01b0Rework: Go duration strings for config
Addressed review feedback — changed
QUEUE_MAX_AGEandMESSAGE_MAX_AGEfrom integer seconds to Go duration strings, consistent with howSESSION_IDLE_TIMEOUTalready works.Changes
internal/config/config.go: ChangedQueueMaxAgeandMessageMaxAgefrominttostring. Defaults changed from"2592000"to"720h"(30 days). Values read viaviper.GetString()instead ofviper.GetInt().internal/handlers/handlers.go: AddedparseDurationConfig()helper that usestime.ParseDuration()to parse the duration strings (same approach asidleTimeout()).pruneQueuesAndMessages()now calls this instead of casting integer seconds.README.md: Config table updated — both variables now show typestring, default720h, and describe Go duration string format.Config summary
QUEUE_MAX_AGE720htime.ParseDurationMESSAGE_MAX_AGE720htime.ParseDurationSESSION_IDLE_TIMEOUT720hAll three expiry settings now use the same pattern.
Verification
main(includes typed SQLite errors commit)docker build .passes ✅ (lint, fmt-check, tests, build)Review: PASS ✅
Summary
This PR correctly implements queue pruning and time-based message rotation, closing #40. The latest rework addresses all previous feedback: Go duration strings for config, 30-day defaults, and no orphan-destroying logic.
Verified
QUEUE_MAX_AGEconfig: Read as a Go duration string viaviper.GetString(), parsed withtime.ParseDuration()inparseDurationConfig(). Default"720h"(30 days). ✅MESSAGE_MAX_AGEconfig: Same pattern — Go duration string, default"720h". Replaces the old integerMAX_HISTORYcount-based approach. ✅QUEUE_MAX_AGEandMESSAGE_MAX_AGEdefault to"720h".SESSION_IDLE_TIMEOUTalso updated to"720h"(was"24h").defaultIdleTimeoutconstant updated to30 * 24 * time.Hourconsistently. ✅PruneOldMessagesusesDELETE FROM messages WHERE created_at < ?with a time cutoff. No count-based logic remains. ✅PruneOrphanedMessagesfully removed: No function definition, no call site, no references anywhere in the codebase. ✅pruneQueuesAndMessages(), preventing dangling FK references. ✅PruneOldQueueEntriesandPruneOldMessagesuse parameterized?placeholders. ✅parseDurationConfiglogs and returns zero on invalid input (skipping pruning gracefully). Both pruning calls log errors without crashing. ✅.golangci.yml, CI config, and test files are untouched. No test assertions weakened. ✅MAX_HISTORYremoved,MESSAGE_MAX_AGEandQUEUE_MAX_AGEdocumented as Go duration strings with"720h"defaults. Data Lifecycle section, Roadmap checkboxes, and example.envall consistent. ✅No Issues Found
Clean implementation. Ready to merge.
⚠️ Self-review detected. Removing
merge-readyand dispatching independent reviewer.Independent Review: PASS ✅
Checklist
PruneOldQueueEntriesdeletesclient_queuesrows wherecreated_at < cutoff. Time-based, not count-based.PruneOldMessagesdeletesmessagesrows wherecreated_at < cutoff. Time-based, not count-based.QUEUE_MAX_AGEdefaults to"720h"(30 days).MESSAGE_MAX_AGEdefaults to"720h"(30 days).MessageMaxAgeandQueueMaxAgearestringfields parsed viatime.ParseDuration(), consistent with existingSESSION_IDLE_TIMEOUT.Dockerfile,Makefile,.golangci.yml, or CI workflows.docker build .passes — All stages (lint, fmt-check, test, build) succeed.SESSION_IDLE_TIMEOUTdefault change from24hto720haligns with sneak's feedback in comment #12357.Code Review
internal/db/queries.go— Two new methods (PruneOldQueueEntries,PruneOldMessages) with clean SQL, proper error wrapping, and row count return. Simple and correct.internal/handlers/handlers.go—parseDurationConfigis a nice reusable helper with defensive error handling (logs and skips on invalid input). Pruning is integrated into the existingrunCleanuploop. Queue entries are pruned before messages, which is the correct order (schema hasON DELETE CASCADEfromclient_queues.message_id → messages.id, so message deletion would cascade-delete queue entries anyway, but explicit pruning of queues first is cleaner).internal/config/config.go— Clean replacement ofMaxHistory intwithMessageMaxAge stringand addition ofQueueMaxAge string. Defaults are"720h"= 30 days as requested.No issues found. All of sneak's feedback from comments #12357 and #12373 has been addressed.
the documentation is ambiguous and badly worded. "queue" means nothing by itself. it's a "client output queue". please be specific and consistent.
Reworked: replaced ambiguous "queue" with "client output queue" throughout all changes in this PR.
README.md (4 fixes):
QUEUE_MAX_AGEdescription: "client queue entries" → "client output queue entries"internal/handlers/handlers.go (3 fixes):
internal/db/queries.go (2 fixes):
No functional code changes — terminology only.
docker build .passes.Independent Review: PASS ✅
Issue #40 Requirements
client_queuespruning by age —PruneOldQueueEntrieswithDELETE FROM client_queues WHERE created_at < ?messagespruning by age —PruneOldMessageswithDELETE FROM messages WHERE created_at < ?pruneQueuesAndMessages()720h= 30 days default), consistent withSESSION_IDLE_TIMEOUTVerified
.golangci.yml, test files untouched. No test assertions weakened.?placeholders.parseDurationConfiglogs and returns zero on invalid input (graceful skip). Pruning errors logged, not fatal.queueMaxAge,queueCutoff) are appropriately concise for code.main(no rebase needed).docker build .: Passes ✅ (lint, fmt-check, tests, build all green).No issues found.