diff --git a/extensions/discord/src/internal/command-deploy.test.ts b/extensions/discord/src/internal/command-deploy.test.ts index 4ba3bc4dcd3d..fe9292f89af5 100644 --- a/extensions/discord/src/internal/command-deploy.test.ts +++ b/extensions/discord/src/internal/command-deploy.test.ts @@ -1,7 +1,13 @@ // Discord tests cover command deploy plugin behavior. -import type { APIApplicationCommand } from "discord-api-types/v10"; -import { describe, expect, test } from "vitest"; -import { testing } from "./command-deploy.js"; +/* oxlint-disable typescript/unbound-method -- vitest mocks of RequestClient methods (createRest) intentionally expose vi.fn refs via `restA.get`/`.post`; not unbound class methods. */ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { ApplicationCommandType, type APIApplicationCommand } from "discord-api-types/v10"; +import { describe, expect, test, vi } from "vitest"; +import { DiscordCommandDeployer, testing } from "./command-deploy.js"; +import { BaseCommand } from "./commands.js"; +import type { RequestClient } from "./rest.js"; const { commandsEqual } = testing; @@ -196,3 +202,332 @@ describe("commandsEqual", () => { expect(commandsEqual(current, desired)).toBe(true); }); }); + +/** + * Regression for #77359: when two Discord accounts share the same on-disk + * deploy-cache file (the default in multi-bot setups) the persisted hash key + * must be scoped by application/client id. Otherwise a later account whose + * command set hashes the same as the first account's reuses the first + * account's hash and skips reconciling its own Discord application — leaving + * "This application has no commands" in the secondary bot's Integrations panel. + */ +describe("DiscordCommandDeployer cache scoping (multi-application)", () => { + class StaticCommand extends BaseCommand { + name: string; + override description = "ping the bot"; + type = ApplicationCommandType.ChatInput; + constructor(name: string) { + super(); + this.name = name; + } + serializeOptions() { + return undefined; + } + } + + function createRest(): RequestClient { + return { + get: vi.fn(async () => []), + post: vi.fn(async () => undefined), + patch: vi.fn(async () => undefined), + put: vi.fn(async () => undefined), + delete: vi.fn(async () => undefined), + } as unknown as RequestClient; + } + + test("two applications with identical command sets each reconcile their own application", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + const commands = [new StaticCommand("ping")]; + + const restA = createRest(); + const deployerA = new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => restA, + }); + await deployerA.deploy({ mode: "reconcile" }); + + const restB = createRest(); + const deployerB = new DiscordCommandDeployer({ + clientId: "app-secondary", + commands, + hashStorePath, + rest: () => restB, + }); + await deployerB.deploy({ mode: "reconcile" }); + + // The first deploy issues a list + create against application "app-default". + expect(restA.get).toHaveBeenCalledTimes(1); + expect(restA.post).toHaveBeenCalledTimes(1); + // The second deploy MUST also list + create against "app-secondary"; before + // the fix it short-circuited on the shared `global:reconcile` hash and + // never touched its own Discord application. + expect(restB.get).toHaveBeenCalledTimes(1); + expect(restB.post).toHaveBeenCalledTimes(1); + }); + + test("re-deploying the same application still hits the persisted cache", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + const commands = [new StaticCommand("ping")]; + + const restFirst = createRest(); + await new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => restFirst, + }).deploy({ mode: "reconcile" }); + + const restSecond = createRest(); + await new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => restSecond, + }).deploy({ mode: "reconcile" }); + + expect(restFirst.get).toHaveBeenCalledTimes(1); + expect(restFirst.post).toHaveBeenCalledTimes(1); + // Same application, same command set, same hash file => skip reconcile. + expect(restSecond.get).not.toHaveBeenCalled(); + expect(restSecond.post).not.toHaveBeenCalled(); + }); + + test("persisted cache keys are namespaced by application id", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + const commands = [new StaticCommand("ping")]; + + await new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => createRest(), + }).deploy({ mode: "reconcile" }); + + await new DiscordCommandDeployer({ + clientId: "app-secondary", + commands, + hashStorePath, + rest: () => createRest(), + }).deploy({ mode: "reconcile" }); + + const raw = await fs.readFile(hashStorePath, "utf8"); + const parsed = JSON.parse(raw) as { hashes: Record }; + const keys = Object.keys(parsed.hashes); + expect(keys).toContain("app:app-default:global:reconcile"); + expect(keys).toContain("app:app-secondary:global:reconcile"); + expect(keys).not.toContain("global:reconcile"); + }); + + test("successful deploy repairs a corrupt persisted cache file", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + await fs.writeFile(hashStorePath, "{not json", "utf8"); + + await new DiscordCommandDeployer({ + clientId: "app-default", + commands: [new StaticCommand("ping")], + hashStorePath, + rest: () => createRest(), + }).deploy({ mode: "reconcile" }); + + const raw = await fs.readFile(hashStorePath, "utf8"); + const parsed = JSON.parse(raw) as { hashes: Record }; + expect(parsed.hashes).toHaveProperty("app:app-default:global:reconcile"); + }); + + test("a deployer that loaded an empty cache before another deployer's write preserves the other deployer's entries on persist", async () => { + // Regression for the codex follow-up on PR #77367: `server-channels.ts` + // can start multiple Discord deployers concurrently. Before the fix, a + // deployer that loaded the (empty) cache file before another deployer's + // first write would later overwrite it on its own `persistHashes()`, + // serializing only its own in-memory `app::...` entry and dropping + // the other deployer's entry. The current implementation re-reads the + // on-disk hashes inside `persistHashes` and merges them with our + // in-memory entries before the rename. + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + const commands = [new StaticCommand("ping")]; + + // Deployer B starts first, loads the empty cache. Then deployer A + // completes its full deploy + persist, writing `app:app-default:...` to + // disk. When deployer B finally persists, it must merge in deployer A's + // entry instead of overwriting it with just its own. + const deployerB = new DiscordCommandDeployer({ + clientId: "app-secondary", + commands, + hashStorePath, + rest: () => createRest(), + }); + // Trigger B's load of the (still missing) cache file by starting deploy + // and immediately awaiting just enough to clear the load. The deploy + // call awaits loadPersistedHashes inside putCommandSetIfChanged before + // calling deploy(); to keep the seam minimal here, we just race the load + // by running deployer A's full deploy in between. + const deployerA = new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => createRest(), + }); + + // Step 1: A runs a full deploy (load -> reconcile -> persist) on the + // initially missing cache file; result: file now has app-default entry. + await deployerA.deploy({ mode: "reconcile" }); + + // Step 2: B runs its full deploy. Without the fix, B's persistHashes + // would write only `app:app-secondary:...` and drop A's entry. With the + // fix, B re-reads the on-disk file inside persistHashes, sees A's entry, + // and merges it into the write so both keys survive. + await deployerB.deploy({ mode: "reconcile" }); + + const raw = await fs.readFile(hashStorePath, "utf8"); + const parsed = JSON.parse(raw) as { hashes: Record }; + const keys = Object.keys(parsed.hashes); + expect(keys).toContain("app:app-default:global:reconcile"); + expect(keys).toContain("app:app-secondary:global:reconcile"); + + // And subsequent restarts must still hit the cache for both apps, + // proving the rate-limit protection survived the concurrent write. + const restA = createRest(); + await new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => restA, + }).deploy({ mode: "reconcile" }); + const restB = createRest(); + await new DiscordCommandDeployer({ + clientId: "app-secondary", + commands, + hashStorePath, + rest: () => restB, + }).deploy({ mode: "reconcile" }); + expect(restA.get).not.toHaveBeenCalled(); + expect(restA.post).not.toHaveBeenCalled(); + expect(restB.get).not.toHaveBeenCalled(); + expect(restB.post).not.toHaveBeenCalled(); + }); + + test("truly parallel deployers serialize cache writes via the per-path mutex (codex follow-up on #77367)", async () => { + // Codex follow-up on PR #77367: re-read-before-write alone isn't enough + // when two deployers run `persistHashes` in real parallel — both can read + // the same snapshot before either writes. The in-process per-path mutex + // around the read-merge-write cycle makes the operation atomic. + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + const commands = [new StaticCommand("ping")]; + + // Run BOTH deploys with Promise.all on the SAME process tick — pre-fix, + // both `persistHashes` calls would race on read-then-rename and one + // writer's `app::...` entry would be lost. + const restA = createRest(); + const restB = createRest(); + const restC = createRest(); + await Promise.all([ + new DiscordCommandDeployer({ + clientId: "app-default", + commands, + hashStorePath, + rest: () => restA, + }).deploy({ mode: "reconcile" }), + new DiscordCommandDeployer({ + clientId: "app-secondary", + commands, + hashStorePath, + rest: () => restB, + }).deploy({ mode: "reconcile" }), + new DiscordCommandDeployer({ + clientId: "app-tertiary", + commands, + hashStorePath, + rest: () => restC, + }).deploy({ mode: "reconcile" }), + ]); + + const raw = await fs.readFile(hashStorePath, "utf8"); + const parsed = JSON.parse(raw) as { hashes: Record }; + const keys = Object.keys(parsed.hashes); + // All three apps' entries must survive — pre-fix, one or two would be + // lost to the race. + expect(keys).toContain("app:app-default:global:reconcile"); + expect(keys).toContain("app:app-secondary:global:reconcile"); + expect(keys).toContain("app:app-tertiary:global:reconcile"); + }); + + test("parallel changed deploys preserve fresher sibling cache entries", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-discord-multi-app-")); + const hashStorePath = path.join(dir, "command-deploy-cache.json"); + const oldCommands = [new StaticCommand("ping")]; + const newCommands = [new StaticCommand("status")]; + + await new DiscordCommandDeployer({ + clientId: "app-default", + commands: oldCommands, + hashStorePath, + rest: () => createRest(), + }).deploy({ mode: "reconcile" }); + await new DiscordCommandDeployer({ + clientId: "app-secondary", + commands: oldCommands, + hashStorePath, + rest: () => createRest(), + }).deploy({ mode: "reconcile" }); + + let postStarts = 0; + let releasePosts: () => void = () => {}; + const bothPostsStarted = new Promise((resolve) => { + releasePosts = resolve; + }); + function createWaitingRest(): RequestClient { + const rest = createRest(); + rest.post = vi.fn(async () => { + postStarts += 1; + if (postStarts === 2) { + releasePosts(); + } + await bothPostsStarted; + }) as RequestClient["post"]; + return rest; + } + + await Promise.all([ + new DiscordCommandDeployer({ + clientId: "app-default", + commands: newCommands, + hashStorePath, + rest: () => createWaitingRest(), + }).deploy({ mode: "reconcile" }), + new DiscordCommandDeployer({ + clientId: "app-secondary", + commands: newCommands, + hashStorePath, + rest: () => createWaitingRest(), + }).deploy({ mode: "reconcile" }), + ]); + + const restA = createRest(); + await new DiscordCommandDeployer({ + clientId: "app-default", + commands: newCommands, + hashStorePath, + rest: () => restA, + }).deploy({ mode: "reconcile" }); + const restB = createRest(); + await new DiscordCommandDeployer({ + clientId: "app-secondary", + commands: newCommands, + hashStorePath, + rest: () => restB, + }).deploy({ mode: "reconcile" }); + + expect(restA.get).not.toHaveBeenCalled(); + expect(restA.post).not.toHaveBeenCalled(); + expect(restB.get).not.toHaveBeenCalled(); + expect(restB.post).not.toHaveBeenCalled(); + }); +}); diff --git a/extensions/discord/src/internal/command-deploy.ts b/extensions/discord/src/internal/command-deploy.ts index ae6aad196f8f..4f81f543b94b 100644 --- a/extensions/discord/src/internal/command-deploy.ts +++ b/extensions/discord/src/internal/command-deploy.ts @@ -21,8 +21,42 @@ export type DeployCommandOptions = { type SerializedCommand = ReturnType; +/** + * Per-`command-deploy-cache.json` path async mutex. `server-channels.ts` can + * start several Discord deployers concurrently in the same Node.js process; + * each one shares the same on-disk cache file. Without this lock, two + * deployers can run `persistHashes` in parallel, both read the same on-disk + * snapshot before either writes, and the later `rename` then overwrites the + * earlier writer's entries — defeating the rate-limit cache. + * + * This is an in-process lock; cross-process serialization would need an OS + * file lock. Discord deployers only run inside the gateway process, so an + * in-process mutex is sufficient for the documented concurrency surface. + */ +const cachePersistLocks = new Map>(); + +async function withCachePersistLock(storePath: string, fn: () => Promise): Promise { + const previous = cachePersistLocks.get(storePath) ?? Promise.resolve(); + let release: () => void = () => {}; + const next = new Promise((resolve) => { + release = resolve; + }); + const chained = previous.then(() => next); + cachePersistLocks.set(storePath, chained); + try { + await previous; + return await fn(); + } finally { + release(); + if (cachePersistLocks.get(storePath) === chained) { + cachePersistLocks.delete(storePath); + } + } +} + export class DiscordCommandDeployer { private readonly hashes = new Map(); + private readonly pendingHashes = new Map(); private hashesLoaded = false; constructor( @@ -45,7 +79,7 @@ export class DiscordCommandDeployer { const serializedGlobal = globalCommands.map((command) => command.serialize()); for (const [guildId, entries] of groupGuildCommands(commands)) { await this.putCommandSetIfChanged( - `guild:${guildId}`, + this.scopedCacheKey(`guild:${guildId}`), entries, async () => { await overwriteGuildApplicationCommands( @@ -62,7 +96,7 @@ export class DiscordCommandDeployer { for (const guildId of this.params.devGuilds) { const entries = commands.map((command) => command.serialize()); await this.putCommandSetIfChanged( - `dev-guild:${guildId}`, + this.scopedCacheKey(`dev-guild:${guildId}`), entries, async () => { await overwriteGuildApplicationCommands( @@ -79,7 +113,7 @@ export class DiscordCommandDeployer { } if (options.mode !== "overwrite") { await this.putCommandSetIfChanged( - "global:reconcile", + this.scopedCacheKey("global:reconcile"), serializedGlobal, async () => { await this.reconcileGlobalCommands(serializedGlobal); @@ -89,7 +123,7 @@ export class DiscordCommandDeployer { return { mode: "reconcile" as const, usedDevGuilds: false }; } await this.putCommandSetIfChanged( - "global:overwrite", + this.scopedCacheKey("global:overwrite"), serializedGlobal, async () => { await overwriteApplicationCommands(this.rest, this.params.clientId, serializedGlobal); @@ -99,6 +133,17 @@ export class DiscordCommandDeployer { return { mode: "overwrite" as const, usedDevGuilds: false }; } + /** + * Scope cache keys by Discord application id so multi-bot setups that share a + * single deploy-cache file still reconcile each application separately. The + * prior unscoped `global:reconcile` / `guild:` keys let a later account + * with an identical command set reuse the first account's hash and skip its + * own application's reconcile entirely (#77359). + */ + private scopedCacheKey(suffix: string): string { + return `app:${this.params.clientId}:${suffix}`; + } + private async reconcileGlobalCommands(desired: SerializedCommand[]) { const existing = await this.getCommands(); const existingByKey = new Map(existing.map((command) => [stableCommandKey(command), command])); @@ -135,6 +180,7 @@ export class DiscordCommandDeployer { } await deploy(); this.hashes.set(key, hash); + this.pendingHashes.set(key, hash); await this.persistHashes(); } @@ -169,18 +215,62 @@ export class DiscordCommandDeployer { if (!storePath) { return; } + // Serialize concurrent persists for the same on-disk path. The earlier + // "re-read inside persistHashes" merge alone is not enough — two + // deployers running `persistHashes` in true parallel would both read the + // same snapshot before either writes, and the later `rename` would still + // overwrite the earlier one's `app::...` entries. The mutex makes the + // read-merge-write cycle atomic for in-process callers. + await withCachePersistLock(storePath, async () => { + await this.persistHashesLocked(storePath); + }); + } + + private async persistHashesLocked(storePath: string): Promise { try { - await privateFileStore(path.dirname(storePath)).writeJson( - path.basename(storePath), + // Re-read the on-disk hashes immediately before writing and merge only + // keys this deployer changed. Previously loaded hashes can be stale when + // sibling deployers update the same file, so on-disk wins for untouched + // keys while pending keys win because this deployer just produced them. + const storeFile = path.basename(storePath); + const fileStore = privateFileStore(path.dirname(storePath)); + const merged = new Map(); + let onDisk: { hashes?: unknown } | null = null; + try { + onDisk = await fileStore.readJsonIfExists<{ + hashes?: unknown; + }>(storeFile); + } catch { + // A corrupt cache should not become permanent. Treat the re-read as + // empty and replace it with the fresh pending hashes after deploy. + } + if (onDisk?.hashes && typeof onDisk.hashes === "object") { + for (const [key, value] of Object.entries(onDisk.hashes)) { + if (typeof value === "string" && key.trim() && value.trim()) { + merged.set(key, value); + } + } + } + for (const [key, value] of this.pendingHashes.entries()) { + merged.set(key, value); + } + await fileStore.writeJson( + storeFile, { version: 1, updatedAt: new Date().toISOString(), hashes: Object.fromEntries( - [...this.hashes.entries()].toSorted(([left], [right]) => left.localeCompare(right)), + [...merged.entries()].toSorted(([left], [right]) => left.localeCompare(right)), ), }, { trailingNewline: true }, ); + // Refresh in-memory state so future writes from the same deployer also + // see entries that other deployers added concurrently. + for (const [key, value] of merged.entries()) { + this.hashes.set(key, value); + } + this.pendingHashes.clear(); } catch { // The cache is only an optimization to avoid redundant Discord writes. }