Skip to content

fix(relay): stop dead accept work, spread control rotations, fail direct probes fast - #18565

Closed
Jinwoo-H wants to merge 174 commits into
mainfrom
relay-reconnect-fleet-accept
Closed

Jinwoo-H wants to merge 174 commits into
mainfrom
relay-reconnect-fleet-accept

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

The phone asks the relay to connect it to the Mac; the relay has to check a shared database first. Under today's database lock storm the check took longer than the phone was willing to wait, so the relay finished work for a phone that had already hung up (that is the already_bound log line). Separately, every Mac renews its relay link on a fixed 55-minute clock, so hundreds of Macs that reconnected together after a cell restart renew together forever and hammer the same lock in waves. And the phone was spending 3.5 s retrying a dead Wi-Fi path before trying the relay.

This PR: stops the relay working for phones that left, spreads the renewals out, and lets the phone give up on dead Wi-Fi at once.

Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​457 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​455
Prod 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​846 $\color{#cf222e}{\Huge{\mathbf{−}}}$​10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​836

Incident

2026-09-04 ~01:05Z, Samsung phone (mobile-rearch @ 22574ed, pre-#18518) paired to macOS 1.4.197-adhoc.20260903232359, host digest 666077865f2e on cell c27. After a few seconds in the background the host dot stayed grey. Phone log: direct Opening WebSocket → close 1006 x3 (backoff 500/1000/2000 ms), then relay dial failed Error: replacement session authentication timed out 24 s later (two flat 12 s dials). Everything #18518 did not cover is here.

Root cause

The cell's acceptClient runs four serialized Postgres calls; the fourth (acquireActivity for the confirmation lease) contends for the global relay_cells FOR UPDATE lock. Under contention the cell finishes the DB phase after the phone's bound has fired, then PendingHostDataReservation.bind throws host_data_reservation_already_bound because the phone's close already released the reservation. Every "first frame handler failed" line in the window is that post-mortem.

1. Lock contention fleet-wide (23:00Z–01:15Z), and it is worse today

window orca_relay_postgres_transaction_retry per hour (cells) cell sqlFailuresDelta per hour
Sep 2 18Z – Sep 3 07Z 660–1470 680–1620
Sep 3 08Z – 16Z 3600–7100 3700–7700
Sep 3 23Z 7468 7585
Sep 4 01Z 1243 1267
  • 5000/5000 sampled cell retries are 55P03; every director retry after 00:40Z is phase=cell-inventory.
  • Every cell reports sqlLatencyMsMax 1.0–1.2 s in nearly every 30 s sample (23:25Z–01:14Z), which is the pre-fix(cloud): bound and yield the relay's global cell-inventory lock #18521 1 s pool lock_timeout. Cells still run the old image; only the director got fix(cloud): bound and yield the relay's global cell-inventory lock #18521 (deployed 22:12Z, revision 00560-bek).
  • Not load: fleet controls 25.7k → 26.9k across the window; Cloud SQL CPU 46–53% both days. No cloud-* deploy touched production in 04Z–13Z; c15/c19/c22 restarted 06:05Z and c20 08:20Z (MIG autoheal). I could not attribute the 08Z step to a single trigger.
  • c27 on the incident itself: sqlLatencyMsMax 1.0–2.0 s and sqlFailuresDelta 1–8 per sample, pendingSplices 0, poolWait up to 327 ms during 01:04–01:07Z. first frame handler failed host_data_reservation_already_bound on 12 instance ids (c28 x8, c27 x7, c29 x3, ...), 31 events 23:06Z–01:01Z.

2. The 00:50:14–17Z wave is the desktop's scheduled control rotation, not a rollout

  • No cloud-* workflow ran between 22:12Z (director deploy) and 01:00Z (clock-skew monitor). No director revision change, no SQL maintenance.
  • 00:49–00:51Z: 2745 control closes on 19 instances; 1157/1632 code 1006 and 973/1030 code 4408 control rebound have ageMs in the 53-minute bin.
  • Relay grants a flat 55 min lease; desktop rebinds 60–120 s early. Seed: c27 autoheal recreate 23:23:23Z (compute.instances.repair.recreateInstance on relay-c27-j5ff, back listening 23:31:06Z) dumped ~420 controls onto other cells. They rebound together at 00:25Z (c27 4408 burst, sqlLatencyMsMax 3904 ms, poolWait 2001 ms) and again 00:49–00:50Z; harmonics at 23:55Z and 00:04Z. Each rebind is an activateControl transaction on the inventory lock, so the wave is a direct contention spike.

3. Host 666077865f2e (all on c27, instance 4123247033296491207)

time code ageMs app meaning
00:27:45Z 4408 control rebound 3227932 adhoc.20260903210744 scheduled rotation
00:52:24Z 1006 1478513 adhoc.20260903210744 app quit
00:52:35Z assignment granted lane=sticky cell=c27 adhoc.20260903232359 new build connects
01:44:55Z 1006 3135052 adhoc.20260903232359 rotation (gen 2, splices 0)
02:23:15Z 1006 2243182 adhoc.20260903232359 rebind, splice intact

No drain, no 4404, no wrong-cell. The new build's control is stable.

4. Phone: direct probe blocks relay recovery on a dead LAN

On foreground the direct client's notifyForeground restarts its own reconnect loop; an instant 1006 puts it in reconnecting. openAuthenticatedDirectEndpoint only resolved on connected/disconnected/auth-failed, so the probe held the supervisor's operation mutex for its full 12 s bound while direct walked 500/1000/2000 ms redials, and relay recovery queued behind it.

Changes

Relay (cloud/apps/relay)

  • acceptClient checks socket.readyState after each serialized DB call and abandons once the phone has hung up: releases the capacity reservation, fails the credential reservation, releases the activity lease it just acquired (previously leaked ~90 s to expiry cleanup), never reaches bind.
  • New structured event orca_relay_client_accept_abandoned {stage, elapsedMs} (no identities) and metric fields clientAcceptsAbandonedByStageDelta / clientAcceptAbandonedMsMax on orca_relay_runtime_metrics.
  • Control lease jittered symmetrically: 55 min ± 5 min on activate and rebind (CONTROL_LEASE_MS, CONTROL_LEASE_JITTER_MS). Same-minute cohorts drift apart across cycles at an unchanged mean rebind rate. Auth expiry (5 min token) and the 75 s silence watchdog are enforced independently of the lease. No wire change: leaseExpiresAt was always a server-chosen absolute time and old desktops honor it as before.

Desktop (src/main/runtime/relay/relay-origin-pool.ts)

  • Rotation early window widened from 1–2 min to 1–6 min.

Phone (mobile/src/transport/mobile-direct-endpoint-probe.ts)

Tests

  • host-session-client-accept.test.ts (new): abandon after slow acquireActivity (lease released, reservation failed, no conn-open, no bind, event logged without host id); abandon after slow reserveCredential (no activity acquired); live phone still gets conn-open; lease grant centred at exactly 55 min with both edges pinned; rebind re-rolls jitter. Disabling the readyState check fails 2; a no-jitter mutant and a shortening-only mutant each fail 2.
  • relay-observability.test.ts: new delta fields aggregate and reset.
  • relay-session-broker.test.ts: two same-lease hosts rebind ~5 min apart. Reverting to the 60 s window fails it.
  • mobile-direct-endpoint-probe.test.ts: all candidates entering reconnecting fails the probe immediately with no leftover timer. mobile-endpoint-supervisor.test.ts: relay recovery is not blocked behind a probe stuck in its redial loop. Reverting the probe change fails both.

Gates: pnpm typecheck:tsc:node, cloud relay typecheck + full relay vitest project (50 files / 473), mobile tsc --noEmit + npx vitest run src/transport (91 files / 669), pnpm run check:code-quality:changed, oxfmt on mobile files.

Not applied (deploy-owned)

The 23 GCE cells still run the pre-#18521 image with the 1 s pool lock_timeout; that is why every cell's sqlLatencyMsMax pins at ~1.0–1.2 s. Rolling them is cloud-deploy-relay-production-same-cap (one cell at a time). This PR's relay change also needs that roll to reach the cells.

Linked Issue

Fixes # (none filed; incident-driven from the 2026-09-04 ~01:05Z mobile relay reconnect report and the fleet log evidence above)

Visual Proof

N/A: no UI change. Behavior is covered by the unit tests listed under Tests; the phone-side effect is a shorter grey/yellow dot on foreground, not a rendering change.

Testing

  • Automated tests added/updated (see Tests above); every fix was mutation-checked by reverting it and confirming its tests fail
  • Gates: pnpm typecheck:tsc:node, pnpm run check:code-quality:changed, cloud relay typecheck + full relay vitest project, mobile tsc --noEmit + npx vitest run src/transport, oxfmt on mobile files
  • Manual device test pending: a rebuilt APK with fix(mobile): stage-aware relay dial bound so a slow cell is not hung up on #18518 is being installed separately; the relay change needs the cell roll to reach production

Agent skill upstream boundary

  • Not applicable

Notes

Wire compatibility: leaseExpiresAt was always a server-chosen absolute timestamp; old desktops honor a shorter grant unchanged, and new desktops against old cells keep the flat 55 min. No new opcode or field. Cross-platform: desktop change is timer math in main; mobile change is transport-only. SSH: unaffected.

Checklist

  • This PR is small and focused (three coordinated fixes for one incident, each independently revertible)
  • I explained what changed and why (including ELI5)
  • N/A for visual proof with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered
  • Lint/typecheck/test pass locally for the touched packages; CI covers the rest

…ect probes fast

Incident 2026-09-04 ~01:05Z: after a background/foreground cycle the phone's
relay dial timed out inside the cell's acceptClient DB phase while the fleet
was in a cell-inventory lock storm (55P03 retries ~7.5k/h vs a ~1k/h floor).

Relay (cloud/apps/relay)
- acceptClient checks socket.readyState after each serialized Postgres call
  and abandons the accept once the phone has hung up, releasing the capacity
  reservation, failing the credential reservation, and releasing the activity
  lease it just acquired instead of leaking it to expiry cleanup and then
  throwing host_data_reservation_already_bound at bind.
- New structured event orca_relay_client_accept_abandoned {stage, elapsedMs}
  and runtime-metric fields clientAcceptsAbandonedByStageDelta /
  clientAcceptAbandonedMsMax so the "phone gave up behind the lock" rate is
  quantifiable per cell.
- Control lease grants are jittered: 55 min minus [0, 10 min). Every host
  that (re)connected in the same minute rebound as one cohort every ~54 min
  (c27 autoheal recreate at 23:23Z re-homed ~420 controls; ~1.1k 1006 +
  ~1k 4408 "control rebound" closes landed in a 3 s window at 00:50:14Z),
  and each rebind is an activateControl transaction on the inventory lock.
  No wire change: leaseExpiresAt was always a server-chosen absolute time.

Desktop (src/main/runtime/relay)
- Control rotation rebinds 1-6 min early instead of 1-2 min, so a re-homed
  cohort spreads across cycles rather than pinning one phase for the life of
  the process.

Phone (mobile/src/transport)
- openAuthenticatedDirectEndpoint treats 'reconnecting' as a failed probe. On
  a dead LAN the foreground direct dial dies with an instant 1006 and the
  direct client enters its own 500/1000/2000 ms backoff; the probe used to
  wait out its full 12 s bound holding the supervisor mutex, so relay recovery
  queued behind three doomed redials. The stage-aware bound from #18518 is
  unaffected.

Not done here: rolling the 23 GCE cells onto the post-#18521 image (500 ms
lock_timeout) is a deploy owned by cloud-deploy-relay-production-same-cap.
@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The relay now detects abandoned client accepts during assignment, credential, and activity operations. It cleans up reservations and records stage-specific metrics. Control leases use symmetric random jitter, with injectable randomness and updated rotation scheduling. Mobile direct probes fail fast when sessions reconnect, allowing immediate relay recovery. Tests cover these paths. An incident findings document records reconnect investigation results, rollout inputs, decisions, and operational queries.

Merge Risk: 🔵 Low · up to 8ebff

The relay recovery behavior is covered by targeted tests, but the incident findings still contain retry-gate guidance that can lead to incorrect operational decisions, and the document has a Markdown formatting violation. These are bounded merge-readiness issues rather than a demonstrated runtime regression.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed and covers the incident, root cause, changes, testing, compatibility, and deployment scope. However, the required Linked Issue section explicitly states that no issue was f… Add a valid linked issue reference in the Linked Issue section, such as Fixes #1234``. If no issue exists, file one before merging and update the description with its number or link. Do not use Fixes # (none filed).
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the three main changes: abandoning dead relay accept work, spreading control rotations, and failing direct probes quickly.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description is detailed and covers the incident, root cause, changes, testing, compatibility, and deployment scope. However, the required Linked Issue section explicitly states that no issue was filed.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. (1 skipped: 1 unsupported.)


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: f0e78279-861a-4e30-9ee7-9b2a1aed4d8b

📥 Commits

Reviewing files that changed from the base of the PR and between 7106101 and fdf6fb2.

📒 Files selected for processing (10)
  • cloud/apps/relay/src/host-session-client-accept.test.ts
  • cloud/apps/relay/src/host-session-registry.ts
  • cloud/apps/relay/src/relay-observability.test.ts
  • cloud/apps/relay/src/relay-observability.ts
  • cloud/apps/relay/src/relay-server.ts
  • mobile/src/transport/mobile-direct-endpoint-probe.test.ts
  • mobile/src/transport/mobile-direct-endpoint-probe.ts
  • mobile/src/transport/mobile-endpoint-supervisor.test.ts
  • src/main/runtime/relay/relay-origin-pool.ts
  • src/main/runtime/relay/relay-session-broker.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

CodeRabbit: a phone that hung up while resolveResume was in flight still
paid for resolveInviteForMove and assignments.resolve. Check after each
awaited lookup; regression test asserts neither later lookup runs.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

This PR addresses three distinct failure modes surfaced by the 2026-09-04 relay lock-contention incident: dead acceptClient work, synchronized control-rotation cohorts, and a phone-side probe that blocked relay recovery on a dead LAN.

  • Abandon dead accept work — acceptClient now re-checks socket.readyState after each serialized Postgres call and abandons (releasing the capacity reservation, failing the credential reservation, and releasing the freshly-acquired activity lease) once the phone has hung up, instead of finishing the work and throwing host_data_reservation_already_bound.
  • Spread control rotations — the relay grants control leases in 55 min − [0, 10 min) and the desktop widens its rebind window from 1–2 min to 1–6 min, so same-minute cohorts drift apart across cycles.
  • Fail direct probes fast — the phone's direct-endpoint probe treats reconnecting as a failed candidate, so relay recovery no longer queues behind a 12 s bound on a dead LAN.
  • Abandon observability — a structured orca_relay_client_accept_abandoned event (no identities) plus clientAcceptsAbandonedByStageDelta / clientAcceptAbandonedMsMax metric fields.

The abandonedByClient closure is placed correctly relative to resource acquisition (no resource is released at a stage where it was never acquired), and release() / failReservation are idempotent or best-effort so the close-handler path in relay-server.ts and the abandon path cannot double-release the ledger unit. The new tests are meaningful — the lease-jitter and broker-spreading assertions would fail against the pre-PR 60 s window, and the abandon tests would fail with the readyState check removed.

ℹ️ One-sided lease jitter raises steady-state rotation frequency ~15%

Both spreading mechanisms shorten the rotation period but never lengthen it. The relay jitter (55 min − [0, 10 min), mean ~50 min) combined with the widened desktop early window (1–6 min, mean ~3.5 min) drops the mean control rotation from ~53.5 min to ~46.5 min — roughly 15% more activateControl transactions on the contended inventory lock, in exchange for breaking the synchronized cohort burst. This is almost certainly net-positive given the incident was a burst (lock-timeout) problem rather than a volume problem, but worth confirming the higher steady-state rate is acceptable; a symmetric jitter (e.g. [50, 60) min) would spread the cohort without raising the mean.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes (delta against the prior pullfrog review at fdf6fb2) — one new commit, e5ccd4a, tightens the acceptClient abandonment window on the relay.

  • Abandon between assignment lookups — acceptClient now awaits resolveResume and resolveInviteForMove as separate steps, re-checking abandonedByClient('assignment') after each, instead of resolving the full resolveResume ?? resolveInviteForMove chain before the first abandon check. A phone that hangs up during resolveResume no longer triggers a doomed resolveInviteForMove (or assignments.resolve) round trip.
  • Coverage — a new test drives a slow resolveResume, closes the phone mid-lookup, and asserts resolveInviteForMove, assignments.resolve, and reserveCredential are never called while the 'assignment' abandon event fires.

This directly addresses the remaining gap from the initial review (the abandon check ran only after the whole assignment chain resolved). The new assertions are meaningful — they would fail if the interleaved abandonedByClient('assignment') checks were removed.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Pullfrog: shortening-only jitter raised the mean rebind rate ~15%. Grant
55 min +/- 5 min instead; same cohort spread, unchanged steady-state load.
Auth expiry (5 min token) and the 75 s silence watchdog are enforced
separately, so a grant up to 60 min risks nothing.
@Jinwoo-H

Jinwoo-H commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Took Pullfrog's one-sided-jitter point: aa3a7ef makes the relay grant symmetric (55 min ± 5 min), so the cohort spread is unchanged and the mean rebind rate stays at today's. Tests now pin the centre at exactly 55 min and both edges; a shortening-only mutant and a no-jitter mutant each fail two tests. The desktop early window stays 1–6 min (client-side, moves the mean by ~2 min).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes (delta against the prior pullfrog review at e5ccd4a) — one new commit, aa3a7ef, makes the control-lease jitter symmetric.

  • Symmetric lease jitter — controlLeaseExpiresAt() now grants 55 min ± [0, 5 min) (CONTROL_LEASE_JITTER_MS reduced to 5 min) instead of the one-sided 55 min − [0, 10 min). This resolves the rotation-frequency note from the initial review: the mean grant is now exactly 55 min (mean rebind rate unchanged) while same-minute cohorts still drift apart across cycles.
  • Coverage — the lease test now pins the shortest (random → 0), centered (random → 0.5), and longest (random → 0.999999) edges, and the rebind test re-rolls the jitter around the symmetric midpoint. A reverting asymmetric-jitter mutant would fail the centered and longest assertions.

The offset arithmetic is correct: floor((random()*2 − 1) · 5min) yields offsets in [−5min, +5min), and the desktop's widened 1–6 min early-rotation window composes with it without re-introducing the one-sided shortening.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: c6f2ec94-ffd9-46f2-ad72-515cbb3ab879

📥 Commits

Reviewing files that changed from the base of the PR and between aa3a7ef and d338b34.

📒 Files selected for processing (1)
  • RELAY-RECONNECT-FINDINGS.md

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Comment on lines +71 to +72
bar; I have not. Best odds for a clean 15 min are 02–04Z and 17–18Z (9/12 five-minute windows under
300 in each).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use consecutive-window evidence for the 15-minute gate.

9/12 windows below 300 does not prove a clean 15-minute period. The gate requires three consecutive five-minute windows. Also, 02–04Z spans 24 five-minute windows, while 17–18Z spans 12. Report the exact denominator and the number of qualifying three-window runs before using this as roll-timing guidance.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 1bae41cb-bac9-4950-a22e-46e51af650fc

📥 Commits

Reviewing files that changed from the base of the PR and between d338b34 and 8ebff89.

📒 Files selected for processing (1)
  • cloud/docs/relay-reconnect-2026-09-findings.md

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.


Evidence, cell c7 instance 4545742188814054238, 2026-09-04:

```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a language tag to the log fence.

The fence on Line [36] triggers Markdownlint MD040. Change it to text or another accurate language tag.

Proposed fix
-```
+```text
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
```
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)

