Repository navigation
fix(cloud): recalibrate the relay monitor's exhausted-retry freeze to a measured bar - #18569
Conversation
… a measured bar The pre-drain dry-run froze at minute one on relayPostgresRetryExhausted: 0 in every run since #18521 reached the director, blocking the cell roll that carries the same fix. #18521 made contended request-path waiters fail fast (500 ms) instead of succeeding slowly, so exhaustion is now a steady contention rate: 236/236 five-minute windows non-zero over 23 h; post-#18521 p50 42 / p90 147 / max 220 fleet-wide; the 2026-08-23 incident peaked at 467. 300 clears every measured healthy window and stays under the incident shape. /v1/assign 503 share was unchanged by #18521 (13.9% vs 12.3%).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe incident monitor threshold for Merge Risk: ⚪ Minimal · up to The relay monitor now permits up to 300 exhausted PostgreSQL retries per five minutes and freezes above that level. The updated boundary coverage and aligned documentation leave no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, measurements, testing, scope, and pending dry-run verification. However, the required Linked Issue section is incomplete because it states "Fixes # (none filed)".
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cloud/apps/relay-ops/src/incident-monitor.test.ts (1)
135-139: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the exact 300-event boundary.
The freeze condition is “over 300,” but this test covers only 220 and 301. A regression from
>to>=would pass while freezing at exactly 300. Add a 300-event sample and assertgreenbefore the 301-event case.Proposed test addition
+ const atLimit = healthySample() + atLimit.sources['relay-logs']!.signals['relay.postgres_retry_exhausted'] = signal(300) + expect(evaluateIncidentSample(atLimit, startedAt).status).toBe('green') + const incident = healthySample() incident.sources['relay-logs']!.signals['relay.postgres_retry_exhausted'] = signal(301)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: a863f318-fd1d-40df-bde5-87083d2836b1
📒 Files selected for processing (3)
cloud/apps/relay-ops/src/incident-monitor.test.tscloud/apps/relay-ops/src/incident-monitor.tscloud/docs/relay-incident-monitor.md
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only — the threshold change itself is sound and well-tested; two consistency items worth a look.
Reviewed changes
- Threshold recalibration —
relayPostgresRetryExhaustedmoves from0to300per five minutes, with the measurement basis (fleet-wide window stats plus the 467 incident peak) captured in the comment and adjacent comments updated. - Test rewrite — the "freezes on a single exhausted transaction" test becomes a boundary test pinning
220/300green and301freeze withthreshold: 300, so reverting the constant fails it. - Docs — the threshold table row and implementation-log entry are updated to
over 300.
The semantics check out: relay.postgres_retry_exhausted is a five-minute delta sum (relayFiveMinuteDeltaSignal), and the rule uses max (value > threshold), so 300 is green and 301 freezes — matching the test and the relayPostgresRetries boundary style.
ℹ️ Sibling cloud/apps/relay comments still teach the zero-tolerance contract
The change redefines the exhausted-retry signal from "any terminal failure freezes the gate" to "over 300/5min freezes the gate", but three comments in the relay package (not the relay-ops package this PR touches) still describe the old contract:
cloud/apps/relay/src/assignment-store.ts:341— "one terminal failure freezes the incident gate."cloud/apps/relay/src/cell-inventory-lock-census.test.ts:174— "relayPostgresRetryExhausted freezes the incident gate at zero."cloud/apps/relay/src/cell-inventory-lock-contention.test.ts:303— "the incident monitor freezes at zero exhausted transactions."
Technical details
# Update stale zero-tolerance comments
## Affected sites
- cloud/apps/relay/src/assignment-store.ts:341 — `pool-default` rationale cites a single terminal failure freezing the gate.
- cloud/apps/relay/src/cell-inventory-lock-census.test.ts:174 — "freezes the incident gate at zero".
- cloud/apps/relay/src/cell-inventory-lock-contention.test.ts:303 — "freezes at zero exhausted transactions".
## Required outcome
- Reword each to reflect the 300-per-5min bar (terminal failures now inflate the exhausted-retry counter toward the 300/5min bar, not freeze on a single failure), so the rationale stays accurate.ℹ️ Alert policy still pages at > 0
google_monitoring_alert_policy.relay_postgres_retry_exhausted (both Cloud Run and GCE conditions) still uses threshold_value = 0, so it keeps paging on the steady contention rate until the follow-up Terraform change lands. The PR body already flags this as out of scope — worth confirming it is tracked as a real sequencing item, since it will keep firing continuously in the meantime.
Technical details
# Recalibrate the exhausted-retry alert policy
## Affected sites
- cloud/infra/terraform/relay-observability.tf:447 and :469 — `threshold_value = 0` on the exhausted-retry conditions (ALIGN_SUM over 300s mirrors the monitor's five-minute delta).
## Required outcome
- Recalibrate to 300 per 300s in its own observability-root change.
## Open questions for the human
- Is a tracking issue needed so the continuously-firing alert is not lost?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Pullfrog item 1 done in 011c7ac: the three relay-package comments now describe the bounded 300/5min bar. Item 2 (Terraform alert policy at > 0) is intentionally left for its own observability-root change; noted in the PR body. |
… a measured bar (stablyai#18569) * fix(cloud): recalibrate the relay monitor's exhausted-retry freeze to a measured bar The pre-drain dry-run froze at minute one on relayPostgresRetryExhausted: 0 in every run since stablyai#18521 reached the director, blocking the cell roll that carries the same fix. stablyai#18521 made contended request-path waiters fail fast (500 ms) instead of succeeding slowly, so exhaustion is now a steady contention rate: 236/236 five-minute windows non-zero over 23 h; post-stablyai#18521 p50 42 / p90 147 / max 220 fleet-wide; the 2026-08-23 incident peaked at 467. 300 clears every measured healthy window and stays under the incident shape. /v1/assign 503 share was unchanged by stablyai#18521 (13.9% vs 12.3%). * test(cloud): pin the exhausted-retry freeze boundary at exactly 300 * docs(cloud): reword relay comments that still described the zero exhausted-retry bar
ELI5
Before any production cell roll, a 15-minute health check has to pass. One of its rules says "zero database lock give-ups allowed". Since #18521 made the relay give up on a contended lock quickly instead of waiting a full second, the fleet always has a few dozen give-ups per five minutes, so the health check fails at minute one every time and nobody can roll the cells. This PR sets that rule to a number the healthy fleet actually stays under while still catching the real incident.
What Changed
cloud/apps/relay-ops/src/incident-monitor.ts:relayPostgresRetryExhausted0 → 300 per five minutes, with the measurement basis in the comment; adjacent comments that referred to the zero bar updated.cloud/apps/relay-ops/src/incident-monitor.test.ts: the "freezes on signal(1) with threshold 0" test becomes "220 is green, 301 freezes with threshold 300". Reverting the constant to 0 fails it.cloud/docs/relay-incident-monitor.md: threshold table row and an implementation-log entry with the basis.Why
Every
cloud-monitor-relay-productiondry-run since the director deploy at 22:12Z on 2026-09-03 froze on this bar (runs 33746931085, 33747826907, 33812032190).cloud-deploy-relay-production-same-caprequires a fresh green dry-run, so the 23 GCE cells are stuck on the pre-#18521 image with the 1 s lock wait, which is exactly what keepssqlLatencyMsMaxpinned at 1.0–1.2 s on every cell (see #18565).Measured from
jsonPayload.event="orca_relay_postgres_transaction_exhausted"fleet-wide (director + cells), summed per five minutes, 2026-09-03T03Z to 2026-09-04T02Z:300 clears every healthy window measured and stays below the incident shape.
relayPostgresRetries: 300remains the ~10x discriminator for incident-class contention and is unchanged. User-facing/v1/assign503 share did not move with #18521 (13.9% old image vs 12.3% new, same evening), so exhaustion is not a proxy for user harm.Post-roll the cells will also fail fast, so the fleet-wide exhausted count will rise further while retries fall; the bar leaves room for that (roughly 2.5x the current p90).
Not in this PR
cloud/infra/terraform/relay-observability.tfhasgoogle_monitoring_alert_policy.relay_postgres_retry_exhaustedwith the same> 0threshold on both the Cloud Run and GCE conditions, so that alert has been firing continuously since #18521. It should be recalibrated to the same 300 per 300 s, but that is a Terraform apply on the observability root and belongs in its own change.Linked Issue
Fixes # (none filed; unblocks the cell roll described in the #18565 report)
Visual Proof
N/A: threshold constant, test, and docs only.
Testing
cloud/apps/relay-ops:vitest run12 files / 76 passed;tsc --noEmitclean; mutation check (bar back to 0) fails the new test.Agent skill upstream boundary
Checklist