diff --git a/CHANGELOG.md b/CHANGELOG.md index d19cff090..f1485a4a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,12 @@ the sentence a malformed range already got. A rule like this saved earlier match - An existing Windows clone checks text files out with LF only after `git rm -r --cached . && git reset --hard` on a clean tree. +### Deleting a channel twice is recorded once + +A second `DELETE` of the same channel, from a retry or a second tab, still answers 204 as before. +It no longer tells every member again, and no longer writes another `channel.deleted` row to the +audit trail for a deletion that did not happen. + ### A Bot's saved reply in a group is no longer replaced by a later error In a group conversation, a Bot's reply was saved, then handed on: to Activity, to any consent diff --git a/server/src/agents/lifecycle-reset.ts b/server/src/agents/lifecycle-reset.ts index ae2bedd30..215303a14 100644 --- a/server/src/agents/lifecycle-reset.ts +++ b/server/src/agents/lifecycle-reset.ts @@ -61,7 +61,8 @@ export type BotReset = ReturnType; export function createBotReset(options: { database: Database; - softDeleteChannel: (actor: AgentActor, channelId: string) => Promise; + // Whether the channel was still there to delete is the store's business, not the reset's. + softDeleteChannel: (actor: AgentActor, channelId: string) => Promise; }) { const { database, softDeleteChannel } = options; diff --git a/server/src/channels/routes.ts b/server/src/channels/routes.ts index c43930ec0..0ea077fb4 100644 --- a/server/src/channels/routes.ts +++ b/server/src/channels/routes.ts @@ -197,7 +197,11 @@ export type ChannelStore = { * Throws ChannelNotFoundError for a non-member and ChannelPackageOwnedError for a channel the * tenant package defines, which configuration owns rather than any member. */ - softDelete(actor: AgentActor, channelId: string): Promise; + /** + * True when this call deleted the channel; false when it was already deleted. A repeat is a + * no-op, so it announces nothing and its route records nothing. + */ + softDelete(actor: AgentActor, channelId: string): Promise; recordActivity( actor: AgentActor, channelId: string, @@ -661,7 +665,7 @@ export function createChannelStore( }, async softDelete(actor, channelId) { - await database.transaction( + return await database.transaction( async (transaction) => { const [row] = await transaction .select({ packageId: channels.packageId }) @@ -681,10 +685,17 @@ export function createChannelStore( throw new ChannelPackageOwnedError(channelId); } // The guard on deletedAt is what makes a repeat call a no-op rather than a new stamp. - await transaction + const stamped = await transaction .update(channels) .set({ deletedAt: new Date(), updatedAt: new Date() }) - .where(and(eq(channels.id, channelId), isNull(channels.deletedAt))); + .where(and(eq(channels.id, channelId), isNull(channels.deletedAt))) + .returning({ id: channels.id }); + /* + * And the rest of the no-op: a repeat changed nothing, so nothing is announced. Without + * this, every repeat told every member again, and the route wrote another + * `channel.deleted` row to an append-only trail for a deletion that had not happened. + */ + if (stamped.length === 0) return false; // Read on this transaction, so the members told are the ones the channel had when it was // hidden. Soft leaves the membership rows in place, so this reads the same list a repeat @@ -713,6 +724,7 @@ export function createChannelStore( await transaction.execute( sql`select pg_notify(${CHANNEL_ACTIVITY_TOPIC}, ${JSON.stringify(event)})`, ); + return true; }, { isolationLevel: "read committed" }, ); @@ -1201,8 +1213,9 @@ export function createChannelRoutes( routes.delete("/:channelId", requireUser, async (context) => { const channelId = context.req.param("channelId"); try { - await store.softDelete(context.var.actor, channelId); - await recordDeleted(context, channelId); + const deleted = await store.softDelete(context.var.actor, channelId); + // A repeat is still 204, but it deleted nothing, and the trail records acts, not attempts. + if (deleted !== false) await recordDeleted(context, channelId); return context.body(null, 204); } catch (error) { return mapStoreError(context, error); diff --git a/server/tests/channel-events.integration.test.ts b/server/tests/channel-events.integration.test.ts index c3e2a6d7d..04c426dde 100644 --- a/server/tests/channel-events.integration.test.ts +++ b/server/tests/channel-events.integration.test.ts @@ -451,6 +451,27 @@ describe("channel change delivery", () => { expect(watched.of(other.id)).toEqual([]); }); + test("announces nothing for deleting a channel already deleted", async () => { + const owner = await createTestUser("Twice-Deleting Member"); + const other = await createTestUser("Other Member"); + const channel = await createSharedChannel(owner, other); + await store.softDelete(owner, channel.id); + + const hub = createChannelEventHub(); + const watched = watch(hub, [owner.id, other.id]); + const listener = await startChannelActivityListener(databaseUrl, hub); + + try { + await expect(store.softDelete(owner, channel.id)).resolves.toBe(false); + await new Promise((resolve) => setTimeout(resolve, 500)); + } finally { + await listener.stop(); + } + + expect(watched.of(owner.id)).toEqual([]); + expect(watched.of(other.id)).toEqual([]); + }); + test("announces nothing for a pin on a deleted channel", async () => { const owner = await createTestUser("Pinning Member"); const channel = await createSharedChannel( diff --git a/server/tests/channel-routes.test.ts b/server/tests/channel-routes.test.ts index fb8c1a701..9cc5e8423 100644 --- a/server/tests/channel-routes.test.ts +++ b/server/tests/channel-routes.test.ts @@ -577,6 +577,16 @@ describe("channel delete audit", () => { ]); }); + test("a repeat delete answers 204 and writes no second row", async () => { + // The store's answer for a channel that was already deleted: nothing changed. + const response = await appWithAudit( + fakeStore({ softDelete: async () => false }), + ).request("http://openbot.test/channel-1", { method: "DELETE" }); + + expect(response.status).toBe(204); + expect(audited).toEqual([]); + }); + /* Same discipline as bot-lifecycle-audit.test.ts: the trail records acts, not attempts. */ test("a refused change writes nothing", async () => { const store = fakeStore({ @@ -1429,10 +1439,13 @@ describe("channel soft delete", () => { const created = await persistentStore.create(actor, [agentId]); createdChannelIds.push(created.id); - await persistentStore.softDelete(actor, created.id); - await expect( - persistentStore.softDelete(actor, created.id), - ).resolves.toBeUndefined(); + await expect(persistentStore.softDelete(actor, created.id)).resolves.toBe( + true, + ); + // Resolves rather than throws, and says it deleted nothing, so nothing is announced or recorded. + await expect(persistentStore.softDelete(actor, created.id)).resolves.toBe( + false, + ); }); test("refuses to delete a channel the caller is not a member of", async () => {