mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 21:49:18 +01:00
fix(server): fence reassessment against changed execution authority
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
c89a242fe4
commit
97719a8ac5
6 files changed
+101
-4
No files matched your search
@@ -2490,3 +2490,13 @@ embedded-PostgreSQL failures and the external-database test repairs are preserve
|
||||
in the owner audit. Approval/interaction/run evidence and workspace producers,
|
||||
concurrent authority, backlog cost, startup wiring and periodic removal remain.
|
||||
Master remains 1fbbef45a8; older stack gates and the full original scope stay open.
|
||||
|
||||
Slice 50 authority checkpoint: two real commit-boundary regressions reproduced
|
||||
stale completion after the source run failed or another run took task execution.
|
||||
The scheduled retired-review path now rechecks both facts under existing locks.
|
||||
All 115 status-corpus cases pass. A retained-history startup assertion failed in
|
||||
the combined run; after waiting for bounded startup batches while enforcing the
|
||||
saved retry deadline, all 28 lifecycle cases and server types pass. The owner
|
||||
audit preserves failures and handoff/contract producer findings. Remaining
|
||||
business-fact races, producer coverage, cost, wiring, periodic removal and PR
|
||||
acceptance are still open. Nothing merged; the full stack goal is incomplete.
|
||||
@@ -456,3 +456,58 @@ Evidence: `/tmp/sleep50-events-before.log` and `.status` (four failures),
|
||||
this checkpoint are terminal. Case-handoff creation/cancellation, remaining
|
||||
contract/ownership/authority producers and the full acceptance tasks listed above
|
||||
remain outstanding; the old periodic reassessment path is still enabled.
|
||||
|
||||
## Scheduled retired-review authority checkpoint
|
||||
|
||||
Two commit-boundary regressions reproduce unsafe completion: after reassessment
|
||||
reads a succeeded run, a competing writer can change its outcome to failed or
|
||||
bind the task to a different run without changing the task's status version.
|
||||
Both previously committed a `done` decision. The scheduled source contract now
|
||||
identifies retired automatic-review reassessment, and the existing status
|
||||
committer rechecks run success and task execution ownership under its existing
|
||||
coordinator/task/run locks. It adds no outer transaction, lock order, provider
|
||||
call, or database query. Explicit legacy reconciliation remains unchanged.
|
||||
|
||||
The two regressions assert that the competing state survives, no new decision
|
||||
commits, and reassessment succeeds once the original authority is restored.
|
||||
All 115 status-corpus cases pass. The combined corpus/lifecycle run initially
|
||||
finished with 142 passes and one failed lifecycle assertion: retained test
|
||||
history needs multiple startup batches, so the first registered wake can be the
|
||||
next batch rather than the target's future retry. The retry test now waits for
|
||||
startup batches to settle while asserting that the target stays in review,
|
||||
then verifies the exact saved deadline before and after restart. All 28 lifecycle
|
||||
cases pass against that same retained-history database; final server types pass.
|
||||
This is 143 distinct passing cases across runs, not a green initial combined run.
|
||||
|
||||
Evidence: `/tmp/sleep50-authority-before.log` and `.status` (two failures),
|
||||
`/tmp/sleep50-authority-after.log` and `.status` (115 corpus passes and 27/28
|
||||
lifecycle passes), `/tmp/sleep50-authority-lifecycle.log` and `.status` (28 passes),
|
||||
and `/tmp/sleep50-authority-final-types.log` and `.status` (pass). All launched
|
||||
handles reached terminal status. No remote acceptance or host sleep is claimed.
|
||||
|
||||
Producer audit notes for the next checkpoint:
|
||||
|
||||
- Handoff evidence is accepted for every persisted non-cancelled state. Delivery,
|
||||
completion, leases and retries do not change that classification. Agent-created
|
||||
handoffs require the source run to be running and own the task; their generated
|
||||
UUID is returned after creation. Board requests can use the current execution
|
||||
binding. Do not add wake traffic to every handoff lease renewal.
|
||||
- Task mutation cancellations are in the issue transaction, which also signals
|
||||
reassessment. Direct handoff cancellation, delivery cancellation, and run-control
|
||||
cancellation still require their own transaction/activation audit; no complete
|
||||
handoff producer-coverage claim is made here.
|
||||
- Superseded external-review withdrawal is called by the status committer inside
|
||||
its decision transaction, which already signals native finalization and chat
|
||||
milestones. OAuth authorization-start separately reopens/creates a pending
|
||||
confirmation; its interaction writes remain to be audited.
|
||||
- The evidence classifier checks target-agent existence, not invokable status.
|
||||
Do not add status-change fanout based only on the name of that check.
|
||||
- Completion-contract creation is serialized with the per-company/task advisory
|
||||
lock in `ensureNativeCompletionContract`. Checking the latest revision before
|
||||
the commit transaction is still insufficient. Audit its callers' lock order
|
||||
before extending the scheduled commit guard.
|
||||
|
||||
Latest-contract, workspace-barrier, governance, dependency and other business-fact
|
||||
races remain open, along with remaining producers, query cost/backlog proof,
|
||||
startup wiring, periodic removal, publication and PR gates. The full original
|
||||
stack scope, older unresolved gates and master reconciliation remain outstanding.
|
||||
@@ -2453,7 +2453,7 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => {
|
||||
return seeded;
|
||||
}
|
||||
|
||||
it.each(["future", "claim", "unknown claim", "assessment", "result", "phase", "decision", "effects", "ownership"] as const)(
|
||||
it.each(["future", "claim", "unknown claim", "assessment", "result", "phase", "decision", "effects", "ownership", "run outcome", "task execution"] as const)(
|
||||
"guards scheduled reassessment against a changed %s at the commit boundary", async changed => {
|
||||
const seeded = await seedAutomaticReview();
|
||||
await dismissAutomaticCompletionReviews(db, seeded.issueId);
|
||||
@@ -2476,6 +2476,12 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => {
|
||||
: changed === "phase" ? { phase: "terminal_failure" }
|
||||
: changed === "decision" ? { decisionId: null } : null;
|
||||
const [originalRun] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, seeded.runId));
|
||||
const [originalIssue] = await db.select().from(issues).where(eq(issues.id, seeded.issueId));
|
||||
const replacementRunId = randomUUID();
|
||||
if (changed === "task execution") await db.insert(heartbeatRuns).values({
|
||||
id: replacementRunId, companyId, agentId, status: "running", runtimeMode: "native",
|
||||
nativeIssueId: seeded.issueId, contextSnapshot: { issueId: seeded.issueId },
|
||||
});
|
||||
const originalEffects = await db.select().from(statusDecisionEffects).where(eq(statusDecisionEffects.decisionId, selected.decisionId!));
|
||||
expect(originalEffects.length).toBeGreaterThan(0);
|
||||
const transaction = db.transaction.bind(db);
|
||||
@@ -2486,6 +2492,10 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => {
|
||||
if (changed === "ownership") await db.update(heartbeatRuns).set({ status: "running",
|
||||
runtimeMode: "native", errorCode: "native_execution_ownership_unverified", nativePhase: "terminal_failure" })
|
||||
.where(eq(heartbeatRuns.id, seeded.runId));
|
||||
if (changed === "run outcome") await db.update(heartbeatRuns).set({ status: "failed" })
|
||||
.where(eq(heartbeatRuns.id, seeded.runId));
|
||||
if (changed === "task execution") await db.update(issues).set({ executionRunId: replacementRunId })
|
||||
.where(eq(issues.id, seeded.issueId));
|
||||
return transaction(callback, options);
|
||||
}) as typeof db.transaction);
|
||||
try {
|
||||
@@ -2506,6 +2516,15 @@ describe("P6-31 Section 18.13 executable status-authority corpus", () => {
|
||||
await db.update(heartbeatRuns).set({ status: originalRun.status, runtimeMode: originalRun.runtimeMode,
|
||||
errorCode: originalRun.errorCode, nativePhase: originalRun.nativePhase }).where(eq(heartbeatRuns.id, seeded.runId));
|
||||
}
|
||||
if (changed === "run outcome") {
|
||||
expect((await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, seeded.runId)))[0].status).toBe("failed");
|
||||
await db.update(heartbeatRuns).set({ status: originalRun.status }).where(eq(heartbeatRuns.id, seeded.runId));
|
||||
}
|
||||
if (changed === "task execution") {
|
||||
expect((await issueService(db).getById(seeded.issueId))!.executionRunId).toBe(replacementRunId);
|
||||
await db.update(issues).set({ executionRunId: originalIssue.executionRunId }).where(eq(issues.id, seeded.issueId));
|
||||
await db.delete(heartbeatRuns).where(eq(heartbeatRuns.id, replacementRunId));
|
||||
}
|
||||
await db.update(nativeRunFinalizations).set({ phase: selected.coordinatorPhase,
|
||||
resultId: seeded.resultId, assessmentId: selected.assessmentId, decisionId: selected.decisionId,
|
||||
nextAttemptAt: null, leaseOwner: null, leaseExpiresAt: null }).where(eq(nativeRunFinalizations.runId, seeded.runId));
|
||||
|
||||
@@ -416,13 +416,21 @@ describe("native reassessment worker lifecycle", () => {
|
||||
await dismissAutomaticCompletionReviews(db, seeded.issueId);
|
||||
const future = new Date(Date.now() + 60_000);
|
||||
await db.update(nativeRunFinalizations).set({ nextAttemptAt: future }).where(eq(nativeRunFinalizations.runId, seeded.runId));
|
||||
const waitForSavedDeadline = async () => {
|
||||
await vi.waitFor(async () => {
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
expect(idleWorkSnapshot().active).toBe(0);
|
||||
expect(coordinator!.nextWakeAt()).toBe(future.getTime());
|
||||
expect(await status(seeded.issueId)).toBe("in_review");
|
||||
}, { timeout: 15_000 });
|
||||
};
|
||||
await start();
|
||||
expect(coordinator!.nextWakeAt()).toBe(future.getTime());
|
||||
await waitForSavedDeadline();
|
||||
await vi.advanceTimersByTimeAsync(20_000);
|
||||
expect(await status(seeded.issueId)).toBe("in_review");
|
||||
await coordinator!.stop(); coordinator = undefined;
|
||||
await start();
|
||||
expect(coordinator!.nextWakeAt()).toBe(future.getTime());
|
||||
await waitForSavedDeadline();
|
||||
await vi.advanceTimersByTimeAsync(Math.max(0, future.getTime() - Date.now())); await flush();
|
||||
expect(await status(seeded.issueId)).toBe("done");
|
||||
expect(await db.select().from(statusDecisions).where(eq(statusDecisions.issueId, seeded.issueId))).toHaveLength(2);
|
||||
|
||||
@@ -207,7 +207,7 @@ export async function reassessNativeStatusFromSource(
|
||||
decision,
|
||||
requireReassessmentSource: options.onlyWhenDue ? {
|
||||
phase: row.coordinatorPhase, resultId: resultRow.id, assessmentId: assessment.id,
|
||||
decisionId: row.decisionId, agentId: row.agentId,
|
||||
decisionId: row.decisionId, agentId: row.agentId, retiredAutomaticReview: !!retiredAutomaticReview,
|
||||
} : undefined,
|
||||
requireNoPendingChildCompletion: decision.statusAction === "done" ? childCompletionRecipient : undefined,
|
||||
supersedesCommittedDecisionId: row.coordinatorPhase === "committed"
|
||||
|
||||
@@ -1602,6 +1602,7 @@ export async function commitNativeStatusDecision(input: {
|
||||
requireNoPendingChildCompletion?: NativeChildCompletionRecipient;
|
||||
requireReassessmentSource?: {
|
||||
phase: string; resultId: string; assessmentId: string; decisionId: string | null; agentId: string;
|
||||
retiredAutomaticReview: boolean;
|
||||
};
|
||||
requireModelRejectionOwner?: { agentId: string; reviewContext: NativeReviewAssignmentContext | null };
|
||||
requireProviderFailureOwner?: { agentId: string; reviewContext: NativeReviewAssignmentContext | null };
|
||||
@@ -1670,6 +1671,10 @@ export async function commitNativeStatusDecision(input: {
|
||||
eq(heartbeatRuns.nativeIssueId, input.issueId), eq(heartbeatRuns.agentId, expected.agentId),
|
||||
)).for("update").limit(1).then(rows => rows[0]);
|
||||
if (!run || run.runtimeMode !== "native" || isNativeRunnerOwnershipHeld(run)) throw new NativeStatusRaceError();
|
||||
if (expected.retiredAutomaticReview && (run.status !== "succeeded" ||
|
||||
(issue.executionRunId !== null && issue.executionRunId !== input.runId))) {
|
||||
throw new NativeStatusRaceError();
|
||||
}
|
||||
const pendingEffect = expected.decisionId ? await tx.select({ id: statusDecisionEffects.id })
|
||||
.from(statusDecisionEffects).where(and(
|
||||
eq(statusDecisionEffects.companyId, input.companyId), eq(statusDecisionEffects.issueId, input.issueId),
|
||||
|
||||
Reference in new issue
Block a user