From 1bbfc6959036699b780f12f8ed9b8fc47b41c7c4 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 24 Oct 2026 14:00:17 -0700 Subject: [PATCH] fix(server): retain full target scans with scoped transaction hints Co-Authored-By: Paperclip --- doc/idle-decision-targets.md | 15 +++++++++++++++ ...2026-10-24-recurring-server-sleep-stack.md | 19 +++++++++++++++++++ .../__tests__/decision-expiry-work.test.ts | 10 +++++++--- .../__tests__/decision-target-work.test.ts | 14 ++++++++++++++ server/src/services/decision-target-work.ts | 2 +- 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/doc/idle-decision-targets.md b/doc/idle-decision-targets.md index 5883ed0f9b4..e68eb0abe8a 100644 --- a/doc/idle-decision-targets.md +++ b/doc/idle-decision-targets.md @@ -107,3 +107,18 @@ A local query probe with 40,000 completed decisions, one open decision, and 40,0 target links used existing indexes: scoped lookup read five buffers (0.012 ms), full open-page lookup two (0.003 ms). The probe was rolled back. No new schema index is needed for this change. Full PR CI and fresh review remain required. + +## Review and CI repair + +Review found that an absent deletion hint was dropped when the outer transaction +also carried a scoped hint. Both delete-before-update and update-before-delete +regressions reproduced the missing expiry after the target links cascaded away. +Deletion now sends the explicit `all` hint, which survives alongside scoped hints. +The repaired target suite passes 20 cases; the TTL suite passes 14 cases. + +The initial CI run failed the TTL worker's drain assertion. It measured SQL +before already-admitted sweeps had finished. The test now joins those specific +sweep promises before measuring passive SQL; unresolved-intent holds remain +intact. Waiting for the entire idle count to reach zero was tested and rejected: +that count correctly includes the retained recovery holds. No production drain +or retry behavior changed for this test repair. diff --git a/doc/plans/2026-10-24-recurring-server-sleep-stack.md b/doc/plans/2026-10-24-recurring-server-sleep-stack.md index 856149cc62d..41013294a78 100644 --- a/doc/plans/2026-10-24-recurring-server-sleep-stack.md +++ b/doc/plans/2026-10-24-recurring-server-sleep-stack.md @@ -602,3 +602,22 @@ run hit a PostgreSQL lock timeout, and the following test saw the experimental flag left enabled. No CI retry has been used. Reproduce the two tests on the exact parent head before classifying a retry or repair. This parent is not yet claimed green. The full original inventory remains in scope. + +Slice 11 #17482 now has all checks passing at `a5b5994c1d`, Greptile 5/5, +no unresolved findings, and no merge conflict. Its one browser-shard retry passed +after both affected cases passed locally on the exact head (2/2). The original +fixture-cleanup lock timeout remains documented in the PR. + +Slice 12 is #17486 at initial head `5062efb368`. Review found that deletion's +absent hint could be dropped when the same outer transaction included a scoped +hint. Both operation orders failed new regressions before the explicit `all` +hint repair. The repaired target/TTL selection passes 34 cases. CI also exposed +a TTL test race: passive-SQL measurement now joins already-admitted sweeps, +without dropping unresolved recovery holds. Fresh CI/review remain required. + +The fresh master audit at `991c47bc6e` identified an additional recurring-server +owner: the Grok routine dispatcher starts a one-second settings/dispatch scan +when public MCP is configured, even when Grok is disabled. Include this in the +full stack as slice 13, separately from decision retention. The disabled baseline +issued three settings queries in 3.2 seconds. Its durable prepared dispatches, +atomic claim, and existing uncertain-outcome policy must remain authoritative. diff --git a/server/src/__tests__/decision-expiry-work.test.ts b/server/src/__tests__/decision-expiry-work.test.ts index aa9ab4c6813..72801d2e4fc 100644 --- a/server/src/__tests__/decision-expiry-work.test.ts +++ b/server/src/__tests__/decision-expiry-work.test.ts @@ -59,8 +59,10 @@ const support = externalUrl ? { supported: true } : await getEmbeddedPostgresTes async function start() { coordinator = createDeliveryWorkCoordinator({ owner: db, canRun: () => !draining, canReconcile: () => true, onError: error => errors.push(error) }); - await registerDecisionContinuationWork(coordinator, service(), () => !draining).ready; - await registerDecisionExpiryWork(coordinator, service(), () => !draining).ready; + const continuation = service(), expiry = service(); + await registerDecisionContinuationWork(coordinator, continuation, () => !draining).ready; + await registerDecisionExpiryWork(coordinator, expiry, () => !draining).ready; + return { continuation, expiry }; } async function quiet() { await vi.waitFor(async () => { @@ -151,12 +153,14 @@ const support = externalUrl ? { supported: true } : await getEmbeddedPostgresTes }); it("keeps a failed expiry retry through drain and preserves pending state", async () => { - const row = await create(new Date(Date.now() + 30_000)); await start(); + const row = await create(new Date(Date.now() + 30_000)); const workers = await start(); + const sweeps = [vi.spyOn(workers.expiry, "sweepTtlExpiry"), vi.spyOn(workers.continuation, "sweepContinuations")]; await db.execute(sql`create function decision_expiry_fail() returns trigger language plpgsql as $$ begin if NEW.status = 'expired' then raise exception 'expiry unavailable'; end if; return NEW; end $$`); await db.execute(sql`create trigger decision_expiry_fail before update on decisions for each row execute function decision_expiry_fail()`); await vi.waitFor(async () => { await vi.advanceTimersByTimeAsync(30_000); expect(errors.length).toBeGreaterThan(0); }); draining = true; expect((await service().get(row.id))?.status).toBe("open"); + await Promise.allSettled(sweeps.flatMap(sweep => sweep.mock.results.filter(result => result.type === "return").map(result => result.value))); const reads = vi.spyOn(db, "select"); await vi.advanceTimersByTimeAsync(60_000); expect(reads).not.toHaveBeenCalled(); reads.mockRestore(); await db.execute(sql`drop trigger decision_expiry_fail on decisions`); diff --git a/server/src/__tests__/decision-target-work.test.ts b/server/src/__tests__/decision-target-work.test.ts index d6df1c41b2b..a4e443c8e6d 100644 --- a/server/src/__tests__/decision-target-work.test.ts +++ b/server/src/__tests__/decision-target-work.test.ts @@ -165,6 +165,20 @@ const support = externalUrl ? { supported: true } : await getEmbeddedPostgresTes await expired(row.id); await quiet(); expect(wake).toHaveBeenCalledTimes(1); }); + it.each([true, false])("preserves a deletion mixed with a scoped hint in one transaction, deleteFirst=%s", async deleteFirst => { + const row = await create(); const otherId = randomUUID(); + await db.insert(issues).values({ id: otherId, companyId, title: "Unrelated", status: "todo", responsibleUserId: userId }); + await start(); await quiet(); + await db.transaction(async tx => { + const scoped = issueService(tx as unknown as typeof db); + if (deleteFirst) await scoped.remove(targetId); + await scoped.update(otherId, { status: "done", actorUserId: userId }); + if (!deleteFirst) await scoped.remove(targetId); + }); + expect(await db.select().from(decisionTargetIssues).where(eq(decisionTargetIssues.decisionId, row.id))).toHaveLength(0); + await expired(row.id); await quiet(); expect(wake).toHaveBeenCalledTimes(1); + }); + it("retains a first target change through drain without passive SQL", async () => { const row = await create(); await start(); await quiet(); draining = true; await change("cancelled"); const reads = vi.spyOn(db, "select"); await vi.advanceTimersByTimeAsync(120_000); diff --git a/server/src/services/decision-target-work.ts b/server/src/services/decision-target-work.ts index e3ae6fee108..44e2e6175dc 100644 --- a/server/src/services/decision-target-work.ts +++ b/server/src/services/decision-target-work.ts @@ -3,7 +3,7 @@ import { DELIVERY_QUEUES, notifyDeliveryWork } from "./delivery-work-notificatio import type { decisionService } from "./decisions.js"; export function notifyDecisionTargetWork(transaction: object, hint?: `issue:${string}` | `decision:${string}`) { - return notifyDeliveryWork(transaction, DELIVERY_QUEUES.decisionTargets, hint); + return notifyDeliveryWork(transaction, DELIVERY_QUEUES.decisionTargets, hint ?? "all"); } export function registerDecisionTargetWork(