mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-19 20:44:16 +02:00
fix(grok): invalidate readiness before task inspection
Close partial recovery admission and verification races without reversing task and binding lock order. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
8a2bbf09bc
commit
33e4361911
4 files changed
+51
-1
No files matched your search
@@ -89,6 +89,8 @@ routine is healthy. After inspecting Grok, use Clear recovery hold and verify
|
||||
the connection again. Paperclip retires the false file obligation only when the
|
||||
original admission denied workspace access, the assignment is fenced, the
|
||||
coordinator has settled, and the exact managed Runner leases have stop receipts.
|
||||
Connection readiness is invalidated before task inspection, so a partial failure
|
||||
cannot retain an old verified connection while removing a task hold.
|
||||
The inspection preserves the old result, action uncertainty, and no-replay hold.
|
||||
Send a new task message to request fresh work. Inspection does not certify global
|
||||
Grok termination or delete any previously retained allocation. Such allocations
|
||||
|
||||
@@ -280,3 +280,12 @@ negative assertion saw the earlier readiness gate instead of the revoked-session
|
||||
gate; the corrected test checks both before and after re-verification. Server
|
||||
typecheck passes after narrowing the native workspace union. Final current-head
|
||||
CI, review, deployed historical recovery and fresh Stop are still release gates.
|
||||
|
||||
A second review caught a partial-transaction ordering gap. Routine readiness now
|
||||
commits as invalid before the separate task inspection transactions. New tests
|
||||
hold the task lock to prove readiness is already gone and verification remains
|
||||
blocked; an unrelated active assignment cannot cause a task hold to disappear.
|
||||
All 43 Grok routine tests pass. The first follow-up CI attempt also timed out in
|
||||
one unchanged run-history case at 15 seconds; all 91 tests in that file pass
|
||||
locally. The timeout remains a recorded failure with cause unproven. The new
|
||||
source head requires fresh CI and review.
|
||||
@@ -563,6 +563,39 @@ describe("Grok inspection of historical workspace-less Stop", () => {
|
||||
expect(await db.select().from(activityLog).where(and(eq(activityLog.companyId, f.company.id), eq(activityLog.action, "grok.workspace_obligation_inspected")))).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("does not retire task holds if another assignment prevents recovery", async () => {
|
||||
const f = await historicalHold();
|
||||
const [other] = await db.insert(heartbeatRuns).values({ companyId: f.company.id, agentId: f.agent.id,
|
||||
status: "running", invocationSource: "assignment", triggerDetail: "system" }).returning();
|
||||
await db.insert(dotRunnerAssignments).values({ ...f.work, id: randomUUID(), runId: other!.id,
|
||||
normalizedSessionId: randomUUID(), turnId: randomUUID(), status: "accepted" });
|
||||
await expect(f.service.recover(f.company.id, f.agent.id, f.principal.grant.userId)).rejects.toThrow("current Paperclip work session");
|
||||
const [run] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.run.id));
|
||||
expect(hasRequiredWorkspaceRecovery(run!.resultJson)).toBe(true);
|
||||
expect(await f.service.recoveryNeeded(f.company.id, f.agent.id)).toBe(true);
|
||||
});
|
||||
|
||||
it("commits verification invalidation before waiting for historical task inspection", async () => {
|
||||
const f = await historicalHold();
|
||||
let recovery: Promise<unknown> | undefined;
|
||||
try {
|
||||
await db.transaction(async tx => {
|
||||
await tx.select().from(issues).where(eq(issues.id, f.task.id)).for("update");
|
||||
recovery = f.service.recover(f.company.id, f.agent.id, f.principal.grant.userId);
|
||||
void recovery.catch(() => {});
|
||||
await vi.waitFor(async () => {
|
||||
expect(await f.service.state(f.company.id, f.agent.id, f.pairing.bindingId)).toMatchObject({ verified: false, recoveryNeeded: true });
|
||||
});
|
||||
await expect(verification(f)).rejects.toThrow("recover");
|
||||
const [binding] = await db.select().from(dotAgentBindings).where(eq(dotAgentBindings.id, f.pairing.bindingId));
|
||||
expect(binding).toMatchObject({ status: "connected", readyAt: null });
|
||||
const [run] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.run.id));
|
||||
expect(hasRequiredWorkspaceRecovery(run!.resultJson)).toBe(true);
|
||||
});
|
||||
} finally { await recovery; }
|
||||
expect(await f.service.state(f.company.id, f.agent.id, f.pairing.bindingId)).toMatchObject({ verified: false, recoveryNeeded: false });
|
||||
});
|
||||
|
||||
it.each(["cleanup_pending", "receipt_mismatch", "controller_owned", "assignment_active", "running", "operator_revoked", "foreign_company"])("does not retire an old obligation when %s", async condition => {
|
||||
const f = await historicalHold();
|
||||
if (condition === "cleanup_pending") await db.update(environmentLeases).set({ cleanupStatus: "failed" }).where(eq(environmentLeases.id, f.lease.id));
|
||||
@@ -581,6 +614,9 @@ describe("Grok inspection of historical workspace-less Stop", () => {
|
||||
} else await expect(f.service.recover(f.company.id, f.agent.id, f.principal.grant.userId)).rejects.toThrow();
|
||||
const [run] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, f.run.id));
|
||||
expect(hasRequiredWorkspaceRecovery(run!.resultJson)).toBe(true);
|
||||
if (["cleanup_pending", "receipt_mismatch", "controller_owned"].includes(condition)) {
|
||||
expect(await f.service.state(f.company.id, f.agent.id, f.pairing.bindingId)).toMatchObject({ verified: false, recoveryNeeded: true });
|
||||
}
|
||||
expect(await f.continueWork()).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -51,7 +51,6 @@ export function grokRoutineService(db: Db, deliver: Delivery = deliverGrokRoutin
|
||||
|
||||
async function recover(companyId: string, agentId: string, operatorId: string) {
|
||||
await authorizeExternalBotOperator(db, companyId, operatorId);
|
||||
await inspectGrokWorkspaceObligations(db, companyId, agentId, operatorId);
|
||||
await db.transaction(async tx => {
|
||||
// All routine writers serialize on the binding before dispatch/routine rows.
|
||||
// Include retired bindings so recovery cannot bypass an old activity hold.
|
||||
@@ -68,6 +67,10 @@ export function grokRoutineService(db: Db, deliver: Delivery = deliverGrokRoutin
|
||||
await logActivity(tx as unknown as Db, { companyId, actorType: "user", actorId: operatorId, action: "grok.recovery_inspected",
|
||||
entityType: "agent", entityId: agentId, details: { inspectedRemoteWork: true, remoteTerminationConfirmed: false, requiresVerification: true } });
|
||||
});
|
||||
// Commit loss of readiness before retiring task holds. If inspection fails
|
||||
// or waits for a task lock, the old verification cannot admit new work.
|
||||
// The remaining task hold also prevents verification during this gap.
|
||||
await inspectGrokWorkspaceObligations(db, companyId, agentId, operatorId);
|
||||
return { recovered: true, requiresVerification: true, remoteTerminationConfirmed: false };
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user