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}

[scottschreckengaust]

Same-millisecond tie mis-reports position. created_at is new Date().toISOString() (ms granularity), so two tasks created in the same millisecond produce identical strings; with strict <, neither counts the other as ahead and both report the same queue_position — exactly the fan-out-burst case (#331) this feature targets. The ULID task_id is monotonic with creation, so use it as a deterministic tiebreak:

Suggested change

	} else if (typeof item.created_at === 'string' && item.created_at < record.created_at) {
	} else if (
	typeof item.created_at === 'string'
	&& (item.created_at < record.created_at
	|| (item.created_at === record.created_at
	&& typeof item.task_id === 'string'
	&& item.task_id < record.task_id))
	) {

Also add a get-task test with two rows sharing record.created_at to pin this — the current position test uses four distinct timestamps and never a tie.