Skip to content

fix(queue): fence job claims and drain in-flight work on shutdown - #224

Open
jerelvelarde wants to merge 7 commits into
mainfrom
jerel/cpk-7925-worker-poll-loop
Open

fix(queue): fence job claims and drain in-flight work on shutdown#224
jerelvelarde wants to merge 7 commits into
mainfrom
jerel/cpk-7925-worker-poll-loop

Conversation

@jerelvelarde

@jerelvelarde jerelvelarde commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Third split of #187 — worker correctness. generator.ts went out separately as #223; /health and railway.toml are deliberately not here (see the last section, it's the important one).

Advances #181, and half of #153: the fence guarantees one row's writes belong to one execution. It does nothing about two different AI_RESPONSE rows for the same ticket — different ids, different tokens, both proceed. That half is #191's Message.ticketId_responseKey constraint and the responseState machine, not this.

Claims are now fenced

updateJobProgress takes a claimToken and writes through updateMany gated on status: 'PROCESSING' and a matching token, backed by a new nullable Job.claimToken column.

The old prisma.job.update({ where: { id } }) had no way to distinguish the live claim from a stale execution — and stale executions genuinely exist here: a job whose lock went stale gets reclaimed, while the original handler is still running, because a timeout race in this codebase does not cancel the handler it raced. That older execution could keep writing progress over the new claim's. Now it writes zero rows.

Also in scope:

  • Reclaim against each type's own timeout rather than one global value, so a 300s type isn't reclaimed on a 120s clock.
  • A recovery grace before crash-abandoned claims count toward dead-letter, so a worker restart doesn't burn a job's attempts.
  • Shutdown that actually drains — one shared drain across repeated stop() calls, and it waits for an in-flight poll rather than racing it.

queue.test.ts is included, and the split derivation didn't predict that

Worth flagging, because it's a hole in how the third PR's scope was derived. queue.test.ts was excluded from the file list on the grounds that it "also appears in #191" — but the two changes are unrelated:

Without them the suite fails 11 tests on prisma.$executeRaw is not a function — the new reclaimStaleJobs issues raw SQL the existing prisma mock never had to provide. So "appears in another split" wasn't sufficient grounds to exclude it; the file needed the union, not either version.

The five tests it adds are the ones that make the above claims falsifiable rather than asserted: stale-reclaim per type timeout, old execution not clobbering a reclaimed claim, crash-abandoned claims respecting the grace, drain shared across repeated stops, and no claiming after shutdown begins.

⚠️ Ordering vs #191: this branch is cut from main and therefore holds 6 / 10 for those two assertions. If #191 lands first, this needs those two lines bumped to 7 / 11 on rebase. Two lines, but it will conflict rather than merge cleanly, so it's worth knowing in advance.

Why /health is deferred a third time

I went in expecting to include it and changed my mind on the evidence.

The stated condition for landing the /health rework was that it "cannot land until the worker poll loop is fixed, since the poll loop is what makes it wrong" — four reviewers having found it makes a healthy worker report 503 and get restarted mid-job. That condition is not met by this branch.

claimJobsByType still does await Promise.allSettled(...) over each type's batch, so poll() blocks for the full duration of the work it dispatched and lastPollTime stops advancing. buildHealthResponse compares that against STALE_POLL_MS (60s) with no exemption for active jobs, and AI_RESPONSE carries a 120s timeout.

So a worker doing exactly what it is supposed to be doing would answer 503 "stalled" after 60 seconds, and Railway would restart it mid-job. The failure mode the reviewers described is still live.

Closing it needs one of two changes, neither of which belongs in a PR about claim fencing:

  1. dispatch without awaiting the batch (which is also what makes per-type concurrency limits actually bind), or
  2. exempt active work from the stale-poll check.

#182 and #184 therefore stay open. Separately, health.ts wires through apps/worker/src/index.ts, which #191 already modifies — so even a correct /health would want to follow #191.

Verification

packages/outpost: 62 test files, 1087 tests passing.

Every blocker from review is pinned by a mutation that used to pass:

Mutation Before After
reclaim back to runAt = NOW() passed fails 1
reclaim back to the observing worker's config passed fails 3
drop lockUntil from the claim sites n/a fails 1
both count > 0count >= 0 passed fails 4

Not verified: no Docker or local Postgres in this environment, so the reclaim
and claim SQL is unexecuted. Tests mock prisma, so CI doesn't run it either —
migrate deploy exercises the migration and the drift gate, but the queries
first run for real in staging.

The limit of a database fence

The fence is a DB guard, so in an interleaving where a live claim gets reclaimed,
the first execution's external side effects have already happened. lockUntil
makes that interleaving much rarer, but rarer is not impossible.

AI_RESPONSE is covered by #191's response-key constraint. ESCALATION,
HUBSPOT_SYNC and TRACKER_SYNC are not, and this PR is what introduces
automatic re-execution — so they need their own idempotency keys. Filed rather
than fixed here.

Local note: a fresh worktree needs pnpm install and npx prisma generate --schema=db/prisma/schema.prisma, or scheduler.test.ts fails to load on .prisma/client/default and looks like a real failure.


Migration hazards — read before pulling this branch

If you ran migrate dev while sitting on this branch, you have a stale migration applied. The folder was renamed 20260811220000_add_job_claim_token20260820120000_add_job_claim_fencing (the old stamp sorted before four already-applied migrations). migrate deploy/migrate dev will fail on any database that recorded the old name. Reset your local database, or prisma migrate resolve --rolled-back 20260811220000_add_job_claim_token. Ephemeral CI databases are unaffected.

The index now lives in its own migration. 20260820120100_index_job_status_lockuntil. Prisma wraps each migration file in one transaction, so keeping CREATE INDEX alongside the ALTER TABLE ADD COLUMN held that statement's ACCESS EXCLUSIVE lock across the index build and blocked reads for the duration. Split, the ALTER commits first and the index build takes only SHARE: writes wait, reads do not. Deliberately no lock_timeout — a statement that times out aborts the migration into the finished_at = NULL state apps/worker/start.sh exists to diagnose, and waiting is the more recoverable failure of the two.

Worth checking the staging Job row count before this promotes to prod. CONCURRENTLY is impossible inside Prisma's transaction, so the build is a plain one. Sub-second on a table JOB_CLEANUP keeps small; a queue stall if Job is ever large.

What only staging can settle

The reclaim and claim SQL is still unexecuted — the tests mock prisma, so CI does not run it either. migrate deploy will exercise the migrations and the drift gate, but the queries themselves first run for real in staging. Specifically unverified: whether the disjunction's planner actually uses Job_status_lockUntil_idx, whether (jsonb ->> "Job".type)::double precision and <param> * INTERVAL '1 millisecond' resolve under Prisma's parameter typing, and LEAST(<float8>, $n) against a Prisma-typed integer parameter.

Third split of #187, worker correctness only. `/health` and railway.toml are
deliberately NOT here — see below.

A claim is now fenced by `Job.claimToken`. `updateJobProgress` takes the token
and writes through `updateMany` gated on `status: 'PROCESSING'` plus a matching
token, so a stale execution — one whose job was reclaimed after its lock went
stale, and which is still running because a timeout race does not cancel the
handler — can no longer clobber the live claim's progress. The old
`prisma.job.update({ where: { id } })` had no way to tell the two apart.

Also here: reclaiming stale PROCESSING jobs against each type's own timeout
rather than one global value, a recovery grace before crash-abandoned claims
count toward dead-letter, and a shutdown that shares one drain across repeated
stop calls and waits for an in-flight poll instead of racing it.

