Review: #487 Emit pending_orders_with_expired_session gauge metric

Verdict: NEEDS_REFINEMENT

Template Completeness

• [x] Type — "Feature"
• [x] Lineage — standalone, 2026-04-17 Utah Invitational discovery narrative
• [x] Repo — forgejo_admin/basketball-api
• [x] User Story — Ava as the consumer, alert rule as the so-that
• [x] Context — references branch 290-payment-pipeline-observability, explains why WebhookErrorRate/WebhookStale don't cover this failure mode
• [x] File Targets — present but inaccurate (see below)
• [x] Acceptance Criteria — 6 items, each verifiable
• [x] Test Expectations — 2 unit tests with run command
• [x] Constraints — present but over-prescriptive (see Recommendation)
• [x] Checklist — PR opened / tests pass / gauge visible / clean refresh logged
• [x] Related — blocks Ticket C2 (pal-e-platform#295, verified open)

Traceability

• [ ] story:observability label — present on board item, but OFF-TAXONOMY. project-westside-basketball user-stories uses WS-S{N} (WS-S1 through WS-S33). Siblings #488 and #486 were already reconciled to story:WS-S11 and story:WS-S22 respectively during this morning's refinement pass. #487 was missed. Closest existing story: WS-S11 ("As an admin, I want to create Stripe checkout sessions for jerseys/fees so that payments are automated") — reasonable home since the metric defends the same payment-session surface.
• [ ] story note — not applicable until label is reconciled. Once retagged to WS-S11, the existing entry in project-westside-basketball user-stories covers it.
• [ ] arch:payment-pipeline label — present on board item, but NO BACKING NOTE. search_notes(query="arch-payment-pipeline") returned zero results. Sibling #488 was retagged to arch:dataflow-westside-basketball (which DOES exist as a linked note on the project page). Same reconciliation should apply to #487.
• [x] Forgejo issue — https://forgejo.tail5b443a.ts.net/forgejo_admin/basketball-api/issues/487, open, contains full spec.

File Targets

• [ ] src/basketball_api/metrics.py — ISSUE: this file does not exist. The ticket hedges with "or wherever Prometheus metrics are currently registered — confirm pattern by reading existing basketball_api_up, webhook_errors_total, etc." but then assumes a metrics.py module as the landing spot. Actual live pattern:
• [ ] "APScheduler job running every 5 minutes" — ISSUE: APScheduler is not a current dependency. Grep across ~/basketball-api for APScheduler|apscheduler|BackgroundScheduler|AsyncIOScheduler returned zero matches. Adding it introduces a new runtime dependency that isn't justified in Context. The ticket already lists the correct fallback (FastAPI startup event + asyncio task) which matches the existing src/basketball_api/main.py:38 lifespan pattern. The preference ordering is backwards.
• [x] Model field Order.stripe_checkout_session_id — verified, referenced in src/basketball_api/models.py and 5 other files. Query pattern (select pending orders with non-null session id) is viable against the real schema.
• [x] Order.status = 'pending' — verified as a live status value in the codebase.

Repo Placement

Dependencies

• Blocks pal-e-platform#295 — verified open. pal-e-platform#295 explicitly expects basketball_api_pending_orders_with_expired_session gauge with a category label (tournament/monthly) and proposes both TournamentOrdersWithExpiredSessions and MonthlyOrdersWithExpiredSessions alert rules. #487's AC "Gauge includes at minimum a category label (tournament, monthly, jersey)" matches that contract. Gauge metric name + label schema is a cross-repo API contract — if Dev agent changes either during implementation, pal-e-platform#295 breaks silently. Worth adding to AC: "metric name and label keys MUST match pal-e-platform#295 File Targets verbatim."
• Sibling #488 (board item #1023, currently in needs_approval) — sets 30-day TTL on Stripe sessions. Once #488 deploys, new sessions won't expire for 30 days, so the gauge value from #487 will approach zero for new orders. The metric still has value (surfaces legacy sessions + future edge cases), but the "local run surfaces 18+6=24" AC in #487 is only true before #488 deploys. Doesn't block ordering, but worth noting in Context.
• Sibling #486 (regen blast, item #1022) — independent. #486 rewrites Utah orders; #487 observes the class.
• Sibling #489 (pattern spike, item #1025) — independent. #489 may eventually change the mint pattern; if it does, #487's gauge contract holds regardless.

Acceptance Criteria

• AC-1 "gauge exposed at /metrics" — verifiable by curl of the pod's /metrics.
• AC-2 "category label (tournament, monthly, jersey)" — verifiable. Matches pal-e-platform#295's expectations.
• AC-3 "refreshes at least every 5 minutes" — verifiable by timestamp observation or test harness.
• AC-4 "Stripe API failures log a warning, do not crash, leave gauge at its last value" — testable via mocked Stripe client throwing.
• AC-5 "Local run: current production state surfaces 18 + 6 = 24" — depends on #488 not yet being deployed. Reasonable snapshot AC but time-sensitive.
• AC-6 "Integration test with mocked Stripe client" — testable. Test Expectations section covers this with unit-test granularity (2 pending / 1 expired → gauge=1).

Blast Radius

• Stripe API load: "18+ API calls every 5 min" stated as acceptable. For context, Stripe default rate limit is 100 read ops/sec — this is 0.06/sec averaged, negligible. OK.
• Internal inconsistency: File Targets suggests "consult a cached session table" as an optimization; Constraints says "Do NOT add a new DB table unless truly necessary. Prefer in-memory refresh." These contradict. Dev agent should treat "no new table" as the binding constraint and drop the cached-table branch from the prompt.
• Gauge vs Counter: Constraint is correct (Gauge — count decreases as parents pay). No concern.
• Kaniko/CI: new dependency (if agent picks APScheduler) would land in pyproject.toml, triggering a fresh wheel build. Avoidable by sticking with asyncio/lifespan.
• Downstream: Grafana dashboards in pal-e-platform (basketball-api golden-signals) will auto-pick up the new metric via Prometheus scrape. No change needed until #295 adds the panel.

Decomposition Assessment

Scope / Solution-Spec Assessment (Lucas 2026-04-17 policy)

• File Targets ⇒ numbered 4-step query algorithm. "Select all Order rows where... / For each, call stripe.checkout.Session.retrieve OR consult a cached session table / Count those where session.status == 'expired' / Set gauge value; label by product.category if cheap." This is a solution spec, not a file target. AC already expresses the outcome; the algorithm is a Dev agent decision.
• Constraints ⇒ library prescriptions. "Prefer Gauge over Counter" (correct but derivable from AC wording — "Gauge refreshes"), "Do NOT add a new DB table unless truly necessary" (protects an invariant, OK), "Match existing metrics style in basketball-api (module path, naming prefix)" (ambiguous — Dev agent has to grep anyway). The Gauge-vs-Counter and module-style items are over-specification.
• File Targets ⇒ "Preferred: APScheduler job." Recommends a library that isn't in the codebase. This is the most load-bearing over-specification because it nudges Dev agent toward a dependency change that isn't justified and obscures the already-correct fallback (asyncio in lifespan).

Recommendation

• [BODY] Remove the 4-step numbered query algorithm from "File Targets" — the AC already specifies the outcome. Replace with a one-line pointer: "Existing metrics live in routes/health.py and routes/webhooks.py (module-local, not centralized). Follow that pattern."
• [BODY] Remove the "Preferred: APScheduler" recommendation. APScheduler is not a current dependency. Replace with: "Refresh task runs in the existing FastAPI lifespan (src/basketball_api/main.py:38). No new runtime dependencies."
• [BODY] Remove the contradictory "consult a cached session table" branch — the "no new DB table" constraint already rules it out.
• [BODY] Remove the "match existing metrics style" Constraint as redundant with the File Targets pointer.
• [BODY] Remove the "Prefer Gauge over Counter" Constraint — AC-3 ("refreshes at least every 5 minutes") already implies Gauge semantics.
• [BODY] Add AC: "Metric name MUST be basketball_api_pending_orders_with_expired_session and MUST expose a category label with values in {tournament, monthly, jersey}. This is a cross-repo contract with pal-e-platform#295 — changes here break the alert rule silently."
• [BODY] Add to Context: "Sibling #488 (30-day TTL) will reduce this gauge's value for new orders once deployed. AC-5's '18 + 6 = 24' snapshot is valid against current main; re-measure after #488 lands."
• [LABEL] Retag board item #1024: replace story:observability with story:WS-S11 and replace arch:payment-pipeline with arch:dataflow-westside-basketball. Matches the sibling reconciliation on #488/#486 from earlier today.
• [SCOPE] (project-wide, not #487-specific but surfaced here) Complete the payment-pipeline story-cluster reconciliation — #489 also retains story:payment-reliability + arch:stripe-checkout and needs the same treatment as #488/#486/#487.