feat: auto-archive inactive chats with audit trail - #24642
Conversation
Docs preview |
johnstcn
changed the title
d8d9e0e to
4583b50
Compare
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
🤖
Clean implementation. The batch-paginated archive loop, the family-activity LATERAL subquery, and the status-aware filtering are well thought through. Test coverage is thorough: 8 subtests covering disabled state, active/inactive, child-activity propagation, status filtering, pinned exemptions, multi-owner audits, idempotency, and batch pagination.
5 reviewers (Test Auditor, Database, Go Architect, Edge Case Analyst, Contract Auditor) ran in parallel. After cross-checking for contradictions and weak evidence, 1 nit survived. Most flagged concerns (authz asymmetry on GetChatAutoArchiveDays, audit coupling in dbpurge, nil-deref on auditor load) are consistent with established codebase patterns (GetChatRetentionDays, autobuild.NewExecutor, ActivateDormantUser).
4583b50 to
641ee3a
Compare
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured PR. The LATERAL family-activity subquery, partial index targeting, and batch-limited purge loop are solid. Test coverage is thorough: 6 subtests with a real Postgres backend, deterministic multi-tick control via quartz.Mock, and proportional batch-size testing. The decision log in the PR description is exemplary.
3 P2, 10 P3, 1 Nit, 2 Notes. 16 reviewers ran in parallel; Ging-Go and Takumi reported no findings (Go idioms and concurrency are clean).
The dominant theme is that the cascade step (c.root_chat_id = t.id) applies no per-child filtering for status or pin_order, creating a gap between the documentation's unqualified guarantees ("Chats whose status indicates ongoing work are never auto-archived", "Pinned conversations are never auto-archived") and the actual behavior. The same cascade shape exists in manual ArchiveChatByID, so this is a consistent convention, not a regression. The simplest fix is likely scoping the doc language to root chats rather than adding complexity to the cascade.
This is a first-round panel review. Findings are the panel's output, not a request for changes.
"Shall I show you? ♥" (Hisoka, on what happens to the audit trail under load)
🤖 This review was automatically generated with Coder Agents.
9e35586 to
1766474
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. 12 of 16 R1 findings addressed, 3 acknowledged, 1 contested (DEREM-16 closed by panel vote, 4/4).
All three P2s are fixed: docs scoped to root chats (DEREM-1, DEREM-4), dispatch timeout removed with children skipped from audit (DEREM-2), non-existent Behavior tab reference replaced with API instructions (DEREM-3). The ORDER BY now matches the partial index (DEREM-5). Test coverage expanded with boundary, deleted-message, and all-four-status tests (DEREM-7/8/9). GET auth aligned with retention (DEREM-10).
The DEREM-2 fix (audit roots only, skip children) introduced a doc/code mismatch in the audit section. Two stale comments survived the DEREM-10 fix. 2 P3, 2 Nit new.
"Oh, this is a nice test suite!" (Bisky, on the 10-subtest harness)
🤖 This review was automatically generated with Coder Agents.
81ffa45 to
e1e03ba
Compare
d0cc93d to
ddbfe37
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3. All 4 R2 findings addressed (e1e03ba). Branch rebased (migration renumbered 000475 -> 000477). Netero R3 clean across all 8 sections.
21 findings across 3 rounds: 17 fixed, 3 accepted, 1 panel-closed. 1 new P3.
Mafuuu and Razor reported no findings. Razor verified every doc claim against the code; all match. The implementation is sound.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
| rest of `dbpurge` during the first run. If you need to prevent the | ||
| initial backfill entirely during a migration window, set | ||
| `auto_archive_days = 0` before upgrading and re-enable it when you | ||
| are ready. |
There was a problem hiding this comment.
This is actually not possible. You need to upgrade in order to have this setting. Should we make auto-archive opt-in (i.e. default = 0)?
There was a problem hiding this comment.
Round 4. DEREM-21 addressed (23ac3ba). All 21 findings from R1-R3 resolved: 18 fixed, 3 accepted, 1 panel-closed.
Netero R4: clean. Panel (Bisky, Mafuuu, Razor): Mafuuu and Razor reported no findings. Razor verified every policy model claim, SQL query property, and doc assertion against the code. One P4 from Bisky: MultipleOwners asserts audit attribution but not the chats' Archived state; other subtests cover that class of assertion.
22 findings across 4 rounds, 21 resolved. The implementation is thorough and well-tested.
🤖 This review was automatically generated with Coder Agents.
| COALESCE(activity.last_activity_at, c.created_at) AS last_activity_at | ||
| FROM chats c | ||
| LEFT JOIN LATERAL ( | ||
| SELECT MAX(cm.created_at) AS last_activity_at |
There was a problem hiding this comment.
If a chat message is edited, does that change created_at?
There was a problem hiding this comment.
The UpdateChatMessageByID query does not. But editing a chat message in the UI calls the patchChatMessage handler. This calls EditMessage, which soft-deletes the original and all subsequent messages, and then inserts a new message with a fresh created_at. So, yes.
| )::timestamptz AS last_activity_at | ||
| FROM archived a | ||
| LEFT JOIN to_archive t ON t.id = a.id | ||
| ORDER BY (a.root_chat_id IS NULL) DESC, a.owner_id ASC, a.created_at ASC, a.id ASC; |
There was a problem hiding this comment.
Doing all of this in a single query is efficient, but I worry about having repeated logic (e.g. archival cascades to children) in multiple places. This is a case where inefficiency might be beneficial (pull target IDs and call regular archival path).
There was a problem hiding this comment.
How about GetAutoArchivableChatIDs + ArchiveChatsByIDs? Could do it as a separate post-merge followup.
3 participants
Adds a background job in
dbpurgethat periodically archives chats inactive beyond a configurable threshold. Each archived root chat gets a background audit entry taggedchat_auto_archive. Disabled by default.AutoArchiveInactiveChatsSQL query with LATERAL last-activity subquery and partial index on archive candidatessite_configs-backedauto_archive_dayssetting with admin-only PUT, any-authenticated-user GETroot_chat_id; pinned chats and active threads exemptpatchChat) behaviorStacked PR #24643 adds per-owner digest notifications.
Stacked PR #24704 adds the requisite UI controls.