`queue.test.ts` is included, which the split derivation did not predict. It was
excluded from the third PR's file list because it also appears in #191 — but
#191's edit is only the job-type counts, while the worker tests this branch
needs live in #187's version of the same file. Without them the suite fails 11
tests on `prisma.$executeRaw is not a function`: the new `reclaimStaleJobs`
issues raw SQL that the existing prisma mock never had to provide.

WHY /health IS STILL DEFERRED, a third time:

Four reviewers previously found the /health rework makes a healthy worker
report 503 and get restarted mid-job, and the stated condition for landing it
was "the poll loop is fixed, since the poll loop is what makes it wrong". That
condition is NOT met by this branch. `claimJobsByType` still awaits
`Promise.allSettled` over each type's batch, so `poll()` blocks for the full
duration of the work it dispatched and `lastPollTime` does not advance.
`buildHealthResponse` compares it against `STALE_POLL_MS` (60s) with no
exemption for active jobs, and AI_RESPONSE has a 120s timeout — so a worker
doing exactly what it should would answer 503 "stalled" after 60s and Railway
would restart it mid-job.

Closing that needs one of two changes, neither of which belongs in this PR:
dispatch without awaiting the batch, or exempt active work from the stale-poll
check. #182 and #184 stay open. `health.ts` also wires through
apps/worker/src/index.ts, which #191 already modifies.

Advances #153, #181

Verified: packages/outpost — 61 files, 1017 tests pass.
@linear-code

linear-code Bot commented Aug 19, 2026

Copy link
Copy Markdown
CPK-7925 3. Open + merge the third split PR — worker correctness (unblocks response quality)

The third of the three PRs split out of #187. Not yet opened#191's description names it as "third (worker correctness) to follow."

Scope — derived from the files #187 carries that neither split does

apps/worker/railway.toml
apps/worker/src/health.ts
apps/worker/src/__tests__/health.test.ts
packages/outpost/queue/src/worker.ts
packages/outpost/queue/src/create-job.ts
packages/outpost/queue/src/__tests__/worker-concurrency.test.ts
packages/outpost/db/prisma/migrations/20260811220000_add_job_claim_token/migration.sql
packages/outpost/ai/src/generator.ts
packages/outpost/ai/src/generator.test.ts

Roadmap issues it should close or advance

  • #181shutdown() does not drain in-flight jobs
  • #182/health reports ok for a stopped or wedged worker
  • #184 — worker PORT/HEALTH_PORT precedence inverted (railway.toml + health.ts)
  • #153 — per-type claiming serializes job types (worker.ts, create-job.ts, the job-claim-token migration)

Worth confirming that list against the actual diff when the PR is opened rather than assuming it — the file overlap is strong evidence of scope, not a promise of what the code does.

Why this is the response-quality gate

packages/outpost/ai/src/generator.ts is in this set and in no other open PR. Three response-quality issues live in that file:

  • #149temperature sent unconditionally (generator.ts:117 and :185)
  • #146 — the streaming path skips the groundedness gate
  • #178 — the generator's empty-response behaviour

Until this lands, work on any of the three either conflicts with this branch or gets rewritten by it.

One thing to decide when opening it: whether the generator.ts changes belong here at all. They are the only non-worker files in the set. If they can be dropped or landed separately, three response-quality issues unblock immediately instead of waiting on worker correctness.

CPK-7982 Review PR #224 — fence job claims and drain in-flight work on shutdown

CopilotKit/outpost#224 by jerelvelarde, opened 2026-08-19 12:48Z off main. +452/-104 across 6 files — the largest of the three. CI green (Lint/Typecheck/Test 5m7s, zizmor). MERGEABLE / BLOCKED on review. Branch jerel/cpk-7925-worker-poll-loop.

232+ 23-  packages/outpost/queue/src/__tests__/queue.test.ts
171+ 75-  packages/outpost/queue/src/worker.ts
 38+  2-  packages/outpost/queue/src/__tests__/worker-concurrency.test.ts
  8+  4-  packages/outpost/queue/src/create-job.ts
  2+  0-  .../migrations/20260811220000_add_job_claim_token/migration.sql
  1+  0-  packages/outpost/db/prisma/schema.prisma

