Skip to content

fix: isolate terminal sessions and serialize sync#138

Draft
centdix wants to merge 1 commit intomainfrom
code-cleanup-optimization
Draft

fix: isolate terminal sessions and serialize sync#138
centdix wants to merge 1 commit intomainfrom
code-cleanup-optimization

Conversation

@centdix
Copy link
Contributor

@centdix centdix commented Mar 15, 2026

Summary

This PR fixes the terminal attach ownership bug, prevents overlapping PR sync runs, coalesces reconciliation on hot paths, and restores the frontend terminal test baseline.

Changes

  • isolate backend terminal sessions per websocket attach id so one viewer cannot detach or hijack another
  • add shared async helpers, serialize PR monitor runs, and coalesce reconciliation while forcing refreshes after lifecycle mutations
  • harden frontend terminal theme updates, add regression coverage, and route lifecycle hook debug output through the gated logger

Test plan


  • bun run --cwd backend check

  • bun test backend/src/tests/lifecycle-service.test.ts

  • bun test backend/src/tests/agent-service.test.ts backend/src/tests/agent-runtime.test.ts backend/src/tests/runtime-events.test.ts backend/src/tests/project-runtime.test.ts backend/src/tests/setup.test.ts backend/src/tests/session-service.test.ts backend/src/tests/worktree-storage.test.ts backend/src/tests/git-adapter.test.ts backend/src/tests/domain-policies.test.ts

  • bun test backend/src/tests/pr.test.ts backend/src/tests/reconciliation-service.test.ts backend/src/tests/terminal-adapter.test.ts

  • bun test bin/src

  • bun run --cwd frontend test

  • bun run --cwd frontend check

  • bun run build

Generated with Claude Code

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Mar 15, 2026

Claude finished @centdix's task in 1m 55s —— View job


Code Review: PR #138 — Isolate terminal sessions and serialize sync

  • Read all changed files and understand the diff
  • Review backend async helpers (lib/async.ts)
  • Review terminal adapter changes
  • Review server.ts session isolation
  • Review reconciliation service changes
  • Review PR service changes
  • Review lifecycle service changes
  • Review hooks adapter changes
  • Review frontend changes
  • Review test coverage
  • Post review summary

Overall Assessment

This is a well-structured PR that addresses several real problems: terminal session isolation, reconciliation coalescing, PR sync serialization, and debug logging hygiene. The changes are clean, well-tested, and the architecture decisions are sound.


Terminal Session Isolation (server.ts, terminal.ts)

The core fix — keying terminal sessions by attachId (worktreeId + UUID) instead of worktreeId alone — is correct and well-executed. This prevents one WebSocket viewer from detaching or hijacking another viewer's session on the same worktree.

The WsData type properly tracks both worktreeId and attachId, and cleanup on close correctly uses attachId. No issues here.


Async Helpers (lib/async.ts)

mapWithConcurrency — Clean extraction from pr-service.ts. One minor improvement over the original: the concurrency calculation now handles empty arrays gracefully with items.length || 1.

startSerializedInterval — Well-designed coalescing primitive. The state machine (running / rerunRequested / stopped) is clear and correct:

  • If a tick fires while already running, it sets rerunRequested and returns
  • After completion, it checks rerunRequested and re-executes if needed
  • Multiple overlapping ticks collapse into a single rerun

The dependency injection for scheduleEvery/cancelSchedule makes it properly testable, which the tests confirm.


Reconciliation Service Changes

The addition of freshness gating and in-flight coalescing to ReconciliationService is a good performance improvement:

  1. Freshness check (default 500ms) — prevents redundant reconciliations on hot paths like apiGetProject
  2. In-flight coalescing — concurrent callers share the same reconciliation promise
  3. Force option — lifecycle mutations (create/open/close/remove) bypass freshness with { force: true }

The reconcilerunReconcile split is clean. The mapWithConcurrency refactor inside runReconcile enables parallel worktree reconciliation (bounded to 4), which should help repos with many worktrees.

One observation: In reconcile(), lastReconciledAt is set inside the .then() of reconcilePromise, but inFlight is cleared in .finally(). This is correct — the timestamp is only updated on success, meaning a failed reconciliation won't prevent retries. Good design choice.


Server.ts — Runtime Event Handling

The change to apiRuntimeEvent is an improvement. Previously it always called reconcile before applyEvent. Now it tries applyEvent first, and only reconciles on "Unknown worktree id" errors (with a retry). This avoids unnecessary reconciliation on the hot path while still self-healing when the runtime state is stale.

The resolveTerminalWorktree change follows the same pattern — check local state first, reconcile only if needed.


PR Service — Serialized Monitor

Replacing the raw setInterval + fire-and-forget pattern with startSerializedInterval prevents overlapping PR sync runs. The run function is now properly async and awaits syncPrStatus, meaning a slow sync won't pile up concurrent runs.


Hooks & Lifecycle — Debug Logging

Replacing console.debug with log.debug in hooks.ts and lifecycle-service.ts routes debug output through the gated logger, which is the correct pattern for this codebase. The log messages are also slightly cleaned up (e.g., envKeys=${count} instead of listing all keys).


Frontend Terminal Changes

The termReady flag and term! definite assignment assertion fix a race condition where $effect could fire before onMount completed, potentially accessing term before initialization. The new guard if (termReady && terminalTheme && term.options) is correct.

The test coverage for theme updates is a good regression test.


Test Coverage

  • terminal-adapter.test.ts — Integration test using isolated tmux sessions verifies the core fix (two concurrent attaches produce two independent sessions, detaching one leaves the other intact). Well-designed test with proper cleanup.
  • pr.test.ts — Tests for mapWithConcurrency (ordering, concurrency limiting, empty input) and startSerializedInterval (coalescing, stop behavior) are thorough.
  • reconciliation-service.test.ts — Tests coalescing behavior and freshness gating with a controlled clock.
  • Terminal.test.ts — Theme update regression test.

No Issues Found

This is a clean, well-thought-out PR. The session isolation fix is correct, the coalescing/serialization primitives are well-tested, and the changes are minimal and focused. No bugs, security issues, or performance concerns identified.

@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
webmux f9c70f6 Mar 15 2026, 01:34 AM

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