Skip to content

Commit a7cefa9

Browse files
Discard conversation titles that arrive after deletion (#926)
* Discard conversation titles that arrive after deletion Implemented and locally validated with OpenAI Codex assistance. * Move the late title CHANGELOG entry below the upgrade notes Unreleased opens with the Before upgrading list, and the entries for each change follow it. The entry sat above that list; it now leads the entries after it. --------- Co-authored-by: David McKay <david@copilotkit.ai>
1 parent b3e4584 commit a7cefa9

3 files changed

Lines changed: 95 additions & 6 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,12 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
7878
`COMPUTER_MEMORY_HIGH_PERCENT=0` and `COMPUTER_TAB_IDLE_MS=0` (Helm: `computers.memoryHighPercent`
7979
and `computers.tabIdleMs`) keep the old behaviour.
8080

81+
### A late conversation title is discarded after deletion
82+
83+
A generated title that arrives after a conversation was deleted is discarded. The naming job is
84+
settled without changing the deleted row or announcing a new summary, and a missing conversation is
85+
not reported as successfully named.
86+
8187
### Concurrent routine edits keep their limit and saved schedule
8288

8389
An update now reads the current routine under the same owner lock used to count and write it. An

‎server/src/channels/summary.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -254,11 +254,24 @@ async function summariseOne(
254254
const applied = await transaction
255255
.update(channels)
256256
.set({ summary: title, summaryAt: new Date(), updatedAt: new Date() })
257-
.where(and(eq(channels.id, channelId), isNull(channels.summary)))
257+
.where(
258+
and(
259+
eq(channels.id, channelId),
260+
isNull(channels.summary),
261+
isNull(channels.deletedAt),
262+
),
263+
)
258264
.returning({ id: channels.id });
259-
// Somebody else named it between the claim and this write. Their title stands, and this work
260-
// is done rather than still owed.
261-
if (applied.length === 0) return "written";
265+
if (applied.length === 0) {
266+
// Another writer's title stands, but a conversation deleted while the model worked is gone.
267+
// Both settle the queue item; only a surviving conversation counts as named.
268+
const [remaining] = await transaction
269+
.select({ id: channels.id })
270+
.from(channels)
271+
.where(and(eq(channels.id, channelId), isNull(channels.deletedAt)))
272+
.limit(1);
273+
return remaining ? "written" : "nothing to name it with";
274+
}
262275

263276
const members = await transaction
264277
.select({ userId: channelMemberships.userId })

‎server/tests/channel-summary.integration.test.ts‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
import { afterAll, afterEach, describe, expect, test } from "bun:test";
22
import { randomUUID } from "node:crypto";
33
import { FIRING_FRAME, frameFiring } from "@openbot/shared/routine-firing";
4-
import { eq } from "drizzle-orm";
4+
import { eq, sql } from "drizzle-orm";
5+
import postgres from "postgres";
6+
import {
7+
CHANNEL_ACTIVITY_TOPIC,
8+
type ChannelActivityEvent,
9+
} from "../src/channels/events";
510
import { createAgentProfileStore } from "../src/agents/profile-store";
611
import type { AgentActor } from "../src/agents/profile-types";
712
import { createChannelStore } from "../src/channels/routes";
@@ -360,11 +365,12 @@ describe("naming a claimed conversation", () => {
360365
// Offered before it was named, which is exactly the race two replicas produce.
361366
await queue.offer({ kind: CHANNEL_SUMMARY_KIND, key: channel.id });
362367

363-
await summariseClaimedChannels(
368+
const report = await summariseClaimedChannels(
364369
options({ title: titler("A second opinion") }),
365370
);
366371

367372
expect((await summaryOf(channel.id))?.summary).toBe("Named already");
373+
expect(report.written).toContain(channel.id);
368374
});
369375

370376
test("leaves the conversation unnamed when the model has no answer", async () => {
@@ -432,6 +438,70 @@ describe("naming a claimed conversation", () => {
432438
expect(item?.attempts).toBe(1);
433439
});
434440

441+
test.each(["soft", "hard"] as const)(
442+
"a %s delete while the model is naming a conversation is final",
443+
async (kind) => {
444+
const owner = await createUser();
445+
const channel = await createUsedChannel(owner);
446+
await offer(channel.id);
447+
const started = Promise.withResolvers<void>();
448+
const resume = Promise.withResolvers<void>();
449+
const work = summariseClaimedChannels(
450+
options({
451+
title: async () => {
452+
started.resolve();
453+
await resume.promise;
454+
return "A title that arrived after deletion";
455+
},
456+
}),
457+
);
458+
void work.catch(started.reject);
459+
const notifications = postgres(databaseUrl, { max: 1 });
460+
const barrier = `summary-barrier-${randomUUID()}`;
461+
const observed = Promise.withResolvers<void>();
462+
const summaries: string[] = [];
463+
try {
464+
await notifications.listen(CHANNEL_ACTIVITY_TOPIC, (payload) => {
465+
if (payload === barrier) {
466+
observed.resolve();
467+
return;
468+
}
469+
const event = JSON.parse(payload) as ChannelActivityEvent;
470+
if (event.channelId === channel.id && event.summary !== undefined)
471+
summaries.push(event.summary);
472+
});
473+
await started.promise;
474+
if (kind === "soft")
475+
expect(await store.softDelete(owner, channel.id)).toBe(true);
476+
else await database.delete(channels).where(eq(channels.id, channel.id));
477+
resume.resolve();
478+
const report = await work;
479+
// NOTIFY is commit-ordered. This marker follows the completed worker transaction, so
480+
// observing it also observes any summary announcement that worker could have committed.
481+
await database.execute(
482+
sql`select pg_notify(${CHANNEL_ACTIVITY_TOPIC}, ${barrier})`,
483+
);
484+
await observed.promise;
485+
expect(summaries).toEqual([]);
486+
const row = await summaryOf(channel.id);
487+
if (kind === "soft") {
488+
expect(row?.summary).toBeNull();
489+
expect(row?.summaryAt).toBeNull();
490+
} else expect(row).toBeUndefined();
491+
expect(report.written).not.toContain(channel.id);
492+
const [item] = await database
493+
.select({ finishedAt: workItems.finishedAt })
494+
.from(workItems)
495+
.where(eq(workItems.key, channel.id));
496+
expect(item?.finishedAt).not.toBeNull();
497+
} finally {
498+
resume.resolve();
499+
await Promise.allSettled([work]);
500+
await notifications.end({ timeout: 1 });
501+
}
502+
},
503+
);
504+
435505
test("a soft-deleted conversation is dropped the same way", async () => {
436506
const owner = await createUser();
437507
const channel = await createUsedChannel(owner);

0 commit comments

Comments
 (0)