Stop multiple ADE brains from evicting each other off the relay - #949
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThe PR adds machine-wide relay ownership enforcement, suppresses reconnect loops after relay eviction, deduplicates runtime spawning, exposes relay outage telemetry in CLI status and doctor output, and adds desktop relay-offline banners plus internal analytics support. ChangesRelay coordination and health reporting
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 💡 1📝 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 |
A machine can run several ADE brains (the release app, dev `serve` instances, embedded fallbacks). Every signed-in one of them started the relay tunnel client and the account-directory publisher, and the relay Durable Object keeps exactly one host control socket per machineKey — so the brains evicted each other with close code 4505 in a ~4s loop and ADE Relay stayed dead for all of them, silently. Only `ade doctor` reported it; the desktop UI showed nothing and phones still listed the machine as recently reachable. The tunnel gate checked `sharedSyncListener != null`, i.e. listener PRESENCE, while the comment above it described exclusive ownership. A secondary brain binds an ephemeral fallback listener without ever winning the machine-wide sync host lease, so it passed the check and dialed. - Gate the tunnel AND the account-directory publisher on actually holding the sync host singleton lease, via a new authority registry on syncHostSingleton and a small subscribing gate. The lease is taken after bootstrap runs, so the gate subscribes rather than sampling once, and stops both subsystems if the lease is later lost. - Take the lease on the projectless listener path too: binding the shared listener is hosting sync, with or without a project scope. - Treat relay close 4505 as what it is — another process on this machine claimed the slot — instead of a network drop: suppress redialing, retry on a 60s floor at most 3 times, then stop and report an actionable reason through sync status and `ade doctor`. Suppression is enforced at connectControl, since the 1s account-lease poll dials directly. - Floor reconnect backoff at 1s with decorrelated jitter. Full jitter from zero let two rivals resample near-zero delays forever, and neither ever stayed up the 5s needed to reset the attempt counter. - Surface a relay-offline banner on desktop, quiet for the first two minutes of an outage but immediate for the process-conflict case. Uses a new relayControlFailingSinceMs, because lastFailureAt restamps on every retry and can never measure outage length. - Stop leaking zombie brains: claim the RPC socket BEFORE the sync-host startup loop (which retries forever by design, so a brain whose socket was taken never reached the bind check and lived on — we found 18 on one dev socket), suppress duplicate detached spawns for a socket that already has a live recent one, and reap the child when the dev launcher gives up waiting for it. Verified live: a second brain built from this branch ran 8m alongside the release brain, logged `sync.tunnel_start_skipped` with hasSyncListener true and holdsSyncHostLease false — exactly the case the old gate let through — never dialed the relay, and the primary's relay control socket stayed the same one it had opened an hour earlier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
/quality dual-review found the lease gate reacted to a transient as if it
were a real loss of authority, and that the reaction was itself lossy.
ProjectScopeRegistry.performSyncHostSwitch deactivates the previous sync
host BEFORE activating the target, so within one brain the lease reads
"not held" for the width of every project switch. The gate stopped the
tunnel there — and `stop()` nulls the host listener, which is otherwise
attached exactly once per runtime at construction. The tunnel came back
with a live control socket and no bridge, so every phone connect was
rejected with "host sync listener unavailable" until the brain restarted,
while the UI showed relay as connected. The account-directory publisher
was destroyed and rebuilt on the same edge.
- Re-attach the host listener on every gate-driven start, and ride out a
5s authority gap before tearing anything down. A real loss outlives the
handoff; a handoff never does.
- Only clear eviction suppression when the gate has actually observed a
release. A second project's gate, constructed while the lease is held,
was resetting the 4505 re-attempt budget on every project open.
- Reset the eviction state when a control socket reaches the stability
window. The budget only ever counted up, so four self-healed evictions
over a long-running brain stopped relay for good — and `ade doctor`
reported a healthy relay as owned by another process, permanently.
- Reset lastBackoffDelayMs there too: decorrelated jitter samples from the
previous delay, so the first drop after a healthy session could wait up
to a minute instead of a second.
- Re-arm once after 10 minutes of terminal suppression, so the banner's
advice ("quit the other ADE process") is actually true. Nothing else
redialed.
- One resetControlEvictionState() for all reset sites; the three previous
sites each cleared a different subset.
- Spawn suppression now reports success so the caller still gets its
connect-with-retry instead of failing immediately, and a deliberate
shutdown clears the record it would otherwise be suppressed by.
- Move the spawn record out of the world-writable system temp dir into
~/.ade, hash its key, and stop the suite writing to the real ADE home.
- Dedupe the socket-ownership check into assertBrainSocketUnowned, drop a
dead clamp in computeBackoffMs, and stop the desktop shell re-rendering
on every sync-status push.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rtup loop /test parity review found two honesty gaps left by the previous commits. A host whose relay tunnel is suppressed still advertised its relay URL in pairing connect info, brain_status and hello_ok, because isCloudRelayUsable only checked account sign-in. Phones therefore kept dialing a route the host had deliberately abandoned, and a phone that had already saved the candidate never purged it — the purge is driven by an explicitly null cloudRelayWssUrl. Route health deliberately still reports relay as enabled-but-suppressed, so doctor and the banner keep their reason. The pre-loop socket claim added earlier only sees a socket that ALREADY exists. Two brains started together on a fresh path both pass it; one wins the lease and binds, and the loser waits in the forever-retrying sync-host startup loop without ever reaching its own bind check — the original zombie shape. The loop now takes an abortIf hook, checked before each retry, and `ade serve` aborts with the existing socket_owned_by_other contract once another brain provably owns the socket. Also from the parity passes: - The `ade code` TUI had the same unguarded detached spawn; it now shares the spawn record and clears it on the stale-socket recovery path. - `ade sync status --text` gains a `relay failing since` row, the one relay signal with no CLI surface. - New internal `ade_relay_suppressed` analytics event, edge-triggered once per suppression episode with a 24h dedupe, carrying only a bounded attempt count and a coarse code. This is the only way to see in the field whether multi-brain hygiene actually holds. Docs and dashboard spec updated with it. - Internal docs describe the lease, the 4505 regime and the new brain startup ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ade-cli/src/cli.ts (1)
16300-16307: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThis ownership failure skips
disposeServeResources().Every other
socket_owned_by_otherpath (Lines 16238-16243, 16276-16283) tears down serve resources before throwing. Here the throw escapes with the shared sync listener bound andbrainSyncHostLeasestill held, so the singleton lock file is left describing a process that is about to die and the sync port stays bound until exit.🛡️ Proposed fix
if (fs.existsSync(socketPath)) { - await assertBrainSocketUnowned(socketPath); + try { + await assertBrainSocketUnowned(socketPath); + } catch (error) { + await disposeServeResources(); + throw error; + } try { fs.unlinkSync(socketPath); } catch {} }🤖 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/cli.ts` around lines 16300 - 16307, Update the socket ownership handling around assertBrainSocketUnowned so the socket_owned_by_other failure disposes serve resources before propagating the error. Reuse the same cleanup path used by the other ownership checks, ensuring the shared sync listener and brainSyncHostLease are released before throwing.
🧹 Nitpick comments (2)
apps/ade-cli/src/services/sync/syncTunnelClientService.ts (2)
621-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
reconnectBackoffMssilently overrides the post-eviction floor.
args.reconnectBackoffMs?.(attempt) ?? options.delayMsmeans any consumer supplying the backoff hook loses the 60s eviction floor and redials into the war. It is documented as a test seam, but the precedence is easy to trip over; consider honoring an explicitoptions.delayMsfirst, or passing a reason to the hook.♻️ Optional precedence tweak
- const computedDelay = args.reconnectBackoffMs?.(attempt) - ?? options.delayMs - ?? computeBackoffMs(attempt, Math.random, lastBackoffDelayMs); + const computedDelay = options.delayMs + ?? args.reconnectBackoffMs?.(attempt) + ?? computeBackoffMs(attempt, Math.random, lastBackoffDelayMs);Note that the existing eviction tests rely on the current order only where no hook is supplied, so this change should be test-neutral — worth confirming.
🤖 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/syncTunnelClientService.ts` around lines 621 - 645, Update scheduleReconnect so an explicit options.delayMs takes precedence over args.reconnectBackoffMs, preserving the post-eviction delay floor even when the backoff hook is configured; retain the hook for reconnects without an explicit delay and leave the existing normalization and timer behavior unchanged.
669-734: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEviction retries also inflate the network
attemptcounter.
scheduleReconnect({ delayMs })incrementsattemptand setslastBackoffDelayMs = 60s+, so the "eviction bookkeeping is kept separate fromattempt" comment at Line 370 isn't quite true: after an eviction episode that never reaches the stability window, ordinary network reconnects inherit a maxed-out exponential/decorrelation state. Behaviorally conservative (longer waits), just worth acknowledging or resetting alongside the suppression state.🤖 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/syncTunnelClientService.ts` around lines 669 - 734, Update the eviction retry path in noteControlReplaced and scheduleReconnect so control-replacement retries do not increment the ordinary network attempt counter or update lastBackoffDelayMs. Keep those counters reserved for normal reconnects, and ensure clearControlSuppression still resets suppression state without inheriting eviction backoff.
🤖 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/relayTunnelAuthorityGate.ts`:
- Around line 97-125: Serialize tunnel lifecycle transitions in the relay tunnel
authority gate so a new start cannot overlap a pending stop and be shut down by
it; update startTunnel, stopTunnel, and apply to queue operations or use a
generation guard while preserving lease/listener behavior. Add an interleaving
test using a deferred tunnel.stop() that releases the lease, begins stopping,
reacquires it, and verifies the restarted tunnel remains active after the
deferred stop settles.
---
Outside diff comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 16300-16307: Update the socket ownership handling around
assertBrainSocketUnowned so the socket_owned_by_other failure disposes serve
resources before propagating the error. Reuse the same cleanup path used by the
other ownership checks, ensuring the shared sync listener and brainSyncHostLease
are released before throwing.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncTunnelClientService.ts`:
- Around line 621-645: Update scheduleReconnect so an explicit options.delayMs
takes precedence over args.reconnectBackoffMs, preserving the post-eviction
delay floor even when the backoff hook is configured; retain the hook for
reconnects without an explicit delay and leave the existing normalization and
timer behavior unchanged.
- Around line 669-734: Update the eviction retry path in noteControlReplaced and
scheduleReconnect so control-replacement retries do not increment the ordinary
network attempt counter or update lastBackoffDelayMs. Keep those counters
reserved for normal reconnects, and ensure clearControlSuppression still resets
suppression state without inheriting eviction backoff.
🪄 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 Plus
Run ID: 04add91c-c86a-4d92-8a0d-201d42f851cf
⛔ 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/logging.mdis excluded by!docs/**
📒 Files selected for processing (26)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/commands/doctor.test.tsapps/ade-cli/src/commands/doctor.tsapps/ade-cli/src/services/runtime/runtimeSpawnRecord.test.tsapps/ade-cli/src/services/runtime/runtimeSpawnRecord.tsapps/ade-cli/src/services/sync/relayTunnelAuthorityGate.test.tsapps/ade-cli/src/services/sync/relayTunnelAuthorityGate.tsapps/ade-cli/src/services/sync/syncHostSingleton.test.tsapps/ade-cli/src/services/sync/syncHostSingleton.tsapps/ade-cli/src/services/sync/syncHostStartupLoop.test.tsapps/ade-cli/src/services/sync/syncHostStartupLoop.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/services/sync/syncTunnelClientService.test.tsapps/ade-cli/src/services/sync/syncTunnelClientService.tsapps/ade-cli/src/tuiClient/__tests__/connection.test.tsapps/ade-cli/src/tuiClient/connection.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/shared/types/productAnalytics.tsapps/desktop/src/shared/types/sync.tsscripts/dev-shared.mjsscripts/posthog/dashboard-spec.mjs
arul28
force-pushed
the
ade/pr1-brain-tunnel-hygiene-f82b793f
branch
from
July 29, 2026 19:29
a81d112 to
ade91bc
Compare
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ade91bcea6
ℹ️ 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".
| */ | ||
| const noteControlReplaced = (): void => { | ||
| controlReplacedAttempts += 1; | ||
| controlSuppressedReason = RELAY_CONTROL_REPLACED_MESSAGE; |
There was a problem hiding this comment.
When a running tunnel receives close code 4505 while the desktop is already open, this updates controlSuppressedReason without triggering a sync-status notification. The tunnel callback wired in bootstrap.ts only requests an account-directory publish, while AppShell performs one seed read and otherwise relies exclusively on sync-status events. On an idle machine, the new suppression banner therefore remains hidden until some unrelated sync activity emits a status snapshot; add a status-change notification for this transition and its recovery.
Useful? React with 👍 / 👎.
| void seed.call(syncApi).then(apply).catch(() => {}); | ||
| } | ||
| const dispose = syncApi.onEvent?.((event) => { | ||
| if (event.type === "sync-status") apply(event.snapshot); |
There was a problem hiding this comment.
When the current project is remote-bound, syncApi.onEvent can deliver the remote runtime's sync-status snapshot because the preload subscription fans out both local IPC and subscribeRemoteSyncStatusEvents. Applying that payload directly overwrites the local snapshot seeded by getLocalStatus, so a remote relay outage can show a warning for this machine, or remote health can hide this machine's outage. Treat the event only as an invalidation and reread getLocalStatus, as useSyncConnections already does.
Useful? React with 👍 / 👎.
| const { hasRecentRuntimeSpawn, recordRuntimeSpawn } = await import( | ||
| "./services/runtime/runtimeSpawnRecord" | ||
| ); | ||
| if (hasRecentRuntimeSpawn(socketPath)) return true; |
There was a problem hiding this comment.
When two CLI processes reach an absent record concurrently, both can pass this check before either child is spawned and recorded, so both launch detached brains; recordRuntimeSpawn then merely overwrites the same JSON file. This is the burst-start scenario the new record is intended to prevent, and on named-pipe platforms the losing brain cannot use the new socket liveness abort and can remain in the sync-host retry loop indefinitely. Use an atomic per-socket claim/lock around the check-and-spawn sequence rather than a read followed by a later write.
Useful? React with 👍 / 👎.
Addresses all four review findings from CodeRabbit and Codex on #949. Publish relay suppression transitions (Codex). A running tunnel that took a 4505 updated controlSuppressedReason without emitting a sync-status snapshot, and the desktop only seeds once and then listens. On an idle machine the new banner therefore stayed hidden until some unrelated sync activity happened along — exactly the silence this change exists to end. Entering and leaving suppression now publish route state, and syncService grew notifyRouteStateChanged so the machine-level tunnel can nudge a project-scoped emitter. Treat routed sync events as invalidation, not payload (Codex). On a remote-bound project the preload subscription fans out the remote runtime's snapshot too, so applying it directly let a remote outage raise a warning about this machine, or let remote health mask this machine's own. AppShell now re-reads getLocalStatus on every event, matching useSyncConnections. Make the runtime spawn claim atomic (Codex). Two CLI processes reaching an absent record concurrently both passed the check and both launched a detached brain — the burst the record exists to prevent, and on named-pipe platforms the loser cannot even use the socket-liveness abort. The TUI already had the right primitive, so withSocketSpawnLock moves to services/runtime/socketSpawnLock.ts and now wraps check-and-spawn on both paths. Three independent reviewers flagged this duplication; it is now one mechanism. Serialize tunnel lifecycle transitions (CodeRabbit). stop() and start() are async, so a lease reacquired mid-teardown could be shut down by the stop already in flight. A generation token guards both, and a stop that settles after a newer start re-establishes the tunnel instead of leaving it down. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 17, 2026
…or screens - Diagnostic report + "Report issue": redacted report (paths, users, emails, tokens, IPs, hostnames stripped; PostHog installId kept for correlation), copied to clipboard + saved, prefilled GitHub issue opened; button on every failure surface; `ade report-issue [--open]` CLI counterpart. - systemd installer gets the same handover as launchd (it had none); shared serviceHandover.ts; Windows `supervised` requires an identity-verified pid; brain re-checks its socket inode so the #949 zombie stays closed; --no-sync brains no longer wipe the crash-loop record; `starting` handled by ade brain restart, ade connect, ade setup, TUI repair (never spawns a rival brain). - install-runtime.sh stages, preflights the staged binary, promotes by rename and rolls back on failure; CI runs the new rollback test. - Error/recovery screens rebuilt on a shared kit (recovery, renderer/page crash, transition alert, CTO, storage cleanup, welcome notices, update banner) — verified with real screenshots at 900/1400. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 17, 2026
…or screens - Diagnostic report + "Report issue": redacted report (paths, users, emails, tokens, IPs, hostnames stripped; PostHog installId kept for correlation), copied to clipboard + saved, prefilled GitHub issue opened; button on every failure surface; `ade report-issue [--open]` CLI counterpart. - systemd installer gets the same handover as launchd (it had none); shared serviceHandover.ts; Windows `supervised` requires an identity-verified pid; brain re-checks its socket inode so the #949 zombie stays closed; --no-sync brains no longer wipe the crash-loop record; `starting` handled by ade brain restart, ade connect, ade setup, TUI repair (never spawns a rival brain). - install-runtime.sh stages, preflights the staged binary, promotes by rename and rolls back on failure; CI runs the new rollback test. - Error/recovery screens rebuilt on a shared kit (recovery, renderer/page crash, transition alert, CTO, storage cleanup, welcome notices, update banner) — verified with real screenshots at 900/1400. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 17, 2026
* Stop reporting a slow-starting ADE brain as a broken one Fresh installs and every auto-update could land on "Updated the app, but the background service couldn't be set up — click Repair" followed by the full-screen "ADE couldn't open this project", and Repair only made it worse. The brain was healthy every time; it just had not answered yet. Three things conspired: - The brain bound `ade.sock` only AFTER the mobile sync-host startup loop (project scope/DB open, sync port band, lease), so desktop reachability was coupled to phone-sync hosting and took seconds even on a fast machine. - The launchd installer gave the replacement 10 s to answer, then reported `replacement_responsive` as a failed install; the desktop turned that into a failed update step and skipped its own connect wait entirely. - Every Repair path killed the booting brain and restarted the same 10 s race, so on a slow machine it could never finish starting. Fixes: - Brain: bind the RPC socket first, start the sync host in the background (0.4 s to socket vs 3.3 s measured); cross-channel conflicts still end the brain and are recorded; a switch superseded by a concurrent RPC caller is adopted, not read as "no project". - Service manager (mac + Windows): 30 s shared handover budget, probe timeout no longer kills a slow-starting probe, a live-but-quiet replacement returns `ok:true, starting:true`, and a young (<120 s) unresponsive brain behind an unchanged definition is waited on rather than restarted — vetoed by a fresh crash-loop record. `restarted` tells the trust-reset caller what happened. - Desktop: parse `starting`, age the streak of installs (not each attempt), wait 90 s for the socket after a (re)install, new `brain_starting` diagnosis with no Repair offered and auto-reopen polling, repair steps streamed live over IPC, restart budget 90 s. - Copy/dead ends: recovery screen keeps a way out and offers a plain reopen, transition banner gets "Try again", CTO failure pane gets "Try again", BrainRepairButton shows the real reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Report-issue diagnostics, Linux/Windows handover parity, and real error screens - Diagnostic report + "Report issue": redacted report (paths, users, emails, tokens, IPs, hostnames stripped; PostHog installId kept for correlation), copied to clipboard + saved, prefilled GitHub issue opened; button on every failure surface; `ade report-issue [--open]` CLI counterpart. - systemd installer gets the same handover as launchd (it had none); shared serviceHandover.ts; Windows `supervised` requires an identity-verified pid; brain re-checks its socket inode so the #949 zombie stays closed; --no-sync brains no longer wipe the crash-loop record; `starting` handled by ade brain restart, ade connect, ade setup, TUI repair (never spawns a rival brain). - install-runtime.sh stages, preflights the staged binary, promotes by rename and rolls back on failure; CI runs the new rollback test. - Error/recovery screens rebuilt on a shared kit (recovery, renderer/page crash, transition alert, CTO, storage cleanup, welcome notices, update banner) — verified with real screenshots at 900/1400. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * quality: fix hostname redaction, redact issue title, ownership-checked socket unlink, staged preflight in dest dir, shared handover helpers/budgets, shared diagnostics collector, recovery state table, error surface card Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * quality (round 2): attempted-roots registry so a failed first open can still be repaired, case-folded root compare, machine-only diagnostics on unknown root, installer stages runtime on the install volume, comment/dead-code cleanups Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * quality (round 3): Windows installer stages/preflights/promotes under the ADE home, attempted-roots recorded only after repo resolution, ENOENT unlink is absent, single-writer attempted-roots registry Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: consolidate serviceManager suites, prune render-only/silent-pass tests, pin issue_report analytics + machine-only report, TUI/CLI/docs parity (/report-issue, starting states, ade doctor starting row) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ship: apply post-rebase quality revalidation — crash-loop veto, real Windows startup probe, no recursive runtime status, secret-scan fixture Commit-bound quality revalidation of the af6bee5 delta on the rebased head: - brainStartupState: veto "starting" when the brain is crash-looping (recentCrashLoopForAdeHome), so a supervisor respawning a dying brain every few seconds no longer reads as young-and-starting forever and `ade doctor` no longer exits 0 on a permanently broken brain. - brainStartupState: replace the dead win32 branch (its `running` meant "the brain answered", false by construction here) with a real supervisor-record probe using the installer's own predicate (restartCount 0, live runtime pid, young runtimeStartedAtMs). Absent record reports installed: null, not false. - cli: shouldProbeBrainStartupState skips the probe under ADE_DISABLE_RUNTIME_SERVICE_INSTALL=1 (the Windows readiness probe spawns `ade runtime status`, which re-entered the same path) and for a --socket override that is not the machine socket; always set detail; render the starting verdict in --text via formatBrainStatus so the field has a reader. - doctor: drop the zero-injector readBrainStartupState dependency. - TUI: the starting screen no longer repeats/contradicts itself with the raw error line; report-issue copy drops literal backticks and dev-facing wording. - diagnostics: share writeDiagnosticReportFile instead of a second copy of the 0o700/0o600 write; restore the redaction-copy assertion on the desktop Report issue disclosure. - tests: register the watchdog temp homes with the existing cleanup; assemble the synthetic JWT fixture at runtime so gitleaks stops failing secret-scan. - docs: de-duplicate the report-issue analytics section in logging.md and correct its install-id claim to match getDistinctId(); document brainStartupState; match the ade-code starting-screen description. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ship: iteration 2 — fix typecheck-desktop, validate-docs and windows-foundation, address CodeRabbit review CI: - productAnalyticsService.test.ts: drop the `as Record<string, unknown>` cast that no longer typechecks against the sanitizer's parameter (all values are already valid property values). - cli.test.ts: annotate the new cr-sqlite serve test with DARWIN-GATE — booting a real brain needs the macOS-only extension — so the platform-gate ratchet stays at its recorded baseline instead of growing. Review (verified against the code, stale claims dismissed): - projectRecoveryService: a fresh sync_host failure no longer asks for repair while the endpoint answers, and brain_starting is judged before socket reachability — this branch binds the socket before the brain can answer, so the old order reported a booting brain as a stolen socket. - Analytics identity: getDistinctId() loads persisted state and returns null unless analytics are effective, and the CLI honours the disabled marker, so an opted-out install never carries an id into a report. - Diagnostics redaction: placeholders are no longer re-wrapped, and a bracketed real name (`user=<ada>`) is still redacted rather than mistaken for one. - registerIpc: a projectless window can file an issue and diagnose or repair a root it names; the known-roots check still refuses an unknown one. - Installer: signal traps clean up once and exit 129/130/143, a Ctrl-C during the promoted binary's version check restores the backup instead of deleting it, and the post-promotion message believes the disk, not the restore flag. - A crashed Work route leaves for Lanes instead of remounting itself; the storage cleanup dialog ignores a completion from a previous open; the copy button moved out of `<summary>`; repair step labels are exhaustive. - cli.test.ts serve timings fit inside the test timeout, so a stuck brain reports its diagnostic and still kills its child. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ship: iteration 3 — fix the Windows launchd test, redact fine-grained tokens, keep a rollback copy through an interrupted install CI: - serviceManager/common.test.ts: the consolidated launchd handover test drove the host's real parent-pid probe, which on Windows reads "unknown" and fails safe into the self-mutation block. Inject the ancestry like every other install test in the file — this case is about handover windows, not parents. Review (Codex, CodeRabbit; verified against the code): - Diagnostics: redact fine-grained GitHub PATs (`github_pat_`) and the other prefixes ADE actually accepts, run the email rule before the account-name rule so a name that is also an email local part still hides the domain, and keep model ids and snake_case identifiers out of the token rule. - `ade report-issue --open` copies the report before opening the issue, which is what the template it opens tells the user to paste; `--json` reports it. - Installer: an interrupt between promoting the runtime and verifying the new binary now puts BOTH back, and neither backup is deleted unless its restore actually landed or the new binary passed its check. Windows tracks the verified binary separately from the promoted one and mirrors the rule. - Recovery: diagnostic steps are bounded and fail soft, a future install timestamp is not "starting", and the startup streak resets only when the machine service itself connects — not on an isolated runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
ade doctorand sync status accuracy for relay failures.