feat(hub,web): support scheduling messages for future delivery#590
feat(hub,web): support scheduling messages for future delivery#590junmo-kim wants to merge 4 commits intotiann:mainfrom
Conversation
5b45f12 to
289be9d
Compare
There was a problem hiding this comment.
Findings
-
[Major] Mature scheduled messages can be dropped after a later message advances the CLI cursor —
releaseMatureScheduledMessagesreuses the row's originalseq, but the CLI drops any incoming message whoseseq <= lastSeenMessageSeq(cli/src/api/apiSession.ts:280). Schedule seq=10, send immediate seq=11, then when seq=10 matures the CLI ignores it and the DB row remains uninvoked. Evidencehub/src/sync/messageService.ts:465.
Suggested fix:private readonly seenMessageIds = new Set<string>() private handleIncomingMessage(message: { id?: string; seq?: number; localId?: string | null; content: unknown }): void { const id = typeof message.id === 'string' ? message.id : null if (id && this.seenMessageIds.has(id)) return const seq = typeof message.seq === 'number' ? message.seq : null if (!id && seq !== null && this.lastSeenMessageSeq !== null && seq <= this.lastSeenMessageSeq) return if (id) this.seenMessageIds.add(id) if (seq !== null) this.lastSeenMessageSeq = Math.max(this.lastSeenMessageSeq ?? 0, seq) // existing parse/enqueue logic }
-
[Major] Reconnect backfill can deliver future scheduled messages immediately —
/cli/sessions/:id/messagesstill uses the normalgetMessagesAfterstream, and the CLI enqueues every backfilled user message (cli/src/api/apiSession.ts:347). A reconnect beforescheduledAttherefore bypasses the new mature-scan path and sends the prompt early. Evidencehub/src/sync/messageService.ts:149.
Suggested fix:export function getDeliverableMessagesAfter(db: Database, sessionId: string, afterSeq: number, limit = 200, now = Date.now()): StoredMessage[] { const safeLimit = Number.isFinite(limit) ? Math.max(1, Math.min(200, limit)) : 200 const rows = db.prepare(` SELECT * FROM messages WHERE session_id = ? AND seq > ? AND (scheduled_at IS NULL OR scheduled_at <= ?) ORDER BY seq ASC LIMIT ? `).all(sessionId, afterSeq, now, safeLimit) as DbMessageRow[] return rows.map(toStoredMessage) }
-
[Major] Scheduled attachment sends can persist dangling file paths — the API accepts
attachmentstogether withscheduledAt, but uploads are per CLI session andsendSessionDeathremoves that upload directory (cli/src/api/apiSession.ts:517). If the CLI exits before the scheduled send matures, the stored attachment paths become invalid; the agent later receives@pathreferences to deleted files (cli/src/utils/attachmentFormatter.ts:11). Evidencehub/src/web/routes/messages.ts:19.
Suggested fix:}).refine( (data) => data.scheduledAt == null || !data.attachments?.length, { message: 'scheduled messages with attachments are not supported until uploads are retained through invocation', path: ['attachments'] } )
Summary
Review mode: initial
Found delivery correctness issues in the scheduling path. The missing tests are schedule -> later immediate -> mature, reconnect before scheduledAt, and scheduled attachments after CLI/session cleanup.
Testing
Not run (automation)
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Session-end sweep can drop mature scheduled messages before delivery —
getMatureQueuedLocalMessagesincludes rows wherescheduled_at <= now, andsweepImmediateQueuedOnSessionEndimmediately stamps every returnedlocalIdas invoked. If a CLI re-attaches afterscheduledAtand exits before the 5sreleaseMatureScheduledMessagestick emits the row, or exits after emit but beforemessages-consumed, the scheduled prompt is marked sent without the ack/re-emit guarantee. Evidencehub/src/store/messages.ts:215.
Suggested fix:export function getImmediateQueuedLocalMessages( db: Database, sessionId: string ): StoredMessage[] { const rows = db.prepare(` SELECT * FROM messages WHERE session_id = ? AND invoked_at IS NULL AND local_id IS NOT NULL AND scheduled_at IS NULL ORDER BY seq ASC `).all(sessionId) as DbMessageRow[] return rows.map(toStoredMessage) }
Summary
Review mode: follow-up after new commits
One delivery correctness issue remains in the scheduling/session-end interaction. Missing coverage: mature scheduled row present at session end before a messages-consumed ack should remain queued for the release/re-emit path.
Testing
Not run (automation)
HAPI Bot
Adds a `scheduled_at` column to the messages table so a single row can
represent both immediate and time-deferred messages, plus the partial index
that makes the mature-scan query cheap.
- V9 migration: idempotent ALTER + partial index, multi-hop tested for
V6/V7/V8 -> V9 and fresh-DB.
- StoredMessage / DecryptedMessage gain the optional `scheduledAt` field.
- addMessage takes a `scheduledAt` parameter and throws when it is set
without a localId — without an ack path, the row would be stamped
invoked_at immediately and the schedule would be silently lost.
- New data-layer queries:
* getMatureScheduledMessages — rows ready to fire (idx-backed).
* getDeliverableMessagesAfter — CLI reconnect backfill that excludes
future-scheduled rows (replaces the previous unfiltered backfill).
* getImmediateQueuedLocalMessages — non-scheduled queued rows, used
only by the session-end sweep; excludes all scheduled rows so the
mature-scan path retains its no-stamp + re-emit-until-ack contract.
* getUninvokedLocalMessages — surfaces queued rows (immediate + future
scheduled) for the Web floating bar on refresh / secondary clients.
This commit is the schema/data layer; the hub orchestration that uses
these queries lands in the next commit.
…iants, sweep contract)
Builds on the schema layer to wire the actual schedule -> mature -> emit ->
ack -> stamp pipeline through the hub. No new timer thread: the existing
5 s expireInactive tick now also calls releaseMatureScheduledMessages, and
the existing CLI messages-consumed ack remains the sole writer of
invoked_at.
- MessageService.sendMessage:
* future-scheduled rows are stored only and skipped from the realtime
/cli emit (Date.now() is re-read post-insert to avoid a TOCTOU window
where a borderline row could be misclassified as future).
* scheduled + non-empty attachments throws — attachment paths live under
the CLI session upload directory which sendSessionDeath purges, so a
mature emit after the CLI exits would dereference deleted files.
- MessageService.releaseMatureScheduledMessages:
* emits each mature row to /cli but does NOT call markMessagesInvoked.
On hub crash or runner reattach the next 5 s tick re-emits; the row
drops out only when the CLI ack stamps invoked_at.
- MessageService.sweepImmediateQueuedOnSessionEnd:
* uses getImmediateQueuedLocalMessages (scheduled_at IS NULL) so all
scheduled rows — mature or future — survive session end. Stamping a
mature scheduled row here would skip it from the next mature-scan
(filter on invoked_at IS NULL) and silently drop the user's prompt.
* parameter renamed from `now` to `invokedAt` since the SQL no longer
consumes it as a cutoff.
- MessageService.cancelQueuedMessage:
* future-scheduled rows are short-circuited — the runner never received
them, so asking the CLI for ack would always return not-found and the
existing PR tiann#568 contract would misinterpret that as "CLI consumed it"
and stamp invoked_at. Direct DELETE + message-cancelled SSE instead.
- /cli reconnect backfill (cli.ts route) switches to
getDeliverableMessagesAfter so a reconnect during a scheduled window
cannot replay future rows ahead of their scheduled_at.
- REST POST schema (messages.ts route) gains two refines: scheduledAt
requires localId (mirrors the addMessage invariant) and scheduledAt is
capped at 7 days from now to prevent zombie rows. The
scheduled+attachments rejection is also enforced here as a fast 400
before the service-layer throw is reached.
- Session-end handler comment updated to describe the new contract.
Test additions cover: future emit suppression, mature emit + no
markInvoked, hub cold-restart re-emit, cancel x mature race, REST schema
boundary cases (localId / 7-day cap / attachments combo), service-layer
throw on attachments + scheduledAt, and three R4 regression tests
including the emitted-but-not-acked-then-session-end leg.
…very
Scheduled messages keep the seq assigned at insertion time, so a row
scheduled for T+1h (seq=10) can be released after a later immediate
message (seq=11) has already advanced the runner's lastSeenMessageSeq
cursor. A pure seq <= cursor filter drops the mature emit and the row
stays uninvoked forever (re-emitted every 5 s, dropped every 5 s).
Replace the per-instance cursor with an exported IncomingMessageFilter:
- Dedup by message id when present (authoritative identity), with a
bounded LRU (default 256). On dedup hit the entry is delete + re-add
to refresh recency, so a re-emitted scheduled id survives bursts of
unrelated immediate ids without being evicted.
- Falls back to the legacy seq-only cursor for messages that arrive
without an id (preserves backward-compatible behaviour exactly).
- Cursor advancement remains monotonic via Math.max(prev, seq), so
reconnect backfill (afterSeq) keeps working.
handleIncomingMessage delegates to incomingFilter.accept({id, seq}); the
backfill path reads incomingFilter.cursorSeq().
Tests cover: mature seq < cursor accepted on new id, exact id duplicate
rejected, seq-only fallback for missing id, monotonic cursor regardless
of arrival order, LRU eviction at capacity boundary, and recency refresh
on dedup hit so re-emits survive a capacity-sized burst of unrelated ids.
Adds a clock button next to send in the composer that opens a small
floating picker with relative presets (+5m / +15m / +1h / ...) and a
specific datetime tab. Selecting a preset highlights the clock button
in active blue; selecting a specific time also fills the datetime input
inline. Submit posts the message with scheduledAt, the existing queued
bar surfaces the row, and Edit/Cancel work the same way as today's
queued messages.
- ScheduleTimePicker: PendingSchedule discriminated union (preset vs
absolute), resolved at send time so a preset always anchors to the
moment of send rather than the moment of pick. Picker positioning
has a fitsAbove fallback so it never clips the viewport.
- ComposerButtons: clock toggle keyed off the active PendingSchedule.
- HappyComposer + useSendMessage: pass scheduledAt through to the REST
layer; an optimistic localId is always present so the ack path stays
sound (matches the hub-side invariant).
- QueuedMessagesBar:
* sortQueuedMessages — pure exported function — orders the bar
"immediate-queued by createdAt asc, then scheduled by scheduledAt
asc" so the firing order is visually obvious.
* formatScheduledTime — exported, cross-year aware (adds the year
when scheduled_at and now are in different years).
* Edit on a row whose status flipped to invoked between the open and
save shows a useToast feedback rather than silently dropping the
edit.
- SessionChat: shouldAutoClearPendingSchedule pure helper distinguishes
preset-stays-alive from absolute-auto-clears-on-expiry, plus tests.
- en / zh-CN locales gain the picker / bar strings, including the
Specific tab hint that calls out the 7-day cap and the fact that the
CLI catches up the next time it connects after the scheduled time.
353 web tests / 0 fail; assistant-ui composer wiring uses the existing
runtime hooks so no new dependencies are introduced.
b32edca to
c235f97
Compare
There was a problem hiding this comment.
Findings
No findings.
Summary
Review mode: follow-up after new commits
No high-confidence issues found in the latest full diff. The prior scheduled-message session-end drop finding appears addressed by limiting the session-end sweep to immediate queued rows (scheduled_at IS NULL). Residual risk: scheduled delivery still depends on the hub mature-message tick and CLI ack path; covered by added tests in the diff, but not re-run here.
Testing
Not run (automation)
HAPI Bot
What
Lets the user defer a Web composer message until a chosen future time. Hub queues the row, fires it at the scheduled instant, and the existing CLI message-consumed ack is reused — no new dispatch path. UX is a clock button next to send; no modal, no new dependencies.
Why
Today every Web message is consumed by the CLI immediately. Users have asked for deferred send to cover four flows that currently have no workaround:
All four collapse into one mechanism: a
scheduled_atcolumn on the messages table plus one extra line on the existing 5sexpireInactivetick. No new timer, no new socket event, no new SSE type.How
Three commits, dependency-ordered (schema → hub logic → web UI). Each is independently buildable and tested; commit messages have the full per-layer detail.
The non-obvious decisions worth flagging up front:
releaseMatureScheduledMessagesis called from the existing 5sexpireInactivetick.releaseMatureScheduledMessagesdoes not writeinvoked_at. The CLI'smessages-consumedack stays the sole writer, so a hub crash or a disconnected CLI just re-emits on the next tick — the CLI catches up the next time it connects.scheduledAtPOST without alocalIdwould silently lose the schedule (addMessagewould stampinvoked_atimmediately and the mature scan would skip it). Both the REST schema andaddMessagereject that combination.Migration / backward compatibility
PRAGMA user_version = 8manually, or restore from backup.scheduled_atdefaults toNULL, the future-emit branch is skipped (so they go through the immediate path unchanged), and the partial index does not list them (cardinality stays at the count of pending schedules only).Test plan
ackCalledassertion on cancel short-circuit), 335 web tests (48 files, +2 forformatScheduledTimecross-year, +5 forshouldAutoClearPendingSchedulepreset-stays-alive guard),bun typecheckclean across hub/web.migrateFromV8ToV9PRAGMA guard.messageServicesuite: future scheduledAt skips CLI emit, mature scan picks the row up, the cancel × mature-emit race is documented (the existing PR feat(web,hub): cancel queued messages #568 contract is preserved —not-foundack stampsinvoked_at), hub cold-restart re-emits becausereleaseMatureScheduledMessagesdoes not writeinvoked_at, REST schema rejectsscheduledAtwithoutlocalIdandscheduledAt > +7d.HAPI_HOME):+30sschedules across Claude Code, Gemini, and OpenCode (nemotron-3-super-free) all matured withinsched + 1.5–2.5sand stampedinvoked_atexactly once. Gemini happened to be quota-exhausted at the time and replied "Gemini prompt failed: ... quota will reset after 8h34m27s" — which is the exact case this PR is built to solve, and the hub still tracked the schedule cleanly through the downstream rejection.Notes for the reviewer
invokedand stampsinvoked_at— the message is processed, not cancelled. This is a wider window than the immediate-queued case (~5s vs sub-second) but the contract itself is unchanged. A test documents the expected behavior.'new-message'payload-id dedup is not in this PR — it would expand the surface to the CLI; a follow-up PR is the cleaner home.releaseMatureScheduledMessagesis piggybacked onexpireInactiveto avoid a second timer; a comment notes that they share cadence but are not logically the same operation.