mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 02:00:52 +01:00
fix(server): require evidence for feedback recovery
Serialize issue-scoped idempotent wakes and add a partial uniqueness backstop so concurrent operator retries cannot enqueue duplicate delivery runs. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
279a3d7041
commit
2ff8edb14b
8 files changed
+39504
-13
No files matched your search
@@ -0,0 +1,2 @@
|
||||
-- paperclip:migration-safety-ignore large-create-index-not-concurrently: Drizzle migrations run transactionally, so CONCURRENTLY is unavailable because this partial unique index covers only the new feedback-delivery key namespace and is required for atomic at-most-once delivery retries.
|
||||
CREATE UNIQUE INDEX IF NOT EXISTS "agent_wakeup_requests_feedback_delivery_idempotency_uq" ON "agent_wakeup_requests" USING btree ("company_id","idempotency_key") WHERE "agent_wakeup_requests"."idempotency_key" LIKE 'feedback_delivery:%' AND "agent_wakeup_requests"."status" NOT IN ('skipped', 'cancelled');
|
||||
+39303
File diff suppressed because it is too large.
Load diff
+7
@@ -1520,6 +1520,13 @@
|
||||
"when": 1786711898729,
|
||||
"tag": "0218_mushy_jack_murdock",
|
||||
"breakpoints": true
|
||||
},
|
||||
{
|
||||
"idx": 219,
|
||||
"version": "7",
|
||||
"when": 1786910593218,
|
||||
"tag": "0219_cuddly_bloodstrike",
|
||||
"breakpoints": true
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -40,6 +40,9 @@ export const agentWakeupRequests = pgTable(
|
||||
reviewPathRecoveryIdempotencyUq: uniqueIndex("agent_wakeup_requests_review_path_recovery_idempotency_uq")
|
||||
.on(table.companyId, table.idempotencyKey)
|
||||
.where(sql`${table.idempotencyKey} LIKE 'issue_review_path_lost:%' AND ${table.status} <> 'skipped'`),
|
||||
feedbackDeliveryIdempotencyUq: uniqueIndex("agent_wakeup_requests_feedback_delivery_idempotency_uq")
|
||||
.on(table.companyId, table.idempotencyKey)
|
||||
.where(sql`${table.idempotencyKey} LIKE 'feedback_delivery:%' AND ${table.status} NOT IN ('skipped', 'cancelled')`),
|
||||
companyPayloadIssueIdx: index("agent_wakeup_requests_company_payload_issue_idx").on(
|
||||
table.companyId,
|
||||
sql`(${table.payload} ->> 'issueId')`,
|
||||
|
||||
@@ -1563,6 +1563,67 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
expect(runs).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("deduplicates concurrent issue wakes with the same feedback-delivery key", async () => {
|
||||
let releaseAdapter: (() => void) | null = null;
|
||||
const adapterStarted = new Promise<void>((resolve) => {
|
||||
mockAdapterExecute.mockImplementationOnce(async () => {
|
||||
resolve();
|
||||
await new Promise<void>((release) => {
|
||||
releaseAdapter = release;
|
||||
});
|
||||
return {
|
||||
exitCode: 0,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
errorMessage: null,
|
||||
summary: "Handled one idempotent feedback wake.",
|
||||
provider: "test",
|
||||
model: "test-model",
|
||||
};
|
||||
});
|
||||
});
|
||||
const fixture = await seedAssignedTodoNoRunFixture();
|
||||
const heartbeat = heartbeatService(db);
|
||||
const idempotencyKey = `feedback_delivery:${fixture.companyId}:${fixture.issueId}:${fixture.agentId}:manual:1`;
|
||||
const wakeOptions = {
|
||||
source: "on_demand" as const,
|
||||
triggerDetail: "manual" as const,
|
||||
reason: FEEDBACK_DELIVERY_RETRY_WAKE_REASON,
|
||||
idempotencyKey,
|
||||
requestedByActorType: "user" as const,
|
||||
requestedByActorId: "responsible-user",
|
||||
contextSnapshot: {
|
||||
issueId: fixture.issueId,
|
||||
taskId: fixture.issueId,
|
||||
feedbackDeliveryOperatorRequested: true,
|
||||
},
|
||||
};
|
||||
|
||||
const [first, second] = await Promise.all([
|
||||
heartbeat.wakeup(fixture.agentId, wakeOptions),
|
||||
heartbeat.wakeup(fixture.agentId, wakeOptions),
|
||||
]);
|
||||
await Promise.race([
|
||||
adapterStarted,
|
||||
new Promise<never>((_, reject) => setTimeout(() => reject(new Error("feedback wake did not start")), 3_000)),
|
||||
]);
|
||||
|
||||
expect(first?.id).toBeTruthy();
|
||||
expect(second?.id).toBe(first?.id);
|
||||
expect(await db
|
||||
.select({ id: agentWakeupRequests.id })
|
||||
.from(agentWakeupRequests)
|
||||
.where(eq(agentWakeupRequests.idempotencyKey, idempotencyKey))).toHaveLength(1);
|
||||
expect(await db
|
||||
.select({ id: heartbeatRuns.id })
|
||||
.from(heartbeatRuns)
|
||||
.where(eq(heartbeatRuns.agentId, fixture.agentId))).toHaveLength(1);
|
||||
|
||||
if (!releaseAdapter) throw new Error("feedback wake did not reach the adapter");
|
||||
releaseAdapter();
|
||||
await heartbeat.waitForRunExecutionDrain(first?.id ?? "");
|
||||
});
|
||||
|
||||
it("queues exactly one retry when the recorded local pid is dead", async () => {
|
||||
const { agentId, runId, issueId } = await seedRunFixture({
|
||||
agentStatus: "idle",
|
||||
@@ -1776,6 +1837,77 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps feedback exhausted when a successful replay records no handling evidence", async () => {
|
||||
let releaseAdapter: (() => void) | null = null;
|
||||
const adapterStarted = new Promise<void>((resolve) => {
|
||||
mockAdapterExecute.mockImplementationOnce(async () => {
|
||||
resolve();
|
||||
await new Promise<void>((release) => {
|
||||
releaseAdapter = release;
|
||||
});
|
||||
return {
|
||||
exitCode: 0,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
errorMessage: null,
|
||||
summary: "",
|
||||
provider: "test",
|
||||
model: "test-model",
|
||||
};
|
||||
});
|
||||
});
|
||||
const fixture = await seedRunFixture({
|
||||
agentStatus: "idle",
|
||||
processPid: null,
|
||||
processGroupId: null,
|
||||
});
|
||||
const { commentId } = await prepareFeedbackWake(fixture);
|
||||
const heartbeat = heartbeatService(db);
|
||||
|
||||
expect(await heartbeat.reapOrphanedRuns()).toEqual({ reaped: 1, runIds: [fixture.runId] });
|
||||
await Promise.race([
|
||||
adapterStarted,
|
||||
new Promise<never>((_, reject) => setTimeout(() => reject(new Error("feedback retry did not start")), 3_000)),
|
||||
]);
|
||||
|
||||
const retryRun = await db
|
||||
.select()
|
||||
.from(heartbeatRuns)
|
||||
.where(and(
|
||||
eq(heartbeatRuns.agentId, fixture.agentId),
|
||||
sql`${heartbeatRuns.id} <> ${fixture.runId}`,
|
||||
))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!retryRun || !releaseAdapter) throw new Error("feedback retry did not reach the adapter");
|
||||
|
||||
releaseAdapter();
|
||||
expect((await waitForRunToSettle(heartbeat, retryRun.id, 5_000))?.status).toBe("succeeded");
|
||||
await heartbeat.waitForRunExecutionDrain(retryRun.id);
|
||||
|
||||
const recovery = await db
|
||||
.select()
|
||||
.from(issueRecoveryActions)
|
||||
.where(eq(issueRecoveryActions.sourceIssueId, fixture.issueId))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
expect(recovery).toMatchObject({
|
||||
kind: "feedback_delivery",
|
||||
status: "active",
|
||||
cause: "feedback_delivery_exhausted",
|
||||
evidence: {
|
||||
rootWakeupRequestId: fixture.wakeupRequestId,
|
||||
outstandingCommentIds: [commentId],
|
||||
retryGeneration: 1,
|
||||
},
|
||||
});
|
||||
expect(await db
|
||||
.select({ id: activityLog.id })
|
||||
.from(activityLog)
|
||||
.where(and(
|
||||
eq(activityLog.runId, retryRun.id),
|
||||
eq(activityLog.action, "issue.recovery_action_resolved"),
|
||||
))).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("creates one source-scoped recovery action when the feedback replay is exhausted", async () => {
|
||||
const fixture = await seedRunFixture({
|
||||
agentStatus: "idle",
|
||||
|
||||
@@ -10221,6 +10221,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
eq(documentRevisions.createdByRunId, run.id),
|
||||
eq(issueDocuments.companyId, run.companyId),
|
||||
eq(issueDocuments.issueId, issueId),
|
||||
// This system-maintained document is refreshed automatically during
|
||||
// run finalization; it does not prove the agent handled the feedback.
|
||||
sql`${issueDocuments.key} != ${ISSUE_CONTINUATION_SUMMARY_DOCUMENT_KEY}`,
|
||||
))
|
||||
.limit(1)
|
||||
.then((rows) => rows[0] ?? null),
|
||||
@@ -10681,14 +10684,20 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
const delivery = await readFeedbackDeliveryContext(input.run);
|
||||
if (!delivery) return { kind: "not_applicable" as const };
|
||||
|
||||
if (input.run.status === "succeeded") {
|
||||
const handlingEvidence = await findFeedbackHandlingEvidence(input.run, delivery.issueId);
|
||||
|
||||
if (input.run.status === "succeeded" && handlingEvidence) {
|
||||
const activeAction = await recoveryActions.getActiveForIssue(
|
||||
input.run.companyId,
|
||||
delivery.issueId,
|
||||
);
|
||||
if (
|
||||
activeAction
|
||||
&& isSuccessfulFeedbackDeliveryRecoveryMatch({ run: input.run, action: activeAction })
|
||||
&& isSuccessfulFeedbackDeliveryRecoveryMatch({
|
||||
hasHandlingEvidence: true,
|
||||
run: input.run,
|
||||
action: activeAction,
|
||||
})
|
||||
) {
|
||||
const postCommitActivityPublications: ActivityPublication[] = [];
|
||||
const resolvedAction = await db.transaction(async (tx) => {
|
||||
@@ -10749,17 +10758,9 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
}
|
||||
}
|
||||
|
||||
// A successful replay is the terminal delivery signal. It must not be
|
||||
// followed by another exhaustion action merely because the agent chose a
|
||||
// non-comment disposition (for example, a document revision).
|
||||
return {
|
||||
kind: "handled" as const,
|
||||
delivery,
|
||||
handlingEvidence: { kind: "successful_run" as const, id: input.run.id },
|
||||
};
|
||||
return { kind: "handled" as const, delivery, handlingEvidence };
|
||||
}
|
||||
|
||||
const handlingEvidence = await findFeedbackHandlingEvidence(input.run, delivery.issueId);
|
||||
if (handlingEvidence) {
|
||||
return { kind: "handled" as const, delivery, handlingEvidence };
|
||||
}
|
||||
@@ -10777,7 +10778,8 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
});
|
||||
return { kind: "recovery" as const, delivery, recovery };
|
||||
}
|
||||
if (!preLaunchProcessLoss && !input.existingRetryRun) {
|
||||
const successfulWithoutHandlingEvidence = input.run.status === "succeeded";
|
||||
if (!preLaunchProcessLoss && !successfulWithoutHandlingEvidence && !input.existingRetryRun) {
|
||||
return { kind: "not_applicable" as const };
|
||||
}
|
||||
|
||||
@@ -18577,6 +18579,36 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
return { kind: "skipped" as const };
|
||||
}
|
||||
|
||||
// The issue row lock serializes concurrent delivery requests for this
|
||||
// task. Resolve an existing idempotent wake inside the same transaction
|
||||
// so two callers cannot both pass a read-before-write check and enqueue
|
||||
// duplicate runs.
|
||||
if (opts.idempotencyKey) {
|
||||
const existingWakeup = await tx
|
||||
.select({ runId: agentWakeupRequests.runId })
|
||||
.from(agentWakeupRequests)
|
||||
.where(and(
|
||||
eq(agentWakeupRequests.companyId, agent.companyId),
|
||||
eq(agentWakeupRequests.idempotencyKey, opts.idempotencyKey),
|
||||
notInArray(agentWakeupRequests.status, ["skipped", "cancelled"]),
|
||||
))
|
||||
.limit(1)
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (existingWakeup) {
|
||||
const existingRun = existingWakeup.runId
|
||||
? await tx
|
||||
.select()
|
||||
.from(heartbeatRuns)
|
||||
.where(and(
|
||||
eq(heartbeatRuns.id, existingWakeup.runId),
|
||||
eq(heartbeatRuns.companyId, agent.companyId),
|
||||
))
|
||||
.then((rows) => rows[0] ?? null)
|
||||
: null;
|
||||
return { kind: "idempotent" as const, run: existingRun };
|
||||
}
|
||||
}
|
||||
|
||||
if (worktreeExecutionCutoff && issue.createdAt < worktreeExecutionCutoff) {
|
||||
await tx.insert(agentWakeupRequests).values({
|
||||
companyId: agent.companyId,
|
||||
@@ -19305,6 +19337,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
||||
});
|
||||
|
||||
if (outcome.kind === "deferred" || outcome.kind === "skipped") return null;
|
||||
if (outcome.kind === "idempotent") return outcome.run;
|
||||
if (outcome.kind === "coalesced") {
|
||||
await startNextQueuedRunForAgent(agent.id);
|
||||
return outcome.run;
|
||||
|
||||
@@ -110,11 +110,20 @@ describe("successful feedback delivery recovery matching", () => {
|
||||
|
||||
it("matches only a successful run for the same company, issue, and root wake", () => {
|
||||
expect(isSuccessfulFeedbackDeliveryRecoveryMatch({
|
||||
hasHandlingEvidence: true,
|
||||
run: matchingRun,
|
||||
action: matchingAction,
|
||||
})).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects a successful run without same-run handling evidence", () => {
|
||||
expect(isSuccessfulFeedbackDeliveryRecoveryMatch({
|
||||
hasHandlingEvidence: false,
|
||||
run: matchingRun,
|
||||
action: matchingAction,
|
||||
})).toBe(false);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["non-successful run", { run: { ...matchingRun, status: "failed" } }],
|
||||
["different company", { run: { ...matchingRun, companyId: "company-2" } }],
|
||||
@@ -137,6 +146,7 @@ describe("successful feedback delivery recovery matching", () => {
|
||||
["resolved action", { action: { ...matchingAction, status: "resolved" } }],
|
||||
])("rejects a %s", (_label, overrides) => {
|
||||
expect(isSuccessfulFeedbackDeliveryRecoveryMatch({
|
||||
hasHandlingEvidence: true,
|
||||
run: "run" in overrides ? overrides.run : matchingRun,
|
||||
action: "action" in overrides ? overrides.action : matchingAction,
|
||||
})).toBe(false);
|
||||
|
||||
@@ -78,6 +78,7 @@ export function buildFeedbackDeliveryManualRetryIdempotencyKey(input: {
|
||||
}
|
||||
|
||||
export function isSuccessfulFeedbackDeliveryRecoveryMatch(input: {
|
||||
hasHandlingEvidence: boolean;
|
||||
run: {
|
||||
companyId: string;
|
||||
status: string;
|
||||
@@ -92,7 +93,7 @@ export function isSuccessfulFeedbackDeliveryRecoveryMatch(input: {
|
||||
evidence: unknown;
|
||||
};
|
||||
}) {
|
||||
if (input.run.status !== "succeeded") return false;
|
||||
if (input.run.status !== "succeeded" || !input.hasHandlingEvidence) return false;
|
||||
if (
|
||||
input.action.kind !== FEEDBACK_DELIVERY_RECOVERY_ACTION_KIND
|
||||
|| input.action.cause !== FEEDBACK_DELIVERY_RECOVERY_CAUSE
|
||||
|
||||
Reference in new issue
Block a user