fix: bot stuck in "typing" state with no request in flight

src/discord/connector.ts

@@ -94,7 +94,12 @@ export interface SentMessageChunk {
 
 export class DiscordConnector {
   private client: Client
+  // The ONE piece of typing state. Invariant, and the whole safety argument:
+  // a typing timer exists if and only if it is in this map. Both startTyping
+  // and stopTyping mutate it SYNCHRONOUSLY, so there is no window in which a
+  // stop can be missed or a start can register behind one.
   private typingIntervals = new Map<string, NodeJS.Timeout>()
+  private closed = false
   private imageCache = new Map<string, CachedImage>()
   private urlToFilename = new Map<string, string>()  // URL -> filename for disk cache lookup
   private urlMapPath: string  // Path to URL map file
@@ -1456,33 +1461,78 @@ export class DiscordConnector {
 
   /**
    * Start typing indicator (refreshes every 8 seconds)
+   *
+   * Discord's typing state expires after ~10s, so the indicator persists only
+   * because of the refresh interval below. An orphaned interval is therefore
+   * indistinguishable, to a user, from a request that never finishes: the bot
+   * types forever with nothing in flight.
+   *
+   * TWO RULES HOLD THIS TOGETHER. Callers are fire-and-forget (agent/loop.ts
+   * does `startTyping(...).catch(() => {})`, and processBatch never awaits its
+   * activationPromise), so starts and stops for one channel genuinely overlap.
+   *
+   *   1. NEVER AWAIT BEFORE THE MAP WRITE. Registration must stay atomic with
+   *      respect to stopTyping; an await above it reopens a window in which a
+   *      stop clears an empty map and this interval registers behind it. The
+   *      method stays `async` only so `.catch()` callers keep working -- an
+   *      async function runs synchronously until its first await, and there
+   *      must be none before the registration.
+   *   2. COMPARE THE HANDLE, NOT A COUNTER. The Timeout is the identity token:
+   *      the runtime mints it and it cannot be re-derived, so "is this still
+   *      the channel's timer?" has no false positives. A numeric generation
+   *      can be re-minted and will eventually collide.
+   *
+   * Invariant: a typing timer exists if and only if it is in typingIntervals.
    */
   async startTyping(channelId: string): Promise<void> {
-    const channel = await this.client.channels.fetch(channelId) as TextChannel
-
-    if (!channel || !channel.isTextBased()) {
+    if (this.closed) {
       return
     }
 
-    // Send initial typing
-    await channel.sendTyping()
+    // Replace, never shadow: an interval evicted from the map while still
+    // running can never be reached by stopTyping again.
+    this.clearTypingInterval(channelId)
 
-    // Set up interval to refresh
-    const interval = setInterval(async () => {
+    const tick = async (): Promise<void> => {
+      // Identity check, not a value check. If this handle is no longer the
+      // channel's registered timer, we were stopped or replaced.
+      if (this.typingIntervals.get(channelId) !== interval) {
+        return
+      }
       try {
+        const channel = await this.client.channels.fetch(channelId) as TextChannel
+        if (!channel || !channel.isTextBased()) {
+          return
+        }
+        // Re-checked after the await: a stop can land inside a network
+        // round-trip. This narrows that window rather than closing it --
+        // sending is asynchronous and cannot be un-sent -- so the residual is
+        // one stray indicator, which Discord expires by itself in ~10s.
+        if (this.typingIntervals.get(channelId) !== interval) {
+          return
+        }
         await channel.sendTyping()
       } catch (error) {
         logger.warn({ error, channelId }, 'Failed to refresh typing')
       }
-    }, 8000)
+    }
 
+    const interval = setInterval(tick, 8000)
     this.typingIntervals.set(channelId, interval)
+    void tick()
   }
 
   /**
    * Stop typing indicator
+   *
+   * Synchronous with respect to the map, so it always wins: no pending start
+   * holds state that could let it register afterwards.
    */
   async stopTyping(channelId: string): Promise<void> {
+    this.clearTypingInterval(channelId)
+  }
+
+  private clearTypingInterval(channelId: string): void {
     const interval = this.typingIntervals.get(channelId)
     if (interval) {
       clearInterval(interval)
@@ -1765,6 +1815,11 @@ export class DiscordConnector {
     for (const interval of this.typingIntervals.values()) {
       clearInterval(interval)
     }
+    this.typingIntervals.clear()
+    // A tick still awaiting Discord finds its handle absent and returns without
+    // sending. The flag refuses starts arriving after close, which would
+    // otherwise type through a dead client.
+    this.closed = true
     // Clear cache maintenance intervals
     if (this.cacheStatsInterval) clearInterval(this.cacheStatsInterval)
     if (this.evictionInterval) clearInterval(this.evictionInterval)

src/discord/connector.typing.test.ts

@@ -0,0 +1,168 @@
+/**
+ * Tests for the typing indicator's lifecycle.
+ *
+ * WHAT THESE PROTECT: a bot sitting in Discord's "typing" state with no request
+ * in flight. Discord's indicator expires after ~10s, so a stuck one is never a
+ * stale UI state -- an interval is actively re-sending it, orphaned from the
+ * connector's map and unreachable by stopTyping.
+ *
+ * These races are live rather than theoretical because callers are
+ * fire-and-forget: agent/loop.ts calls `startTyping(...).catch(() => {})`, and
+ * processBatch never awaits its activationPromise, so two activations on one
+ * channel overlap freely.
+ *
+ * The invariant under test: a typing timer exists if and only if it is in
+ * `typingIntervals`, and both start and stop mutate that map synchronously.
+ *
+ * Run with: npm test -- connector.typing
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
+import { mkdtempSync, rmSync } from 'fs'
+import { tmpdir } from 'os'
+import { join } from 'path'
+import { DiscordConnector } from './connector.js'
+import { EventQueue } from '../agent/event-queue.js'
+
+const CH = '100000000000000001'
+const REFRESH_MS = 8000
+
+function makeHarness(opts: { fetchGate?: boolean } = {}) {
+  const tmpCache = mkdtempSync(join(tmpdir(), 'connector-typing-test-'))
+  const connector = new DiscordConnector(new EventQueue(), {
+    token: 'fake',
+    cacheDir: tmpCache,
+    maxBackoffMs: 1000,
+  })
+
+  const sendTyping = vi.fn().mockResolvedValue(undefined)
+  const channel = { isTextBased: () => true, sendTyping }
+
+  // When gated, channels.fetch hangs until released -- this is what lets a test
+  // land stopTyping in the middle of startTyping's awaits.
+  let release: (() => void) | undefined
+  const fetch = opts.fetchGate
+    ? vi.fn(() => new Promise((res) => { release = () => res(channel) }))
+    : vi.fn().mockResolvedValue(channel)
+
+  // destroy() is what close() calls; without it the harness throws and the
+  // test fails for a reason that has nothing to do with the code under test.
+  ;(connector as any).client = { channels: { fetch }, destroy: async () => {} }
+
+  return {
+    connector,
+    sendTyping,
+    release: () => release?.(),
+    intervals: () => (connector as any).typingIntervals as Map<string, unknown>,
+    cleanup: () => rmSync(tmpCache, { recursive: true, force: true }),
+  }
+}
+
+describe('typing indicator lifecycle', () => {
+  let h: ReturnType<typeof makeHarness>
+  beforeEach(() => { vi.useFakeTimers() })
+  afterEach(() => { h?.cleanup(); vi.useRealTimers() })
+
+  it('stops refreshing after stopTyping', async () => {
+    h = makeHarness()
+    await h.connector.startTyping(CH)
+    await h.connector.stopTyping(CH)
+
+    h.sendTyping.mockClear()
+    await vi.advanceTimersByTimeAsync(REFRESH_MS * 3)
+    expect(h.sendTyping).not.toHaveBeenCalled()
+  })
+
+  it('a second startTyping does not orphan the first interval', async () => {
+    // Two overlapping activations on one channel. An interval dropped from the
+    // map while still running keeps sending typing forever, and no stopTyping
+    // can reach it again.
+    h = makeHarness()
+    await h.connector.startTyping(CH)
+    await h.connector.startTyping(CH)
+    await h.connector.stopTyping(CH)
+
+    h.sendTyping.mockClear()
+    await vi.advanceTimersByTimeAsync(REFRESH_MS * 3)
+    expect(h.sendTyping).not.toHaveBeenCalled()
+  })
+
+  it('a stopTyping during startTyping wins, rather than being overtaken', async () => {
+    // THE "typing forever with nothing in flight" CASE. A fast activation
+    // finishes and calls stopTyping while a start is still awaiting Discord.
+    // The stop must win; it must not be silently overtaken.
+    h = makeHarness({ fetchGate: true })
+
+    const starting = h.connector.startTyping(CH)   // blocks inside channels.fetch
+    await h.connector.stopTyping(CH)               // lands first; map is empty
+    h.release()                                    // start resumes and registers
+    await starting
+
+    h.sendTyping.mockClear()
+    await vi.advanceTimersByTimeAsync(REFRESH_MS * 3)
+    expect(h.sendTyping).not.toHaveBeenCalled()
+  })
+
+  it('leaves no live timer once stopped', async () => {
+    // Ask the TIMER SYSTEM, not the map: an orphan is by definition the thing
+    // the map no longer holds, so `typingIntervals.size === 0` cannot detect
+    // one. Only the timer count can.
+    h = makeHarness()
+    // The connector opens 2 timers of its own at construction, so the absolute
+    // count is not the signal -- the DELTA is.
+    const baseline = vi.getTimerCount()
+
+    await h.connector.startTyping(CH)
+    await h.connector.startTyping(CH)
+    await h.connector.stopTyping(CH)
+
+    expect(h.intervals().size).toBe(0)
+    expect(vi.getTimerCount()).toBe(baseline)
+  })
+
+  it('still types while a request IS in flight', async () => {
+    // The control. A fix that simply never types would pass every test above.
+    h = makeHarness()
+    await h.connector.startTyping(CH)
+
+    h.sendTyping.mockClear()
+    await vi.advanceTimersByTimeAsync(REFRESH_MS * 3)
+    expect(h.sendTyping.mock.calls.length).toBeGreaterThanOrEqual(3)
+  })
+
+  it('a start still pending at shutdown never installs a timer', async () => {
+    // Shutdown is the sharpest ordering: a start is still awaiting Discord when
+    // the connector closes, and a stop arrives after the close. Nothing in that
+    // sequence may leave a timer behind -- one that survives close() sends
+    // typing through a dead client forever.
+    h = makeHarness({ fetchGate: true })
+
+    const starting = h.connector.startTyping(CH)   // generation 1, blocks in fetch
+    await h.connector.close()                      // clears intervals + generations
+    await h.connector.stopTyping(CH)               // must NOT be able to re-mint 1
+    h.release()
+    await starting
+
+    expect(h.intervals().size).toBe(0)
+    h.sendTyping.mockClear()
+    await vi.advanceTimersByTimeAsync(REFRESH_MS * 3)
+    expect(h.sendTyping).not.toHaveBeenCalled()
+  })
+
+  it('does not send typing after a stop that landed mid-fetch', async () => {
+    // Pins the post-await identity re-check. Every other test here watches
+    // TIMERS; this one watches a single stray SEND, and nothing else would
+    // notice if the re-check disappeared. Note its limit: sending is
+    // asynchronous and cannot be un-sent, so a stop can always land mid-send.
+    // The check narrows that window rather than closing it, leaving at most one
+    // indicator, which Discord expires by itself in ~10s.
+    h = makeHarness({ fetchGate: true })
+
+    await h.connector.startTyping(CH)   // registers synchronously; tick blocks in fetch
+    await h.connector.stopTyping(CH)    // stop wins the map
+    h.release()                         // fetch resolves into a tick that was cancelled
+    await vi.advanceTimersByTimeAsync(0)
+
+    expect(h.sendTyping).not.toHaveBeenCalled()
+  })
+})