This is the worker half of the CPK-7925 split (#223 is the generator half). It should close or advance #181 (shutdown() does not drain in-flight jobs) and #153 (per-type claiming serializes job types) — worth confirming against the actual diff rather than assuming from the title, and adding the Closes #… lines if they hold, since none are present.

⚠️ Conflict risk with PR #191 — check before merging

#224 and #191 share two files:

  • packages/outpost/db/prisma/schema.prisma
  • packages/outpost/queue/src/__tests__/queue.test.ts

#224 also adds the 20260811220000_add_job_claim_token migration, which came from #187's remainder — the same pre-split branch #191 was carved out of. Both are currently based on main and both report MERGEABLE, so whichever lands first forces a rebase on the other. The queue.test.ts overlap is the one to watch: #191 adds substantial tests to that file too (232 new lines here, and #191 rewrites the AI-response side), so a textual merge could succeed while producing a semantically incoherent test file.

Recommend deciding the order explicitly rather than merging on whichever is approved first. CPK-7924 (#191) is Urgent and closes two live bugs (#175, #176), so it likely goes first — which makes this a rebase-then-review, not review-then-rebase.

Where to look

  • Fencing — a claim token in the DB is the right mechanism, but the value is entirely in whether the compare-and-set is genuinely atomic and whether every state transition is guarded on it. A blind update by id anywhere defeats the whole thing.
  • Drain — confirm in-flight jobs actually complete rather than the process merely waiting a fixed interval, and that a job which outlives the drain window is left in a recoverable state, not stranded PROCESSING (which is the defect worker: shutdown() does not actually drain in-flight jobs #181 exists for).
  • create-job.ts at +8/-4 is small but sits on the enqueue path everything depends on.

A cr-loop is queued for this PR. Highest priority of the three: it is the largest, touches the schema, and has a live conflict with an Urgent PR.

Review in Linear

jerelvelarde and others added 2 commits August 19, 2026 05:53
The migration added the column; schema.prisma never declared it, so the
migrations no longer reproduce schema.prisma from scratch and CI's drift gate
correctly failed with `[-] Removed column claimToken` (exit 2).

Exactly the class of defect that gate exists to catch — a migration whose
effect is real but invisible to the schema — so it working here is the system
behaving as designed, not an obstacle.

Caught only in CI because the check needs a live Postgres, which this
environment has neither Docker nor a local server for. `prisma validate` and
the full package suite both pass locally and neither can see this.
@NathanTarbert

Copy link
Copy Markdown
Collaborator

Hey @jerelvelarde,

I deleted my two earlier comments on this PR and replaced them with this one. They were long, they buried the actual blockers, and a few of my claims turned out to be wrong. I re-ran the review with mutation testing enforced (delete the fix, confirm the test fails) and it changed several answers, including retracting some of mine. Everything below is either executed or explicitly marked as reasoning.

Verdict: NEEDS CHANGES. Three blockers. Everything else here is either approved or filed as non-blocking, so you can ignore the rest until the three are in.


Approved: don't change these

The claim-fencing mechanism is correct, and it's the hard part of this PR. gen_random_uuid()::text on claim, RETURNING hands the worker its token, and all five mutation sites are genuine compare-and-sets on (id, status='PROCESSING', claimToken), progress updates included, which is the easy one to forget.

The stopPromise fix is real and correct. Worker.start() registers its own SIGTERM handler at worker.ts:149-158 before the app's at apps/worker/src/index.ts:138. Pre-PR the second, awaited call hit if (!this.running) return and resolved instantly, so prisma.$disconnect() and process.exit(0) ran mid-job. Sharing the drain closes that. Confirmed covered: removing the stopPromise guard fails shares the active-job drain across repeated stop calls.

The migration is safe and additive. Nullable with no backfill is the right call (see the retraction below). It must deploy ahead of the code, and it's a no-op for the running fleet, so that ordering is free.


Blockers

B1. Reclaim resets the retry backoff

worker.ts:252-255 sets "runAt" = NOW() on the requeue branch, while handleFailure at worker.ts:511 uses calculateBackoff(attempt). Two paths that both mean "this attempt failed, retry it", disagreeing.

Walk it: W1 claims a HUBSPOT_SYNC job (timeout 300s), gets SIGKILLed at T=10s on Railway grace expiry. At T=330s W2's reclaim sets it PENDING, attempts=1, runAt=NOW(). W2's next poll, under a second later, claims it and runs the same 5-minute sync against the same failing upstream. Killed again, reclaimed again, retried immediately. All five attempts are gone inside about 25 minutes with zero spacing. A crash-looping worker drives every job to DEAD_LETTER at full speed.

Fix: "runAt" = NOW() + (backoff_ms * INTERVAL '1 millisecond') with the backoff computed per attempts+1 in the policy recordset, or at minimum a fixed floor delay.

B2. The reclaim window comes from the wrong worker

reclaim_after_ms is derived from this.jobTimeouts[type] ?? this.defaultTimeoutMs at worker.ts:238-241, which is the observing process's config. Nothing on the Job row records the timeout the claim was actually granted under.

Rolling deploy, two replicas: W_new has jobTimeouts[HUBSPOT_SYNC] = 300_000 (apps/worker/src/index.ts:78) and claims the job at T=0. W_old is the previous revision, still up for the overlap, without that entry, so its window is defaultTimeoutMs at 60s. At T=60 W_old's reclaim sets the row PENDING, attempts=1, claimToken=NULL, progress=NULL and someone re-claims it. At T=250 W_new's original handler finishes successfully, its updateMany matches zero rows, and the completed work is silently discarded while the job runs a second time. Same failure on any config skew, or if defaultTimeoutMs is ever lowered.

Fix: persist the granted deadline at claim time ("lockUntil" = NOW() + <timeout for this type>) and have the reclaim compare lockUntil < NOW(). That gives the fence and the reclaim one authority instead of two.

B3. A fence rejection is silent on every path

worker.ts:496, worker.ts:518, worker.ts:410-424, create-job.ts:36-39. The fenced writes either don't read count or read it only to suppress a log. count === 0 is the exact event this whole PR exists to produce, and it emits nothing: no log, no counter, no health signal, on any of the four paths.

Two consequences. In B2's interleaving a job completed, the row says PENDING, the work re-executes, and the only trace is a COMPLETED write that vanished, so nobody can tell it's happening. And it inverts prior behavior: handleFailure always logged before this PR, so a fenced retry or dead-letter now loses the failure that caused the timeout in the first place.

Fix: if (result.count === 0) console.warn(...) on all four sites naming the job id and token, and read count on the success path.

Mutation confirming this is untested: changing both count > 0 checks to count >= 0 passes 219/219.


Non-blocking, your call on timing

  • worker.ts:238-241, :260 Reclaim only recovers types the reclaiming worker registers, since policies is built from this.handlers.keys() and the SQL joins on it. A crashed claim of a type this worker doesn't register stays PROCESSING forever, never reclaimed, never dead-lettered, invisible to healthCheck(). No reclaim existed before, so not a regression. A fallback policy row or an absolute-ceiling sweep with no type predicate would close it.
  • worker.ts:184-191 Reclaim failure stops all claiming, and health stays green. await this.reclaimStaleJobs() at :193 sits ahead of every claim in the same try, and lastPollTime is set at :184 before it, so the stale-poll check in buildHealthResponse stays satisfied while nothing is claimed. Deploy the code ahead of the migration and this is a total outage reporting healthy, one log line per second, no restart. Worth its own try/catch so reclaim can't gate claiming.
  • worker.ts:185, :284 The concurrency accounting is unreachable. poll() fully awaits Promise.allSettled over each batch (:309, :363) and schedulePoll only runs after the poll resolves, so activeJobs.size and activeJobsByType are always 0 when availableSlots and typeLimit - activeForType are computed. Both expressions are dead: mutating each to ignore in-flight work passes 219/219. It does not over-fetch, the serialization accidentally bounds it, but per-type limits never bind. On Worker: per-type claiming serializes job types, and per-type limits oversubscribe maxConcurrency #153: the fence protects one row's writes and does nothing about two different AI_RESPONSE rows for the same ticket, which have different ids and different tokens and both proceed. I'd narrow the PR body's "advances Worker: per-type claiming serializes job types, and per-type limits oversubscribe maxConcurrency #153" to say which half, and land the not-awaiting-the-batch change before the accounting means anything.
  • worker.ts:384-395 An unregistered type is still terminally destroyed: FAILED with completedAt and no attempt increment, and job-cleanup.ts:29-45 only deletes COMPLETED and DEAD_LETTER, so the rows accumulate. This PR added lockedAt: null, claimToken: null to that write, which means reclaim can't rescue them either. Reachable only via claimAndProcessJobs, and production sets concurrencyByType so it takes the type-filtered claimJobsByType path. Latent today, live the moment concurrencyByType is emptied or a worker ships without the full handler set. A no-op release back to PENDING with no attempt consumed is the safer failure.
  • worker.ts:463-474, :100-127 The timeout doesn't cancel the handler, it stops waiting for it, and the drain has no deadline. AI_RESPONSE at 120s against Railway's ~30s SIGTERM grace means the graceful drain resolves after SIGKILL already landed, so the real shutdown path is still crash-abandonment plus reclaim, which is why B1 and B2 matter. The fence makes that safe at the row level. That's the honest contribution here and it isn't cancellation. AbortSignal through JobHandlerContext is its own PR.
  • worker.ts:170 void currentPoll.finally(cb) builds a derived promise with no rejection handler. poll() can't reject today, so it's latent, but if a throw ever escapes it's an unhandled rejection under Node's default and stop()'s await this.pollPromise would propagate out, so apps/worker/src/index.ts:131 never reaches $disconnect().
  • create-job.ts:31-35 updateJobProgress is exported from queue/src/index.ts and gained a required third parameter. No out-of-package caller exists (only worker.ts:403), so nothing breaks, but it's a breaking signature change on a public symbol.
  • Migration: no index serves the reclaim's status = 'PROCESSING' AND type = ? AND "lockedAt" < ?, which runs every 1000ms per replica. Existing indexes are [status, runAt] (wrong second column) and [type] (low selectivity), and job-cleanup.ts never deletes FAILED at all, so the table isn't small. Suggest CREATE INDEX CONCURRENTLY "Job_status_lockedAt_idx" ON "Job" ("status", "lockedAt") WHERE "status" = 'PROCESSING', which needs its own non-transactional migration file. Separately, rename the folder: 20260811220000 sorts before six migrations already applied, so applied order stops matching lexicographic order. migrate deploy still applies it, but migrate dev against a dev DB that already has the later ones will want a reset.
  • Side effects outside the row. The fence is a DB guard, so in B2's interleaving the first execution's external effects already happened. AI_RESPONSE is covered by feat(queue): one AI answer per ticket, arbitrated by the database #191's Message.ticketId_responseKey constraint and the responseState machine, not by anything here. ESCALATION, HUBSPOT_SYNC and TRACKER_SYNC have no equivalent, and this PR is what introduces the automatic re-execution.

Tests: this is the part I'd fix first

Five of thirteen mutations survived, and every survivor lands on either the reclaim SQL's semantics or the code this PR edits.

Mutation Result
Delete AND job.type = policy.type from the reclaim survived 35/35
"attempts" = job."attempts" in the reclaim (never consumes an attempt, so it can never reach DEAD_LETTER) survived 35/35
Remove await this.pollPromise from stop() survived 35/35
Ignore in-flight work in availableSlots survived 219/219
Ignore in-flight work in the per-type count survived 219/219

The first two mean the reclaim's headline behavior, per-type timeouts, can be deleted with nothing failing. The third is the one I want to flag hardest, because I praised this line in my deleted comment: the test named waits for an in-flight poll and does not claim after shutdown begins passes with the await removed. Its only real assertion is that $queryRaw wasn't called, which the running=false guard already satisfies, and the stopped === false check passes on microtask ordering either way.

Root cause: $executeRaw is a vi.fn() and the reclaim SQL is never parsed or executed. The tests assert toContain(...) over the template-literal strings. So the CASE branches, the jsonb_to_recordset join, progress = NULL and the runAt reset are all unverified, and a syntax error there would ship green and surface as the silent dark queue above.

Suggested additions: assert runAt on the reclaim branch; a fake-timer test that a second claimJobsForType result for an id already in activeJobs is refused; a count: 0 mock asserting the warn fires; and for the drain, block on $queryRaw rather than $executeRaw and assert stop() is still unresolved after the claim resolves but before the handler does.


Retracting five things I said earlier

Worth being explicit, since you may have started on some of these:

  1. I said dropping claimToken from the dead-letter, retry and no-handler where clauses still passes. Wrong. Mutating the COMPLETED write fails 2 tests, and mutating both handleFailure writes fails 2 more. I never ran it.
  2. I called queue.test.ts:247 a strong adversarial race test. It never calls reclaimStaleJobs, it hand-mutates a fixture, and under the progress-fence mutation it doesn't fail, because the mock compares where.status !== persisted.status and undefined !== 'COMPLETED' accidentally re-fences.
  3. I said claimToken nullable with no backfill lets reclaim double-run in-flight jobs. Wrong. Nullable is correct, NULL means "no live claim", and existing PROCESSING rows are picked up correctly once their window elapses. No backfill needed.
  4. I flagged the missing IF NOT EXISTS. That's correct for Prisma. A re-run would mean the ledger is already wrong.
  5. I said unhandled types are being destroyed today. It's latent, not live, for the concurrencyByType reason above.

And I had two backwards on severity: the backoff reset and the config-skew window were filed as "smaller" and are the two blockers.


Evidence

vitest 4.1.4, single resolution from the lockfile, RUN v4.1.4 in the banner. pnpm install --frozen-lockfile clean in 8.3s on pnpm 10.33.4, then npx prisma generate --schema=db/prisma/schema.prisma, which your PR body correctly warns is required.

Base b11aafb at 1075 passing, head eb2450b2 at 1081, so +6 matching the five new worker tests plus the per-type reclaim test. npx tsc --project queue/tsconfig.json --noEmit clean. #191 is already an ancestor of the base, so the count conflict your body warns about is already resolved and no stale assertions remain.

Not verified, stated plainly: no Postgres or Docker in the review environment, so the reclaim SQL was never executed and prisma migrate diff never ran. The multi-worker interleavings in B1, B2 and the reclaim-scope item are read off the code and the SQL, not reproduced.


B1 and B3 are both small. Happy to take them as patches if you'd rather not context-switch, and B2 is the one worth talking through first since lockUntil changes the schema again.

Three review blockers.

B1 — reclaim reset the retry backoff. The requeue branch set `runAt = NOW()`
while `handleFailure` uses `calculateBackoff`, so two paths that both mean "this
attempt did not finish" disagreed. A worker SIGKILLed on Railway grace expiry got
its job reclaimed, re-claimed under a second later, killed again — five attempts
and DEAD_LETTER inside ~25 minutes with zero spacing, and a crash-looping worker
driving every job there at full speed. Reclaim now mirrors `calculateBackoff` in
SQL (base * 2^attempt plus jitter under a ceiling, on `attempts + 1` to match
`handleFailure`).

B2 — the reclaim window came from the wrong worker. `reclaim_after_ms` was
derived from the *observing* process's `jobTimeouts`, and nothing on the row
recorded the timeout the claim was granted under. Mid-rolling-deploy, a replica
without an entry for a long-running type falls back to `defaultTimeoutMs` and
reclaims a claim that is still live; the original handler then finishes, its
fenced write matches zero rows, and completed work is silently discarded while
the job runs a second time.

Adds `Job.lockUntil`, written at claim time from the claiming worker's own
config, looked up per-type from a jsonb map with the worker default as fallback.
Reclaim compares against that. The row is now self-describing, which also means
the type predicate is gone: a crashed claim of a type this replica does not
handle used to stay PROCESSING forever, never reclaimed and never dead-lettered.
Rows claimed before this deploys have a NULL `lockUntil` and fall to an absolute
15-minute ceiling — well above the largest configured timeout — rather than a
guessed deadline, since the guess is the bug being removed.

B3 — a fence rejection was silent on all four paths. `count === 0` is the event
this PR exists to produce and it emitted nothing; the two `count > 0` checks read
the count only to *suppress* a log, so a fenced retry or dead-letter also lost
the failure that caused the timeout. All four now warn, naming the job and the
token, and the retry/dead-letter paths carry the original error into the fence
warning.

Also, unprompted by a blocker but introduced by this PR: the reclaim sweep sits
ahead of every claim in the same try with `lastPollTime` already set, so a
failing sweep stopped all claiming while health still reported green — a total
outage with nothing to restart it. Now isolated in its own try/catch.

Migration: renamed from `20260811220000` to `20260820120000`. The old stamp
sorted before six already-applied migrations, so applied order no longer matched
lexicographic order. Free to fix now because this has never been applied outside
CI. Adds `Job_status_lockUntil_idx` for the sweep, which runs once per poll
interval per replica and was served by no existing index. Plain, not
CONCURRENTLY: Prisma applies each migration in a transaction, which forbids it.

Tests: every blocker now dies under its mutation, where none did before.
Restoring `runAt = NOW()` fails 1, reverting reclaim to the observer's config
fails 3, dropping `lockUntil` from the claim sites fails 1, and `count >= 0` —
the exact mutation from the review that used to pass the whole suite — fails 4.
1087 tests in the package, prettier clean, typecheck unchanged from base (same
49 pre-existing).

Not verified locally: no live Postgres or Docker in this environment, so the
reclaim and claim SQL is unexecuted. CI's `migrate deploy` will exercise the
migration and the drift gate, but the queries themselves are mocked in tests and
first run for real in staging.
@jerelvelarde

Copy link
Copy Markdown
Collaborator Author

All three blockers fixed in 527645e. This was a much better review than my PR deserved — thanks for re-running it, and for saying which claims you were retracting.

B1 — reclaim reset the backoff

Confirmed and fixed. Your walkthrough is right and I hadn't thought about it at all: two paths that both mean "this attempt did not finish" have to space retries the same way, and I'd written one of them as NOW().

Reclaim now mirrors calculateBackoff in SQL rather than approximating it — LEAST(BACKOFF_BASE_MS * POWER(2, attempts + 1) + random() * BACKOFF_BASE_MS, BACKOFF_MAX_MS), on attempts + 1 to match handleFailure. Postgres has random(), so the jitter carries over too and the two paths don't drift as one gets tuned.

B2 — the window came from the wrong worker

This is the finding I'd have been least likely to find myself, and it's the worst of the three: silently discarded completed work with the job running twice.

Took your fix. Job.lockUntil is written at claim time from the claiming worker's own config, looked up per-type out of a jsonb map with defaultTimeoutMs as fallback so a type this worker has no entry for still gets a deadline. Reclaim compares against that.

One thing I went further on than you asked, and I'd like you to check it: once the row carries its own deadline, the type predicate has no reason to exist, so I dropped it. That folds in your first non-blocking item — a crashed claim of a type this replica doesn't register used to sit PROCESSING forever, never reclaimed, never dead-lettered, invisible to healthCheck(). There's nothing a worker needs to know about a type any more in order to tell that a claim has expired. It also deletes the jsonb_to_recordset join. If you think reclaiming across types is wrong for a reason I've missed, say so and I'll put the predicate back.

Legacy rows: lockUntil is NULL for anything claimed before this deploys, and those fall to an absolute 15-minute ceiling rather than a derived window. Deliberately not "max of my configured timeouts" — that's the same guess B2 is about. 15 minutes is 3× the largest configured timeout (HUBSPOT_SYNC, 300s), and the branch is only reachable during the one deploy that adds the column.

B3 — silent fences

Confirmed, including your mutation: count >= 0 passed the whole suite. Fixed at all four sites. The retry and dead-letter paths carry the original error into the fence warning, since as you say those always logged pre-fence and going quiet loses both the rejection and the failure that caused the timeout. Four new tests; count >= 0 now fails all four.

Also taken: the reclaim-gates-claiming outage

You filed this non-blocking and I'd rather not ship it — it's introduced by this PR (there was no reclaim before), and "total outage reporting healthy, one log line per second, no restart" is the worst failure mode in the list. Own try/catch, so a failing sweep can't stop claiming. Recovering abandoned work is a nice-to-have; claiming new work is the job.

Also taken: the migration rename and the index

Renamed 2026081122000020260820120000. You're right that it sorted before six already-applied migrations, and now is the only time that's free to fix since it's never been applied outside CI.

Index added as Job_status_lockUntil_idx (the predicate changed, so lockUntil is the right second column). Not CONCURRENTLY, and not partial — Prisma applies each migration inside a transaction, which forbids the first, and Prisma can't express the second in schema.prisma, which would put the DB permanently in drift against the gate. That's the same gate that caught my missing claimToken field on the last push, so I'd rather have a slightly wider index than a permanent drift failure. Flagging the trade rather than burying it.

Not taking here, with reasons

The concurrency accounting being unreachable, and narrowing the #153 claim. Both correct. poll() awaits Promise.allSettled per batch, so activeJobs.size is always 0 when the slots are computed, and per-type limits never bind. And you're right that the fence does nothing about two different AI_RESPONSE rows for the same ticket — different ids, different tokens, both proceed; #191's Message.ticketId_responseKey is what covers that, not this. I've narrowed the PR body accordingly. The not-awaiting-the-batch change is its own PR and it's the one blocking the /health rework (#182/#184), because lastPollTime can't stall until the batch stops being awaited. Doing them together.

Unregistered-type destruction at :384-395. Confirmed, including that this PR made it worse by adding lockedAt: null, claimToken: null so reclaim can't rescue those rows either. Agreed a no-op release back to PENDING with no attempt consumed is the right shape. Latent today (production sets concurrencyByType), but it's a data-destroying path and I don't want it in a commit whose diff is about something else.

Timeout doesn't cancel, and the drain has no deadline. Agreed, and thank you for stating the honest contribution plainly — the real shutdown path is still crash-abandonment plus reclaim, and the fence makes that safe at the row level rather than preventing it. AbortSignal through JobHandlerContext is its own PR and I'd rather it be reviewed as one.

void currentPoll.finally(cb) — latent, filing it.

updateJobProgress's breaking signature. Noted. No out-of-package caller exists, so I'm taking the break rather than adding an overload that makes the token optional — optional is how the fence gets bypassed by accident later.

Side effects outside the row. This is the honest limit of the PR and I don't want it lost, so it's going in the body rather than a follow-up: the fence is a DB guard, and in a B2-style interleaving the first execution's external effects have already happened. AI_RESPONSE is covered by #191. ESCALATION, HUBSPOT_SYNC and TRACKER_SYNC are not, and this PR is what introduces automatic re-execution. B2 being fixed makes that interleaving much rarer, but "rarer" isn't "impossible" and those handlers need their own idempotency keys.

What I could not verify

No Docker and no local Postgres in this environment, so the reclaim and claim SQL is unexecuted. The tests mock prisma, so CI doesn't run it either — migrate deploy will exercise the migration and the drift gate, but the queries themselves first run for real in staging. The specific things I'd want a second pair of eyes on: "Job".type resolving inside the SET of an UPDATE ... WHERE id IN (...), and double precision * INTERVAL under Prisma's parameter typing. Both read correct to me and both are patterns already in this file, but I'm not going to claim verified for something I only reasoned about.

Re-review when you have a moment.

@NathanTarbert

Copy link
Copy Markdown
Collaborator

Hey @jerelvelarde,

Verified 527645e with a fresh reviewer. All three blockers are genuinely fixed, and every mutation you claimed dies at exactly the count you claimed. Two new things need closing before this merges, both small.

Blocker Status Evidence
B1 backoff bypass FIXED restoring ELSE NOW() → 1 failure, spaces a reclaimed retry by the same backoff a handler failure would
B2 observer's config FIXED reverting to observer policies → 3 failures; dropping lockUntil from both claim sites → 1; claim writing only defaultTimeoutMs → 1, so the per-type lookup is pinned too
B3 silent fences FIXED on all four count >= 0 at all four report sites → 4 failures. Confirmed the premise at base too: at eb2450b2 that same mutation passed 1081/1081

Head 1087, base 1081, delta +6. Typecheck 49 at head and 49 at base, identical, all pre-existing. Prettier clean.

The backoff mirror is exact. This is the item I most expected to find a divergence in and there isn't one. Term by term against shared/src/utils.ts:40-44: same base, same ceiling, jitter uniform over the same interval, jitter inside the LEAST rather than outside, same attempts + 1 offset. The only differences are DB clock vs app clock, which is the correct choice here since it matches lockedAt/lockUntil, and random() evaluating per row, which is also what you want. Overflow is impossible because the ELSE arm only runs when attempts + 1 < maxAttempts.

And lockUntil is atomic at claim. Written in the same UPDATE as status='PROCESSING' at both sites, so there's no window where a row is PROCESSING with a NULL or stale deadline. A concurrent sweep either sees the pre-claim committed row and skips it, or blocks and re-evaluates under EvalPlanQual. That was the failure mode I was most worried about in the new design and it's closed.


Two things before merge

1. Job_status_lockUntil_idx can't serve the query it was added for

worker.ts:315-322 wraps the deadline test in AND CASE WHEN job."lockUntil" IS NOT NULL THEN … ELSE … END. A CASE over the column isn't a sargable predicate, so Postgres can use the index for the status = 'PROCESSING' prefix only and then filters every PROCESSING row. The migration's justification, that [status, runAt] has the wrong second column, doesn't land: the new index has the right second column and still can't use it.

A disjunction fixes it, and note the constant has to move to the right-hand side or it stays non-sargable even in disjunctive form:

AND (
    (job."lockUntil" IS NOT NULL AND job."lockUntil" < NOW() - (${STALE_RECOVERY_GRACE_MS} * INTERVAL '1 millisecond'))
 OR (job."lockUntil" IS NULL     AND job."lockedAt"  < NOW() - (${LEGACY_RECLAIM_CEILING_MS} * INTERVAL '1 millisecond'))
)

Ideally the index is partial (WHERE status = 'PROCESSING'), which Prisma can't express, so either accept the full index or hand-write it and note it in the schema.

2. The outage guard you added unprompted is the only new behavior with zero coverage

Deleting the reclaim try/catch at worker.ts:235-239 passes the whole suite. That's the change you made on your own initiative, justified as "a total outage with nothing to restart it", and it's the one thing nothing pins. Cheap with the existing mocks: $executeRaw.mockRejectedValueOnce(new Error('boom')), advance timers, assert $queryRaw still ran and the error was logged.

Confirmed the guard itself works, for what it's worth: the catch logs and falls through, if (!this.running) return then the claim, schedulePoll still runs. Claiming is genuinely reachable when the sweep fails.


Worth fixing while you're in there

  • All four release paths leave lockUntil stale. :471, :506, :583, :612 clear lockedAt and claimToken and leave lockUntil at its past value. Inert today because the predicate gates on status='PROCESSING', and the reclaim itself does set it NULL, so it's only the TS paths. But it's a live trap: the first future code path that sets PROCESSING without writing lockUntil gets reclaimed 30s later, mid-flight. Add lockUntil: null to all four.
  • worker.ts:461 is a fifth fenced write with no count check. It sat outside the four I named, so it's now the only silent fence left in the file. warnIfFenced(result.count, job, 'no-handler failure') closes it.
  • The type-blind sweep makes the no-handler tombstone worse. I checked the interleaving I was worried about and it does not open a worse hole in the shipped config: apps/worker/src/index.ts:61-81 is the repo's only new Worker(...) and it sets concurrencyByType for all ten types, so claimJobsByType iterates registered handlers and reclaimed unhandled-type rows sit in PENDING, which beats PROCESSING-forever. But type-blindness is now stated design intent, which makes cross-type traffic a designed-for case, so :461-474's terminal FAILED should become a claim release rather than a tombstone. Flagging rather than patching because handles missing handler gracefully currently asserts the tombstone, so it's a deliberate behavior change, not an oversight.
  • The 15-minute ceiling is safe, but unpinned. Largest configured timeout is 300s, so 300_000 + grace = 330_000 < 900_000. The legacy branch really is one-deploy-only, since every PROCESSING write in the codebase is one of the two claim sites and both write lockUntil. Two nits: the docstring names only HUBSPOT_SYNC when ACCOUNT_SCORING ties it at 300s, and nothing ties the constant to max(jobTimeouts), so a future 20-minute timeout silently breaks the legacy branch for one rollout.

Two test gaps stayed open

  • "attempts" = job."attempts" still survives. This is the one I'd fix: it means a crash-looping job can never reach DEAD_LETTER, and no test notices, because toContain('job."attempts" + 1') is satisfied by the string's other occurrences in the status/completedAt/runAt arms. Assert the SET fragment specifically: toContain('"attempts" = job."attempts" + 1').
  • Re-adding a type predicate survives. The only guard is expect(reclaimSql).not.toContain('job.type = policy.type'), and AND job.type = ANY(${[...handlers]}) walks straight past that literal. Assert the SQL contains no job.type at all.

Both are string assertions against an unexecuted query, which is the structural limit of the approach rather than anything you did.

One ops precondition for the migration rename

The rename is correct and ordering hygiene is now clean. But renaming a folder makes migrate deploy / migrate dev fail on any database that recorded 20260811220000_add_job_claim_token in _prisma_migrations. You wrote it was never applied outside CI, and that's right for ephemeral CI databases — but that folder has been on this branch since eea28f50, so any developer who ran migrate dev while sitting on this branch has it applied and will hit a failed-migration state on pull. Worth a line in the PR description. (Minor: you say the old stamp sorted before six migrations, it's four. Defect is real, count is off.)

Also CREATE INDEX without CONCURRENTLY holds ACCESS EXCLUSIVE on Job for the build, and your reasoning for why CONCURRENTLY is impossible is correct since Prisma wraps each migration in a transaction. Sub-second on a table JOB_CLEANUP keeps small, a queue stall if Job is ever large. Worth checking the staging row count before this promotes to prod.

Still open from the earlier round, not gating

await this.pollPromise in stop(), availableSlots ignoring in-flight work, and the per-type count ignoring in-flight work all still survive their mutations. Unchanged and fine — they belong to the batch-await change, not here.

What only staging can settle

Being explicit since your commit message already was: no live Postgres here, so the reclaim SQL is still unexecuted. Specifically unverified — whether the CASE in the WHERE, float8 * INTERVAL, and LEAST(<float8>, $n) with a Prisma-typed integer parameter all resolve (this is a classic "could not determine data type of parameter" shape); whether the planner uses the new index; and migrate deploy plus the drift gate. Tests assert template strings and bound values, never execution.


Send the two fixes and the lockUntil: null nit and I'll re-verify. Everything else here is yours to schedule.

@NathanTarbert NathanTarbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three blockers verified fixed, and every claimed mutation dies at the claimed count. Two items before merge, both small: the new index can't serve the sweep's predicate (the CASE isn't sargable), and the reclaim try/catch you added unprompted is the one new behavior with zero coverage. Details in the comment above.

NathanTarbert and others added 3 commits August 20, 2026 16:46
Answers the two merge blockers on #224 plus the four adjacent items.

1. `Job_status_lockUntil_idx` could not serve the query it was added for.
   The deadline test was a `CASE` over `lockUntil`, which is not sargable, so
   the planner took the `status` prefix and filtered every PROCESSING row.
   Now a disjunction, with the interval arithmetic on the right-hand side —
   both are required, since `lockUntil + interval < NOW()` is still
   non-sargable in disjunctive form. Recorded next to the index in
   schema.prisma so a future edit does not silently undo it.

2. The reclaim try/catch was the only new behaviour with no coverage.
   Deleting it passed the whole suite. Now pinned by asserting the claim
   still runs after a rejected sweep, and that the failure is logged.

Also taken from the review:

- All five release paths clear `lockUntil`. Inert while the predicate gates
  on `status = 'PROCESSING'`, but the first future path that sets PROCESSING
  without writing a fresh deadline would inherit one already in the past.
- `worker.ts:461`, the no-handler tombstone, was the fifth fenced write and
  the last silent one. Now reports through `warnIfFenced`.
- `"attempts" = job."attempts" + 1` is asserted as a `SET` fragment. The old
  `toContain('job."attempts" + 1')` was satisfied by the string's other
  occurrences, so a job that never accrues an attempt — and so never reaches
  DEAD_LETTER — went unnoticed.
- The type-blindness guard asserts no `job.type` anywhere in the sweep.
  `AND job.type = ANY(...)` walked past the old single-spelling check.
- The legacy ceiling is checked against this worker's own timeouts at
  construction. It stays a fixed constant — deriving it from the observing
  worker's config is the bug `lockUntil` removed — but fixed means it does
  not follow `jobTimeouts` upward, and the two live in different packages.
  The docstring also now names ACCOUNT_SCORING, which ties HUBSPOT_SYNC at 300s.

Call sites enumerated:
- `warnIfLegacyCeilingTooLow` (added, private): one caller, the constructor.
- `warnIfFenced` (existing): one new call site; the other four unchanged.
- `LEGACY_RECLAIM_CEILING_MS` / `STALE_RECOVERY_GRACE_MS`: new references in
  the guard and the predicate; existing uses unchanged.
- `Job.lockUntil`: every writer is a claim (2), the reclaim (1), or a release
  (5). No other writer exists in the repo.

Verification: 1113 tests pass (62 files). Eleven mutations run, each killed by
exactly the test that names it — including deleting the try/catch, reverting
the predicate to a CASE, dropping the attempt increment, re-adding a type
predicate, and removing `lockUntil: null` from each release path. Queue
typecheck 49 errors at head and 49 at base, identical and all pre-existing
(unbuilt workspace). Prettier clean, no collateral reformatting.
… DB errors

A seven-agent review of the previous commit found one surviving mutation that
matters more than everything else here, plus a set of silent paths around it.

## The surviving mutation

Deleting the `lockUntil` SET from `claimJobsForType` — and from that one query
only — left all 1113 tests green. Production sets `concurrencyByType`
(apps/worker/src/index.ts), so `claimJobsForType` is the only claim path it ever
takes. Every job it claimed would have carried `lockUntil = NULL` and fallen into
the 15-minute legacy branch forever, which is the exact failure this PR exists to
remove. The only covered claim path was the one production never runs.

Two claim queries now carry byte-identical SET clauses duplicated by hand, so
there is a test asserting they are equal as well as one covering the per-type
query directly. Both die under the mutation above.

## Bookkeeping is not the work

- The COMPLETED write sat inside the handler's own `try`, so a Prisma blip on it
  routed into `handleFailure`: the row went back to PENDING carrying a database
  error as the *job's* error, and the retry re-ran every side effect the handler
  had already produced. That is the duplicate execution the claim token exists to
  detect, manufactured from a bookkeeping failure. The write now sits outside, and
  a failure there leaves the row for the sweep and says so.
- `Promise.allSettled` discarded its results and `processJob` has no outer catch,
  so a rejection there produced no output at all — not the Prisma error, and not
  the handler failure it was recording. Rejections are now reported.
- `updateJobProgress` could reject into a handler that awaits it, failing a job
  that was running fine. Progress is telemetry; it can no longer fail the job.

## The sweep had no voice and one blind spot

- Its row count was computed once per poll on every replica and thrown away. It
  is the earliest signal that replicas are dying, so it is logged when non-zero.
- It ran behind the capacity check, so a saturated replica stopped recovering
  abandoned work exactly when the backlog was largest. Moved ahead of it.
- A PROCESSING row with neither `lockUntil` nor `lockedAt` matched no arm and sat
  there forever — unreachable from the two claim paths, which is precisely the
  assumption a last-resort sweep should not make. Third arm on `updatedAt`.
- It overwrote `error` unconditionally, including on the arm that dead-letters,
  destroying the failure that actually killed the job at the point an operator
  goes looking. Now appended, bounded with `left(…, 200)`.
- `POWER(2, attempts + 1)` is unbounded and `maxAttempts` is settable per row.
  float8 overflow would abort the sweep for every row, not just the offending
  one — and JS saturates gracefully there, so the mirror diverged at the extreme.
  Exponent clamped at 30, which saturates to BACKOFF_MAX_MS exactly as JS does.

## Claims that were not true

- The fence message asserted "this job ran more than once". `count === 0` proves
  only that the claim was lost; the sweep may have dead-lettered the row, in which
  case it will never run again. Reworded to what is known.
- A missing `claimToken` would silently unfence every write in `processJob`,
  because Prisma drops a `where` key whose value is `undefined`. Such a row is now
  refused and left for the sweep rather than run unfenced.
- The sargability comment claimed both arms kept their column bare "for the index".
  Only the first arm can use it; `lockedAt` is not in the index. Corrected, and the
  test assertion that pretended to guard it is gone.
- `buildHealthResponse` does not exist. The method is `healthCheck()` — and the
  health server answers 200 unconditionally without consulting it, so the outage
  the catch guards against would not have been restarted either. Said plainly.
- The migration claimed it "must deploy ahead of the code". `start.sh` runs
  `migrate deploy` at container start, so it ships with the code; what makes it
  safe is the additive-nullable shape, not sequencing.
- The index migration is split out. Prisma wraps each file in one transaction, so
  keeping it with the ADD COLUMN held that statement's ACCESS EXCLUSIVE across the
  index build. Split, the ALTER commits first and the build takes only SHARE.
  No `lock_timeout`: a timeout aborts the migration into the P3009 state
  `start.sh` exists to diagnose, and waiting is the more recoverable failure.

## Tests were checking the mock against itself

`vi.mock('@copilotkit/outpost/shared')` re-declared MAX_JOB_ATTEMPTS,
BACKOFF_BASE_MS and BACKOFF_MAX_MS as literals, so every assertion written
against those numbers asserted nothing: editing the real constants left the file
green. They now come from `vi.importActual`. Also `$queryRaw` was never re-armed
between tests, so a test that set no claim result inherited the previous one's.

Call sites enumerated:
- `processClaimedJobs` (added, private): both claim paths, no other caller.
- `handleFailure` (added a `jobType` parameter): two callers, both in
  `processJob`, both updated. Private, no out-of-class caller.
- `updateJobProgress` (behaviour changed, signature unchanged): every caller
  reaches it through `context.reportProgress`; all of them await it, which is why
  the change matters and why none needed editing.
- `Job.lockUntil`: unchanged from the previous commit — two claims, the sweep,
  five releases.

Verification: 1125 tests pass (62 files). Fifteen mutations run, each killed by
exactly the test that names it — including deleting `lockUntil` from the
production claim path alone, letting the two claim clauses diverge, putting the
completion write back inside the handler's try, dropping the allSettled report,
moving the sweep back behind the capacity check, and editing the real constants
to catch mock drift. Queue typecheck 48 errors against a 49 baseline, all
pre-existing. Prettier clean, no collateral reformatting.
@jerelvelarde

Copy link
Copy Markdown
Collaborator Author

Both merge items fixed in dbc865e, and then a seven-agent round on that commit found something worse than either of them — fixed in 2e7153f. Taking them in order.

The two you asked for

1. Job_status_lockUntil_idx could not serve its own query. You were right that the migration's justification did not land. The CASE is gone; the predicate is a disjunction with the interval arithmetic on the right-hand side, which as you noted is the half that matters — lockUntil + interval < NOW() stays non-sargable even in disjunctive form. Recorded next to the index in schema.prisma so a future edit does not quietly undo it, and pinned by a test that asserts CASE is absent from the WHERE and that the column stays bare.

2. The outage guard had no coverage. Confirmed — deleting the try/catch passed the whole suite. Now pinned by asserting the claim still runs after a rejected sweep and that the failure is logged.

Also taken: lockUntil: null on all five release paths, warnIfFenced on the no-handler tombstone at :461, the "attempts" = job."attempts" + 1 SET-fragment assertion, the no-job.type-anywhere assertion, and ACCOUNT_SCORING in the ceiling docstring. On tying the constant to max(jobTimeouts): I did not derive it — that is the B2 bug coming back through another door — but the constructor now checks this worker's own timeouts against the ceiling and says so if one outgrows it. Eleven mutations, each killed by exactly the test that names it.

Then the review round found a surviving mutation that matters more

Deleting the lockUntil SET from claimJobsForType — that query alone — left all 1113 tests green.

apps/worker/src/index.ts sets concurrencyByType, so claimJobsForType is the only claim path production ever takes. Every job it claimed would have carried lockUntil = NULL and fallen into the 15-minute legacy branch forever — the exact failure this PR exists to remove. The one covered claim path was the one production never runs: queue.test.ts:260 constructs its worker without concurrencyByType, so it exercises claimAndProcessJobs.

The two claim queries carry byte-identical SET clauses duplicated by hand, so there is now a test covering the per-type query directly and a second asserting the two clauses are equal. Both die under that mutation. (I looked at hoisting the fragment into one place; Prisma.sql is used nowhere in this repo and introducing it here felt like the wrong PR for a new pattern.)

Silent paths around it, all fixed

  • The COMPLETED write sat inside the handler's own try. A Prisma blip on it routed into handleFailure, so the row went back to PENDING carrying a database error as the job's error, and the retry re-ran every side effect the handler had already produced. That is the duplicate execution the claim token exists to detect, manufactured from a bookkeeping failure. Moved out; a failure there now leaves the row for the sweep and says so.
  • Promise.allSettled discarded its results and processJob has no outer catch, so a rejection produced no output at all — not the Prisma error, not the handler failure it was recording. Verified: the combined console output for a whole poll was the empty string.
  • The sweep's row count was thrown away. Every row it moves is a claim a worker took and never released; it is the earliest signal replicas are dying, computed once per poll on every replica and discarded. Logged when non-zero.
  • The sweep ran behind the capacity check, so a saturated replica stopped recovering abandoned work exactly when the backlog was largest. Latent today for the activeJobs reason we both know about, live the moment that changes. Moved ahead of it.
  • A PROCESSING row with neither lockUntil nor lockedAt matched no arm and sat there forever. Unreachable from the two claim paths — which is precisely the assumption a last-resort sweep should not be making, and the same stranding your own docstring celebrates removing. Third arm on updatedAt.
  • The sweep overwrote error unconditionally, including on the arm that dead-letters, destroying the real failure at the one surface where it matters. Appended now, bounded with left(…, 200).
  • POWER(2, attempts + 1) is unbounded and maxAttempts is per-row and settable through createJob. float8 overflow aborts the sweep for every row, not just the offending one — and JS saturates gracefully there (Math.min(Infinity, MAX)), so your term-by-term mirror diverges at the extreme. Exponent clamped at 30, which saturates to BACKOFF_MAX_MS exactly as the JS does.
  • updateJobProgress could reject into a handler that awaits it, failing a job that was running fine. Progress is telemetry; it can no longer fail the job it describes.
  • A missing claimToken would silently unfence every write in processJob, because Prisma drops a where key whose value is undefined. Such a row is now refused and left for the sweep rather than run unfenced. You and three reviewers landed on the ?? 'none' dead branch independently; this is what was actually behind it.

Claims of mine that were not true

  • The fence message asserted "this job ran more than once". count === 0 proves only that the claim was lost — the sweep may have dead-lettered the row, in which case it will never run again. Reworded.
  • buildHealthResponse does not exist. It is healthCheck() — and the health server answers 200 unconditionally without consulting it, so the outage that catch guards against would not have been restarted either. Said plainly now rather than implying a mitigation that is not there.
  • I wrote that both arms keep their column bare "for the index". Only the first can use it; lockedAt is not in Job_status_lockUntil_idx. The assertion that pretended to guard that is gone rather than left looking like a defence.
  • The migration said it "must deploy ahead of the code". start.sh runs migrate deploy at container start, so it ships with the code; what makes it safe is the additive-nullable shape, not sequencing.

Tests were checking the mock against itself

vi.mock('@copilotkit/outpost/shared') re-declared MAX_JOB_ATTEMPTS, BACKOFF_BASE_MS and BACKOFF_MAX_MS as literals, so editing the real constants left the whole file green — including the assertions guarding your backoff mirror. They come from vi.importActual now, and changing either real constant fails a test. Also $queryRaw was never re-armed between tests, so a test that set no claim result inherited the previous one's.

On the index itself — your call, not mine

Two reviewers independently argued the index may not earn its keep: PROCESSING is bounded by replicas × maxConcurrency and JOB_CLEANUP keeps the table small, [status, runAt]'s prefix already narrows to that handful, and status/lockUntil both change on every claim and every release — so it adds maintenance to the two hottest write paths for a scan that was already cheap. I kept it, because you asked for it and because the argument for it is that the sweep's cost should not scale with table size as the queue grows. But I have written that trade honestly into the schema comment rather than the justification that does not survive the "PROCESSING is tiny" observation, and if you would rather drop it, say so and I will.

I have also split the index into its own migration — Prisma's per-file transaction meant the build was happening under the ALTER's ACCESS EXCLUSIVE rather than its own SHARE. Deliberately no lock_timeout: it aborts into the P3009 state start.sh exists to diagnose. Migration rename hazard and the staging preconditions are now in the PR body, as you asked.

Numbers: 1125 tests, 62 files. Fifteen mutations in the second round, all killed. Queue typecheck 48 against a 49 baseline — one lower, because JobResult stopped being an unused import.

Still unexecuted, and I want to be as plain about it as you were: no Postgres here, so the reclaim and claim SQL has still never run. The third arm, left(job.error, 200), the string concatenation on the error column and the clamped POWER are all new SQL that CI does not execute either. Staging is the first place any of it runs.

Filed rather than fixed

Six findings from the round are real, load-bearing, and belong to PRs we have already agreed are separate — I will open issues rather than smuggle them in here. The one I would look at first is deterministic type starvation in claimJobsByType: per-type limits sum to 14 against maxConcurrency: 10, so under a broad backlog the global budget is exhausted partway down the insertion-ordered list and the tail — TRACKER_SYNC, JOB_CLEANUP, GITHUB_REACTION_POLL, PENDING_RESPONSE_SWEEP — is never reached. processedCount > 0 re-polls at zero delay and makes the identical choice, so it is indefinite, not delayed. PENDING_RESPONSE_SWEEP is the handler that rescues AI responses stranded in PENDING, which makes it worth looking at against the current Discord complaints.

@NathanTarbert

Copy link
Copy Markdown
Collaborator

Went through this one against current head 2e7153f, since the review on it predates your Aug 25 pushes.

Every item from that review is closed, and I checked each by mutation rather than by reading:

  • Index predicate. worker.ts:392-410 is now a three-arm disjunction with the interval on the right-hand side. Restoring the CASE form fails 2 tests, including keeps lockUntil bare on one side so the index can serve the predicate.
  • Reclaim outage guard. Present at worker.ts:292-296; deleting it fails the reclaim sweep > cannot stop the worker claiming when it fails.
  • Silent fences. warnIfFenced at worker.ts:58-63, wired at :619 and :698; flipping count > 0 to count >= 0 at :63, :762 and :793 fails 4.
  • The lockUntil SET on the claim path. This is the one that previously survived — dropping it from claimJobsForType only (worker.ts:498-506) now fails 2, including claims identically whichever path is taken.
  • lockUntil: null is on all four release paths and in the reclaim SET.

Baseline at head is 62 files / 1125 tests, all passing. git merge --no-ff origin/main is clean, and the suite on the merge result is 65 files / 1234 tests green — so the 6/107/11 conflict the body warns about is already absorbed.

So this needs a re-review to clear the standing changes-requested, and a merge of main (30 commits behind, merges clean).

Two notes for that pass. The body still says 62 files / 1087 tests and carries the 1087-era mutation table, where head is 1125 — worth refreshing since we've been holding other PRs to that. And the reclaim/claim SQL still hasn't executed anywhere: no Postgres in CI or locally, so the three-arm disjunction, left(job.error, 200), and the clamped POWER(2, LEAST(attempts + 1, 30)) are asserted as template strings and staging is the first place they run. The body says as much, so it's a deploy precondition rather than a gap, but it's worth someone watching the first reclaim sweep there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants