Fix relay reliability, web client over relay, and Connections local/remote scoping#846
Conversation
…ections scoping Relay (brain-side; infra was healthy — 426 host route, matching claim secret): - Stop tearing down the relay control socket while it is still connecting — a transient account-lease re-check or overlapping reconnect no longer closes an in-flight socket, and one transient lease/refresh failure is tolerated up to real token expiry. This is why the socket never established (lastSuccessAt null) and no relay endpoint was published, so the web client timed out. - Relay observability: capture the real close code / unexpected-response status and body, split lastControlOpenAt from lastBridgeValidationAt, log sync_tunnel.* events, and surface a relay skipReason in `ade sync status`. - Confirmed-409 recovery path to re-mint the relay identity (defense in depth). Web client over relay: - Fix the Files tab showing "no files": project switches could bind after failure, file requests used a stale active project id, and request failures were masked as empty — now switches validate, the bound id is explicit, and real failures surface. - Fix a second connections-pane null-deref crash. - Latency (client-side only, net-lower relay traffic, zero added Cloudflare cost): PR lists serve from one cached prs.getMobileSnapshot, identical hot reads coalesce, the redundant immediate retry is removed, and changeset bursts stop triggering unrelated refetch storms. Connections panel: - The "This Mac" card, pairing code, and device lists now always represent the physically-local machine via a new local-only sync status path, so a remote-bound window no longer shows the remote machine mislabeled as "This Mac" or its stale browsers. Device lists are scoped/labeled, and pairing mutations are held read-only while remote-bound to avoid hitting the wrong machine. Cost: account-directory publish heartbeat 30s -> 60s (90s online window keeps margin), halving that surface's request volume; relay DO hibernation unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Single-source the peer->device projection and the coalescing read-cache (were duplicated across the web adapter and the connections panel by the parallel workstreams). - Relay routeHealth exposes one skipReason field (was a redundant reason+skipReason that consumers read inconsistently). - Extract useSyncConnections from the oversized SyncDevicesSection. - Correctness: a recoverable transient error no longer caches an empty fallback for 3s (same class as the files-tab-empty bug), and a failed getLocalStatus degrades instead of falling through to showing the remote machine's data as "This Mac". Both regression-tested. - Mechanical: domain->event map, RelayClaimError param-property. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ay data model Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ct lan candidate kind The relay observability change added a getLoopbackValidationStatus() call in the shared sync service; the desktop test's host mock needed the method. The pairing-address test's stale 'saved' expectation is corrected to 'lan' to match the authoritative candidate-kind behavior (production code unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughChangesThe pull request updates relay identity recovery and diagnostics, heartbeat fallback scheduling across CLI, desktop, and iOS clients, machine-local sync status APIs, remote-bound desktop device scoping, and web adapter caching, invalidation, project binding, and PR/file read behavior. Relay control and identity recovery
Desktop local sync scope
Web adapter caching and binding
Heartbeat fallback scheduling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/sync/syncCloudRelayStore.ts`:
- Around line 201-209: Update rotateMachineIdentity and setRelayUrl to use a
shared locked read-modify-write primitive for every whole-file configuration
update, rather than locking rotations only. Ensure the primitive acquires the
rotation lock, loads the latest config while locked, applies the update, writes
the result, and releases the lock; preserve rotateMachineIdentity’s
expectedMachineKey check and return behavior.
- Around line 157-175: Update acquireRotationLock and its release logic to use
an owner token/PID in the lock file, avoiding stale-lock deletion when the
recorded owner is still alive. When releasing, read the lock contents and unlink
only if they still match this acquisition’s token; preserve locks belonging to a
replacement owner and handle missing files safely.
- Around line 129-141: Update the identity selection logic around
validMachineKey, validSecret, and mintIdentity so that if either persisted field
fails validation, both machineKey and secret come from the newly minted
credential pair. Only reuse persisted values when both fields are valid, and
preserve the existing missing-identity error check.
In `@apps/ade-cli/src/services/sync/syncTunnelClientService.ts`:
- Line 159: Replace the boolean claimed cache used by claimOnce() with cached
relay-origin and machine-key identity values, and only skip claiming when both
match the current relay URL and identity() result. Reset or update these cached
values when either changes so external store rotation and relay changes trigger
a new claim. Add a regression test covering external rotation and repeated claim
attempts with the new identity.
In
`@apps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsx`:
- Around line 54-76: Wrap the ConnectionsPanel test body after assigning
window.ade in a try/finally block so cleanup always runs, including when
rendering or assertions fail. In finally, unmount the rendered view when
available, dispose the adapter, and restore the saved previousAde global.
In `@apps/desktop/src/renderer/webclient/adapter/infra/cacheKey.ts`:
- Around line 1-10: Update stableCacheKey so undefined, null, NaN, and
positive/negative Infinity produce distinct, deterministic cache-key
representations instead of collapsing to "null". Use explicit type tags or
another JSON-wire-compatible canonicalization, and add regression coverage
confirming at least null and undefined remain distinct.
In `@apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts`:
- Around line 12-19: Update the cache entry lifecycle in get and the
corresponding write path to schedule eviction when each entry’s TTL reaches
expiresAt. Remove the entry only if the map still contains that exact entry, so
newer values for the same key are preserved; retain the existing read-time
expiry check as a fallback.
In `@apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts`:
- Around line 50-62: Update getProjectId() so that when project is bound but
neither boundProjectId nor a matching catalogProjects entry exists, it returns
null instead of falling back to client.getStatus().activeProjectId. Preserve the
existing bound ID and catalog match precedence, and retain the active-project
fallback only when no project is bound.
In `@apps/desktop/src/renderer/webclient/adapter/misc.ts`:
- Around line 181-185: Update the sync adapter object around getStatus and
getLocalStatus so routed status and physical-local status are represented
separately. Ensure useSyncConnections can detect when ADE Web is bound to
another machine, and expose device management as unavailable in that case rather
than enabling unsupported no-op mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 676f397e-6081-49e6-b322-836479d97307
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (47)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.tsapps/ade-cli/src/services/sync/syncCloudRelayStore.test.tsapps/ade-cli/src/services/sync/syncCloudRelayStore.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncLoopbackCollision.test.tsapps/ade-cli/src/services/sync/syncPeerService.test.tsapps/ade-cli/src/services/sync/syncPeerService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/services/sync/syncTunnelClientService.test.tsapps/ade-cli/src/services/sync/syncTunnelClientService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/desktop/src/renderer/components/settings/useSyncConnections.tsapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsxapps/desktop/src/renderer/webclient/adapter/files.tsapps/desktop/src/renderer/webclient/adapter/index.tsapps/desktop/src/renderer/webclient/adapter/infra/cacheKey.tsapps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.tsapps/desktop/src/renderer/webclient/adapter/infra/commandCaller.tsapps/desktop/src/renderer/webclient/adapter/infra/invalidation.tsapps/desktop/src/renderer/webclient/adapter/infra/projectState.tsapps/desktop/src/renderer/webclient/adapter/lanes.tsapps/desktop/src/renderer/webclient/adapter/misc.tsapps/desktop/src/renderer/webclient/adapter/project.tsapps/desktop/src/renderer/webclient/adapter/prs.tsapps/desktop/src/renderer/webclient/adapter/remoteRuntime.tsapps/desktop/src/renderer/webclient/shell/WebClientRoot.tsxapps/desktop/src/renderer/webclient/sync/__tests__/sync.test.tsapps/desktop/src/renderer/webclient/sync/connection.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADETests/ADETests.swift
…tatus Address review threads on PR #846: - syncCloudRelayStore: regenerate machineKey+secret as one atomic pair; scope the rotation lock to a live PID/token owner (no age-only steal); serialize every whole-file config write through that lock. - syncTunnelClientService: key the claim cache by (relay origin, machine key) so a rotated identity forces a fresh claim. - webclient cacheKey: type-tagged, key-sorted, injective encoding that distinguishes missing/undefined/null/non-finite args (no collisions). - coalescingReadCache: evict resolved entries when their TTL expires. - projectState: a bound-but-unresolved project returns null, never another project's active id while switching. - webclient misc: keep routed host status distinct from physical local ADE Web status; disable local device management while remote-bound. - Tests restore window.ade / dispose the adapter in finally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed
Note: the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da6c792515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…me is down Codex re-review of da6c792 surfaced: - cacheKey encoder: the typeof switch had no exhaustive tail, so tsc (TS2366) failed — add a throwing default for unreachable typeof values. - misc localSyncSnapshot used "idle", not a SyncTailnetDiscoveryState (TS2322) — use "disabled" for the web client's inactive discovery. - useSyncConnections.refresh: settle getStatus/listDevices independently from getLocalStatus so a remote-bound window whose routed runtime is offline still renders the physical Mac instead of a load error; routed context is kept best-effort and never clears the local snapshot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed
Only remaining red is |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts (1)
64-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the invalid-
machineKeycase independently.This only exercises an invalid
secret, despite requiring both credentials to rotate when either half is invalid. Add the converse case with a valid secret and malformedmachineKey.Proposed additional coverage
+ it("regenerates both halves when the persisted machine key is invalid", () => { + const oldSecret = "b".repeat(48); + fs.writeFileSync(filePath, `${JSON.stringify({ + machineKey: "too-short", + secret: oldSecret, + relayUrl: "https://relay.example.com", + })}\n`); + + const repaired = createSyncCloudRelayStore({ filePath }).getConfig(); + + expect(repaired.machineKey).toMatch(/^[a-f0-9]{32}$/); + expect(repaired.secret).toMatch(/^[a-f0-9]{48}$/); + expect(repaired.secret).not.toBe(oldSecret); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts` around lines 64 - 78, Add an independent test alongside the existing regeneration test for a valid 48-character hexadecimal secret paired with a malformed machineKey, then call createSyncCloudRelayStore({ filePath }).getConfig() and assert both credentials are regenerated, including that the new machineKey differs from the persisted invalid value and the repaired configuration is written back.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/sync/syncCloudRelayStore.ts`:
- Around line 232-245: Update releaseRotationLock so the finally block does not
return when the current owner token differs from lease.owner.token. Instead,
conditionally perform the unlinkSync cleanup only when the tokens match,
preserving the existing ENOENT handling and allowing any closeSync exception to
propagate.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts`:
- Around line 64-78: Add an independent test alongside the existing regeneration
test for a valid 48-character hexadecimal secret paired with a malformed
machineKey, then call createSyncCloudRelayStore({ filePath }).getConfig() and
assert both credentials are regenerated, including that the new machineKey
differs from the persisted invalid value and the repaired configuration is
written back.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb8a5206-7b6b-4355-ac99-5640e764221c
📒 Files selected for processing (11)
apps/ade-cli/src/services/sync/syncCloudRelayStore.test.tsapps/ade-cli/src/services/sync/syncCloudRelayStore.tsapps/ade-cli/src/services/sync/syncTunnelClientService.test.tsapps/ade-cli/src/services/sync/syncTunnelClientService.tsapps/desktop/src/renderer/components/settings/useSyncConnections.tsapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsxapps/desktop/src/renderer/webclient/adapter/infra/cacheKey.tsapps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.tsapps/desktop/src/renderer/webclient/adapter/infra/projectState.tsapps/desktop/src/renderer/webclient/adapter/misc.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/webclient/adapter/tests/connectionsPane.test.tsx
- apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts
- apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts
- apps/ade-cli/src/services/sync/syncTunnelClientService.test.ts
- apps/desktop/src/renderer/components/settings/useSyncConnections.ts
- apps/ade-cli/src/services/sync/syncTunnelClientService.ts
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
CodeRabbit: the finally block's early return could swallow a pending fs.closeSync exception (noUnsafeFinally). Nest the best-effort unlink under the ownership-token check instead of returning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Pushed the final review fix ( The only remaining CI red is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63da993a8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // two host intervals avoids a second app-level ping/pong cycle on healthy | ||
| // relay connections while still probing well inside the host's mobile miss | ||
| // budget. | ||
| return UInt64(serverMs * 2) * 1_000_000 |
There was a problem hiding this comment.
Keep iOS silence probes inside the host miss budget
When the iOS client is on an expensive/constrained path and inbound traffic stops without a close event, this doubled fallback interval is passed into syncShouldProbeTransportAfterHeartbeatSilence, which multiplies it again (3x/4x). That delays the transport probe to 6 or 8 host heartbeat intervals, while syncHostService drops mobile peers after 6 missed host heartbeats, so constrained clients can wait until after the host has already timed them out before forcing a reconnect. Keep the probe threshold based on the advertised host interval, or otherwise cap the fallback probe below the host mobile miss limit.
Useful? React with 👍 / 👎.
Summary
Dogfooding follow-up (v1.2.29). Fixes the relay never connecting, the web client being near-unusable over relay, and the Connections panel conflating local vs remote-bound machines.
Relay (brain-side; infra confirmed healthy — 426 host route, matching claim secret):
relay.skipReasoninade sync status— no more black box.Web client over relay:
Connections panel:
sync.getLocalStatusIPC → the "This Mac" card, pairing code, and device lists always represent the physically-local machine, so a remote-bound window no longer shows the remote machine mislabeled as "This Mac" or its stale browsers. Pairing mutations held read-only while remote-bound (can't hit the wrong machine).Cost: account-directory publish heartbeat 30s→60s (90s online window keeps margin), halving that surface's requests; relay DO hibernation unchanged. Target: ≤$5/mo Cloudflare at 30-50 users.
Testing
/quality dual-review (0 Blockers; relay-lifecycle security invariants verified — sign-out closes relay, no dual sockets, tolerance bounded by token expiry; 8 findings applied incl. 2 correctness fixes with regression tests). /test: logging gate not-applicable (no analytics changes), docs validated (194 files), parity all-clean (mobile contract-safe both skew directions, CLI formatter surfaces skipReason, TUI unaffected), CI shard 3/8 green (766), sync/account suites green.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR improves ADE Relay reliability, web-client behavior over relay, and local-versus-remote sync scoping. The main changes are:
Confidence Score: 5/5
Safe to merge with low risk.
No accepted bugs were found in the changed paths reviewed. The highest-risk relay lifecycle and local/remote scoping paths include targeted tests and conservative guards around lease expiry, identity rotation, project cache keys, and mutation routing.
No files require special attention.
What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Web as Web/Desktop Client participant IPC as Preload/IPC participant Local as Local Sync Runtime participant Remote as Remote-bound Runtime participant Relay as ADE Relay participant Dir as Account Directory Web->>IPC: sync.getLocalStatus() IPC->>Local: local machine sync.getStatus Local-->>IPC: physical Mac snapshot IPC-->>Web: This Mac + local devices Web->>IPC: routed project/status/file/PR read alt remote-bound window IPC->>Remote: project-scoped runtime action/sync Remote->>Relay: paired WebSocket traffic when off-LAN Relay-->>Remote: tunneled sync frames Remote-->>IPC: routed result else local project IPC->>Local: local runtime action/sync Local-->>IPC: local result end IPC-->>Web: scoped result Local->>Relay: claim + host control socket Relay-->>Local: control open / pipe requests Local->>Dir: publish reachable endpoints Relay-->>Local: 409 claim conflict Local->>Local: rotate relay identity once Local->>Dir: request republish after validation%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Web as Web/Desktop Client participant IPC as Preload/IPC participant Local as Local Sync Runtime participant Remote as Remote-bound Runtime participant Relay as ADE Relay participant Dir as Account Directory Web->>IPC: sync.getLocalStatus() IPC->>Local: local machine sync.getStatus Local-->>IPC: physical Mac snapshot IPC-->>Web: This Mac + local devices Web->>IPC: routed project/status/file/PR read alt remote-bound window IPC->>Remote: project-scoped runtime action/sync Remote->>Relay: paired WebSocket traffic when off-LAN Relay-->>Remote: tunneled sync frames Remote-->>IPC: routed result else local project IPC->>Local: local runtime action/sync Local-->>IPC: local result end IPC-->>Web: scoped result Local->>Relay: claim + host control socket Relay-->>Local: control open / pipe requests Local->>Dir: publish reachable endpoints Relay-->>Local: 409 claim conflict Local->>Local: rotate relay identity once Local->>Dir: request republish after validationReviews (3): Last reviewed commit: "Avoid returning from finally in relay-lo..." | Re-trigger Greptile