mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 22:56:58 +01:00
fix(wake-queue): deliver user comments whose wake a restart lost or a stale continuation dropped (#19992)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A user's follow-up comment on a task must reach the agent exactly once, through restarts (invariant 4) > - The final unmasked local chaos storm (96 tasks, 31 restarts) passed 94 tasks. The 2 failures were both lost user comments > - In t69, a restart killed the server between saving the comment and writing its wake. In t72, a comment's own run was cancelled as stale, because the earlier run completed the task first without reading the comment > - This pull request adds a reconciler rule that gives a lost comment its wake, and saves the comment before a stale cancellation > - The benefit is that a user's message is never silently dropped when a restart or a completion race hits at the wrong moment ## Linked Issues or Issue Description Refs #19956 (work-intent outbox), #19944, #19831 (plan, invariant 4). **What happened?** In chaos wave `w-20261108T055444-s9401` (local, unmasked, codex and claude, 30-min K13 storm, master `46bf913fb9`), 94 of 96 tasks passed. Two failed: - **t69, R1 `rested_not_done`.** The user's comment was saved at 06:14:59.358 and reopened the task to `todo`. A K1 SIGKILL fired 28 ms later, before the wake was written. No wake row exists. Recovery skips a `todo` task whose last run succeeded, so the task sat with no run until the wave ended. - **t72, R4 `concurrent_runs`.** A comment posted during a run queued its own run. That run was claimed at 06:16:04.874. The first run's completion marked the task `done` 2 ms later. The comment's run was then cancelled with `continuation_task_ownership_changed` before dispatch, and the comment was lost. **Expected behavior** Each accepted user comment is delivered once, unless the task is cancelled or reassigned, or a person closes it. **Steps to reproduce** `node cli/node_modules/tsx/dist/cli.mjs tests/restart-chaos/main.ts --replay w-20261108T055444-s9401/t69` (or `.../t72`). ## What Changed - `recovery/ownership-invariant-report.ts`: a new reconciler-owned I4 rule, `user_comment_without_wake`. It finds a user comment that meets all of these conditions: - it is newer than every run of the task; - the task is assigned, is not a conversation, has no live run or lock, and has at least one earlier run; - no wake request carries the comment. A comment younger than 30 s is not reported yet, because it may still get its normal wake. Its end-of-grace time is returned as `nextDueAt`. - `native-runtime/native-finalization-reconciler.ts`: the backstop step is now exported as `applyOwnershipReportBackstops`. For each such comment it saves the missing outbox receipt (`executionWait: wake_lost`) and promotes it. - `work-intent-outbox.ts`: new `saveUndeliveredUserComments`, which the reconciler rule and the stale-run path both use. It saves only live user comments that no completed run received. It writes nothing for a task that is reassigned, is a conversation, or has a status the caller does not allow. - `heartbeat/queue.ts`: the saved-receipt pass that already sends pause-saved messages also sends `wake_lost` receipts. It uses the same per-comment consumption check and the same chat-input exclusion. - `heartbeat/stale-comment-continuation.ts` and `heartbeat.ts`: before a stale continuation is cancelled, its undelivered user comments are saved as a deferred receipt. The release drain then reopens the `done` task and delivers them once. This applies only when an agent run that started before the comment completed the task after it. A task that a person marks done is left closed. - `tests/restart-chaos/grader.ts`: R4 ignores runs cancelled before dispatch (`continuation_task_ownership_changed`) when it looks for overlapping execution. Those runs never executed. Comment consumption still counts them. ## Verification Each focused file ran alone in one process (`--maxWorkers=1`): - `heartbeat-held-comment-wake.test.ts` (22 pass). This includes two new cases: - t69 shape: the reconciler rule fires, the backstop delivers the comment once, and a second pass does nothing; - t72 shape: the stale run is cancelled, the task reopens, and the comment is delivered once. Both fail with the rule or the save disabled, and pass with them. - `ownership-reconciliation-report.test.ts` (23 pass). It includes the rule's boundaries: a carried comment, a too-young comment (deadline only) and an already-read comment. - `tests/restart-chaos/grader.test.ts` (17 pass). The new R4 case fails with the old grader. A cancelled run that is not a pre-dispatch cancellation still fails R4. - Unchanged and passing: comment wake batching (27), refused assignment wake (3), wake-queue postgres adapter (103), stale queue invalidation (68), deferred receipt work (45). The first version also reopened a task that a person had marked done, which broke one deferred-receipt test. That case is now excluded. - The server package typecheck and the restart-chaos harness typecheck pass. - Same-session chaos context on master: K6 crash loop `w-20261108T063326-s9402`: 6 of 6 pass. The K4 long-hold wave is still running. After merge, I'll replay only the targeted fault shapes (t69 and t72). ## Risks - The reconciler rule runs on each report pass. It is bounded by the 1-hour lookback and the per-kind row cap, and it excludes conversation tasks, so chat input keeps its own authorization. - A comment younger than 30 s waits up to the end of its grace period before the backstop acts. - The stale-run save reopens a `done` task only for a comment that the completing run could not have read. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, with tool use and code execution in Claude Code. ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
4638e5ea30
commit
8475826d2d
10 files changed
+446
-21
No files matched your search
@@ -18,12 +18,14 @@ import type { createDeliveryWorkCoordinator } from "../delivery-work-coordinator
|
||||
import {
|
||||
createOwnershipReportState,
|
||||
DEFERRED_INTENT_NOT_PROMOTED,
|
||||
USER_COMMENT_DROPPED_BY_STALE_RUN,
|
||||
USER_COMMENT_WITHOUT_WAKE,
|
||||
ownershipReportFingerprint,
|
||||
recheckSettlingKinds,
|
||||
runOwnershipReconciliationPass,
|
||||
type OwnershipReport,
|
||||
} from "../recovery/ownership-invariant-report.js";
|
||||
import { promoteDeferredWorkIntents } from "../work-intent-outbox.js";
|
||||
import { deferredWorkIntentWait, promoteDeferredWorkIntents, saveUndeliveredUserComments } from "../work-intent-outbox.js";
|
||||
import { createHash, randomUUID } from "node:crypto";
|
||||
import { and, asc, desc, eq, gt, inArray, isNotNull, isNull, lt, lte, not, notInArray, or, sql, type SQL } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
@@ -824,13 +826,37 @@ export const OWNERSHIP_REPORT_INTERVAL_MS = 30_000;
|
||||
/** Section E backstop: a deferred intent whose hold cleared without a
|
||||
* release event is promoted through the one promotion function. Promotion
|
||||
* re-runs normal admission, so a wrong report never bypasses a gate. */
|
||||
async function promoteMissedDeferredIntents(db: Db, report: OwnershipReport) {
|
||||
export async function applyOwnershipReportBackstops(db: Db, report: OwnershipReport): Promise<{ retryNeeded: boolean }> {
|
||||
let retryNeeded = false;
|
||||
// Rules: an undelivered user comment newer than the task's last run gets a
|
||||
// wake, and a comment a stale continuation dropped reopens its done task.
|
||||
// Save the missing outbox entry, then promote it.
|
||||
for (const violation of report.violations.filter(row =>
|
||||
row.reason === USER_COMMENT_WITHOUT_WAKE || row.reason === USER_COMMENT_DROPPED_BY_STALE_RUN)) {
|
||||
if (!violation.companyId || !violation.issueId) continue;
|
||||
const lost = violation.reason === USER_COMMENT_WITHOUT_WAKE;
|
||||
try {
|
||||
const [issue] = await db.select({ assigneeAgentId: issues.assigneeAgentId }).from(issues)
|
||||
.where(and(eq(issues.companyId, violation.companyId), eq(issues.id, violation.issueId)));
|
||||
if (!issue?.assigneeAgentId) continue;
|
||||
const saved = await saveUndeliveredUserComments(db, { companyId: violation.companyId, issueId: violation.issueId,
|
||||
agentId: issue.assigneeAgentId, commentIds: [violation.recordId], allowedStatuses: lost ? ["todo", "in_progress"] : ["done"],
|
||||
...(lost ? { executionWait: deferredWorkIntentWait("wake_lost", "This message's wake was lost; it is sent now.") } : {}) });
|
||||
if (saved) await db.transaction(tx => promoteDeferredWorkIntents(tx, { companyId: violation.companyId!,
|
||||
trigger: "reconciler_backstop", issueId: violation.issueId }));
|
||||
} catch (error) {
|
||||
// The message still has no saved receipt: keep a pass due.
|
||||
retryNeeded = true;
|
||||
logger.warn({ err: error, commentId: violation.recordId }, "could not save a user comment without a wake");
|
||||
}
|
||||
}
|
||||
const companies = new Set(report.violations.filter(violation => violation.reason === DEFERRED_INTENT_NOT_PROMOTED)
|
||||
.flatMap(violation => violation.companyId ? [violation.companyId] : []));
|
||||
for (const companyId of companies) {
|
||||
await db.transaction(tx => promoteDeferredWorkIntents(tx, { companyId, trigger: "reconciler_backstop" }))
|
||||
.catch(error => logger.warn({ err: error, companyId }, "deferred intent backstop promotion failed"));
|
||||
.catch(error => { retryNeeded = true; logger.warn({ err: error, companyId }, "deferred intent backstop promotion failed"); });
|
||||
}
|
||||
return { retryNeeded };
|
||||
}
|
||||
|
||||
export function registerOwnershipReconciliationWork(
|
||||
@@ -895,12 +921,13 @@ export function registerOwnershipReconciliationWork(
|
||||
lastPassAt = Date.now();
|
||||
pendingWake = false;
|
||||
const report = await runOwnershipReconciliationPass(db, { state });
|
||||
await promoteMissedDeferredIntents(db, report);
|
||||
const backstop = await applyOwnershipReportBackstops(db, report);
|
||||
const fingerprint = ownershipReportFingerprint(report);
|
||||
const changing = report.inProgress > 0 && fingerprint !== lastFingerprint;
|
||||
// A settling kind's live owner can die without writing a signal, so
|
||||
// keep one bounded re-check due while such records exist.
|
||||
const fullPassDue = changing || report.awaitingSecondProof > 0 || report.retryNeeded;
|
||||
// keep one bounded re-check due while such records exist. A backstop
|
||||
// that could not save or promote keeps one due as well.
|
||||
const fullPassDue = changing || report.awaitingSecondProof > 0 || report.retryNeeded || backstop.retryNeeded;
|
||||
watchOnly = !fullPassDue && report.settleWatch > 0;
|
||||
const followUp = fullPassDue || report.settleWatch > 0
|
||||
? Date.now() + OWNERSHIP_REPORT_INTERVAL_MS : null;
|
||||
|
||||
Reference in new issue
Block a user