mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 15:35:12 +01:00
refactor(server): move the admission half of the deferred wake state machine into the wake-queue module (#13136)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The heartbeat service admits wake requests while an issue has an active execution run. > - That admission branch mixes wake policy, database reads, and database writes in one service. > - This structure makes the wake-queue boundary hard to test and extend. > - This pull request moves the admission policy and its database adapter into the wake-queue module. > - The result keeps heartbeat orchestration small and makes the admission behavior testable in isolation. ## Linked Issues or Issue Description **What existing behavior does this improve?** The heartbeat service now delegates deferred wake admission to the wake-queue module. The module keeps the existing merge, defer, and ordinary-wake outcomes. **Subsystem affected** server/ — REST API and orchestration services. **Current behavior** The heartbeat service contains a 146-line branch that reads wake state, chooses an outcome, and writes the result. **Proposed behavior** The wake-queue module owns the pure admission decision and the adapter reads and writes. The heartbeat service calls one module method. **Reason and benefit** This boundary reduces service coupling and lets module tests cover the admission policy. The change keeps the existing reason strings and outcomes. **Breaking changes** None. The change preserves the current behavior and public API. **Additional context** This pull request follows [PR #13132](https://github.com/paperclipai/paperclip/pull/13132), which merged the first slice of this refactor. I searched GitHub for duplicate and related pull requests before opening this pull request. ## What Changed - Move deferred wake admission policy into `server/src/modules/wake-queue`. - Add module ports and a PostgreSQL adapter for the admission reads and writes. - Keep the existing wake outcomes and stored reason strings. - Extend the module boundary check to reject service imports from the application layer. - Add unit and adapter tests for the moved behavior. ## Verification - `node --test scripts/check-module-boundaries.test.mjs` passes. - The `server/src/modules/wake-queue` suite passes 58 tests. - The eight pinned heartbeat and queued-comment tests remain unchanged and require CI verification. - Every continuous-integration check must reach a terminal green state before merge. ## Risks The refactor changes the location of wake admission logic. A missed adapter condition could change deferred wake behavior. The tests cover the policy outcomes and the adapter writes. The residual tenant-scope risk remains documented in the review record. ## Model Used OpenAI Codex, GPT-5, tool use and code execution. The implementation author ran the tests and prepared the commit set. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes: #` / `Refs: #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
0bff1d5cb6
commit
ae0c1fbbd5
11 files changed
+888
-136
No files matched your search
@@ -1,6 +1,7 @@
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import {
|
||||
agentWakeupRequests,
|
||||
agents,
|
||||
@@ -14,8 +15,13 @@ import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
startEmbeddedPostgresTestDatabase,
|
||||
} from "../../../__tests__/helpers/embedded-postgres.js";
|
||||
import { createPostgresWakeQueueAdapter } from "./postgres.js";
|
||||
import {
|
||||
createAdmissionTransactionScope,
|
||||
createPostgresWakeQueueAdapter,
|
||||
createWakeAdmissionWriter,
|
||||
} from "./postgres.js";
|
||||
import type { WakeQueuePostgresAdapterDeps } from "./postgres.js";
|
||||
import type { TransactionScope } from "../application/ports.js";
|
||||
|
||||
// Proves the atomicity and company-scope properties the security review
|
||||
// requires: every mutation names `companyId` in its own SQL `WHERE` clause,
|
||||
@@ -436,4 +442,105 @@ describeEmbeddedPostgres("wake-queue postgres adapter", () => {
|
||||
expect(issueRow?.executionRunId).toBe(retryRunId);
|
||||
expect(issueRow?.checkoutRunId).toBeNull();
|
||||
});
|
||||
|
||||
// The admission half opens no transaction of its own: `heartbeat.ts` still
|
||||
// owns it. These tests drive the admission writer directly against a
|
||||
// transaction they open themselves, the same way `heartbeat.ts` will.
|
||||
describe("wake admission", () => {
|
||||
// Review test (b): an admission adapter mutation with a foreign company
|
||||
// affects no row.
|
||||
it("refuses to merge into a deferred wake for a company that does not own it, and leaves the wake untouched", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const otherCompanyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const wakeId = await seedDeferredWake({ companyId, agentId, issueId });
|
||||
const writer = createWakeAdmissionWriter();
|
||||
|
||||
await expect(
|
||||
db.transaction(async (tx) => {
|
||||
const scope = createAdmissionTransactionScope(otherCompanyId, tx as unknown as Db);
|
||||
await writer.mergeIntoExistingDeferredWake(scope, {
|
||||
companyId: otherCompanyId,
|
||||
existingDeferredWakeId: wakeId,
|
||||
mergedPayload: { issueId, foo: "bar" },
|
||||
nextCoalescedCount: 5,
|
||||
});
|
||||
}),
|
||||
).rejects.toThrow();
|
||||
|
||||
const wakeRow = (await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.id, wakeId)))[0];
|
||||
expect(wakeRow?.status).toBe("deferred_issue_execution");
|
||||
expect(wakeRow?.coalescedCount).not.toBe(5);
|
||||
expect(wakeRow?.payload).not.toHaveProperty("foo");
|
||||
});
|
||||
|
||||
// Review test (b), the coalesce write: a foreign company affects no row
|
||||
// on the active execution run either.
|
||||
it("refuses to coalesce into a run for a company that does not own it, and leaves the run and the wake table untouched", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const otherCompanyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const runId = await seedRun({ companyId, agentId, status: "running", contextSnapshot: { taskKey: "issue-1" } });
|
||||
const writer = createWakeAdmissionWriter();
|
||||
|
||||
await expect(
|
||||
db.transaction(async (tx) => {
|
||||
const scope = createAdmissionTransactionScope(otherCompanyId, tx as unknown as Db);
|
||||
await writer.coalesceIntoActiveExecutionRun(scope, {
|
||||
companyId: otherCompanyId,
|
||||
activeExecutionRunId: runId,
|
||||
mergedContextSnapshot: { taskKey: "issue-1", commentId: "c1" },
|
||||
agentId,
|
||||
source: "on_demand",
|
||||
triggerDetail: null,
|
||||
payload: null,
|
||||
requestedByActorType: "user",
|
||||
requestedByActorId: null,
|
||||
idempotencyKey: null,
|
||||
});
|
||||
}),
|
||||
).rejects.toThrow();
|
||||
|
||||
const runRow = (await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)))[0];
|
||||
expect(runRow?.contextSnapshot).toEqual({ taskKey: "issue-1" });
|
||||
const wakeRows = await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.companyId, otherCompanyId));
|
||||
expect(wakeRows).toHaveLength(0);
|
||||
});
|
||||
|
||||
// Review test (d): a transaction-port call cannot use the root database
|
||||
// executor. The module rejects a missing scope handle and a mismatched
|
||||
// scope handle, and neither case writes a row.
|
||||
it("rejects a missing transaction scope and a mismatched one, without writing through the root executor", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const otherCompanyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const writer = createWakeAdmissionWriter();
|
||||
const newWakeInput = {
|
||||
companyId,
|
||||
agentId,
|
||||
source: "automation",
|
||||
triggerDetail: null,
|
||||
payload: { issueId },
|
||||
requestedByActorType: "system",
|
||||
requestedByActorId: null,
|
||||
idempotencyKey: null,
|
||||
};
|
||||
|
||||
await expect(
|
||||
writer.insertNewDeferredWake(undefined as unknown as TransactionScope, newWakeInput),
|
||||
).rejects.toThrow(/transaction scope/);
|
||||
|
||||
await expect(
|
||||
db.transaction(async (tx) => {
|
||||
const mismatchedScope = createAdmissionTransactionScope(otherCompanyId, tx as unknown as Db);
|
||||
await writer.insertNewDeferredWake(mismatchedScope, newWakeInput);
|
||||
}),
|
||||
).rejects.toThrow(/different company/);
|
||||
|
||||
const rows = await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.companyId, companyId));
|
||||
expect(rows).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -33,6 +33,7 @@ import {
|
||||
parseObject,
|
||||
readNonEmptyString,
|
||||
} from "../domain/values.js";
|
||||
import { requireTransactionScopeTx, TransactionScope } from "../application/ports.js";
|
||||
import type {
|
||||
DeferredWakeCandidate,
|
||||
InvokableAgentSnapshot,
|
||||
@@ -41,6 +42,8 @@ import type {
|
||||
LockedIssueExecution,
|
||||
ReleaseTransactionResult,
|
||||
RunSnapshot,
|
||||
WakeAdmissionReader,
|
||||
WakeAdmissionWriter,
|
||||
WakeQueueHost,
|
||||
WakeQueueTransaction,
|
||||
} from "../application/ports.js";
|
||||
@@ -628,6 +631,142 @@ async function recordNativeTerminalRecoveryIfNeeded(tx: Db, run: HeartbeatRunRow
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the temporary transaction-scope handle the admission port needs.
|
||||
* `heartbeat.ts` calls this through `createWakeQueue`'s own wrapper; it
|
||||
* never builds a `TransactionScope` itself.
|
||||
*/
|
||||
export function createAdmissionTransactionScope(companyId: string, tx: Db): TransactionScope {
|
||||
return TransactionScope.create(companyId, tx);
|
||||
}
|
||||
|
||||
function requireAdmissionTx(scope: TransactionScope | null | undefined, companyId: string): Db {
|
||||
return requireTransactionScopeTx(scope, companyId) as Db;
|
||||
}
|
||||
|
||||
export function createWakeAdmissionReader(): WakeAdmissionReader {
|
||||
return {
|
||||
async isSameExecutionAgent(
|
||||
scope,
|
||||
{ companyId, activeExecutionRunAgentId, issueExecutionAgentNameKey, agentNameKey },
|
||||
) {
|
||||
const tx = requireAdmissionTx(scope, companyId);
|
||||
const executionAgent = await tx
|
||||
.select({ name: agents.name })
|
||||
.from(agents)
|
||||
.where(and(eq(agents.id, activeExecutionRunAgentId), eq(agents.companyId, companyId)))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
const executionAgentNameKey =
|
||||
normalizeAgentNameKey(issueExecutionAgentNameKey) ?? normalizeAgentNameKey(executionAgent?.name);
|
||||
return Boolean(executionAgentNameKey) && executionAgentNameKey === agentNameKey;
|
||||
},
|
||||
|
||||
async findExistingDeferredWake(scope, { companyId, agentId, issueId }) {
|
||||
const tx = requireAdmissionTx(scope, companyId);
|
||||
const row = await tx
|
||||
.select()
|
||||
.from(agentWakeupRequests)
|
||||
.where(
|
||||
and(
|
||||
eq(agentWakeupRequests.companyId, companyId),
|
||||
eq(agentWakeupRequests.agentId, agentId),
|
||||
eq(agentWakeupRequests.status, DEFERRED_WAKE_STATUS),
|
||||
sql`${agentWakeupRequests.payload} ->> 'issueId' = ${issueId}`,
|
||||
),
|
||||
)
|
||||
.orderBy(asc(agentWakeupRequests.requestedAt))
|
||||
.limit(1)
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!row) return null;
|
||||
const payload = parseObject(row.payload);
|
||||
return {
|
||||
id: row.id,
|
||||
payload,
|
||||
deferredContext: parseObject(payload[DEFERRED_WAKE_CONTEXT_KEY]),
|
||||
coalescedCount: row.coalescedCount,
|
||||
};
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
export function createWakeAdmissionWriter(): WakeAdmissionWriter {
|
||||
return {
|
||||
async coalesceIntoActiveExecutionRun(scope, input) {
|
||||
const tx = requireAdmissionTx(scope, input.companyId);
|
||||
const now = new Date();
|
||||
const mergedRun = await tx
|
||||
.update(heartbeatRuns)
|
||||
.set({ contextSnapshot: input.mergedContextSnapshot, updatedAt: now })
|
||||
.where(and(eq(heartbeatRuns.id, input.activeExecutionRunId), eq(heartbeatRuns.companyId, input.companyId)))
|
||||
.returning()
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!mergedRun) {
|
||||
// The compare-and-set write affected no row. Throw to roll the
|
||||
// transaction back instead of recording a coalesced wake against a
|
||||
// run this write never touched.
|
||||
throw new Error("wake-queue: the coalesce target run was not found for this company");
|
||||
}
|
||||
await tx.insert(agentWakeupRequests).values({
|
||||
companyId: input.companyId,
|
||||
agentId: input.agentId,
|
||||
source: input.source,
|
||||
triggerDetail: input.triggerDetail,
|
||||
reason: "issue_execution_same_name",
|
||||
payload: input.payload,
|
||||
status: "coalesced",
|
||||
coalescedCount: 1,
|
||||
requestedByActorType: input.requestedByActorType,
|
||||
requestedByActorId: input.requestedByActorId,
|
||||
idempotencyKey: input.idempotencyKey,
|
||||
runId: mergedRun.id,
|
||||
finishedAt: now,
|
||||
});
|
||||
return mergedRun as unknown as Record<string, unknown>;
|
||||
},
|
||||
|
||||
async mergeIntoExistingDeferredWake(scope, input) {
|
||||
const tx = requireAdmissionTx(scope, input.companyId);
|
||||
const rows = await tx
|
||||
.update(agentWakeupRequests)
|
||||
.set({
|
||||
payload: input.mergedPayload,
|
||||
coalescedCount: input.nextCoalescedCount,
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(
|
||||
and(
|
||||
eq(agentWakeupRequests.id, input.existingDeferredWakeId),
|
||||
eq(agentWakeupRequests.companyId, input.companyId),
|
||||
eq(agentWakeupRequests.status, DEFERRED_WAKE_STATUS),
|
||||
),
|
||||
)
|
||||
.returning({ id: agentWakeupRequests.id });
|
||||
if (rows.length === 0) {
|
||||
// The compare-and-set write affected no row: a concurrent writer
|
||||
// already moved this wake off `deferred_issue_execution`. Roll the
|
||||
// transaction back instead of leaving the merge half-applied.
|
||||
throw new Error("wake-queue: the deferred wake to merge into was not found for this company");
|
||||
}
|
||||
},
|
||||
|
||||
async insertNewDeferredWake(scope, input) {
|
||||
const tx = requireAdmissionTx(scope, input.companyId);
|
||||
await tx.insert(agentWakeupRequests).values({
|
||||
companyId: input.companyId,
|
||||
agentId: input.agentId,
|
||||
source: input.source,
|
||||
triggerDetail: input.triggerDetail,
|
||||
reason: "issue_execution_deferred",
|
||||
payload: input.payload,
|
||||
status: DEFERRED_WAKE_STATUS,
|
||||
requestedByActorType: input.requestedByActorType,
|
||||
requestedByActorId: input.requestedByActorId,
|
||||
idempotencyKey: input.idempotencyKey,
|
||||
});
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
export function createPostgresWakeQueueAdapter(db: Db, deps: WakeQueuePostgresAdapterDeps): IssueLockWriter {
|
||||
return {
|
||||
async withIssueExecutionLock(input, fn): Promise<ReleaseTransactionResult & { run: RunSnapshot }> {
|
||||
|
||||
Reference in new issue
Block a user