Skip to content

fix(hub): mark queued sessions thinking before Codex task_start#514

Merged
tiann merged 3 commits intotiann:mainfrom
xiaobaifly7:pr/hub-send-thinking-immediate
Apr 25, 2026
Merged

fix(hub): mark queued sessions thinking before Codex task_start#514
tiann merged 3 commits intotiann:mainfrom
xiaobaifly7:pr/hub-send-thinking-immediate

Conversation

@xiaobaifly7
Copy link
Copy Markdown
Contributor

Summary

  • mark a session as thinking=true immediately after the hub accepts a user message
  • keep that queued-thinking state through short thinking=false keepalive heartbeats until a real task start arrives or a short grace window expires
  • add sync-layer regression coverage for immediate thinking, grace-window preservation, and expiry

Problem

When a remote Codex message was sent from the web app, the UI could stay on online for a while even though the message had already been accepted by the hub. The status only switched later when Codex emitted task_started or a later alive update.

In practice this produced a visible delay where the message was already sent but the session state did not reflect that work had begun yet.

Fix

The hub now sets queued sessions to thinking=true as soon as sendMessage() succeeds, and preserves that state across short false heartbeats from the CLI until either:

  • a real thinking=true update arrives
  • the grace window expires

This keeps the UI responsive without leaving sessions stuck in thinking forever.

Tests

  • bun test src/sync/aliveEvents.test.ts
  • bun test src/sync/sessionModel.test.ts

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Queued sends now refresh session liveness without any delivery confirmation — markMessageQueued() sets active=true and bumps activeAt immediately after messageService.sendMessage(), but that send path only writes to SQLite and emits to the Socket.IO room without an ack. If the CLI drops just before the 30s timeout, /sessions/:id/messages still passes requireActive, this code resets the liveness window, and the hub keeps showing a dead session as active/thinking even though no agent accepted the message. Evidence hub/src/sync/sessionCache.ts:270, hub/src/sync/sessionCache.ts:271; context hub/src/sync/messageService.ts:107, hub/src/web/routes/guards.ts:29.
    Suggested fix:
// A queued web message is not proof that the CLI is still alive.
if (!session.active) return

session.thinking = true
session.thinkingAt = nextTime
session.updatedAt = Math.max(session.updatedAt, nextTime)
// keep activeAt unchanged; only session-alive should extend liveness

Summary

  • Review mode: initial
  • 1 major issue found: queued message acceptance now extends session liveness from the hub side instead of waiting for a CLI heartbeat, so a near-timeout disconnect can be misreported as active/thinking and timeout cleanup is delayed.

Testing

  • Not run (automation environment: bun not installed)

HAPI Bot

Comment thread hub/src/sync/sessionCache.ts Outdated
const previousUpdatedAt = session.updatedAt

session.active = true
session.activeAt = Math.max(session.activeAt, nextTime)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] markMessageQueued() is resetting activeAt from the hub side even though sendMessage() only emits to the Socket.IO room and never confirms that any CLI actually received the message (hub/src/sync/messageService.ts:107). If the agent disconnected just before the 30s timeout, /sessions/:id/messages still passes requireActive, this line extends the liveness window, and the UI will keep a dead session active/thinking for another timeout period.

Suggested fix:

if (!session.active) return

session.thinking = true
session.thinkingAt = nextTime
session.updatedAt = Math.max(session.updatedAt, nextTime)

@xiaobaifly7
Copy link
Copy Markdown
Contributor Author

@tiann 可以看看我这个pr吗

@tiann
Copy link
Copy Markdown
Owner

tiann commented Apr 24, 2026 via email

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Queued-thinking expiry depends on hub/CLI clock alignment, so the 15s grace window can stretch far past its limit when the agent machine clock lags behind the hub. markMessageQueued() stores pendingThinkingUntil from hub Date.now(), but handleSessionAlive() preserves and clears that state by comparing against CLI-supplied heartbeat time. Because the CLI sends session-alive timestamps from its own clock, a skewed client can keep thinking=true pinned until its local clock catches up instead of expiring after 15 real seconds. Evidence hub/src/sync/sessionCache.ts:190, hub/src/sync/sessionCache.ts:196, hub/src/sync/sessionCache.ts:273; context cli/src/api/apiSession.ts:145, cli/src/api/apiSession.ts:490.
    Suggested fix:
const now = Date.now()
const preserveQueuedThinking = !requestedThinking && pendingThinkingUntil > now

session.thinking = requestedThinking || preserveQueuedThinking
session.thinkingAt = t
if (requestedThinking || pendingThinkingUntil <= now) {
    this.pendingThinkingUntilBySessionId.delete(session.id)
}

Summary

  • Review mode: follow-up after new commits
  • 1 major issue found: the new queued-thinking grace window mixes hub time and CLI heartbeat time, so clock skew can keep a session stuck in thinking well past the intended 15 seconds.

Testing

  • Not run (bun not installed in automation environment)
  • Missing coverage: skewed CLI heartbeat timestamps relative to hub time during queued-thinking expiry

HAPI Bot

Comment thread hub/src/sync/sessionCache.ts Outdated
const previousCollaborationMode = session.collaborationMode
const pendingThinkingUntil = this.pendingThinkingUntilBySessionId.get(session.id) ?? 0
const requestedThinking = Boolean(payload.thinking)
const preserveQueuedThinking = !requestedThinking && pendingThinkingUntil > t
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] pendingThinkingUntil is written from hub time in markMessageQueued() but read back against CLI heartbeat time here. Since the CLI emits session-alive with its own Date.now() (cli/src/api/apiSession.ts:145, cli/src/api/apiSession.ts:490), any machine clock lag will keep preserveQueuedThinking true long past the intended 15s grace window and leave the UI stuck in thinking.

Suggested fix:

const now = Date.now()
const preserveQueuedThinking = !requestedThinking && pendingThinkingUntil > now

session.thinking = requestedThinking || preserveQueuedThinking
session.thinkingAt = t
if (requestedThinking || pendingThinkingUntil <= now) {
    this.pendingThinkingUntilBySessionId.delete(session.id)
}

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings
No findings.

Summary

  • Review mode: follow-up after new commits
  • No issues found in the latest full diff. The follow-up addresses the prior clock-skew finding by expiring queued-thinking state against hub time and adds coverage for skewed heartbeats.

Testing

  • Not run (automation)

HAPI Bot

@xiaobaifly7
Copy link
Copy Markdown
Contributor Author

@tiann 搞定了,麻烦合并

@tiann tiann merged commit 3a2a02c into tiann:main Apr 25, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants