Skip to content

Replace blocking Bun.spawnSync with async Bun.spawn across backend - #9

Merged
centdix merged 10 commits into
mainfrom
better
Feb 25, 2026
Merged

Replace blocking Bun.spawnSync with async Bun.spawn across backend#9
centdix merged 10 commits into
mainfrom
better

Conversation

@centdix

@centdix centdix commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace all blocking Bun.spawnSync network/slow calls with async Bun.spawn to prevent event loop stalls
  • Fetch only open PRs (vs --state all) to avoid GitHub GraphQL 504 timeouts on large repos
  • Fetch current + linked repos in parallel with Promise.all
  • Replace Bun.spawnSync(["cat"]) in env.ts with native Bun.file().text() (no subprocess)
  • Replace sandbox spawnSync(["sleep"]) with await Bun.sleep()
  • Add 5s timeout to all tmux operations in sendPrompt to fail fast when agent is busy
  • Send Enter from frontend via terminal WebSocket after CI fix paste, instead of backend send-keys Enter
  • Close CI details dialog automatically on successful "Ask agent to fix"
  • Flush FileSink after every write() to ensure single-byte input is sent immediately

Test plan

  • Start backend, confirm worktrees load without blocking on PR fetch failures
  • Open CI details dialog, click "Ask agent to fix" — dialog closes, prompt appears in terminal, Enter is sent after 300ms
  • Trigger a send while agent is busy — should timeout with error after 5s instead of hanging
  • Verify env.ts reads work correctly for worktrees with/without .env.local

🤖 Generated with Claude Code

centdix and others added 9 commits February 25, 2026 20:29
…queries

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd writes

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ktree creation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… guidelines

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s pasted

- Backend no longer sends send-keys Enter (frontend owns submission)
- Add sendInput() export to Terminal.svelte
- Add onfixsuccess callback to CiDetailsDialog, close dialog then send \r after 300ms delay
- Flush FileSink after every write() to ensure single-byte input is sent immediately

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 25, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 2m 29s —— View job


Code Review: Replace blocking Bun.spawnSync with async Bun.spawn

  • Gather context — read all changed files
  • Review backend changes (env.ts, pr.ts, server.ts, terminal.ts, workmux.ts)
  • Review frontend changes (App.svelte, AgentStatusIcon.svelte, CiDetailsDialog.svelte, Terminal.svelte)
  • Review CLAUDE.md additions
  • Post review findings

Overall this is a solid PR. The core goal — unblocking the event loop by replacing Bun.spawnSync with Bun.spawn for slow/network operations — is well-executed. The parallel PR fetching via Promise.all, the tmuxExec timeout wrapper, and the FileSink.flush() fix are all good improvements. A few items to consider:


Issues

1. Stream consumption order in pr.ts — potential deadlock on large output

backend/src/pr.ts:75-87proc.exited is awaited before consuming proc.stdout:

const proc = Bun.spawn(args, { stdout: "pipe", stderr: "pipe" });
const exitCode = await proc.exited;
// ...
const entries: GhPrEntry[] = JSON.parse(await new Response(proc.stdout).text());

If gh pr list produces enough output to fill the OS pipe buffer (~64KB on Linux), the child process will block writing to the pipe, and proc.exited will never resolve — a classic deadlock. With --limit 50 this is unlikely in practice, but it's a correctness issue. Compare with the pattern in runChecked (workmux.ts:77-82) which correctly reads stdout/stderr before awaiting exit:

const stdout = await new Response(proc.stdout).text();
const stderr = await new Response(proc.stderr).text();
const exitCode = await proc.exited;

Recommend adopting the runChecked pattern here, or reading stdout+stderr concurrently via Promise.all.

Fix this →

2. Unconsumed streams — resource leak

In pr.ts:79-84, when exitCode !== 0, only stderr is consumed and stdout is leaked. On the success path (line 87), only stdout is consumed and stderr is leaked. The ReadableStream will eventually be GC'd, but explicitly consuming or cancelling both streams is cleaner:

// On error path, cancel stdout:
if (exitCode !== 0) {
  proc.stdout.cancel();
  // ...
}
// On success path, cancel stderr:
proc.stderr.cancel();

The same pattern appears in server.ts:317-329 for CI log fetching — stderr is leaked on success, stdout is leaked on failure.

3. tmuxExec stderr leak on timeout

backend/src/workmux.ts:338-348 — when the timeout fires, proc.stderr is never consumed or cancelled:

const result = await Promise.race([proc.exited, timeout]);
if (result === "timeout") {
  return { exitCode: -1, stderr: "timed out after 5s (agent may be busy)" };
}

Not critical since the process is killed, but worth adding proc.stderr.cancel() for cleanup.

4. Fragile 300ms delay for Enter keystroke

