Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/trigger-sdk/src/v3/ai-shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,3 +278,30 @@ export function waitBeforeEofResubscribe(attempt: number, signal: AbortSignal):
if (signal.aborted) done();
});
}

/**
* Combine multiple `AbortSignal` instances into a single signal, falling back
* to manual listener forwarding on runtimes without `AbortSignal.any` (such as
* Safari / iOS Safari < 17.4).
*/
export function anyAbortSignal(signals: AbortSignal[]): AbortSignal {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Safari fix lacks a release note

The anyAbortSignal change fixes a customer-facing SDK failure, but this PR adds no changeset. The fix lacks the required patch release entry.

Learn more

The repository's AGENTS.md and CONTRIBUTING.md require a changeset for user-facing fixes in public packages. This change lets customers use chat transport on older Safari versions, so it meets that requirement.

Example: A customer upgrading the SDK to avoid AbortSignal.any is not a function cannot find this fix in the package's changeset-driven release notes.

Recommended fix: Add a patch changeset for @trigger.dev/sdk with a user-focused description of Safari chat compatibility.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

if (typeof AbortSignal.any === "function") {
return AbortSignal.any(signals);
}

const controller = new AbortController();
const aborted = signals.find((s) => s.aborted);
if (aborted) {
controller.abort(aborted.reason);
return controller.signal;
}

for (const s of signals) {
s.addEventListener("abort", () => controller.abort(s.reason), {
once: true,
signal: controller.signal,
});
Comment on lines +299 to +303

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Finished streams retain abort listeners

On older Safari, anyAbortSignal keeps listeners after streams close without an abort. Reusing a caller signal accumulates retained callbacks and stream state across subscriptions.

Learn more

The fallback removes a source listener only when the composed controller aborts. An AgentChat subscription can close on a settled or empty response without aborting its controller. A browser reconnect also creates an outer composed signal whose controller remains un-aborted after an ordinary turn completes. A long-lived caller signal therefore retains the fallback's listener and captured controller for each such subscription.

Example: A page reuses one AbortController.signal for ten reconnects on Safari 16. Each completed reconnect leaves a listener on that signal, even though no stream remains open.

Recommended fix: Give the fallback an explicit disposal mechanism and call it on stream teardown, including the outer reconnect composition; alternatively ensure every composed controller aborts on every terminal path. Verify normal completion and settled-EOF cleanup as well as cancellation.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

}
return controller.signal;
}

3 changes: 2 additions & 1 deletion packages/trigger-sdk/src/v3/chat-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
} from "@trigger.dev/core/v3";
import type { ChatInputChunk, ChatTaskWirePayload } from "./ai-shared.js";
import {
anyAbortSignal,
chatRunTags,
MAX_EOF_RESUBSCRIBES,
slimSubmitMessageForWire,
Expand Down Expand Up @@ -815,7 +816,7 @@ export class AgentChat<TAgent = unknown> {

const internalAbort = new AbortController();
const combinedSignal = abortSignal
? AbortSignal.any([abortSignal, internalAbort.signal])
? anyAbortSignal([abortSignal, internalAbort.signal])
: internalAbort.signal;

// Detached once the stream ends (see the `finally` below): a caller reusing
Expand Down
70 changes: 70 additions & 0 deletions packages/trigger-sdk/src/v3/chat.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2950,4 +2950,74 @@ describe("TriggerChatTransport", () => {
expect(textChunks).toHaveLength(2);
});
});

