Fix tmux dashboard resilience#239
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 13 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR tightens tmux client-session matching to exact ChangesTmux client sessions and dashboard lifecycle hardening
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/multiplexer/dashboard-control.ts (1)
321-337: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStop success verification once the timeout has settled the repair.
If the timeout fires while
probeRuntimeGuard()is awaiting,succeed()can resume and still callrefreshDashboardModelThroughApi()before the finalsettledcheck. Bail after each await so a timed-out repair cannot keep mutating dashboard state.Proposed fix
const probed = await probeRuntimeGuard(projectRoot); + if (settled) return; if (probed.kind !== "ok") { fail(`aimux repair completed but the control plane is still ${describeRuntimeGuardState(probed)}`); return; } - if ( - isDashboardLifecycleCurrent(host, lifecycle) && - !(await refreshDashboardModelThroughApi(host, { force: true, lifecycle })) - ) { - fail("aimux repair completed but dashboard data is still unavailable"); - return; + if (isDashboardLifecycleCurrent(host, lifecycle)) { + const refreshed = await refreshDashboardModelThroughApi(host, { force: true, lifecycle }); + if (settled) return; + if (!refreshed) { + fail("aimux repair completed but dashboard data is still unavailable"); + return; + } }Also applies to: 355-364
🤖 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 `@src/multiplexer/dashboard-control.ts` around lines 321 - 337, The succeed flow in dashboard-control.ts can continue after the repair timeout has already settled the operation, because it only checks settled before and after the full sequence. Update succeed() to bail out immediately after each await (after probeRuntimeGuard() and before/after refreshDashboardModelThroughApi()) so a timed-out repair cannot call refreshDashboardModelThroughApi() or otherwise mutate dashboard state; use the existing settled guard and clearRepairTimeout/settled handling in the succeed path, and apply the same pattern to the related timeout-guarded logic around the referenced repair flow.src/dashboard/targets.ts (1)
82-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBackfill the session build stamp before returning a live dashboard.
The new shell validation reads
@aimux-dashboard-buildfrom the host session, but this early return skips the Line 107setSessionOption(...). Existing dashboards that were already live before this change will keep failing validation until something forces a reload.Suggested fix
if (!options.forceReload) { const live = findLiveDashboardTarget(projectRoot, tmux); - if (live) return live; + if (live) { + tmux.setSessionOption(live.dashboardSession.sessionName, "`@aimux-dashboard-build`", dashboardBuildStamp); + return live; + } }🤖 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 `@src/dashboard/targets.ts` around lines 82 - 85, The early return in findOrCreateDashboardTarget skips backfilling the host session build stamp for already-live dashboards, so shell validation keeps failing on existing sessions. Before returning the live target from findLiveDashboardTarget, make sure the session option is initialized by calling setSessionOption with `@aimux-dashboard-build` on the live session, while preserving the existing forceReload behavior in findOrCreateDashboardTarget.
🤖 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 `@scripts/tmux-control.sh`:
- Around line 206-207: The project-root validation in tmux-control.sh is
checking the wrong session metadata, which can reject valid client dashboards.
Update the target_project_root lookup in the validation flow to use the derived
host session variable from the earlier normalization logic, specifically the
validate_host_session path in the same block, so the `@aimux-project-root` check
reads from the host session instead of the original client session.
In `@src/runtime-restart.ts`:
- Around line 148-165: The steal-lock acquisition helper leaves an empty lock
directory behind if writing the owner file fails, which can block later restarts
until the stale timeout. Update tryAcquireRuntimeRestartStealLock so both
acquisition branches clean up the created stealPath when
writeFileSync(joinLockOwnerPath(...)) throws, and make sure the stale-lock retry
path in the same function applies the same cleanup before returning null.
In `@src/tmux/runtime-manager.ts`:
- Around line 386-389: The cleanup in `RuntimeManager`’s link/move failure path
is too broad and can unlink a pre-existing window that was not created by this
call. Update the catch block around the `move-window`/`swap-window` flow so it
only removes links that were newly created during the current operation, using
the relevant link-tracking variables in `runtime-manager.ts` (for example the
`linked` state and the window-linking logic around the catch path). Ensure
previously existing links in `clientSessionName` are preserved when repair
fails.
---
Outside diff comments:
In `@src/dashboard/targets.ts`:
- Around line 82-85: The early return in findOrCreateDashboardTarget skips
backfilling the host session build stamp for already-live dashboards, so shell
validation keeps failing on existing sessions. Before returning the live target
from findLiveDashboardTarget, make sure the session option is initialized by
calling setSessionOption with `@aimux-dashboard-build` on the live session, while
preserving the existing forceReload behavior in findOrCreateDashboardTarget.
In `@src/multiplexer/dashboard-control.ts`:
- Around line 321-337: The succeed flow in dashboard-control.ts can continue
after the repair timeout has already settled the operation, because it only
checks settled before and after the full sequence. Update succeed() to bail out
immediately after each await (after probeRuntimeGuard() and before/after
refreshDashboardModelThroughApi()) so a timed-out repair cannot call
refreshDashboardModelThroughApi() or otherwise mutate dashboard state; use the
existing settled guard and clearRepairTimeout/settled handling in the succeed
path, and apply the same pattern to the related timeout-guarded logic around the
referenced repair flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9ea20d6-75c3-4c9e-8185-650073f7d9fe
📒 Files selected for processing (20)
scripts/tmux-control.shsrc/dashboard/targets.test.tssrc/dashboard/targets.tssrc/main.tssrc/meta-dashboard-model.test.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/runtime-guard.test.tssrc/multiplexer/runtime-guard.tssrc/runtime-coherence.test.tssrc/runtime-coherence.tssrc/runtime-restart.test.tssrc/runtime-restart.tssrc/tmux/control-script.test.tssrc/tmux/doctor.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.tssrc/tmux/session-names.ts
✅ Action performedReview finished.
|
|
Fixed the final independent reviewer findings in 8994bc4: dashboard relink fallback now fails if it cannot put the dashboard in slot 0, and timed-out repair children now escalate from SIGTERM to SIGKILL after a bounded grace period so the repair lock cannot be held indefinitely. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Fixed the latest independent reviewer finding in fa0d9ae: SIGKILL no longer releases the repair lock immediately; the lock remains held until the child exit event or later dead-owner reclaim. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/multiplexer/dashboard-control.test.ts (1)
841-872: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert the owner PID liveness probe.
This test would still pass if stale repair locks were never reclaimed. Since the contract is “do not reclaim while the recorded owner is alive,” assert that the mocked
process.killprobe is made before restoring the spy.Proposed test tightening
try { startRuntimeGuardRepair(host as never, { kind: "stale", reason: "service-mismatch" }); + expect(killSpy).toHaveBeenCalledWith(987654, 0); } finally { killSpy.mockRestore(); }🤖 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 `@src/multiplexer/dashboard-control.test.ts` around lines 841 - 872, The test for startRuntimeGuardRepair currently only checks the final state, so it can pass even if the owner-liveness check is never exercised. Tighten the assertion by verifying the mocked process.kill probe is called with the recorded owner PID from owner.json before restoring the spy, and keep the existing expectations that spawn is not called and the lock remains in place. Use the existing startRuntimeGuardRepair, killSpy, and owner.json setup to ensure the “owner still alive” path is actually covered.
🤖 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 `@src/runtime-restart.ts`:
- Around line 299-308: The append fallback in runtime-restart should not leave a
dashboard window behind when slot-0 relinking fails. Update the try/catch around
tmux.linkWindowToSession in runtime-restart.ts so that if the fallback link
succeeds at a non-zero index or throws, the newly linked dashboard window is
removed or moved back before recording the error. Use the existing sessionName,
dashboardTarget, linked.windowIndex, and errorMessage handling to locate the
logic and ensure failed relinks do not pollute the client session.
---
Nitpick comments:
In `@src/multiplexer/dashboard-control.test.ts`:
- Around line 841-872: The test for startRuntimeGuardRepair currently only
checks the final state, so it can pass even if the owner-liveness check is never
exercised. Tighten the assertion by verifying the mocked process.kill probe is
called with the recorded owner PID from owner.json before restoring the spy, and
keep the existing expectations that spawn is not called and the lock remains in
place. Use the existing startRuntimeGuardRepair, killSpy, and owner.json setup
to ensure the “owner still alive” path is actually covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e216abfd-0219-4901-a475-e85e81db59f9
📒 Files selected for processing (10)
scripts/tmux-control.shsrc/dashboard/targets.test.tssrc/dashboard/targets.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/runtime-restart.test.tssrc/runtime-restart.tssrc/tmux/control-script.test.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/tmux-control.sh
- src/dashboard/targets.ts
- src/multiplexer/dashboard-control.ts
- src/tmux/runtime-manager.ts
- src/tmux/runtime-manager.test.ts
- src/runtime-restart.test.ts
- src/tmux/control-script.test.ts
|
Fixed the latest independent reviewer finding in 2143c22: dashboard repair stale-lock reclamation now uses a separate reclaim lock and revalidates the owner before deleting the main repair lock. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Sub-agent review finding resolved in c90068f: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Sub-agent review findings resolved in ec0c102: dashboard relink move/swap now targets stable |
|
@coderabbitai review |
1 participant
Summary
Verification
Summary by CodeRabbit