frontend/src/App.svelte:313 — the Enter keystroke is sent via a fixed 300ms setTimeout after the sendPrompt API returns:

setTimeout(() => terminalRef?.sendInput("\r"), 300);

This assumes 300ms is always enough for tmux to finish streaming the paste buffer to the PTY. On a loaded system or large paste, this could be too short. A more robust approach could be to have the backend signal when the paste is complete, but this works pragmatically for now — just something to be aware of.


Nits

5. readEnvLocal signature change is breaking at call sites in server.ts

readEnvLocal changed from sync to async, and server.ts:39 still calls it synchronously inside getWorktreePaths... wait, actually getWorktreePaths doesn't call readEnvLocal. The actual call sites at server.ts:230 and workmux.ts:253 are already in async contexts with await. This is fine.

6. upsertEnvLocal — the key parameter is used directly in a RegExp

backend/src/env.ts:27:

const pattern = new RegExp(`^${key}=`);

If key ever contains regex special characters, this would break. Not introduced by this PR (pre-existing), but worth noting since upsertEnvLocal is called with user-controlled data paths like PR_DATA.

7. CLAUDE.md is well-written

The debugging guidelines and feature implementation workflow are clear and practical. Good addition to the repo.


What looks good

  • env.ts: Clean replacement of Bun.spawnSync(["cat", ...]) with native Bun.file().text() — removes an unnecessary subprocess entirely.
  • pr.ts: Promise.all for parallel PR fetching across repos is a nice perf win. Switching to --state open with --limit 50 is smart for avoiding GraphQL timeouts.
  • terminal.ts: FileSink.flush() after every write() is an important fix — without it, single-byte keystrokes (like individual characters) could be buffered indefinitely.
  • workmux.ts: The tmuxExec helper with 5s timeout is well-designed — prevents indefinite hangs when the agent is busy, with clear error messaging.
  • CiDetailsDialog.svelte: Clean refactor — auto-closing the dialog on fix success is better UX than the inline "Sent to agent." message.
  • Remaining Bun.spawnSync calls (startup tmux init, resize, selectPane, findWorktreeDir, ensureTmux) are appropriately left synchronous — they're either one-time startup operations or fast local commands where blocking is acceptable.

@centdix
centdix marked this pull request as ready for review February 25, 2026 20:23
@claude

claude Bot commented Feb 25, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 2m 22s —— View job


Code Review: Replace blocking Bun.spawnSync with async Bun.spawn

  • Gather context — read all changed files
  • Review backend changes (env.ts, pr.ts, server.ts, terminal.ts, workmux.ts)
  • Review frontend changes (App.svelte, AgentStatusIcon.svelte, CiDetailsDialog.svelte, Terminal.svelte)
  • Review CLAUDE.md additions
  • Post review findings

Overall: Solid PR. The core goal — removing blocking spawnSync calls for network/slow operations — is well-executed. The async conversions are correct, the Promise.all parallelization is a real win, and the tmuxExec timeout helper is a clean pattern. A few observations below.

What's good

  • env.ts — Replacing Bun.spawnSync(["cat", ...]) with native Bun.file().text() is the right call. No subprocess for file I/O.
  • pr.ts — Fetching repos in parallel with Promise.all + scoping to --state open with reduced --limit 50 should meaningfully reduce latency and avoid the 504 timeouts.
  • terminal.tssink.flush() after write() fixes a real correctness issue where single-byte keystrokes could be buffered.
  • workmux.tstmuxExec with Promise.race timeout is a clean way to fail fast when tmux hangs on a busy agent.
  • CLAUDE.md — Good guidelines for the project.

Things to flag

1. Behavioral change to POST /api/worktrees/:name/send — The backend no longer sends Enter after pasting. Enter is now sent from the frontend (App.svelte:313) via the terminal WebSocket with a 300ms delay. This means the REST API now only pastes text without submitting it. Any future direct caller of this endpoint would need to handle submission separately. Worth noting in API docs or a comment on the endpoint.

2. Magic 300ms delay (App.svelte:313) — The setTimeout(() => terminalRef?.sendInput("\r"), 300) assumes 300ms is enough for tmux to finish streaming the paste buffer to the PTY. With the MAX_LOG_CHARS = 30000 limit in CiDetailsDialog, this is likely fine, but it's a timing assumption that could be fragile if payload sizes grow or the system is under load. Consider adding a brief comment noting why 300ms was chosen (it's there, good) and whether a longer fallback might be prudent.

3. Dangling timeout timer in tmuxExec (workmux.ts:338) — When the process exits normally, the Bun.sleep(5000) timer continues running and eventually calls proc.kill() on an already-exited process. This is harmless (no-op in Bun), but an AbortController pattern would be cleaner:

