Repository navigation
Conversation
Add a scheduled sdk_message.retention job that deletes consumed/failed messages in archived worker sessions older than a configurable sdkMessageRetentionDays setting (default off). Deletion respects guardrails: only archived worker sessions not tied to space tasks, skips sessions with active message_delivery jobs, cascades replacement edges, cleans message_search_content / delivery_turn_end, and recomputes the visible message count. Each run is bounded to a batch and self-schedules.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.3 (Z.ai)
Model: glm-5.3 | Client: HyperNeo | Provider: Z.ai
Diff: +929/−85 across 8 files (6 code, 2 test) — code 287 | tests 642 | comments 0 | other 0 lines added
Round-1 whole-PR review. The PR is ~69% test lines — a favorable risk profile for a destructive-path change. The four prior Devin/Codex threads are verified resolved on this head.
Verified safe (traced, with evidence):
- Deletion scope provably narrows to user-archived plain worker sessions.
archivedis terminal (no un-archive RPC;message.sendandsession.messages.retryrefuse archived sessions — message-persistence.ts:197-206, session-handlers.ts:1193-1196), and the colon-id + type +taskId/spaceId/roomIdcontext guards exclude every space/room/conv/task/long-horizon session class (space-runtime-service.ts:573,639; task-agent-manager.ts:710; long-term-agent-session.ts:1-3). The guard idiom mirrors the established search-eligibility predicate (sdk-message-repository.ts:338-344). - Pending-delivery guard matches the queue's only payload shape (
$.sessionId— message-delivery.ts:27-138, message-delivery-outbox.ts:44-68) and the repo's own json_extract convention (job-queue-repository.ts:281-458). send_statusNULL→consumed matches five pre-existing display predicates; the only NULL writer issaveSDKMessage(already-delivered SDK stream), andsaveUserMessagealways writes explicit status (sdk-message-repository.ts:479-545, 591-997).- SELECT→DELETE race is impossible: the whole method is synchronous on bun:sqlite in the single-writer daemon.
- Query plan is indexed (EXPLAIN QUERY PLAN:
idx_sessions_status_last_active→idx_sdk_messages_session_timestamp_id→idx_job_queue_dequeue); no full scan. - Search consistency: archived sessions are already de-indexed at archive time (session-repository.ts:204-205) and at eligibility time (sdk-message-repository.ts:323), so the per-row search delete is defensive; the FTS trigger cleanup matches production DDL (migrations.ts:7293-7317).
- Replacements cascade is real in production (
ON DELETE CASCADEmigrations.ts:8818 +PRAGMA foreign_keys = ONdatabase-core.ts:37). Danglingtarget_uuidedges (target deleted, source retained) are benign — the read path hides only messages whose own uuid is a target (sdk-message-repository.ts:587-590) — and self-heal once sources age past the cutoff. - Failure semantics match the cleanup.handler peer (work-then-enqueue; 3 retries then dead; re-seeded at daemon restart; stale-claim reclaim covers crash-mid-run).
- app.ts import reorder is cosmetic (only the new handler import + registration + startup enqueue are functional); zero-comments rule clean; the web row follows the githubPollingInterval pattern with the intended empty-disables divergence.
Findings: 1 P1, 1 P2 → REQUEST_CHANGES. Details in the line comments.
Passing observations (not blocking):
- The write path persists
sdkMessageRetentionDayswithout daemon-side validation (blind merge, settings-repository.ts:100-105). Read-side guards (finite/>0, clamp to MAX) neutralize it and this mirrors the existinggithubPollingIntervalprecedent; if you touch it, coerce/reject non-integers inprepareGlobalSettingsUpdatefor both fields. - Enqueue-next-run happens after the work (cleanup.handler parity); memory-consolidation.handler enqueues before working and thus survives a throwing handler — a one-line improvement if you want the stronger form.
- The NOT EXISTS re-parses pending delivery payloads per candidate row; indexed and bounded by the 7-day job_queue cleanup, fine at current scale, hoistable to a per-session precomputed set if it ever shows in profiles.
- The new web input omits the
maxattribute its sibling row carries.
Verdict: REQUEST_CHANGES — P0: 0, P1: 1, P2: 1.
🤖 Review by glm-5.3 (Z.ai)
Model: glm-5.3 | Client: HyperNeo | Provider: Z.ai
Diff: +929/−85 across 8 files — code 287 | tests 642 | comments 0 | other 0
P1 — one un-yieldable 50k-row synchronous block per run, repeated every 5 min during catch-up on the exact DB this task targets (packages/daemon/src/lib/job-handlers/sdk-message-retention.handler.ts:6)
A retention run executes the 50k-candidate SELECT plus up to ~150k point statements inside a single this.db.transaction(...) — all synchronous on bun:sqlite (handler.ts:6,33; sdk-message-repository.ts:1381,1417-1425). The daemon is single-process: while this block runs, WebSocket traffic, RPC, SDK ingestion, and timers are frozen (WAL only protects other processes). Reading each candidate row traverses the sdk_message blob's overflow pages (sdk_uuid, timestamp, send_status all sit after the blob in the record) — precisely the vdbeColumnFromOverflow I/O pattern that dominates this ticket's CPU profile. On the ticket's 31 GB / 3.69M-row DB, enabling 30-day retention drains ~74 batches over ~6 hours with a plausible multi-second full-daemon freeze every 5 minutes (magnitude is an estimate; the structural un-interruptibility is not). That inverts the task's goal — the retention mechanism itself becomes the recurring load spike.
Smallest sufficient fix: bound per-run event-loop occupancy — chunk the deletion into small sub-transactions (hundreds to low-thousands of rows) with a yield between chunks (the repo method becomes async; the job processor already awaits handlers), keeping the hasMore → 5-min reschedule; or alternatively drop RETENTION_BATCH_LIMIT to the low thousands with a shorter hasMore delay.
Also unaddressed from the task's Verification section: the demonstration on a synthetic large DB (size reduction with queries still correct). Please run it (a scaled-down synthetic DB with realistic payload sizes is fine) and post the numbers to the PR — rows deleted, per-run wall time, drain duration; that same run quantifies and validates the stall bound above.
P2 — the invalidation fan-out after deletion is untested (packages/daemon/src/storage/repositories/sdk-message-repository.ts:1427-1431)
Both new suites construct new SDKMessageRepository(db as any) without a reactiveDb, so notifySessionsChanged and notifyChange('sdk_messages', …) in that loop are silent no-ops and nothing asserts that affected sessions emit sessions/sdk_messages invalidations after rows are deleted. If a refactor drops or breaks the loop, tests stay green and open UIs keep rendering deleted history and stale badge counts — and "search/history behavior after archival matches the chosen policy" is task criterion #1.
Ask: one repository test that passes a stubbed reactiveDb capturing emissions and asserts both table notifications (with the sessionId scope) for affected sessions and none for untouched ones. ~15 lines alongside the existing suite.
Recommendation: REQUEST_CHANGES
Make deleteExpiredArchivedSessionMessages async and delete in 500-row sub-transactions with an event-loop yield between chunks, so a 50k-row run never holds the single-process daemon in one synchronous block. Add a test pinning sessions/sdk_messages invalidation fan-out for affected sessions.
Synthetic-DB demonstration (addressing the P1 verification ask)Ran the retention path end-to-end on a scaled in-memory synthetic DB mirroring the production schema (sessions, sdk_messages, replacements, delivery_turn_end, job_queue, message_search_content + FTS), with realistic payload sizes (1.5–8 KB per message, 325 MB total). Scale: 122,000 messages seeded across 10 archived sessions (120,000 eligible old messages) + 2 active sessions.
Stall-bound validation (P1): the deletion is now chunked into 500-row sub-transactions with an event-loop yield between chunks, so a full batch never blocks the single-process daemon in one synchronous block. The measured max gap between 2 ms ticks during the entire drain is ~244 ms (dominated by the initial 50k-row candidate SELECT, which only reads id/session_id/sdk_uuid/timestamp — no The demo script is reproducible from the PR review notes if needed. |
|
Addressed both requested changes in P1 — un-yieldable synchronous block: P2 — invalidation fan-out untested: added a repo test with a stubbed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2af3f4b714
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AND sm.session_id NOT LIKE '%:%' | ||
| AND COALESCE(json_extract(s.session_context, '$.taskId'), '') = '' |
There was a problem hiding this comment.
When retention is enabled, an archived legacy worker with a UUID-style ID and empty context is deleted even if space_tasks.task_agent_session_id still references it. That relationship is explicitly treated as a task-session association in SpaceTaskRepository (space-task-repository.ts:99-101), but this query checks only the context field and ID format, so the task's durable transcript can be hard-deleted despite the policy excluding Space task sessions.
Useful? React with 👍 / 👎.
| AND COALESCE(json_extract(s.session_context, '$.taskId'), '') = '' | ||
| AND COALESCE(json_extract(s.session_context, '$.spaceId'), '') = '' | ||
| AND COALESCE(json_extract(s.session_context, '$.roomId'), '') = '' |
There was a problem hiding this comment.
If any archived candidate session has a malformed non-null session_context, SQLite throws malformed JSON from these json_extract calls. The repository already handles this legacy-data case with json_valid before extracting elsewhere (sdk-message-repository.ts:459-463); here the exception exhausts the retention job's retries before it can schedule its successor, stopping retention for the entire running daemon until restart.
Useful? React with 👍 / 👎.
| for (const sessionId of affectedSessions) { | ||
| if (this.supportsVisibleMessageCount()) this.recomputeVisibleMessageCount(sessionId); |
There was a problem hiding this comment.
If the daemon exits or a later chunk throws after one or more 500-row transactions have committed, these post-loop counter updates never run. On the retry, sessions whose expired rows were all removed by an earlier committed chunk are no longer selected or added to affectedSessions, so their persisted visible_message_count remains permanently stale and shows an incorrect badge if the session is later unarchived; recompute the affected sessions as each chunk commits or otherwise persist the pending repairs.
Useful? React with 👍 / 👎.
|
Closing: 1,100+ commits behind and nothing from it landed. sdk_messages retention stays tracked in #2339; re-cut from dev. Branch retained. |
1 participant
Adds a scheduled
sdk_message.retentionjob that deletes old consumed/failed messages in archived sessions, gated by a newsdkMessageRetentionDaysglobal setting (default off).Retention policy (chosen deliberately — please review):
workersessions only — space/room/task-agent sessions and any session referenced by a space task are excluded.message_deliveryjobs and any message still in a pending delivery state; replacement edges cascade;message_search_content/FTS anddelivery_turn_endare cleaned;visible_message_countrecomputed.Follow-up considered but out of scope: TTL on active sessions' messages and a cold-table history shim.