[warning] 36-36: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Source: Linters/SAST tools

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The one new commit since the prior review adds an agent working-notes file to a public repo, leaking internal infrastructure detail. This should not land as-is.

Reviewed changes — delta against the prior pullfrog review at aa3a7ef: a single new commit d338b344ec ("docs: relay reconnect investigation findings and roll status") adds RELAY-RECONNECT-FINDINGS.md at the repo root (166 lines). No code changed.

⚠️ Agent scratch committed to a public repo, disclosing internal infrastructure detail

RELAY-RECONNECT-FINDINGS.md reads as ephemeral agent working memory — it opens with "Working notes … Kept current across context compactions", and contains first-person operational state ("Stopped by me", "I will not raise it unasked", "My recommendation: B", "Decision needed from the owner"). That is not durable repo documentation, and it will be stale within hours (it pins specific run IDs, timestamps, and "CI green (23)").

More concretely, the repo is public (stablyai/orca, visibility PUBLIC), and this file discloses internal operational detail: a raw GCE instance ID (4545742188814054238), the full production/rollback image digests (sha256:85bf6799…, sha256:5aedbca5…), the 23-cell topology and per-cell crash counts, an internal hostname (c7.relay.onorca.dev), and the rollout confirmation command shape (ROLL_RELAY_SAME_CAP <target-digest> production-gce-c7). None of these are credentials, but together they are a meaningful disclosure of production topology to a public repo, and none of it belongs there.

