diff --git a/docs/discord-bridge.md b/docs/discord-bridge.md index 582d7b73..70beb5f8 100644 --- a/docs/discord-bridge.md +++ b/docs/discord-bridge.md @@ -122,7 +122,7 @@ The text explains `codeman notify "message" --urgent`. `codeman` is on the PATH ## Rotating and revoking - **Rotate:** in the Developer Portal choose **Bot**, **Reset Token**, paste the new token into Codeman and press **Send test**. -- **Revoke:** kick the bot from the server or delete the application, then delete `~/.codeman/discord.json`. +- **Revoke:** kick the bot from the server or delete the application, then choose **Remove token** in Settings. Deleting `~/.codeman/discord.json` alone does not reach a running bridge, which keeps the token it loaded until Codeman restarts. - If the token is ever pasted somewhere it should not be, reset it straight away. ## Troubleshooting diff --git a/src/discord-bridge.ts b/src/discord-bridge.ts index 85f3a442..c20012f3 100644 --- a/src/discord-bridge.ts +++ b/src/discord-bridge.ts @@ -22,6 +22,7 @@ import { existsSync, mkdirSync } from 'node:fs'; import fs from 'node:fs/promises'; +import { randomUUID } from 'node:crypto'; import { join } from 'node:path'; import { probePathKind } from './utils/bounded-path-probe.js'; import WebSocket from 'ws'; @@ -294,7 +295,8 @@ export async function readDiscordConfig(configDir: string): Promise { - const tmp = `${target}.${process.pid}.tmp`; + // A unique name: two writes at once (two admin tabs) must not share one tmp file. + const tmp = `${target}.${randomUUID()}.tmp`; try { await fs.writeFile(tmp, JSON.stringify(data, null, 2), { mode }); await fs.rename(tmp, target); @@ -679,6 +681,8 @@ export class DiscordBridge { /** Brand-new sessions that have not yet been sent the startup hint (non-Claude CLIs). */ private fresh = new Set(); private pending = new Map>(); + /** Tabs closed while their thread was still being created: the creation applies `onSessionDelete` to it. */ + private closedWhilePending = new Set(); /** The parent channel the thread map was built under. */ private threadsParent = ''; /** True from boot until the restored tabs exist (see hold). */ @@ -822,6 +826,10 @@ export class DiscordBridge { if (!t?.id) return this.fail(new Error('Discord did not return a thread')); // A save moved the bridge to another channel meanwhile: this thread belongs to the old one. if (parent !== this.cfg.parentChannelId) return this.fail(new Error('The parent channel changed')); + if (this.closedWhilePending.has(sessionId)) { + await this.closeThread(t.id); + return this.fail(new Error('The tab was closed')); + } this.threads[sessionId] = t.id; await this.persistThreads(); return t.id; @@ -829,7 +837,10 @@ export class DiscordBridge { return this.fail(err); } finally { // Only this creation's own entry: a newer one for the same tab may have replaced it. - if (this.pending.get(sessionId) === mine.p) this.pending.delete(sessionId); + if (this.pending.get(sessionId) === mine.p) { + this.pending.delete(sessionId); + this.closedWhilePending.delete(sessionId); + } } })()); this.pending.set(sessionId, p); @@ -940,16 +951,31 @@ export class DiscordBridge { await this.api.rename(thread, threadNameFor(name, sessionId)).catch(() => undefined); } - /** Session tab deleted: apply `onSessionDelete` and forget the mapping. */ - async sessionDeleted(sessionId: string): Promise { - // Per-tab state goes whether or not the tab ever had a thread, so these maps cannot grow for the life of the server. + /** + * Drop the per-tab state only, keeping the thread and its map entry. For a session that is discarded + * but can come back under the same id (a failed reboot-restore rebuild stays on offer for a retry). + * Per-tab state goes whether or not the tab ever had a thread, so these maps cannot grow for the life of the server. + */ + forgetTab(sessionId: string): void { this.lastReply.delete(sessionId); this.names.delete(sessionId); this.fresh.delete(sessionId); + } + + /** Session tab deleted: apply `onSessionDelete` and forget the mapping. */ + async sessionDeleted(sessionId: string): Promise { + this.forgetTab(sessionId); + // A thread still being created is closed by its creation, which finds no entry to remove here. + if (this.pending.has(sessionId)) this.closedWhilePending.add(sessionId); const thread = this.threads[sessionId]; if (!thread) return; delete this.threads[sessionId]; await this.persistThreads(); + await this.closeThread(thread); + } + + /** Apply `onSessionDelete` to a thread. */ + private async closeThread(thread: string): Promise { if (!this.cfg.enabled) return; if (this.cfg.onSessionDelete === 'delete') await this.api.remove(thread).catch(() => undefined); else if (this.cfg.onSessionDelete === 'archive') await this.api.archive(thread).catch(() => undefined); diff --git a/src/web/server.ts b/src/web/server.ts index 734c8597..a702cc35 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -3261,7 +3261,14 @@ export class WebServer extends EventEmitter { await this.finalizeRestoredState(restored); void this.discordBridge .release() - .then(() => this.discordBridge.prune(this.sessions.keys())) + .then(() => + // Same keep set as the agent inbox prune: a reboot-restore rebuild reuses the old id. + this.discordBridge.prune([ + ...this.sessions.keys(), + ...Object.keys(this.store.getSessions()), + ...rebootRestoreRegistry.list(() => true).map((entry) => entry.sessionId), + ]) + ) .catch(() => undefined); // Instance-scoped reaper: after restore, `docker rm -f` managed containers of @@ -3847,8 +3854,9 @@ export class WebServer extends EventEmitter { } catch (err) { console.warn(`[Server] releasing the tab layout slot failed: ${getErrorMessage(err)}`); } - // A half-built session never had a thread, but its per-tab bridge state goes with it. - void this.discordBridge.sessionDeleted(sessionId).catch(() => undefined); + // Per-tab bridge state only: a reboot-restore rebuild reuses the old id and stays on offer for a + // retry, so its thread must survive the discard. + this.discordBridge.forgetTab(sessionId); // Any `session:updated` the half-built session emitted before it failed left a // tab on every other open board, and the client's handler is an upsert. this.broadcast(SseEvent.SessionDeleted, { id: sessionId }); diff --git a/test/agent-inbox-server-wiring.test.ts b/test/agent-inbox-server-wiring.test.ts index 41963b73..1b8e5a84 100644 --- a/test/agent-inbox-server-wiring.test.ts +++ b/test/agent-inbox-server-wiring.test.ts @@ -50,7 +50,7 @@ function bareServer(live: string[] = [], persisted: string[] = []): CleanupServe persistDeb: { cancelKey: () => {} }, sse: { cleanupSessionBatches: () => {} }, // A real delete also tells the Discord bridge, which frees its per-tab state and may archive the thread. - discordBridge: { sessionDeleted: async () => {} }, + discordBridge: { sessionDeleted: async () => {}, forgetTab: () => {} }, broadcast: () => {}, }); return server; diff --git a/test/discord-bridge.test.ts b/test/discord-bridge.test.ts index 0ec6e008..3dc483b2 100644 --- a/test/discord-bridge.test.ts +++ b/test/discord-bridge.test.ts @@ -663,6 +663,41 @@ describe('DiscordBridge', () => { b.stop(); }); + it('forgetTab drops the per-tab state but keeps the thread and its map entry', async () => { + await writeDiscordConfig(dir, { ...CFG, onSessionDelete: 'delete' }); + const b = bridge(); + await b.notify(MSG); + posts.length = 0; + b.forgetTab('s1'); + await b.sessionRenamed('s1', 'other'); // first sight again: recorded, no PATCH + expect(posts).toHaveLength(0); + expect(await readThreadMap(dir)).toEqual({ s1: THREAD }); + b.stop(); + }); + + it('a tab closed while its thread is being created has that thread closed and no entry kept', async () => { + await writeDiscordConfig(dir, { ...CFG, onSessionDelete: 'archive' }); + let release!: () => void; + const gate = new Promise((r) => (release = r)); + const b = bridge({}, async (url, init) => { + const path = new URL(String(url)).pathname.replace('/api/v10', ''); + posts.push({ path, method: String(init.method), body: init.body ? JSON.parse(String(init.body)) : undefined }); + if (path.endsWith('/threads')) { + await gate; + return jsonRes({ id: THREAD }); + } + return jsonRes({ id: 'msg' }); + }); + const first = b.notify(MSG); + await vi.waitFor(() => expect(posts.some((p) => p.path.endsWith('/threads'))).toBe(true)); + await b.sessionDeleted('s1'); + release(); + await first; + expect(posts).toContainEqual({ path: `/channels/${THREAD}`, method: 'PATCH', body: { archived: true } }); + expect(await readThreadMap(dir)).toEqual({}); + b.stop(); + }); + it('renames the thread only when the name really changed', async () => { const b = bridge(); await b.notify(MSG); diff --git a/test/discord-push-glue.test.ts b/test/discord-push-glue.test.ts index e062e4d9..ac1e210a 100644 --- a/test/discord-push-glue.test.ts +++ b/test/discord-push-glue.test.ts @@ -25,6 +25,7 @@ type Internals = { sendPushNotifications: (event: string, data: Record) => Promise; broadcast: (event: string, data: unknown) => void; cleanupSession: (id: string, killMux?: boolean, reason?: string) => Promise; + discardPartiallyBuiltSession: (id: string) => Promise; }; function server(): Internals { @@ -35,6 +36,7 @@ function server(): Internals { sessionCreated: vi.fn(async () => undefined), sessionRenamed: vi.fn(async () => undefined), sessionDeleted: vi.fn(async () => undefined), + forgetTab: vi.fn(), sessionIdle: vi.fn(async () => undefined), stop: vi.fn(), }; @@ -125,6 +127,19 @@ describe('broadcast -> Discord tab lifecycle', () => { expect(s.discordBridge.sessionDeleted).toHaveBeenCalledWith('gone'); }); + it('a discarded half-built session clears the per-tab state but keeps its thread', async () => { + const s = server(); + s.sessions.set('rebuild', { + owner: 'u', + ralphTracker: { stopWatchingFixPlan: () => undefined, fullReset: () => undefined }, + removeAllListeners: () => undefined, + stop: async () => undefined, + }); + await s.discardPartiallyBuiltSession('rebuild'); + expect(s.discordBridge.forgetTab).toHaveBeenCalledWith('rebuild'); + expect(s.discordBridge.sessionDeleted).not.toHaveBeenCalled(); + }); + it('ignores an event with no session id', () => { const s = server(); s.broadcast(SseEvent.SessionUpdated, {});