Skip to content

Commit bc252db

Browse files
authored
refactor(sessions): address review — dedupe write-ahead save, log recovery, soften wording
From qa-swarm review of this PR: - Build the PendingPromptRecord once and re-save with `{ ...pending, taskRunId }` instead of assembling the 9-field payload twice (convergent paul + xp). - Log when a written-ahead prompt is actually recovered, so the safety net is observable in the wild. - Soften the "impossible to lose" wording in the doc + comment: persistence is async/best-effort, so it's "very unlikely", not guaranteed. Deferred to a follow-up: an integration test exercising the recovery branch of createNewLocalSession (currently the store mock returns undefined everywhere). Generated-By: PostHog Code Task-Id: 22d61751-8d18-4d56-a79e-4741045320c1
1 parent 1e9cc00 commit bc252db

2 files changed

Lines changed: 33 additions & 27 deletions

File tree

packages/core/src/sessions/pendingPrompt.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,12 @@ import type { Adapter, ExecutionMode } from "@posthog/shared";
1212
* can exceed the 30s `SESSION_VALIDATION_TIMEOUT_MS` budget), or the app is
1313
* reloaded/quit/crashes during the retry window, the prompt is lost.
1414
*
15-
* To make that impossible we persist this record BEFORE any agent/session
16-
* setup is attempted, and only clear it once the prompt has actually been
17-
* delivered to the agent. A persisted record therefore means "this prompt is
15+
* To make that loss very unlikely we persist this record BEFORE any
16+
* agent/session setup is attempted, and only clear it once the prompt has
17+
* actually been delivered to the agent. (Persistence is async and
18+
* best-effort, so it is not an absolute guarantee — a crash in the first
19+
* moments of a cold start can still race the write.) A persisted record
20+
* therefore means "this prompt is
1821
* owed delivery and has not been delivered yet" — the basis for recovering it
1922
* on the next connect, and (in a later change) for a server-side reconciler to
2023
* re-drive orphaned runs.

packages/core/src/sessions/sessionService.ts

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ import {
5757
OFFLINE_SESSION_MESSAGE,
5858
routeLocalConnect,
5959
} from "./connectRouting";
60-
import type { PendingPromptStore } from "./pendingPrompt";
60+
import type { PendingPromptRecord, PendingPromptStore } from "./pendingPrompt";
6161
import {
6262
type PermissionSelectionPlan,
6363
planPermissionResponse,
@@ -1082,13 +1082,25 @@ export class SessionService {
10821082
const effectiveModel = model ?? recovered?.model;
10831083
const effectiveReasoningLevel = reasoningLevel ?? recovered?.reasoningLevel;
10841084

1085-
// Write-ahead the prompt BEFORE any session/agent work. This is the one
1086-
// step that makes losing it impossible: everything below (run creation,
1087-
// the 30s-bounded agent.start, prompt delivery) can throw or be killed by
1088-
// an app restart, and the prompt still survives on disk to be recovered on
1089-
// the next connect. Cleared only once it has actually been delivered.
1085+
if (recovered) {
1086+
// Surface when the safety net actually catches something, so we can tell
1087+
// it's working (and gauge how long prompts sit undelivered).
1088+
this.d.log.info("Recovered a written-ahead prompt for a task", {
1089+
taskId,
1090+
ageMs: Date.now() - recovered.createdAt,
1091+
});
1092+
}
1093+
1094+
// Write-ahead the prompt BEFORE any session/agent work. This is the step
1095+
// that makes losing it very unlikely: everything below (run creation, the
1096+
// 30s-bounded agent.start, prompt delivery) can throw or be killed by an
1097+
// app restart, and the prompt still survives to be recovered on the next
1098+
// connect. Cleared only once it has actually been delivered. (Persistence
1099+
// is async and best-effort, so a crash in the first moments of a cold
1100+
// start can still race it — hence "very unlikely", not "guaranteed".)
1101+
let pending: PendingPromptRecord | undefined;
10901102
if (effectivePrompt?.length) {
1091-
this.d.pendingPrompts.save({
1103+
pending = {
10921104
taskId,
10931105
taskTitle,
10941106
repoPath,
@@ -1098,7 +1110,8 @@ export class SessionService {
10981110
model: effectiveModel,
10991111
reasoningLevel: effectiveReasoningLevel,
11001112
createdAt: recovered?.createdAt ?? Date.now(),
1101-
});
1113+
};
1114+
this.d.pendingPrompts.save(pending);
11021115
}
11031116

11041117
const { client } = auth;
@@ -1111,22 +1124,12 @@ export class SessionService {
11111124
throw new Error("Failed to create task run. Please try again.");
11121125
}
11131126

1114-
// Record the run id on the write-ahead entry so it points at the run the
1115-
// prompt is being delivered to (a server-side reconciler can use this to
1116-
// re-drive an orphaned run).
1117-
if (effectivePrompt?.length) {
1118-
this.d.pendingPrompts.save({
1119-
taskId,
1120-
taskTitle,
1121-
repoPath,
1122-
initialPrompt: effectivePrompt,
1123-
taskRunId: taskRun.id,
1124-
executionMode: effectiveExecutionMode,
1125-
adapter: effectiveAdapter,
1126-
model: effectiveModel,
1127-
reasoningLevel: effectiveReasoningLevel,
1128-
createdAt: recovered?.createdAt ?? Date.now(),
1129-
});
1127+
// Stamp the same write-ahead entry with the run id it's now being
1128+
// delivered to (a server-side reconciler can use this to re-drive an
1129+
// orphaned run).
1130+
if (pending) {
1131+
pending = { ...pending, taskRunId: taskRun.id };
1132+
this.d.pendingPrompts.save(pending);
11301133
}
11311134

11321135
const { customInstructions: startCustomInstructions } = this.d.settings;

0 commit comments

Comments
 (0)