Technical details
# Remove internal relay working notes from the public repo

## Affected sites
- `RELAY-RECONNECT-FINDINGS.md` (repo root, added by `d338b344ec`) — entire file is agent scratch plus internal infra detail.

## Required outcome
- The file must not land in a public repository. Either delete it from the PR, or relocate it to a non-public/internal location (e.g. a private team wiki, a gitignored path, or a private repo) and redact the instance ID, image digests, cell names, and internal hostname before any public exposure.
- Any residual durable documentation (the actual incident postmortem findings) should be a proper `cloud/docs/` doc with internal identifiers removed, not a root-level ALL-CAPS scratch file.

## Suggested approach
- Drop `RELAY-RECONNECT-FINDINGS.md` from this PR. The three code fixes it describes are already documented in the PR body and their own code comments.
- Note the repo's `docs/**` gitignore only re-allows specific subtrees; a root-level file bypasses that convention entirely, which is part of why this slipped through.

## Open questions for the human
- Is any of this (Findings 1–6, the roll inputs) intended to become durable `cloud/docs/` documentation? If so it should be rewritten without first-person agent state and without instance IDs/digests/hostnames.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found. The three new commits are docs-only and address the prior location concern.

Reviewed changes — delta against the prior pullfrog review at d338b34: three new commits (8ebff89, 16c6aef, 7a8d1fa) all touch cloud/docs/relay-reconnect-2026-09-findings.md.

  • Relocated the findings doc — moved RELAY-RECONNECT-FINDINGS.md from the repo root to cloud/docs/, which both cleared the root directory guard CI failure and put it alongside the existing relay ops docs.
  • Recorded the retries-bar decision — added Finding 7 (metric-based recalibration table, 15-minute pass rates, the 300→2000 basis for PR #18580) and resolved the "Decision needed" section to Option B.
  • Recorded the monitor dry-run dispatch inputs — appended the exact gh workflow run cloud-monitor-relay-production.yml invocation to the Roll inputs section.

ℹ️ Still a live agent working-notes file, now with a raw instance ID and full image digests

The move to cloud/docs/ fixes the CI and the file now sits with relay-incident-monitor.md, which already publishes production hostnames and cell lists, so most of this content is consistent with existing convention. Two residual bits still read as agent scratch rather than durable docs: the first-person operational state ("Stopped by me", "My recommendation: B", "Kept current across context compactions") that will go stale within hours, and the raw GCE instance ID (4545742188814054238) plus full sha256: image digests (85bf6799…, 5aedbca5…) that none of the sibling docs publish. None of it is a credential; it is defer-able, but redacting that one instance ID and those two digests before merge would keep this consistent with the surrounding docs.

Technical details
# Redact instance ID / image digests from the public findings doc

## Affected sites
- `cloud/docs/relay-reconnect-2026-09-findings.md:36` — raw GCE instance ID `4545742188814054238`
- `cloud/docs/relay-reconnect-2026-09-findings.md:172-173` — full `sha256:85bf6799…` / `sha256:5aedbca5…` image digests

## Required outcome
- Optional: shorten the instance ID and the two full digests to the truncated form already used elsewhere in the file (`…`), matching the file's own "Host ids are log digests" posture and the sibling docs.

## Open questions for the human
- Is this file intended to remain a live working log (in which case first-person state is fine), or should it be frozen into a durable postmortem at merge time?

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@Jinwoo-H

Jinwoo-H commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Superseded: docs moved to #18958, code rebased onto main in #18959 (desktop rotation change dropped since #18719 shipped a proportional version). Closing.

@Jinwoo-H Jinwoo-H closed this Sep 6, 2026
Jinwoo-H added a commit that referenced this pull request Sep 6, 2026
…ap, and Roll 2 plan (#18958)

Operator record for the 2026-09-04 relay reconnect incident and the Roll 1
same-cap cell image roll (complete 2026-09-05, selector gen 148), plus the
follow-up checklist, roadmap, and the Roll 2 implementation plan.
Docs only; split out of #18565 so the record merges independently of the code.
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
…ap, and Roll 2 plan (stablyai#18958)

Operator record for the 2026-09-04 relay reconnect incident and the Roll 1
same-cap cell image roll (complete 2026-09-05, selector gen 148), plus the
follow-up checklist, roadmap, and the Roll 2 implementation plan.
Docs only; split out of stablyai#18565 so the record merges independently of the code.
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.

1 participant