feat(orchestration): admission queue with deferred pickup (#441)

[nizar-lahlali]

Closes #441. Successor to #331 (fan-out mass-fail against the concurrency cap).

What

When admission hits the per-user concurrency cap, the task is now queued, not failed: it transitions SUBMITTED → QUEUED (a new pre-active state that holds no concurrency slot) and a scheduled AdmissionQueuePickup Lambda drains the queue in FIFO order (by created_at) as slots free up — flipping QUEUED → SUBMITTED and re-invoking the orchestrator.

Design highlights

• Single counter writer. The pickup Lambda does a read-only capacity pre-check; the orchestrator's atomic admissionControl conditional-increment remains the only writer of the concurrency counter. A pickup that loses the admission race harmlessly re-queues (created_at never changes, so FIFO position is preserved). This avoids introducing a second writer — the class of drift bug behind Fan-out orchestration mass-fails against the per-user concurrency cap (no queue/retry) #331.
• Cancel semantics preserved. Every queue transition is conditional on the current status, so a user cancel cleanly wins any race. QUEUED is cancellable via REST (already status-generic) and Slack (list extended). No compute or slot to release.
• Idempotency preserved. Replay with the same Idempotency-Key returns the existing QUEUED task (200) — dedupe path is status-agnostic and untouched.
• Backstops. A task queued > QUEUE_MAX_AGE_SECONDS (default 24 h) is failed with a clear message. The stranded-task reconciler now ages tasks by time-in-current-status so a long queue wait isn't mistaken for a stranded SUBMITTED task right after pickup.
• Orchestrator re-invoke carries a per-pickup nonce so no dedup layer mistakes it for a replay of the create-task invocation.
• Channel feedback: Linear/Jira comment now says "queued — will start automatically", and only on first queue entry (no per-cycle spam).

API / CLI surface

• TaskRecord: queued_at, admission_attempts (internal bookkeeping).
• GET /tasks/{id}: queued_at, queue_position (1-based FIFO rank among the caller's queued tasks, computed at read time, fail-open), estimated_wait_s (coarse heuristic).
• bgagent status / detail view render Queue: position N (est. wait ~Xm) while QUEUED.
• CDK↔CLI types mirrored; check-types-sync passes.

#331 regression test

reconcile-admission-queue.test.ts includes a fan-out-burst regression: 5 children queued above a cap of 3 all survive as QUEUED, drain FIFO (3 picked up, 2 remain), and zero tasks fail.

Testing

• mise run build passes end-to-end (agent quality, CDK compile + synth + 2265 tests across 125 suites, CLI 576 tests, docs build + sync, type/constants drift checks).
• New coverage: state-machine invariants (QUEUED pre-active, disjointness, transitions), queueTask (first-entry vs re-queue, race, no counter writes), pickup handler (FIFO, capacity bounds, per-user isolation, races, invoke-failure, max-age backstop, pagination), get-task queue position (rank, fail-open, left-queue race), CLI rendering.

Notes for reviewers

• The ETA is a deliberately coarse UX hint (ceil(position / cap) × avg-task-duration, default 600 s, env-tunable) — not a promise.
• AdmissionQueuePickup runs every minute; an empty-queue cycle is a single GSI query.
• Marked as potentially breaking in the issue: clients that treated the admission-cap FAILED + admission_rejected event as terminal will now see QUEUED + admission_queued instead.

[scottschreckengaust]

Review — feat(orchestration): admission queue with deferred pickup (#441)

Verdict: COMMENT (non-gating). This is a well-architected, well-bounded, thoroughly documented feature that faithfully implements the approved acceptance criteria of #441. No merge blockers. I am not approving because two real quality issues are worth addressing — a genuine (minor, non-safety) correctness bug in queue-position reporting under the very fan-out-burst scenario this feature targets, and a coverage gap on the load-bearing re-queue integration path — but neither gates merge. Take them as strong suggestions.

Governance (ADR-003) — PASS

• #441 exists, is OPEN, carries approved + P0 + orchestration. Implementation matches its acceptance criteria (durable QUEUED state, FIFO drain by created_at, cancel/idempotency preserved, queue_position/ETA surfaced, #331 regression test). Branch feat/441-admission-queue-deferred-pickup follows the pattern. mergeable=MERGEABLE (the earlier UNKNOWN resolved; main already merged in — no rebase needed). CI green except integ-smoke (pending admin approval).

Vision alignment (VISION.md / ORCHESTRATOR.md) — PASS

• Fire-and-forget default preserved. Deferred pickup keeps unattended async execution: the scheduled drain re-invokes the orchestrator; no user action needed while queued (the Linear/Jira comment even says "No action needed").
• Bounded blast radius & cost — the key tenet, and it holds. The queue is DynamoDB-backed (not an unbounded SQS pile), drained by a 1-minute scheduled Lambda whose empty cycle is a single cheap GSI query. A 24h max-age backstop (QUEUE_MAX_AGE_SECONDS) fails over-age tasks so the queue can never accumulate unbounded zombies. QUEUED holds no concurrency slot and no compute — correctly excluded from ACTIVE_STATUSES (the comment in task-status.ts calls out that counting it would deadlock the queue). No undocumented tenet trade.
• Reviewable outcomes preserved: queued tasks still flow to the same PR pipeline once admitted.

Blocking — NONE

No unbounded queue, no missing bootstrap coverage, no task double-run, no admission race that loses correctness, no undocumented tenet trade.

Non-gating findings (please address)

1. queue_position mis-reports on same-millisecond ties — cdk/src/handlers/get-task.ts:79. created_at is new Date().toISOString() (ms granularity), not the ULID task_id. computeQueueInfo ranks with strict item.created_at < record.created_at, so two tasks created in the same millisecond each see the other as not ahead → both report the same position. This is precisely the #331 fan-out burst (children minted in a tight loop) this feature exists to serve, so bgagent status could show two queued tasks both claiming "position 3". Risk: user-visible UX-only inconsistency (FIFO drain is unaffected — the pickup Lambda uses the GSI's own sort order). Fix: break ties deterministically on the ULID task_id, which is monotonic with creation (inline suggestion below). Add a get-task test with two identical created_at rows pinning the behavior — the current position test (get-task.test.ts:164-167) uses four distinct timestamps and never a tie.

2. Re-queue integration path is untested end-to-end — cdk/src/handlers/orchestrate-task.ts:91-126. The load-bearing FIFO-preservation logic (on a failed admissionControl, pass the re-read current — not the stale step-1 task — to queueTask, and gate channel feedback on current.queued_at === undefined so a pickup-cycle re-queue doesn't reset queued_at/admission_attempts or spam the issue) has no handler-level test. queueTask's re-queue behavior is covered in isolation (orchestrate-task.test.ts:1020), but a regression to queueTask(task) (stale snapshot) would silently reset FIFO bookkeeping and every existing test would still pass. Add a handler test asserting the re-read record is passed and notify-on-cap fires only on first entry.

3. pickUpTask invoke-failure is miscounted in the summary — cdk/src/handlers/reconcile-admission-queue.ts:224 returns false on a failed orchestrator re-invoke (task now SUBMITTED-with-no-pipeline), which the caller counts as skipped_race (line 367), not errors. A systemic invoke outage (bad ARN / throttling / perms) that strands every picked-up task would log at INFO with errors: 0 — indistinguishable from benign cancel races, and no alarm-worthy error_id like the sibling reconcile-stranded-tasks.ts:400-412 pattern. Recovery itself is sound (the task IS reachable by the stranded reconciler — verified: created_at is never rewritten so it stays under the GSI cutoff, and the new age-by-status_created_at logic protects it until it's genuinely stranded). This is purely an observability gap. Fix: increment summary.errors (or a dedicated invoke_failed counter) on that branch and fold it into the escalation at line 381.

Nits

• MAX_CONCURRENT default drift (cosmetic). orchestrator.ts:41 defaults ?? '3'; the new files (get-task.ts:34, reconcile-admission-queue.ts:69, task-api.ts:549, admission-queue-pickup.ts:119) and the orchestrator construct (task-orchestrator.ts:200) default to 10. In the deployed stack MAX_CONCURRENT_TASKS_PER_USER is always set (construct default 10), so all planes agree at 10 and the ?? '3' never fires — but the lingering 3 reads as a real disagreement. confirm-uploads.ts:48 also still uses ?? '3'. Worth aligning the fallbacks to one value in a follow-up. (Not in this PR's diff, so no inline.)
• expireQueuedTask's cancel-race branch (reconcile-admission-queue.ts:266) and the > QUEUE_MAX_AGE_SECONDS boundary lack a direct test (the pickup path has the equivalent ConditionalCheckFailed test). Cheap to add.

Documentation — EXCELLENT

docs/design/ORCHESTRATOR.md is comprehensively updated: new QUEUED state row, state diagram edges, transition table (incl. the 24h backstop), cancel-semantics row, and a rewritten admission-control step 2 explaining the single-writer/re-queue design. The Starlight mirror docs/src/content/docs/architecture/Orchestrator.md is regenerated in the same PR (no stale-mirror CI risk). CLI types.ts is synced (QUEUED + the three new TaskDetail fields) and format.ts renders the queue line. No ADR needed — this is an additive lifecycle state within the existing durable-execution model, not a new architectural decision.

Tests & CI

• Bootstrap synth-coverage (ADR-002): N/A — correctly not updated. The only new CFN resources are AWS::Events::Rule (events:PutRule) and AWS::Lambda::Function (lambda:CreateFunction) — both already in cdk/src/bootstrap/resource-action-map.ts (same Rule+Lambda pattern as ConcurrencyReconciler / StrandedTaskReconciler / PendingUploadCleanup). No new resource type ⇒ no bootstrap-policy / BOOTSTRAP_VERSION bump required. Verified by grepping the diff for new construct instantiations.
• #366 test-performance: PASS. admission-queue-pickup.test.ts caches Template.fromStack() once (module-level cache, bare new App() picks up the global bundling-disable), no per-test synth, no bundling re-enable. task-status.test.ts is a pure constant unit test (no synth).
• Strong behavior-focused coverage otherwise: the named #331 fan-out-burst regression, FIFO ordering, per-user capacity math, pagination, queued_at-set-once, admission_attempts increment, conditional-flip-loses-to-cancel (pickup path), state-machine QUEUED ∉ ACTIVE_STATUSES invariant, CLI rendering + fail-open null. Gaps are findings #1/#2/#3 above.

Note on a claim I investigated and discharged

An automated pass flagged a "concurrency counter double-increment on durable replay" in the admission step as HIGH. I verified against the diff and durable-execution semantics: admissionControl was already inside context.step('admission-control') before this PR (the diff only swaps failTask→queueTask inside the pre-existing step). At-least-once step re-execution is a general durable-execution property — documented in ORCHESTRATOR.md — and is explicitly backstopped by the existing ConcurrencyReconciler (reconciles active_count drift against actual active tasks). It is neither introduced nor worsened by #544, so it is out of scope here.

Review agents run

• pr-review-toolkit:silent-failure-hunter (invoked) — surfaced the invoke-failure miscount (#3) and the durable-replay concern (investigated + discharged as pre-existing).
• pr-review-toolkit:pr-test-analyzer (invoked) — surfaced the tie gap (#1) and the re-queue integration gap (#2).
• code-reviewer / type-design-analyzer — hand-reviewed. Types: TaskStatus/VALID_TRANSITIONS/PRE_ACTIVE_STATUSES are clean; TaskDetail additions are well-modeled (queue_position/estimated_wait_s nullable, never persisted). Bootstrap (ADR-002) verified by hand.

Human heuristics

• Proportionality: appropriate. A durable DynamoDB state + a cheap scheduled drain is the right weight for #441 — not over-built (no SQS/Step Functions), reuses the existing single-writer counter and reconciler backstop.
• Coherence: matches the established reconciler/durable-step patterns already in the codebase.
• Clarity (AI004): the get-task.ts:104 fail-open nosemgrep is correctly justified (queue position is best-effort UX; must not 500 the whole GET). No unjustified silent masking found.
• Appropriateness (AI005): tests assert does-do (condition expressions, flipped IDs in FIFO order, counters, decoded payloads), not should-do.

_{🤖 Generated with Claude Code}

[isadeks]

Review — admission queue with deferred pickup (#441)

Reviewed by reading the code at head (fb9c2320) and verifying the load-bearing claims against the actual GSI/schema. This is a well-architected, well-documented feature — the single-writer discipline (pickup does a read-only capacity pre-check; the orchestrator's atomic admissionControl stays the only counter writer; a lost race harmlessly re-queues) is exactly the right way to avoid the #331 drift class. @scottschreckengaust already did a thorough non-gating review; this confirms his findings against current code and flags which are now addressed.

Overview

SUBMITTED → QUEUED (a pre-active state holding no slot) when the per-user cap is hit, instead of FAILED. A 1-min scheduled Lambda drains FIFO as slots free up (QUEUED → SUBMITTED + orchestrator re-invoke with a per-pickup nonce), with a 24h max-age backstop, cancel-race safety via conditional writes, and read-time queue_position/ETA on GET /tasks/{id}.

Verified correct (the claims that matter)

• FIFO preservation holds at the schema level. StatusIndex is PK: status, SK: created_at (task-table.ts:120-122), so the pickup's ascending scan is genuinely oldest-first, and since created_at never changes on re-queue (only status_created_at moves), FIFO position survives a lost admission race. The central design promise is real, not aspirational.
• QUEUED ∉ ACTIVE_STATUSES — correctly placed in PRE_ACTIVE_STATUSES; the deadlock the comment warns about (queued tasks holding the slots they wait for) is avoided, and the concurrency reconciler won't count them.
• Counter safety — construct grants the pickup grantReadData (not write) on the concurrency table; a test pins "no write actions on UserConcurrencyTable." Single-writer invariant enforced by IAM, not just convention.
• get-task ranking is sound — UserStatusIndex is ProjectionType.ALL, so created_at is available to rank by; querying begins_with(status_created_at, 'QUEUED#') and ranking on the separate created_at is correct.
• Cancel/idempotency — every transition is status-conditional, so a user cancel cleanly wins any race; QUEUED added to Slack's cancellable set + REST is status-generic. queued_at set-once (re-queue preserves it), feedback gated on first entry (no per-cycle spam).

On @scottschreckengaust's three findings — status at this head

• #1 (same-millisecond tie in queue_position) — code FIXED, test still MISSING. computeQueueInfo now breaks ties on task_id (get-task.ts: created_at === record.created_at && item.task_id < record.task_id) — correct, since the ULID is monotonic with creation. But the position test still uses four distinct timestamps; there's no test with two rows sharing a created_at, which is exactly the fan-out-burst tie this fixes. A regression to the tiebreak would pass CI. Please add the tie test to pin it.
• #3 (invoke-failure miscounted) — NOT addressed. pickUpTask still return false on a failed orchestrator re-invoke (reconcile-admission-queue.ts:~224), and the caller counts false as summary.skipped_race (line ~368), not errors. So a systemic invoke outage (bad ARN / throttle / perms) that strands every picked-up task logs at INFO with errors: 0 — indistinguishable from benign cancel races, and no alarmable error_id. Recovery is still sound (the stranded reconciler sweeps these — verified, created_at unchanged keeps them under the GSI cutoff and the new age-by-status_created_at protects them until genuinely stranded), so this is purely observability. Worth incrementing errors (or a dedicated invoke_failed) on that branch so a real outage is visible.
• #2 (re-queue integration path untested) — still applies: queueTask's re-queue is unit-tested in isolation, but the handler-level wiring (pass the re-read current, gate feedback on current.queued_at === undefined) has no test. A regression to queueTask(task) (stale step-1 snapshot) would silently reset FIFO bookkeeping with every existing test still green. A handler test asserting the re-read record is passed would close it.

Additional notes

• MAX_CONCURRENT default drift (Scott's nit, worth repeating): orchestrator.ts + confirm-uploads.ts default ?? '3'; the five new/adjacent call sites default ?? '10'. Harmless in the deployed stack (the env var is always set to 10), but the get-task ETA and the pickup capacity math read ?? '10' while the orchestrator reads ?? '3' — if the env var ever went unset, the pickup would over-pick against the orchestrator's stricter local default and every over-pick would re-queue. Aligning the fallbacks (follow-up) removes a latent footgun.
• Docs (ORCHESTRATOR.md + Starlight mirror) are genuinely excellent — new state, diagram edges, transition table, cancel-semantics row, rewritten admission step. CDK↔CLI types synced.

Verdict

Approve-ready in substance — no correctness or safety blocker (FIFO, single-writer, cancel, deadlock-avoidance all verified). The two things I'd land before merge are the #1 tie test (cheap, pins a fix that's currently unguarded) and #3's error-count (one-line observability fix so a real invoke outage pages). #2 is a strong suggestion. CI green (build (agentcore) runs the full suite).