describe("Safari < 17.4 compatibility (no AbortSignal.any)", () => {
it("streams and propagates caller aborts when AbortSignal.any is undefined", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Safari test lacks required regression comment

The new test has no // Catches: <bug> comment, as required by the repository's testing instructions.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

const originalAny = AbortSignal.any;
Object.defineProperty(AbortSignal, "any", {
value: undefined,
configurable: true,
writable: true,
});

try {
global.fetch = vi.fn().mockImplementation(async (url: string | URL) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Browser regression test uses prohibited mocks

The new Safari test replaces global.fetch with vi.fn() twice. Repository testing rules prohibit mocks in new tests.

Learn more

The repository's AGENTS.md and test guidance prohibit new tests from using mocks, spies, stubs, or fakes. The added Safari test assigns a mocked global.fetch here and again later in the same test.

Example: The compatibility check serves synthetic responses from a vi.fn() implementation instead of exercising the real fetch boundary.

Recommended fix: Use an integration-style test with a real local HTTP endpoint and restore the original environment afterward, or move signal-composition assertions into a focused test that needs no fetch replacement.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

const urlStr = typeof url === "string" ? url : url.toString();
if (isSessionStreamAppendUrl(urlStr)) return defaultAppendResponse();
if (isSessionOutSubscribeUrl(urlStr)) {
return defaultSseResponse([
{ type: "text-delta", id: "p1", delta: "Safari fallback" },
{ type: "trigger:turn-complete" },
]);
}
throw new Error(`Unexpected URL: ${urlStr}`);
});

const transport = new TriggerChatTransport({
task: "my-chat-task",
accessToken: () => "pat",
sessions: {
"chat-safari": { publicAccessToken: "p", isStreaming: true },
},
});

const callerAbort = new AbortController();
const stream = await transport.sendMessages({
trigger: "submit-message",
chatId: "chat-safari",
messageId: undefined,
messages: [createUserMessage("Hi from Safari 16")],
abortSignal: callerAbort.signal,
});
const chunks = await drainChunks(stream);
expect(chunks).toEqual([{ type: "text-delta", id: "p1", delta: "Safari fallback" }]);
Comment on lines +2984 to +2993

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Active caller abort remains untested

The test never aborts callerAbort while a stream is open. Its pre-aborted case cannot detect broken forwarding of later abort events.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.


const preAborted = new AbortController();
preAborted.abort(new Error("user stopped"));
let receivedAbortOnSubscribe = false;
global.fetch = vi.fn().mockImplementation(async (url: string | URL, init?: RequestInit) => {
const urlStr = typeof url === "string" ? url : url.toString();
if (isSessionOutSubscribeUrl(urlStr)) {
receivedAbortOnSubscribe = Boolean(init?.signal?.aborted);
return defaultSseResponse([{ type: "trigger:turn-complete" }]);
}
throw new Error(`Unexpected URL: ${urlStr}`);
});

const resumed = await transport.reconnectToStream({
chatId: "chat-safari",
abortSignal: preAborted.signal,
});
await drainChunks(resumed!);
expect(receivedAbortOnSubscribe).toBe(true);
} finally {
Object.defineProperty(AbortSignal, "any", {
value: originalAny,
configurable: true,
writable: true,
});
}
});
});
});

5 changes: 3 additions & 2 deletions packages/trigger-sdk/src/v3/chat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ function byteLength(body: string): number {
}
import { ChatTabCoordinator } from "./chat-tab-coordinator.js";
import {
anyAbortSignal,
MAX_EOF_RESUBSCRIBES,
slimSubmitMessageForWire,
waitBeforeEofResubscribe,
Expand Down Expand Up @@ -1355,7 +1356,7 @@ export class TriggerChatTransport implements ChatTransport<UIMessage> {
this.activeStreams.set(options.chatId, abortController);

const abortSignal = options.abortSignal
? AbortSignal.any([options.abortSignal, abortController.signal])
? anyAbortSignal([options.abortSignal, abortController.signal])
: abortController.signal;

return this.subscribeToSessionStream(state, abortSignal, options.chatId, {
Expand Down Expand Up @@ -2064,7 +2065,7 @@ export class TriggerChatTransport implements ChatTransport<UIMessage> {
let didSettle = false;
this.activeStreams.set(chatId, internalAbort);
const combinedSignal = abortSignal
? AbortSignal.any([abortSignal, internalAbort.signal])
? anyAbortSignal([abortSignal, internalAbort.signal])
: internalAbort.signal;

if (abortSignal) {
Expand Down