From 266b5a8adfb64e493ff2c4051b47416f65530b79 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 26 Aug 2026 15:46:09 +1000 Subject: [PATCH 1/3] fix: bot stuck in "typing" with no request in flight Discord's typing state expires after ~10s, so a bot that appears to type forever is not showing a stale indicator -- something is actively re-sending it every 8 seconds. That something is an orphaned setInterval in DiscordConnector.startTyping. Two ways one got orphaned. Both are reachable because callers treat typing as fire-and-forget: agent/loop.ts calls `startTyping(...).catch(() => {})`, and processBatch never awaits its activationPromise, so two activations on the same channel can overlap. 1. DOUBLE START. startTyping overwrote typingIntervals[channelId] without clearing what was already there. The first interval was evicted from the map while still running, so stopTyping -- which looks the channel up in that map -- could never reach it again. 2. STOP-BEFORE-START RACE. startTyping awaited channels.fetch() and sendTyping() BEFORE registering its interval. A fast activation reached stopTyping while the map was still empty, cleared nothing, and then the interval registered afterwards with nobody left to stop it. This is the one that produces the reported symptom exactly: typing forever, nothing running. FIX: a per-channel generation counter, claimed synchronously before any await. Every start and every stop bumps it, so a startTyping that was overtaken -- whether by a newer start or by a stop -- notices at each await boundary and declines to register. startTyping also clears any existing interval before installing its own, so nothing is ever dropped while running. destroy() clears both maps. Note the failure was invisible from the map's side: an orphaned interval is precisely the one NOT in typingIntervals. A first version of the leak test asserted `typingIntervals.size === 0` and passed against the bug for that reason; it now asserts on the timer count instead, measured as a delta because the connector opens two timers of its own at construction. Tests: src/discord/connector.typing.test.ts, five cases including a control that fails if the fix simply stopped typing altogether. Mutation-checked -- neutering the generation guard fails exactly three of them, restoring it passes all five. Full suite 477 passed. tsc unchanged (3 pre-existing errors in src/llm/membrane/adapter.ts, present at HEAD, untouched here). --- src/discord/connector.ts | 51 ++++++++++ src/discord/connector.typing.test.ts | 140 +++++++++++++++++++++++++++ 2 files changed, 191 insertions(+) create mode 100644 src/discord/connector.typing.test.ts diff --git a/src/discord/connector.ts b/src/discord/connector.ts index b33b4f4..25a643f 100644 --- a/src/discord/connector.ts +++ b/src/discord/connector.ts @@ -95,6 +95,9 @@ export interface SentMessageChunk { export class DiscordConnector { private client: Client private typingIntervals = new Map() + // Per-channel token guarding the async gap in startTyping. Bumped by every + // start and every stop, so a start that was overtaken can tell. + private typingGenerations = new Map() private imageCache = new Map() private urlToFilename = new Map() // URL -> filename for disk cache lookup private urlMapPath: string // Path to URL map file @@ -1456,17 +1459,53 @@ export class DiscordConnector { /** * Start typing indicator (refreshes every 8 seconds) + * + * Discord's typing state expires after ~10s, so the indicator only persists + * because of the refresh interval below. That makes an orphaned interval + * indistinguishable, to a user, from a request that never finishes -- the bot + * appears to type forever with nothing in flight. + * + * Two things could orphan one, and both are reachable because callers treat + * this as fire-and-forget (agent/loop.ts: `startTyping(...).catch(() => {})`, + * and processBatch never awaits its activationPromise, so two activations on + * one channel overlap): + * - a second start overwrote the map entry, dropping a still-running + * interval that stopTyping could then never reach; + * - stopTyping landing DURING the awaits below cleared an empty map, and + * the interval registered afterwards with nobody left to stop it. + * + * The generation counter fixes both: it is claimed synchronously before any + * await, and both a newer start and any stop invalidate it. */ async startTyping(channelId: string): Promise { + // Claim the channel BEFORE awaiting anything. + const generation = (this.typingGenerations.get(channelId) ?? 0) + 1 + this.typingGenerations.set(channelId, generation) + + // Never leave a previous interval running unreferenced. + this.clearTypingInterval(channelId) + const channel = await this.client.channels.fetch(channelId) as TextChannel if (!channel || !channel.isTextBased()) { return } + // Superseded or stopped while we were awaiting Discord. + if (this.typingGenerations.get(channelId) !== generation) { + return + } + // Send initial typing await channel.sendTyping() + // Checked again: sendTyping is a network round-trip and a fast activation + // can complete inside it. One stray indicator expires on its own in ~10s; + // a stray INTERVAL would not. + if (this.typingGenerations.get(channelId) !== generation) { + return + } + // Set up interval to refresh const interval = setInterval(async () => { try { @@ -1483,6 +1522,13 @@ export class DiscordConnector { * Stop typing indicator */ async stopTyping(channelId: string): Promise { + // Bump first: this is what tells a startTyping still awaiting Discord that + // it has been cancelled, so it declines to register its interval. + this.typingGenerations.set(channelId, (this.typingGenerations.get(channelId) ?? 0) + 1) + this.clearTypingInterval(channelId) + } + + private clearTypingInterval(channelId: string): void { const interval = this.typingIntervals.get(channelId) if (interval) { clearInterval(interval) @@ -1765,6 +1811,11 @@ export class DiscordConnector { for (const interval of this.typingIntervals.values()) { clearInterval(interval) } + this.typingIntervals.clear() + // Bumping generations is not enough on shutdown -- drop them, so a + // startTyping still awaiting Discord cannot register an interval into a + // connector that is going away. + this.typingGenerations.clear() // Clear cache maintenance intervals if (this.cacheStatsInterval) clearInterval(this.cacheStatsInterval) if (this.evictionInterval) clearInterval(this.evictionInterval) diff --git a/src/discord/connector.typing.test.ts b/src/discord/connector.typing.test.ts new file mode 100644 index 0000000..97cd981 --- /dev/null +++ b/src/discord/connector.typing.test.ts @@ -0,0 +1,140 @@ +/** + * Tests for the typing indicator's lifecycle. + * + * THE BUG THESE PIN: a bot sometimes sits in Discord's "typing" state with no + * request in flight. Discord's indicator expires after ~10s, so a STUCK + * indicator is not a stale UI state -- something is actively re-sending it. + * That something is an orphaned setInterval inside DiscordConnector. + * + * Two ways one gets orphaned, both live because activations are fire-and-forget + * (agent/loop.ts calls `startTyping(...).catch(() => {})`, and processBatch + * never awaits its activationPromise, so two activations on one channel can + * overlap): + * + * 1. DOUBLE START. startTyping overwrote typingIntervals[channelId] without + * clearing what was already there, so the first interval was dropped from + * the map while still firing. stopTyping could then only ever clear the + * most recent one. + * + * 2. STOP-BEFORE-START RACE. startTyping awaits channels.fetch() and + * sendTyping() BEFORE registering its interval. A fast activation calls + * stopTyping while the map is still empty -- clearing nothing -- and the + * interval registers afterwards with nobody left to stop it. This is the + * one that produces "typing forever, nothing in flight". + * + * 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) + + ;(connector as any).client = { channels: { fetch } } + + return { + connector, + sendTyping, + release: () => release?.(), + intervals: () => (connector as any).typingIntervals as Map, + cleanup: () => rmSync(tmpCache, { recursive: true, force: true }), + } +} + +describe('typing indicator lifecycle', () => { + let h: ReturnType + 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. Before the fix the first + // interval was dropped from the map while still running, so it kept + // sending typing forever and no stopTyping could ever reach it. + 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. startTyping is + // fire-and-forget, so a fast activation finishes and calls stopTyping while + // startTyping is still awaiting Discord. The stop must not be silently lost. + 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 () => { + // NOT the Map -- the Map is exactly where an orphan ISN'T. Asserting + // typingIntervals.size === 0 passed against the buggy code, because the + // orphaned interval had already been evicted from the map while still + // running. Ask the timer system instead; it can see what the map cannot. + h = makeHarness() + // The connector opens 2 timers of its own at construction, so the absolute + // count is not the signal -- the DELTA is. Asserting 0 here failed against + // correct code, which is its own small lesson about checks. + 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) + }) +}) From f291aadbdecd172a3efa45d69d32793c4166ed01 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 26 Aug 2026 16:27:20 +1000 Subject: [PATCH 2/3] refactor: make the typing races impossible by construction, not guarded greptile's review on PR #17 found a real bug in my first fix, and following it down showed the fix was the wrong SHAPE. THE REPORTED BUG, confirmed by test before fixing: the per-channel generation counter was derived from its own stored value, so close() clearing the map reset its numeric identity. A stopTyping arriving afterwards recomputed (undefined ?? 0) + 1 === 1 -- exactly the generation a pending start still held. That start's guard passed and it installed a refresh interval on a destroyed connector. Classic ABA: the token was not unique, only locally fresh. My own comment had claimed clearing was SAFER than bumping; it was precisely backwards. THE REAL FIX. A generation counter makes the race survivable. It does not remove it. The race existed only because startTyping awaited channels.fetch() and sendTyping() BEFORE registering its interval, and callers are fire-and-forget. So: delete the gap rather than guard it. Nothing is awaited before the map write, the async work moved inside the tick, and the timer handle itself is the identity token -- a Timeout is created by the runtime and cannot be forged, re-derived, or re-minted the way a number can. The invariant is now checkable in one line: A TYPING TIMER EXISTS IF AND ONLY IF IT IS IN typingIntervals. Both start and stop mutate that map synchronously, so there is no window for a stop to be missed or a start to register behind one. The generation counter, the monotonic sequence and their whole surface are gone. Mutation-tested, and it found something: making registration async again fails a test, removing the replace-before-register fails a test, but dropping the post-await identity re-check survived everything. It was untested, not dead -- the other tests watch TIMERS and that one guards a stray SEND. Now pinned, with an honest comment: sending is asynchronous, so a stop can always land mid-send and you cannot un-send. The check narrows that window rather than closing it, and the residual is one indicator Discord expires by itself in ~10s. 7 typing tests, full suite 479 passed, tsc unchanged (3 pre-existing errors in src/llm/membrane/adapter.ts). The connector's typing surface is 83 lines including all of the reasoning above. --- src/discord/connector.ts | 105 ++++++++++++++------------- src/discord/connector.typing.test.ts | 44 ++++++++++- 2 files changed, 98 insertions(+), 51 deletions(-) diff --git a/src/discord/connector.ts b/src/discord/connector.ts index 25a643f..7d86916 100644 --- a/src/discord/connector.ts +++ b/src/discord/connector.ts @@ -94,10 +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() - // Per-channel token guarding the async gap in startTyping. Bumped by every - // start and every stop, so a start that was overtaken can tell. - private typingGenerations = new Map() + private closed = false private imageCache = new Map() private urlToFilename = new Map() // URL -> filename for disk cache lookup private urlMapPath: string // Path to URL map file @@ -1461,70 +1463,73 @@ export class DiscordConnector { * Start typing indicator (refreshes every 8 seconds) * * Discord's typing state expires after ~10s, so the indicator only persists - * because of the refresh interval below. That makes an orphaned interval - * indistinguishable, to a user, from a request that never finishes -- the bot - * appears to type forever with nothing in flight. + * because of the refresh interval below -- which means an orphaned interval + * is indistinguishable, to a user, from a request that never finishes: the + * bot types forever with nothing in flight. * - * Two things could orphan one, and both are reachable because callers treat - * this as fire-and-forget (agent/loop.ts: `startTyping(...).catch(() => {})`, - * and processBatch never awaits its activationPromise, so two activations on - * one channel overlap): - * - a second start overwrote the map entry, dropping a still-running - * interval that stopTyping could then never reach; - * - stopTyping landing DURING the awaits below cleared an empty map, and - * the interval registered afterwards with nobody left to stop it. + * WHY THIS SHAPE. The earlier version awaited channels.fetch() and + * sendTyping() BEFORE registering its interval, and callers are + * fire-and-forget (agent/loop.ts: `startTyping(...).catch(() => {})`, and + * processBatch never awaits its activationPromise). That async gap admitted a + * whole family of races: a second start orphaning the first, a stop landing + * mid-gap and clearing an empty map, and -- with a per-channel counter + * guarding the gap -- a stop after close() re-minting the exact token a + * pending start still held. * - * The generation counter fixes both: it is claimed synchronously before any - * await, and both a newer start and any stop invalidate it. + * Rather than guard the gap, this removes it. Nothing is awaited before the + * map write, so registration is atomic with respect to stopTyping; the async + * work moved inside the tick. The timer handle itself is the identity token, + * which is why the family cannot come back: a Timeout is created by the + * runtime and cannot be forged, re-derived, or re-minted the way a number can. + * + * Stays `async` so the existing `.catch()` callers keep working. An async + * function runs synchronously until its first await, and there is none before + * the registration. */ async startTyping(channelId: string): Promise { - // Claim the channel BEFORE awaiting anything. - const generation = (this.typingGenerations.get(channelId) ?? 0) + 1 - this.typingGenerations.set(channelId, generation) - - // Never leave a previous interval running unreferenced. - this.clearTypingInterval(channelId) - - const channel = await this.client.channels.fetch(channelId) as TextChannel - - if (!channel || !channel.isTextBased()) { - return - } - - // Superseded or stopped while we were awaiting Discord. - if (this.typingGenerations.get(channelId) !== generation) { + if (this.closed) { return } - // Send initial typing - await channel.sendTyping() - - // Checked again: sendTyping is a network round-trip and a fast activation - // can complete inside it. One stray indicator expires on its own in ~10s; - // a stray INTERVAL would not. - if (this.typingGenerations.get(channelId) !== generation) { - return - } + // Replace, never shadow: anything already running for this channel would + // otherwise be evicted from the map while still firing. + this.clearTypingInterval(channelId) - // Set up interval to refresh - const interval = setInterval(async () => { + const tick = async (): Promise => { + // 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 fast activation can finish inside a + // network round-trip. Worst case here is one stray indicator, which + // Discord expires on its own in ~10s; a stray INTERVAL would not. + 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: there is no state a + * pending start could be holding that would let it register afterwards. */ async stopTyping(channelId: string): Promise { - // Bump first: this is what tells a startTyping still awaiting Discord that - // it has been cancelled, so it declines to register its interval. - this.typingGenerations.set(channelId, (this.typingGenerations.get(channelId) ?? 0) + 1) this.clearTypingInterval(channelId) } @@ -1812,10 +1817,10 @@ export class DiscordConnector { clearInterval(interval) } this.typingIntervals.clear() - // Bumping generations is not enough on shutdown -- drop them, so a - // startTyping still awaiting Discord cannot register an interval into a - // connector that is going away. - this.typingGenerations.clear() + // Any tick still awaiting Discord will find its handle absent from the map + // and return without sending. The flag additionally refuses a start that + // arrives 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) diff --git a/src/discord/connector.typing.test.ts b/src/discord/connector.typing.test.ts index 97cd981..e28f1ed 100644 --- a/src/discord/connector.typing.test.ts +++ b/src/discord/connector.typing.test.ts @@ -53,7 +53,9 @@ function makeHarness(opts: { fetchGate?: boolean } = {}) { ? vi.fn(() => new Promise((res) => { release = () => res(channel) })) : vi.fn().mockResolvedValue(channel) - ;(connector as any).client = { channels: { fetch } } + // 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, @@ -137,4 +139,44 @@ describe('typing indicator lifecycle', () => { 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 () => { + // Reported by greptile on PR #17, and correct. The per-channel counter was + // derived from its own stored value, so clearing the map on close() reset + // its numeric identity: a stopTyping arriving afterwards recomputed + // (undefined ?? 0) + 1 === 1 -- exactly the generation the pending start was + // still holding. The start's guard then passed and it installed a refresh + // interval on a destroyed connector, sending typing forever through a dead + // client. Classic ABA: the token was not unique, only locally fresh. + 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, which survived mutation until this + // existed -- the other tests watch TIMERS, and this is about a single stray + // SEND. Note what it is and is not: sending is asynchronous, so a stop can + // always land mid-send and you cannot un-send. The check narrows the window, + // it does not close it, and the residual is one indicator that Discord + // expires on its own in ~10s. Bounded, self-healing, and worth pinning + // precisely because nothing else would notice if it disappeared. + 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() + }) }) From b03f95ca966386ff2826eff31783c469d406f320 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 26 Aug 2026 17:24:01 +1000 Subject: [PATCH 3/3] docs: comments state the rules, not the case for the change Lauren: clean up comment text that is only relevant to someone deciding whether to merge, and not to future readers of the files. The load-bearing content turned out to be shorter as RULES than as history. A maintainer needs 'never await before the map write' and 'compare the handle, not a counter'; they do not need what the previous version did, what a reviewer found, or which PR it happened in. Removed: the version-comparison narrative, the ABA post-mortem, the greptile/PR references, and the asides about my own earlier wrong checks. Kept: the invariant, the two rules that preserve it, the fire-and-forget caller constraint that makes them necessary, and the honest note that the post-await re-check narrows a window rather than closing it. 7 typing tests, 479 full suite, tsc unchanged. --- src/discord/connector.ts | 59 +++++++++++----------- src/discord/connector.typing.test.ts | 74 +++++++++++----------------- 2 files changed, 59 insertions(+), 74 deletions(-) diff --git a/src/discord/connector.ts b/src/discord/connector.ts index 7d86916..40e16e0 100644 --- a/src/discord/connector.ts +++ b/src/discord/connector.ts @@ -1462,37 +1462,35 @@ export class DiscordConnector { /** * Start typing indicator (refreshes every 8 seconds) * - * Discord's typing state expires after ~10s, so the indicator only persists - * because of the refresh interval below -- which means an orphaned interval - * is indistinguishable, to a user, from a request that never finishes: the - * bot types forever with nothing in flight. + * 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. * - * WHY THIS SHAPE. The earlier version awaited channels.fetch() and - * sendTyping() BEFORE registering its interval, and callers are - * fire-and-forget (agent/loop.ts: `startTyping(...).catch(() => {})`, and - * processBatch never awaits its activationPromise). That async gap admitted a - * whole family of races: a second start orphaning the first, a stop landing - * mid-gap and clearing an empty map, and -- with a per-channel counter - * guarding the gap -- a stop after close() re-minting the exact token a - * pending start still held. + * 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. * - * Rather than guard the gap, this removes it. Nothing is awaited before the - * map write, so registration is atomic with respect to stopTyping; the async - * work moved inside the tick. The timer handle itself is the identity token, - * which is why the family cannot come back: a Timeout is created by the - * runtime and cannot be forged, re-derived, or re-minted the way a number can. + * 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. * - * Stays `async` so the existing `.catch()` callers keep working. An async - * function runs synchronously until its first await, and there is none before - * the registration. + * Invariant: a typing timer exists if and only if it is in typingIntervals. */ async startTyping(channelId: string): Promise { if (this.closed) { return } - // Replace, never shadow: anything already running for this channel would - // otherwise be evicted from the map while still firing. + // Replace, never shadow: an interval evicted from the map while still + // running can never be reached by stopTyping again. this.clearTypingInterval(channelId) const tick = async (): Promise => { @@ -1506,9 +1504,10 @@ export class DiscordConnector { if (!channel || !channel.isTextBased()) { return } - // Re-checked after the await: a fast activation can finish inside a - // network round-trip. Worst case here is one stray indicator, which - // Discord expires on its own in ~10s; a stray INTERVAL would not. + // 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 } @@ -1526,8 +1525,8 @@ export class DiscordConnector { /** * Stop typing indicator * - * Synchronous with respect to the map, so it always wins: there is no state a - * pending start could be holding that would let it register afterwards. + * 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 { this.clearTypingInterval(channelId) @@ -1817,9 +1816,9 @@ export class DiscordConnector { clearInterval(interval) } this.typingIntervals.clear() - // Any tick still awaiting Discord will find its handle absent from the map - // and return without sending. The flag additionally refuses a start that - // arrives after close, which would otherwise type through a dead client. + // 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) diff --git a/src/discord/connector.typing.test.ts b/src/discord/connector.typing.test.ts index e28f1ed..047dc26 100644 --- a/src/discord/connector.typing.test.ts +++ b/src/discord/connector.typing.test.ts @@ -1,26 +1,18 @@ /** * Tests for the typing indicator's lifecycle. * - * THE BUG THESE PIN: a bot sometimes sits in Discord's "typing" state with no - * request in flight. Discord's indicator expires after ~10s, so a STUCK - * indicator is not a stale UI state -- something is actively re-sending it. - * That something is an orphaned setInterval inside DiscordConnector. + * 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. * - * Two ways one gets orphaned, both live because activations are fire-and-forget - * (agent/loop.ts calls `startTyping(...).catch(() => {})`, and processBatch - * never awaits its activationPromise, so two activations on one channel can - * overlap): + * 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. * - * 1. DOUBLE START. startTyping overwrote typingIntervals[channelId] without - * clearing what was already there, so the first interval was dropped from - * the map while still firing. stopTyping could then only ever clear the - * most recent one. - * - * 2. STOP-BEFORE-START RACE. startTyping awaits channels.fetch() and - * sendTyping() BEFORE registering its interval. A fast activation calls - * stopTyping while the map is still empty -- clearing nothing -- and the - * interval registers afterwards with nobody left to stop it. This is the - * one that produces "typing forever, nothing in flight". + * 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 */ @@ -82,9 +74,9 @@ describe('typing indicator lifecycle', () => { }) it('a second startTyping does not orphan the first interval', async () => { - // Two overlapping activations on one channel. Before the fix the first - // interval was dropped from the map while still running, so it kept - // sending typing forever and no stopTyping could ever reach it. + // 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) @@ -96,9 +88,9 @@ describe('typing indicator lifecycle', () => { }) it('a stopTyping during startTyping wins, rather than being overtaken', async () => { - // THE "typing forever with nothing in flight" CASE. startTyping is - // fire-and-forget, so a fast activation finishes and calls stopTyping while - // startTyping is still awaiting Discord. The stop must not be silently lost. + // 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 @@ -112,14 +104,12 @@ describe('typing indicator lifecycle', () => { }) it('leaves no live timer once stopped', async () => { - // NOT the Map -- the Map is exactly where an orphan ISN'T. Asserting - // typingIntervals.size === 0 passed against the buggy code, because the - // orphaned interval had already been evicted from the map while still - // running. Ask the timer system instead; it can see what the map cannot. + // 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. Asserting 0 here failed against - // correct code, which is its own small lesson about checks. + // count is not the signal -- the DELTA is. const baseline = vi.getTimerCount() await h.connector.startTyping(CH) @@ -141,13 +131,10 @@ describe('typing indicator lifecycle', () => { }) it('a start still pending at shutdown never installs a timer', async () => { - // Reported by greptile on PR #17, and correct. The per-channel counter was - // derived from its own stored value, so clearing the map on close() reset - // its numeric identity: a stopTyping arriving afterwards recomputed - // (undefined ?? 0) + 1 === 1 -- exactly the generation the pending start was - // still holding. The start's guard then passed and it installed a refresh - // interval on a destroyed connector, sending typing forever through a dead - // client. Classic ABA: the token was not unique, only locally fresh. + // 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 @@ -163,13 +150,12 @@ describe('typing indicator lifecycle', () => { }) it('does not send typing after a stop that landed mid-fetch', async () => { - // Pins the post-await identity re-check, which survived mutation until this - // existed -- the other tests watch TIMERS, and this is about a single stray - // SEND. Note what it is and is not: sending is asynchronous, so a stop can - // always land mid-send and you cannot un-send. The check narrows the window, - // it does not close it, and the residual is one indicator that Discord - // expires on its own in ~10s. Bounded, self-healing, and worth pinning - // precisely because nothing else would notice if it disappeared. + // 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