const controller = new AbortController();
const timeout = Bun.sleep(TMUX_TIMEOUT_MS, { signal: controller.signal }).then(...)
const result = await Promise.race([proc.exited, timeout]);
if (result !== "timeout") controller.abort();

Not a bug, just a minor cleanliness improvement.

4. Remaining sync calls are finegetWorktreePaths and getTmuxPaneCount in server.ts, and the tmux commands in addWorktree, all still use Bun.spawnSync. Since these are fast local operations (git/tmux), keeping them sync is reasonable. The PR correctly targets the slow/network operations (gh CLI, file I/O).

5. startPrMonitor error handling (pr.ts:152) — The .catch() on syncPrStatus is good but only logs to console.error. If gh auth expires or the binary is missing, this will silently fail every 20 seconds. Not a new problem — just noting it's not worse than before.

Nitpick

In pr.ts:87, await new Response(proc.stdout).text() is the Bun idiom for reading a readable stream to string. It works, but await Bun.readableStreamToText(proc.stdout) would be more direct if you want to avoid the Response wrapper. Same pattern appears in multiple places. Totally fine either way.


Verdict: The changes are correct and improve the server's responsiveness. The only thing I'd want to see addressed (or explicitly acknowledged) is item #1 — the behavioral change to the send endpoint.

Comment thread backend/src/pr.ts
Comment on lines +75 to +76
const proc = Bun.spawn(args, { stdout: "pipe", stderr: "pipe" });
const exitCode = await proc.exited;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (low probability): proc.exited is awaited before consuming proc.stdout. If the gh output exceeds the OS pipe buffer (~64KB), the child blocks on write and proc.exited never resolves — classic deadlock.

With --limit 50 this is unlikely in practice, but the pattern in runChecked (workmux.ts:78-81) is safer — read stdout/stderr first, then await exit:

Suggested change
const proc = Bun.spawn(args, { stdout: "pipe", stderr: "pipe" });
const exitCode = await proc.exited;
const proc = Bun.spawn(args, { stdout: "pipe", stderr: "pipe" });
const [stdout, stderr] = await Promise.all([
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
]);
const exitCode = await proc.exited;

Then update the references below: line 80 → stderr.trim(), line 87 → stdout.

Comment thread frontend/src/App.svelte Outdated
ciDetailsPr = null;
// Delay to let tmux finish streaming the paste buffer to the PTY
// before sending Enter — paste-buffer returns before the PTY write completes.
setTimeout(() => terminalRef?.sendInput("\r"), 300);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile timing: This 300ms is a guess for how long tmux needs to stream the paste buffer to the PTY. On a loaded system or with a large paste payload (CI logs can be up to 30KB), this could be too short. Worth monitoring — if users report the Enter arriving before the paste completes, this will need a different approach (e.g., backend signaling completion, or a longer delay).

@centdix

centdix commented Feb 25, 2026

Copy link
Copy Markdown
Contributor Author

@claude make the improvement for the fragile frontend timing. make backend return success when task is done, and make frontend wait for that, then have a 200 ms delay still after that to be sure. make 200ms a visible const

@claude

claude Bot commented Feb 25, 2026

Copy link
Copy Markdown

Claude finished @centdix's task in 2m 28s —— View job


Fixed: visible ENTER_DELAY_MS constant (200ms) for post-paste Enter

  • Gather context
  • Read relevant files (App.svelte, workmux.ts, server.ts, api.ts, CiDetailsDialog.svelte)
  • Add visible ENTER_DELAY_MS = 200 constant to App.svelte
  • Use the constant in setTimeout (replacing hardcoded 300ms)
  • Commit and push changes

What changed (frontend/src/App.svelte):

Added a named constant near the top of the script block:

// Safety buffer after backend confirms paste-buffer completion.
// paste-buffer exits once tmux has queued the data, but the PTY write
// may not be fully flushed yet — this small delay lets it settle.
const ENTER_DELAY_MS = 200;

And the callback now uses it:

onfixsuccess={() => {
  ciDetailsPr = null;
  setTimeout(() => terminalRef?.sendInput("\r"), ENTER_DELAY_MS);
}}

Flow clarification: The backend already awaits paste-buffer completion before returning { ok: true } (workmux.ts:393-396), and the frontend already awaits sendWorktreePrompt before calling onfixsuccess (CiDetailsDialog.svelte:86-87). The 200ms is purely a PTY flush safety buffer after the confirmed success response.
·

Backend already awaits paste-buffer completion before returning,
frontend already awaits the API response, so 200ms is sufficient
as a PTY flush safety buffer (down from 300ms).

Co-authored-by: centdix <centdix@users.noreply.github.com>
@centdix
centdix merged commit cf4feb2 into main Feb 25, 2026
1 check passed
@centdix
centdix deleted the better branch March 1, 2026 15:27
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