fix: notification loop assumes ordered events, bounded prompts, and managed session lifecycle — none hold in production

[ruan330]

Problem

The notification loop in stream_prompt (discord.rs) is built on three assumptions that don't hold in production with long-running ACP backends like Claude Code:

Assumption 1: ACP events arrive in order

The loop breaks when it sees a response with id:

while let Some(notification) = rx.recv().await {
    if notification.id.is_some() {
        break;  // assumes all text chunks arrived before this
    }
}

Reality: end_turn (the response with id) sometimes arrives before the final agent_message_chunk notifications. When this happens, the loop breaks early and text_buf is empty — the user sees "(no response)" even though the agent did respond.

Evidence from production logs:

06:31:39  end_turn id=40, totalTokens=0     ← response arrives
06:31:56  agent_message_chunk: "先"          ← text arrives AFTER response
06:31:57  agent_message_chunk: "更新實作..."  ← more text, already discarded

Assumption 2: Prompts always complete

The loop has no exit condition other than receiving the response id or the channel closing:

while let Some(notification) = rx.recv().await {
    // no timeout, no liveness check
}

Reality: Agents run long tool calls (build commands, test suites) that produce no ACP notifications for minutes. If the tool call never completes (e.g., flutter run is a long-lived app server), the loop blocks forever. Combined with the global pool write lock (#58), this freezes the entire broker.

Evidence: a single flutter run command blocked all sessions for 7+ hours until manual restart.

Assumption 3: Session lifecycle is self-managing

There is no cleanup of Discord threads when the broker restarts. After a restart, the in-memory session pool is empty, but Discord threads from previous sessions remain active. Users typing in these stale threads create new sessions with no conversation context, silently consuming session pool slots.

With max_sessions = 5, three stale threads + two new threads = pool exhausted.

These are one problem, not three

All three stem from the same architectural gap: the notification loop has no resilience against real-world conditions — event ordering violations, unbounded blocking, and lifecycle mismatches.

Fixing them individually produces three localized patches that don't address the underlying fragility. Fixing them together produces a robust notification loop that handles production realities.

Reference implementation

We encountered all three issues in our production deployment (multi-session, Claude Code backend, long-running Flutter builds) and submitted a PR with our fixes:

PR: #77 — includes per-connection locking (#58), alive check, drain window, fallback, and startup cleanup in a single cohesive change.

The key changes:

1. Drain window for event ordering (discord.rs)

After receiving the end_turn response, drain the notification channel for 200ms to capture late-arriving text chunks:

if notification.id.is_some() {
    let drain_until = Instant::now() + Duration::from_millis(200);
    while let Ok(remaining) = timeout_at(drain_until, rx.recv()).await {
        if let Some(n) = remaining {
            if let Some(AcpEvent::Text(t)) = classify_notification(&n) {
                text_buf.push_str(&t);
            }
        } else { break; }
    }
    break;
}

2. Alive check + hard timeout for unbounded blocking (discord.rs)

Replace the bare while let rx.recv() with a tokio::select! that periodically checks process liveness and enforces a hard ceiling:

let prompt_start = Instant::now();
let hard_timeout = Duration::from_secs(30 * 60);
loop {
    tokio::select! {
        msg = rx.recv() => { /* process notification */ },
        _ = sleep(Duration::from_secs(30)) => {
            if !conn.alive() { break; }           // process dead → break
            if prompt_start.elapsed() > hard_timeout { break; }  // safety net
            continue;                               // alive → keep waiting
        }
    }
}

3. Startup thread cleanup (discord.rs, ready handler)

On startup, fetch active threads in allowed channels and archive any created by this bot:

async fn ready(&self, ctx: Context, ready: Ready) {
    for thread in active_threads {
        if thread.owner_id == bot_id && thread.parent_id in allowed_channels {
            thread.edit(archived: true);
        }
    }
}

4. Fallback for empty responses (discord.rs)

If text_buf is empty after draining but tool activity was recorded, compose a fallback from tool lines instead of showing "(no response)":

let final_content = if text_buf.trim().is_empty() && !tool_lines.is_empty() {
    format!("{}\n\n_Task completed but no text response was captured._",
            tool_lines.join("\n"))
} else if text_buf.trim().is_empty() {
    "_(no response)_".to_string()
} else {
    compose_display(&tool_lines, &text_buf)
};

Tradeoffs

Decision	Cost	Why we chose it
200ms drain window	Adds 200ms latency to every prompt completion	Small cost; avoids losing entire responses
30-min hard timeout	Legitimate 30+ min tasks get interrupted	Safety net; 30 min is generous for most use cases
Auto-archive on startup	Can't resume old threads after restart	Old threads have no session context anyway; clean start is safer
Fallback message	Shows tool summary instead of actual response	Better than "(no response)"; user knows work happened

These are our solutions based on our specific use case (multi-session, Claude Code backend, Flutter development). The maintainer may have better approaches — for example, the drain window could be replaced by sequence numbers in the ACP protocol, or the hard timeout could be configurable via config.toml. We're sharing what worked for us as a starting point, not prescribing the solution.

Related

• fix: robust notification loop — per-connection locking, alive check, drain window, and startup cleanup #77 — PR with the complete fix (supersedes fix: per-connection locking to prevent cross-session deadlock during long prompts #59)
• fix: pool write lock held during entire prompt streaming causes cross-session deadlock #58 — pool write lock deadlock (addressed in fix: robust notification loop — per-connection locking, alive check, drain window, and startup cleanup #77)
• fix: use toolCallId for tool status tracking, prevent duplicate messages on long responses #53 — tool status display fix (separate PR, toolCallId matching)

[ruan330]

Additional context: these issues are primarily observed with the claude-agent-acp backend (Claude Code). The root causes may not affect Kiro CLI or Codex backends:

Issue	Claude Code specific?	Why
Event ordering (drain)	Likely yes	claude-agent-acp sends end_turn before final agent_message_chunk — this may be specific to how it bridges Claude Code's output to ACP JSON-RPC
Unbounded blocking	Mostly yes	Claude Code runs long tool calls (build commands, test suites, Agent subagents that spawn child processes). Kiro's tool calls tend to complete quickly
Stale thread cleanup	All backends	This is a Discord lifecycle issue, not backend-specific
Empty response fallback	Likely yes	Consequence of the event ordering issue above

We've only tested with claude-agent-acp@0.25.0 + claude-code@2.1.87. If others using Kiro or Codex backends have not seen these symptoms, it would confirm the ordering issue is specific to the Claude Code ACP adapter.

[chaodu-agent]

Issue at a Glance

Discord User ──► openab ──► Agent CLI (ACP)
                   │
                   ▼
            stream_prompt notification loop
                   │
         ┌─────────┼──────────┐
         ▼         ▼          ▼
    end_turn    text chunk   text chunk
    arrives     arrives      arrives
    FIRST ❌    AFTER        AFTER
         │
         ▼
    loop breaks early
    text_buf empty → "(no response)"

    Also: no timeout → long tool call blocks forever
    Also: no startup cleanup → stale threads exhaust pool

Hi @ruan330,

Confirmed — all three assumptions are visible in the source. The bare while let rx.recv() loop has no drain window, no alive check, and no startup cleanup.

PR #77 is already open with the fix. Note it also supersedes PR #59 (#58). We'll review #77 as the combined fix for both #58 and #76.