Skip to content

Commit a349d02

Browse files
committed
fix(webapp): address #3417 PR review feedback
Three fixes after pushing the Sessions-as-run-manager commit: - `api.v1.sessions.$session.end-and-continue.ts` was destructuring only `{ action }` from `createActionApiRoute`, which means Remix had no handler for OPTIONS preflight on this route. Browser CORS would 405. Sibling routes (`close.ts`) already export `{ action, loader }`. Fix: destructure and export both. - `ensureRunForSession`'s pathological "lost the claim race AND the winner's run was already terminal" branch recursed without bound. In practice progress through the run engine bounds it, but a misconfigured task that crashes before being dequeued could blow the stack. Add a hidden `_attempt` counter, throw `SessionRunManagerError` once it exceeds 3. - `sessionsReplicationService.test.ts` was failing in CI because the sessions-as-run-manager schema migration made `taskIdentifier` and `triggerConfig` required on `Session`. The two `prisma.session.create` calls in the test predate the migration. Add the now-required fields to both fixtures.
1 parent 427541c commit a349d02

3 files changed

Lines changed: 39 additions & 5 deletions

File tree

apps/webapp/app/routes/api.v1.sessions.$session.end-and-continue.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ const ParamsSchema = z.object({
2727
// (PRIVATE) bypasses authorization; a browser holding the session PAT
2828
// can also reach this endpoint, which is fine: if you have the session
2929
// PAT, you own the chat.
30-
const { action } = createActionApiRoute(
30+
const { action, loader } = createActionApiRoute(
3131
{
3232
params: ParamsSchema,
3333
body: EndAndContinueSessionRequestBody,
@@ -129,4 +129,4 @@ const { action } = createActionApiRoute(
129129
}
130130
);
131131

132-
export { action };
132+
export { action, loader };

apps/webapp/app/services/realtime/sessionRunManager.server.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@ export type SessionTriggerConfig = z.infer<typeof SessionTriggerConfigSchema>;
2525

2626
export type EnsureRunReason = "initial" | "continuation" | "upgrade" | "manual";
2727

28+
/**
29+
* Hard cap on how many times `ensureRunForSession` will recurse on the
30+
* pathological "we lost the claim race AND the winner's run was already
31+
* terminal" path. In practice progress through the run engine bounds
32+
* this, but a misconfigured task that crashes before it can be dequeued
33+
* could otherwise loop without limit. After this many attempts we
34+
* surface `SessionRunManagerError` so the caller can 5xx instead of
35+
* blowing the stack.
36+
*/
37+
const ENSURE_RUN_FOR_SESSION_MAX_ATTEMPTS = 3;
38+
2839
type EnsureRunForSessionParams = {
2940
/**
3041
* Session row to operate on. Caller is responsible for the env match —
@@ -42,6 +53,14 @@ type EnsureRunForSessionParams = {
4253
* "preload"` vs `"trigger"`, etc).
4354
*/
4455
payloadOverrides?: Record<string, unknown>;
56+
/**
57+
* @internal Recursion-guard counter for the lost-claim-race retry path.
58+
* Public callers should leave this unset; the function recurses with
59+
* an incremented value on the pathological "winner's run was already
60+
* terminal" branch and throws once it exceeds
61+
* {@link ENSURE_RUN_FOR_SESSION_MAX_ATTEMPTS}.
62+
*/
63+
_attempt?: number;
4564
};
4665

4766
export type EnsureRunResult = {
@@ -69,7 +88,13 @@ export type EnsureRunResult = {
6988
export async function ensureRunForSession(
7089
params: EnsureRunForSessionParams
7190
): Promise<EnsureRunResult> {
72-
const { session, environment, reason, payloadOverrides } = params;
91+
const { session, environment, reason, payloadOverrides, _attempt = 1 } = params;
92+
93+
if (_attempt > ENSURE_RUN_FOR_SESSION_MAX_ATTEMPTS) {
94+
throw new SessionRunManagerError(
95+
`ensureRunForSession exceeded ${ENSURE_RUN_FOR_SESSION_MAX_ATTEMPTS} attempts for session ${session.id} — every triggered run reached a terminal state before claim could resolve`
96+
);
97+
}
7398

7499
// 1. Probe currentRunId.
75100
if (session.currentRunId) {
@@ -153,13 +178,15 @@ export async function ensureRunForSession(
153178
}
154179

155180
// Pathological: winner's run already terminal. Recurse with the fresh
156-
// version. Bounded by run-engine progress — if every triggered run
157-
// dies instantly we'll loop, but that's a deeper bug worth surfacing.
181+
// version. Bounded by `ENSURE_RUN_FOR_SESSION_MAX_ATTEMPTS` so a task
182+
// that always crashes before being dequeued surfaces as an error
183+
// instead of a stack overflow.
158184
return ensureRunForSession({
159185
session: fresh,
160186
environment,
161187
reason,
162188
payloadOverrides,
189+
_attempt: _attempt + 1,
163190
});
164191
}
165192

apps/webapp/test/sessionsReplicationService.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ describe("SessionsReplicationService", () => {
7474
environmentType: "DEVELOPMENT",
7575
organizationId: organization.id,
7676
taskIdentifier: "my-agent",
77+
triggerConfig: {
78+
basePayload: { messages: [], trigger: "preload" },
79+
},
7780
tags: ["user:42", "plan:pro"],
7881
metadata: { plan: "pro", seats: 3 },
7982
},
@@ -174,6 +177,10 @@ describe("SessionsReplicationService", () => {
174177
runtimeEnvironmentId: environment.id,
175178
environmentType: "DEVELOPMENT",
176179
organizationId: organization.id,
180+
taskIdentifier: "my-agent",
181+
triggerConfig: {
182+
basePayload: { messages: [], trigger: "preload" },
183+
},
177184
},
178185
});
179186

0 commit comments

Comments
 (0)