Sign up
View stargazersSign up

fix(server): recover settings and keybinding watches#21972

open6
scheduler-tasks-05/acp-resume
scheduler-tasks-05/watch-recovery
opened15d agobyJJulius Marminge
5 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

A failed or ended settings or keybinding watcher permanently stops updates. Changes during recovery can also be missed.

Change

Restart watches with capped backoff, attach before reconciling, retry failed reconciliation reads, and rebuild settings symlink topology. Preserve cache, debounce, and scoped cancellation behavior.

Rebuilt from @t3dotgg’s #15040, keeping this layer to one concern. Six original claims are already fixed on the pinned main, including configured-home session forking. Those implementations are preserved.

Stack: #21969 → #21970 → #21971 → #21972 → #21974 → #21975 → #21976

Scope and approval

Maintainer takeover authorized for batch scheduler-tasks-05. Layer 4 of seven; coordinator owns landing and approval decisions. This is a focused correction to existing behavior.

Verification

Settings and keybindings suites: 93 passed. Recovery cases verify effective subscriber/cache output after failure or normal end, lazy attachment, failed recovery reads, and a repoint during watch attachment using Deferred and TestClock.

Fresh post-fix verification: workflow/transcript 8, selected ACP 11, complete launch suite 87: 106 distinct passes, independently reproduced by Opus. Four previously failing regressions now pass. Earlier unaffected checks are reused; the historical 422 count is not a fresh run. Full server TS7 found zero errors and one fixture warning (exit 2); the warning was corrected and its scoped fixture/import check passed with zero warnings. Targeted lint and diff checks pass.

R5 Opus and Sol cleared all seven layers and the cumulative diff. Four completed parallel rounds: R1, R3, R4, R5; R2 was cancelled before verdict. Current head-specific before/after evidence and limits.

This layer: d357b9d, based on 6192355. CI will receive one delayed check; unfinished checks are handed to the coordinator.

Headless Linux, Node 24. Native Windows, macOS, iOS, Android, packaged Electron, real account-based provider execution, and relay/tunnel connections were not verified. No UI behavior changed.

Implementation: Codex gpt-6.1-sol; tests: Codex gpt-6-astra; review: Claude claude-opus-5-5 and Codex gpt-6-sol, through T3 Code.

Checks

2 failing · 18 successful · 10 skipped
Some checks failed

Failing2

745cc23
MmacroscopeappBot15d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This production server change adds watcher restart, retry, backoff, startup-readiness, and symlink-topology recovery across settings and keybinding paths. The scope is focused and well-tested, but the new concurrent lifecycle behavior warrants human review.

No code changes detected at 745cc23. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

Ggithub-actionsBot15d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire—5.0 KiB—6.8 KiB✅
CodexThread snapshot wire—3.8 KiB—4.9 KiB✅
CodexLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
CodexLive turn WebSocket decoded—20.9 KiB—29.3 KiB✅
CodexLive turn messages—2—8✅
ClaudeTotal thread wire—5.0 KiB—6.8 KiB✅
ClaudeThread snapshot wire—3.8 KiB—4.9 KiB✅
ClaudeLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
ClaudeLive turn WebSocket decoded—21.2 KiB—29.3 KiB✅
ClaudeLive turn messages—2—8✅

Baseline: unavailable · PR result: 745cc23 · Source CI: failure

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

CcoderabbitaiBot15d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: ff1eabaa-3b30-43bf-b644-a83533a432fe
📥 Commits

Reviewing files that changed from the base of the PR and between d357b9d56bc81785098a0766737a7a9c249057a2 and 745cc238f276256f7a469e11410fdb2f1b4b7349.

📒 Files selected for processing: 2
  • apps/server/src/keybindings.ts
  • apps/server/src/serverSettings.ts

Limit details: You’ve used all 10 included reviews currently available.

📝 Walkthrough

Keybindings and settings watchers now use a shared restart lifecycle. The lifecycle waits for watcher attachment, retries reconciliation after restarts, and restarts after watcher failure or completion. Tests cover recovery and later file changes.

Changes: Watcher Recovery

Layer / File(s)Summary
Watcher restart lifecycle
apps/server/src/restartingWatch.ts		Adds restartingWatch to await watcher readiness, retry reconciliation after restarts, and restart after watcher failure or completion.
Keybindings watcher recovery
apps/server/src/keybindings.ts, apps/server/src/keybindings.test.ts		The keybindings watcher signals attachment and uses the restart lifecycle. Tests cover failure, normal completion, recovery reads, and later file events.
Settings watcher recovery
apps/server/src/serverSettings.ts, apps/server/src/serverSettings.test.ts		The settings watcher coordinates root, symlink, and target watch readiness and uses the restart lifecycle. Tests cover watcher termination, symlink changes, recovery, and subsequent edits.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant KeybindingsOrSettings
3 participant restartingWatch
4 participant FileWatcher
5 participant Revalidation
6 KeybindingsOrSettings->>restartingWatch: Start watcher lifecycle
7 restartingWatch->>FileWatcher: Attach watcher
8 FileWatcher-->>restartingWatch: Signal readiness or terminate
9 restartingWatch->>Revalidation: Reconcile after restart
10 restartingWatch->>FileWatcher: Restart after delay

Suggested reviewers: yordis, t3dotgg

Merge Risk: ⚪ Minimal · up to 745cc

No confirmed watcher-recovery issue blocks merging. Validate watcher readiness on supported platforms as part of normal release checks.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly identifies the server-side fix: recovering settings and keybinding watchers after they stop.
Description checkPassedThe description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure mode, implementation, authorization, test coverage, observed results, and …
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure mode, implementation, authorization, test coverage, observed results, and unverified environments.

✨ Finishing Touches: 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

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

Added 6 commits

  • test(server): cover watcher recovery through settings consumers3e06f24
  • test(server): preserve injected watcher read failures47ded3d
  • fix(server): validate settings watch topology after attachment745cc23
  • fix(server): recover settings and keybinding watches after disconnects75f1c14
  • fix(server): recover when either settings watch ends892e439
  • test(server): cover settings symlink repoint during watch recoveryea95f9e