mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 07:10:34 +01:00
fix(ui): explain monitor waits during active runs
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
a434edf991
commit
c30a7c79bf
5 files changed
+92
-6
No files matched your search
@@ -12,6 +12,13 @@ already-known monitor deadline can fire earlier than the batching window.
|
||||
After all work settles, no periodic monitor scan remains. Pending claims and
|
||||
failed or deferred admissions retain their bounded deadlines.
|
||||
|
||||
A monitor deadline is the earliest time for a check. An active native run or an
|
||||
unfinished finalization holds the check until the prior work settles. The task
|
||||
banner explains the active run wait instead of promising the next scheduler tick
|
||||
or showing it as overdue. Once the run clears, the banner resumes its countdown
|
||||
and offers Check now. Finalization notifications restore monitor scheduling from
|
||||
the saved rows; they do not bypass execution admission.
|
||||
|
||||
Both idle drains and ordinary maintenance drains pause monitor dispatch. The
|
||||
worker retains its pending request without repeatedly reading monitor tables.
|
||||
After an explicit drain stop or TTL expiry, a bounded in-memory retry resumes
|
||||
|
||||
@@ -16,6 +16,7 @@ import { issueService } from "../issues.js";
|
||||
import { createDeliveryWorkCoordinator } from "../delivery-work-coordinator.js";
|
||||
import { DELIVERY_QUEUES } from "../delivery-work-notifications.js";
|
||||
import { registerScheduledWork } from "../scheduled-work.js";
|
||||
import { notifyNativeFinalizationWork } from "../native-finalization-notifications.js";
|
||||
import { idleWorkSnapshot, readTaskDrain, startTaskDrain, stopTaskDrain } from "../task-admission.js";
|
||||
import { conflict } from "../../errors.js";
|
||||
import { createHeartbeatRunPreparation } from "./run-preparation.js";
|
||||
@@ -585,6 +586,36 @@ describe.skipIf(!postgres.supported)("heartbeat scheduling database boundary", (
|
||||
} });
|
||||
}
|
||||
|
||||
it.each(["committed", "terminal_failure"] as const)("reactivates a due monitor after its last finalization hold becomes %s", async phase => {
|
||||
const f = await fixture();
|
||||
const [run] = await db.insert(heartbeatRuns).values({
|
||||
companyId: f.company.id, agentId: f.agent.id, nativeIssueId: f.issue.id,
|
||||
runtimeMode: "native", status: "succeeded", finishedAt: now,
|
||||
}).returning();
|
||||
await db.insert(nativeRunFinalizations).values({
|
||||
companyId: f.company.id, issueId: f.issue.id, runId: run!.id, phase: "arbitrating",
|
||||
});
|
||||
await start(f);
|
||||
await quiet();
|
||||
expect(f.deps.enqueueWakeup).not.toHaveBeenCalled();
|
||||
const settle = (owner: Db) => owner.transaction(async tx => {
|
||||
await notifyNativeFinalizationWork(tx, { companyId: f.company.id, runId: run!.id, issueId: f.issue.id });
|
||||
await tx.update(nativeRunFinalizations).set({ phase }).where(eq(nativeRunFinalizations.runId, run!.id));
|
||||
});
|
||||
await expect(db.transaction(async tx => {
|
||||
await settle(tx as unknown as Db);
|
||||
throw new Error("rollback finalization");
|
||||
})).rejects.toThrow("rollback finalization");
|
||||
await quiet();
|
||||
expect(f.deps.enqueueWakeup).not.toHaveBeenCalled();
|
||||
await settle(db);
|
||||
await advance(31_000);
|
||||
await vi.waitFor(async () => expect((await readIssue(f.issue.id)).monitorNextCheckAt).toBeNull());
|
||||
expect(f.deps.enqueueWakeup).toHaveBeenCalledOnce();
|
||||
expect(await readIssue(f.issue.id)).toMatchObject({ monitorAttemptCount: 1 });
|
||||
await quiet();
|
||||
});
|
||||
|
||||
it.each(["commit", "rollback"] as const)("reconciles worktree activation only after its outer transaction %s", async outcome => {
|
||||
const f = await fixture();
|
||||
const deadline = now.getTime() + 3_600_000;
|
||||
|
||||
@@ -33,7 +33,7 @@ describe("buildMonitorSurfaceCopy", () => {
|
||||
it.each(["retrying", "due-now", "overdue"] as const)("keeps workspace contention neutral when %s", (state) => {
|
||||
const copy = buildMonitorSurfaceCopy(derived({
|
||||
state, source: "scheduled-retry", nextCheckAt: NOW.toISOString(), attemptCount: 4,
|
||||
}), NOW, "workspace_busy");
|
||||
}), NOW, "workspace_busy", 0, "in_progress", true);
|
||||
expect(copy!.bannerTitle).toBe("Waiting for workspace");
|
||||
expect(copy!.stripTitle).toBe("Waiting for workspace");
|
||||
expect(copy!.tone).toBe("info");
|
||||
@@ -90,6 +90,14 @@ describe("buildMonitorSurfaceCopy", () => {
|
||||
expect(copy!.stripTitle).toBe("Resumes in 1h 30m");
|
||||
});
|
||||
|
||||
it("keeps blocked checks paused while a run is active", () => {
|
||||
const copy = buildMonitorSurfaceCopy(derived({ state: "overdue", nextCheckAt: NOW.toISOString() }),
|
||||
NOW, null, 0, "blocked", true);
|
||||
expect(copy!.bannerTitle).toBe("Automatic checks paused");
|
||||
expect(copy!.executionWait).toBeUndefined();
|
||||
expect(copy!.checkUnavailable).toBe(false);
|
||||
});
|
||||
|
||||
it("switches copy for due-now and overdue states", () => {
|
||||
const dueNow = buildMonitorSurfaceCopy(
|
||||
derived({ state: "due-now", nextCheckAt: NOW.toISOString(), attemptCount: 1 }),
|
||||
@@ -146,6 +154,27 @@ describe("IssueMonitorBanner / IssueMonitorComposerStrip rendering", () => {
|
||||
} as unknown as Issue;
|
||||
}
|
||||
|
||||
it.each([-120_000, 0, 120_000])("explains the active run hold on both monitor surfaces (offset: %s)", offset => {
|
||||
const issue = { ...issueWithMonitor(new Date(NOW.getTime() + offset).toISOString()), status: "in_progress" as const };
|
||||
const onCheckNow = vi.fn();
|
||||
const root = createRoot(container);
|
||||
const render = (hasActiveRun: boolean) => flushSync(() => root.render(<>
|
||||
<IssueMonitorBanner issue={issue} hasActiveRun={hasActiveRun} onCheckNow={onCheckNow} />
|
||||
<IssueMonitorComposerStrip issue={issue} hasActiveRun={hasActiveRun} onCheckNow={onCheckNow} />
|
||||
</>));
|
||||
try {
|
||||
render(true);
|
||||
expect(container.textContent).toContain("Monitor waits for current run");
|
||||
expect(container.textContent).toContain("The check can start after the current run finishes saving its work.");
|
||||
expect(container.textContent).not.toMatch(/overdue|next tick|Checking momentarily|wakes the agent now/i);
|
||||
expect(Array.from(container.querySelectorAll("button")).some(button => button.textContent === "Check now")).toBe(false);
|
||||
render(false);
|
||||
expect(container.textContent).not.toContain("Monitor waits for current run");
|
||||
expect(Array.from(container.querySelectorAll("button")).filter(button => button.textContent === "Check now")).toHaveLength(2);
|
||||
if (offset < 0) expect(container.textContent).toContain("overdue by 2m");
|
||||
} finally { flushSync(() => root.unmount()); }
|
||||
});
|
||||
|
||||
it("requires confirmation, keeps the monitor on dismissal, and reports cancellation errors", async () => {
|
||||
const issue = issueWithMonitor(new Date(NOW.getTime() + 60_000).toISOString());
|
||||
const onCancel = vi.fn().mockRejectedValueOnce(new Error("Unable to save monitor")).mockResolvedValueOnce(undefined);
|
||||
|
||||
@@ -67,6 +67,7 @@ export interface MonitorSurfaceCopy {
|
||||
pullRequestReview?: boolean;
|
||||
automaticChecksPaused?: boolean;
|
||||
checkUnavailable?: boolean;
|
||||
executionWait?: boolean;
|
||||
}
|
||||
|
||||
function capitalize(value: string): string {
|
||||
@@ -84,6 +85,7 @@ export function buildMonitorSurfaceCopy(
|
||||
scheduledRetryReason?: string | null,
|
||||
pendingPullRequestReviews = 0,
|
||||
issueStatus?: string,
|
||||
hasActiveRun = false,
|
||||
): MonitorSurfaceCopy | null {
|
||||
if (!isWaitingMonitorState(derived.state) || !derived.nextCheckAt) return null;
|
||||
|
||||
@@ -116,6 +118,18 @@ export function buildMonitorSurfaceCopy(
|
||||
|
||||
const eta = formatMonitorEta(derived.nextCheckAt, now); // "in 2h 12m" | "due now" | "overdue by 18m"
|
||||
const absolute = formatMonitorAbsolute(derived.nextCheckAt, {}, now); // local time, e.g. "Today, 4:08 PM"
|
||||
if (derived.source === "monitor" && hasActiveRun) {
|
||||
const hint = "The check can start after the current run finishes saving its work.";
|
||||
return {
|
||||
bannerTitle: "Monitor waits for current run",
|
||||
stripTitle: "Monitor waits for current run",
|
||||
bannerMeta: [hint, `Scheduled for ${absolute} (your time)`],
|
||||
stripMeta: [hint, `Scheduled for ${absolute}`],
|
||||
tone: "info",
|
||||
executionWait: true,
|
||||
checkUnavailable: true,
|
||||
};
|
||||
}
|
||||
if (derived.source === "monitor" && derived.serviceName?.toLowerCase() === "github" && pendingPullRequestReviews > 0) {
|
||||
return {
|
||||
bannerTitle: "Pull request review requested",
|
||||
@@ -179,7 +193,7 @@ export function buildMonitorSurfaceCopy(
|
||||
};
|
||||
}
|
||||
|
||||
function useMonitorSurfaceCopy(issue: Issue, pendingPullRequestReviews: number): MonitorSurfaceCopy | null {
|
||||
function useMonitorSurfaceCopy(issue: Issue, pendingPullRequestReviews: number, hasActiveRun: boolean): MonitorSurfaceCopy | null {
|
||||
// `nextCheckAt` is stable for a given issue; derive once to seed the ticking
|
||||
// countdown cadence, then re-derive against the live clock so the surfaces
|
||||
// roll scheduled → due → overdue on their own.
|
||||
@@ -188,7 +202,7 @@ function useMonitorSurfaceCopy(issue: Issue, pendingPullRequestReviews: number):
|
||||
return derived.source === "monitor" && areAutomaticMonitorChecksPaused(issue) ? null : derived.nextCheckAt;
|
||||
}, [issue]);
|
||||
const now = useMonitorCountdown(nextCheckAt);
|
||||
return useMemo(() => buildMonitorSurfaceCopy(deriveMonitorState(issue, now), now, issue.scheduledRetry?.scheduledRetryReason, pendingPullRequestReviews, issue.status), [issue, now, pendingPullRequestReviews]);
|
||||
return useMemo(() => buildMonitorSurfaceCopy(deriveMonitorState(issue, now), now, issue.scheduledRetry?.scheduledRetryReason, pendingPullRequestReviews, issue.status, hasActiveRun), [issue, now, pendingPullRequestReviews, hasActiveRun]);
|
||||
}
|
||||
|
||||
function CheckNowButton({
|
||||
@@ -254,6 +268,7 @@ function CancelMonitorButton({ onCancel }: { onCancel: () => Promise<unknown> })
|
||||
|
||||
export interface IssueMonitorSurfaceProps {
|
||||
issue: Issue;
|
||||
hasActiveRun?: boolean;
|
||||
onCheckNow?: (() => void) | null;
|
||||
checkingNow?: boolean;
|
||||
workProducts?: IssueWorkProduct[] | null;
|
||||
@@ -268,6 +283,7 @@ export interface IssueMonitorSurfaceProps {
|
||||
*/
|
||||
export function IssueMonitorBanner({
|
||||
issue,
|
||||
hasActiveRun = false,
|
||||
onCheckNow = null,
|
||||
checkingNow = false,
|
||||
workProducts,
|
||||
@@ -275,7 +291,7 @@ export function IssueMonitorBanner({
|
||||
onCancelMonitor,
|
||||
}: IssueMonitorSurfaceProps) {
|
||||
const reviews = getIssuePullRequests(workProducts).filter(pullRequestNeedsReview);
|
||||
const copy = useMonitorSurfaceCopy(issue, reviews.length);
|
||||
const copy = useMonitorSurfaceCopy(issue, reviews.length, hasActiveRun);
|
||||
if (!copy) return null;
|
||||
|
||||
const canCancel = onCancelMonitor && deriveMonitorState(issue).source === "monitor";
|
||||
@@ -305,6 +321,7 @@ export function IssueMonitorBanner({
|
||||
*/
|
||||
export function IssueMonitorComposerStrip({
|
||||
issue,
|
||||
hasActiveRun = false,
|
||||
onCheckNow = null,
|
||||
checkingNow = false,
|
||||
workProducts,
|
||||
@@ -312,7 +329,7 @@ export function IssueMonitorComposerStrip({
|
||||
className,
|
||||
}: IssueMonitorSurfaceProps & { className?: string }) {
|
||||
const reviews = getIssuePullRequests(workProducts).filter(pullRequestNeedsReview);
|
||||
const copy = useMonitorSurfaceCopy(issue, reviews.length);
|
||||
const copy = useMonitorSurfaceCopy(issue, reviews.length, hasActiveRun);
|
||||
if (!copy) return null;
|
||||
|
||||
return (
|
||||
@@ -336,7 +353,7 @@ export function IssueMonitorComposerStrip({
|
||||
<p className="mt-1.5 text-xs text-muted-foreground">
|
||||
{copy.pullRequestReview
|
||||
? "Review on GitHub, then check status to have the agent verify the result."
|
||||
: copy.workspaceWait
|
||||
: copy.workspaceWait || copy.executionWait
|
||||
? "You can keep sending instructions while the agent waits."
|
||||
: "Sending a reply wakes the agent now — before the scheduled check."}
|
||||
</p>
|
||||
|
||||
@@ -8048,6 +8048,7 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS
|
||||
|
||||
<IssueMonitorBanner
|
||||
key={issue.id}
|
||||
hasActiveRun={hasLiveRuns}
|
||||
onCancelMonitor={() => cancelIssueMonitor.mutateAsync()}
|
||||
issue={issue}
|
||||
workProducts={workProducts}
|
||||
@@ -8629,6 +8630,7 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS
|
||||
hasVisibleMonitorSurface(issue) ? (
|
||||
<IssueMonitorComposerStrip
|
||||
issue={issue}
|
||||
hasActiveRun={hasLiveRuns}
|
||||
workProducts={workProducts}
|
||||
checkError={checkIssueMonitorNow.error ? describeError(checkIssueMonitorNow.error, { action: "check the monitor" }).body : null}
|
||||
onCheckNow={() => checkIssueMonitorNow.mutate()}
|
||||
|
||||
Reference in new issue
Block a user