diff options
Diffstat (limited to 'notes')
| -rw-r--r-- | notes/concurrency-library-investigation.md | 228 | ||||
| -rw-r--r-- | notes/crash-investigation-findings.md | 256 | ||||
| -rw-r--r-- | notes/frontend-design.md | 4 | ||||
| -rw-r--r-- | notes/heartbeat-setting-handoff.md | 130 | ||||
| -rw-r--r-- | notes/memory-leak-investigation-handoff.md | 331 | ||||
| -rw-r--r-- | notes/review-bugs.md | 164 | ||||
| -rw-r--r-- | notes/server-crash-investigation.md | 257 | ||||
| -rw-r--r-- | notes/tool-call-in-thinking-bug.md | 265 | ||||
| -rw-r--r-- | notes/turn-continuity-design.md | 2 |
9 files changed, 1634 insertions, 3 deletions
diff --git a/notes/concurrency-library-investigation.md b/notes/concurrency-library-investigation.md new file mode 100644 index 0000000..abf7dab --- /dev/null +++ b/notes/concurrency-library-investigation.md @@ -0,0 +1,228 @@ +# Concurrency Library Investigation + +> Research-only. No code changes. Investigated 2026-06-26. + +## 1. ai-concurrency-shaper (joeycumines) + +**Repository:** https://github.com/joeycumines/ai-concurrency-shaper + +### What it is +A **standalone reverse proxy** written in **Go**. It sits between your +application and an upstream AI/LLM API (e.g. Anthropic, OpenAI), limiting +concurrent requests to configured HTTP routes. Requests exceeding the limit +block until a slot opens. Non-matching requests pass through unmodified. + +### How it works +- Uses a **token-bucket channel** (Go's `chan struct{}`) as a semaphore. Each + limited request acquires a token; the token is returned when the request + completes (full response body streamed). +- Supports **per-route** concurrency limits via `-limit "POST /v1/chat/completions:4"` + and **global** concurrency via `-global-concurrency 10`. +- Supports **route grouping** — multiple routes can share one limiter via + `@group` syntax. +- Has a **circuit breaker** (trips after N failures in a window, exponential + backoff with phantom concurrency holds). +- Has **concurrency protection flags**: release cooldown, cancel cooldown, + failure hold, adaptive headroom (reduce effective limit by 1 after a 429). +- **429 handling**: skips retrying 429s by default (`-retry-skip-429=true`). +- Optional **TUI dashboard** (Bubble Tea) with live metrics. +- **In-memory only** — no persistence, no external state (single process). + +### Maturity +- **1 star, 1 fork, 7 commits**, created June 10, 2026 (16 days ago). +- **Single contributor** (Joseph Cumines). +- **GPL-3.0 license** (copyleft — incompatible with closed-source distribution). +- Written in **Go**, not JavaScript/TypeScript. + +### Critical finding: language mismatch +This is a **Go binary**, not an npm library. It cannot be imported into a +Bun/TypeScript application. To use it, we would need to: +1. Run it as a **separate process** (a sidecar proxy). +2. Point our provider's `baseURL` at the proxy's listen address. +3. The proxy forwards to the real upstream. + +This fundamentally changes our architecture from "in-process concurrency +management" to "external proxy-based concurrency management." + +### Feature comparison vs our implementation + +| Feature | Our impl | ai-concurrency-shaper | +|---|---|---| +| Per-provider configurable limits | ✅ (runtime API + in-memory) | ✅ (per-route CLI flags, not runtime) | +| Oldest-agent-first scheduling | ✅ (priority queue by promptStartedAt) | ❌ (FIFO semaphore, no priority) | +| Slot held only during token generation | ✅ (acquire/release around stream) | ✅ (token held until response body completes) | +| 429 backoff | ✅ (pause queue, Retry-After aware) | ✅ (circuit breaker + adaptive headroom) | +| Watchdog/deadlock recovery | ✅ (timeout-based slot reclaim) | ❌ (no watchdog; relies on queue-timeout) | +| Runtime-configurable limits | ✅ (HTTP API, no restart) | ❌ (CLI flags only, requires restart) | +| In-memory / no external state | ✅ | ✅ | +| Language | TypeScript (Bun) | Go | +| Integration model | In-process (wraps ProviderContract) | External proxy (separate process) | +| Status reporting API | ✅ (HTTP endpoints) | TUI only (no HTTP API) | +| License | Our code (MIT-style) | GPL-3.0 | + +### What would change to use it +1. Deploy the Go binary as a sidecar process. +2. Change each provider's `baseURL` to point at the proxy. +3. Lose runtime-configurable limits (CLI flags only). +4. Lose oldest-agent-first scheduling (FIFO only). +5. Lose the in-process status API (TUI only, no HTTP). +6. Add a process management dependency (start/stop/monitor the proxy). +7. Accept GPL-3.0 license implications. + +## 2. Drawbacks of using ai-concurrency-shaper + +### Fatal drawbacks +1. **No oldest-agent-first scheduling.** Our implementation prioritizes the + agent whose prompt started longest ago. The proxy uses a simple FIFO + semaphore — no priority queue. This is a core requirement. +2. **No runtime-configurable limits.** Limits are set via CLI flags at startup. + Our frontend has a settings UI to add/remove/update limits at runtime. +3. **Language mismatch.** It's a Go binary, not an npm package. We'd need to + run it as a separate process, adding operational complexity. +4. **No HTTP status API.** The proxy exposes status only via TUI. Our frontend + polls `GET /concurrency/status` for live in-flight/queued/paused state. +5. **GPL-3.0 license.** Copyleft — may be incompatible with our distribution + model. + +### Secondary drawbacks +6. **No watchdog.** If a slot holder dies (e.g. the proxy's connection to the + client drops but the upstream request is still in flight), there's no + timeout-based reclaim. It relies on `-queue-timeout` for queue wait, not + for held slots. +7. **Immature.** 1 star, 7 commits, 16 days old, single contributor. No + community, no battle-testing. +8. **No provider-awareness.** It limits by HTTP route pattern, not by + "provider." Our implementation is keyed on `providerId` (e.g. "umans", + "openai-compat"), which maps naturally to our `ProviderContract.id`. +9. **Operational overhead.** Running a separate Go process alongside the Bun + app adds deployment complexity, monitoring burden, and a failure mode + (proxy down = all requests fail). + +## 3. Other libraries/tools for AI API concurrency + +### p-queue (sindresorhus) +- **npm**, TypeScript, MIT, 4.2k stars, 727k dependents. +- General-purpose promise queue with concurrency control. +- Supports **priority** (`{priority: number}` per task), **rate limiting** + (`intervalCap` + `interval`), **pause/resume**, **timeouts**, **custom + queue class** (for custom scheduling), AbortSignal cancellation. +- **No AI-specific logic** — no 429 detection, no provider concept, no + streaming-awareness. +- Could be used as a **building block** to replace our queue implementation, + but we'd still need the provider wrapper, 429 detection, watchdog, and + status API on top. +- **Feature complete** (maintainer says no further development planned, but + accepts PRs). + +### p-limit (sindresorhus) +- **npm**, TypeScript, MIT, 5.4k dependents. +- Simpler than p-queue — just limits concurrent executions. No queue, no + priority, no pause. Not suitable for our use case (we need queuing). + +### ai-sdk-rate-limiter (piyushgupta344) +- **npm**, TypeScript, zero dependencies. +- Designed for the Vercel AI SDK (`@ai-sdk/*`). Wraps model objects. +- Has **priority queuing** (high/normal/low lanes), **429 backoff with + Retry-After**, **cost tracking & budget enforcement**, **multi-tenant + scopes**, **Redis for multi-instance**, **observability** (Prometheus, + OpenTelemetry). +- Built-in limits for OpenAI, Anthropic, Google, Groq, Mistral, Cohere. +- **Closest to our use case** of any npm library found. +- **Maturity unknown** — couldn't verify star count or maintenance activity + from the docs site. Appears to be a solo project. +- **Concerns**: It's designed for the Vercel AI SDK's model-wrapping pattern. + Our provider architecture uses `ProviderContract.stream()` returning an + `AsyncIterable<ProviderEvent>`, not the Vercel AI SDK's model interface. + We'd need an adapter. It also doesn't expose a status API for the frontend. + +### LiteLLM (BerriAI) +- **Python**, MIT, 51.7k GitHub stars, very mature (39k+ commits). +- Full **LLM gateway/proxy** — not a library. Runs as a separate server. +- Has `enforce_model_rate_limits` for RPM/TPM hard limits. +- Supports **least-busy routing**, **latency-based routing**, **cost-based + routing**, **fallbacks**, **deployment priority**. +- **No concurrency limiting** — it limits requests-per-minute (RPM) and + tokens-per-minute (TPM), not concurrent in-flight requests. RPM is a rate + limit, not a concurrency limit. An agent that sends 4 requests in 1 second + and then waits would be blocked by RPM=60 but not by concurrency=4. +- Would require running a Python proxy server alongside our Bun app. +- Overkill for our use case — it's a full LLM gateway with virtual keys, cost + tracking, guardrails, etc. + +### Portkey AI Gateway +- **Hosted/self-hosted**, open-source (Apache-2.0). +- Full AI gateway with routing, fallbacks, caching, observability. +- Rate limiting is **Enterprise-only** and per-team/per-key, not per-provider + concurrency. +- Same architectural model as LiteLLM (external proxy). + +### Kong AI Gateway +- Built on Kong's API gateway. Enterprise-focused. +- Rate limiting is plugin-based (RPM/TPM), not concurrency-based. +- Same external-proxy model. + +## 4. Recommendation + +### **Keep the custom implementation. Do not switch to a library.** + +### Rationale + +1. **No library matches our requirements.** The core differentiators of our + implementation — **oldest-agent-first scheduling**, **runtime-configurable + per-provider limits**, **in-process status API**, and **stream-aware slot + lifecycle** — are not found in any library or proxy we investigated: + - `ai-concurrency-shaper` is a Go proxy with FIFO, no priority, no runtime + config, no HTTP status API, and GPL-3.0. + - `p-queue` has priority but no AI-specific logic, no 429 detection, no + streaming-awareness, no status API. + - `ai-sdk-rate-limiter` is close but designed for the Vercel AI SDK's model + interface, not our `ProviderContract.stream()` pattern. + - LiteLLM/Portkey/Kong are full gateway proxies with RPM/TPM rate limiting, + not concurrency limiting, and require running a separate server. + +2. **Our implementation is small and well-tested.** The core + `concurrency-manager.ts` is ~280 lines of pure logic with 15 tests and + injected timers. The `provider-wrapper.ts` is ~70 lines with 6 tests. The + total surface is tiny — there's no maintenance burden to justify + offloading to a library. + +3. **The architecture is a perfect fit.** Our `ProviderContract` wrapping + pattern (acquire before stream, release after stream) is the cleanest + possible integration point. An external proxy would add a network hop, + a process to manage, and break the direct relationship between the + orchestrator and the provider. + +4. **Oldest-agent-first scheduling is a hard requirement.** No library or + proxy we found supports priority queuing by turn-start time. This is the + core value proposition of our implementation — older agents complete + sooner, reducing overall wait time. + +5. **Runtime configurability is a hard requirement.** The frontend has a + settings UI for adding/removing/updating limits without restart. No + library supports this — they all require static configuration. + +### What we would lose by switching +- Oldest-agent-first scheduling (no library supports it). +- Runtime-configurable limits (no library supports it). +- In-process status API (no library supports it). +- Direct integration with `ProviderContract` (would need adapter or proxy). +- ~280 lines of well-tested code that we own and control. + +### What we would gain by switching +- Nothing we don't already have. The features libraries offer (circuit + breakers, adaptive headroom, TUI dashboards) are either already in our + implementation (429 backoff, watchdog) or not needed (TUI — we have a web + frontend). + +### When to reconsider +- If we need **multi-instance** concurrency coordination (multiple app + processes sharing limits), we would need Redis-backed state. At that point, + `ai-sdk-rate-limiter` (which has a Redis store) or a custom Redis-backed + extension of our current implementation would be worth evaluating. +- If we need **RPM/TPM rate limiting** (not just concurrency), LiteLLM or + Portkey could complement our concurrency limits. But these are orthogonal + concerns — RPM limits total requests per minute, while concurrency limits + in-flight requests at any moment. +- If `ai-sdk-rate-limiter` matures and adds a status API + custom model + adapters, it could be worth re-evaluating as a replacement for the queue + layer (while keeping our provider wrapper + status API). diff --git a/notes/crash-investigation-findings.md b/notes/crash-investigation-findings.md new file mode 100644 index 0000000..485e02d --- /dev/null +++ b/notes/crash-investigation-findings.md @@ -0,0 +1,256 @@ +# Production Crash Investigation — Findings + +> **Definitive post-investigation record.** Supersedes `notes/memory-leak-investigation-handoff.md` +> (which contained three material errors — see §3). Authored by opencode (umans-glm-5.2) on +> 2026-06-28, incorporating an independent Gemini review (`crash-review-report.md`) whose pivotal +> finding (the SSH pool) overturned an earlier wrong conclusion. + +**Last updated:** 2026-06-28 +**Status:** Root cause of the **live** (1.3.14) crash is confirmed and fixable in Dispatch code. +The native segfault is **not** root-caused and may already be fixed by the 1.3.14 upgrade — +needs observation under real load. + +--- + +## 0. TL;DR + +- The production server was crash-looping (~20 restarts on Jun 28). **Two distinct failure modes.** +- **Live crash (Bun 1.3.14):** `exit-1 "Timed out while waiting for handshake"`. Root cause is a + **Dispatch bug** in `packages/ssh/src/pool.ts`: the pooled `ssh2.Client` has **no permanent + `'error'` listener** (it's removed after connect by `cleanup()`, pool.ts:266), so a post-connect + ssh2 error emits `'error'` with no listener → uncaught → **no `process.on("uncaughtException")` + guard in main.ts** → process exit-1. Fixable in Dispatch code; no runtime change needed. +- **Segfaults:** all on **Bun v1.3.13** (`Bun v1.3.13 (bf2e2cec)` printed at each). **Zero + segfaults observed on 1.3.14** so far. Not root-caused; may be fixed by the upgrade. +- The prior "~2.5 GB/hour memory leak" framing is **wrong** (see §3). Idle memory is flat at 84 MB. +- **Recovery on restart** is repair-and-seal, not resume: conversations are swept `active→idle` + and partial turns reconciled to a consistent state, but in-flight turns are sealed (user re-sends). + +--- + +## 1. The live crash — SSH pool missing error listener (CONFIRMED, Dispatch bug) + +### Crash signature (the only crash on the current 1.3.14 binary) +``` +error: Timed out while waiting for handshake + at emitError (node:events:51:13) + at <anonymous> (/$bunfs/root/dispatch-server:16:75617) +Bun v1.3.14 (Linux x64) +→ Main process exited, code=exited, status=1/FAILURE +``` +- 09:22:04 JST crash; last telemetry sample (09:21:52) showed `rss=116MB, activeConversations=4`. +- systemd reported 2.7 GB peak, but the crash was **exit-1 (self-exit), not OOM/SIGKILL** — memory + was a symptom, not the kill trigger. + +### Root cause chain +1. The error string `"Timed out while waiting for handshake"` is **ssh2's**, not Bun's TLS. It is + hardcoded in `ssh2/lib/client.js:1114`: + ```js + this._readyTimeout = setTimeout(() => { + const err = new Error('Timed out while waiting for handshake'); + this.emit('error', err); // ← ssh2 emits 'error' on the ssh2.Client + sock.destroy(); + }, this.config.readyTimeout); + ``` + (Bun's own TLS string is the different `"TLS handshake timeout"` — confirmed via `strings` on + the binary.) +2. In `packages/ssh/src/pool.ts`, `doConnect` attaches `client.on("error", onError)` **only during + the connect promise** and removes it in `cleanup()` (pool.ts:266) on success. `buildConnection` + (pool.ts:101-175) returns a long-lived `SshConnection` (keepalive 30s, idle-reap 15m) and + **never attaches a permanent `'error'` listener** to the pooled client. +3. So after a successful connect, the `ssh2.Client` sits in the pool with no `'error'` listener. A + later ssh2 error (re-handshake/re-key/keepalive timeout emitting the string above) → + `emit('error')` → `emitError (node:events)` with no listener → EventEmitter default throws → + uncaught. +4. `packages/host-bin/src/main.ts` has **only `SIGINT`/`SIGTERM` handlers** (main.ts:280-281) — + **no `process.on("uncaughtException")` / `process.on("unhandledRejection")`**. So the uncaught + error exits the process (exit-1), killing every in-flight turn — not just the one using SSH. + +### Why capping concurrency is NOT the fix +A single pooled SSH connection dropping crashes the server regardless of how many turns are +concurrent. Capping `activeConversations` would only reduce frequency, not fix the root cause — +and it cripples a product built around concurrent agents. (The existing provider-concurrency +limiter, capped at 3-4, already bounds concurrent *provider streams*; SSH connections are +tool-execution connections and bypass it.) + +### The fix (root cause) +1. **`packages/ssh/src/pool.ts` — `buildConnection`:** attach a permanent `'error'` listener to + the `ssh2.Client` that tears down the connection, sets `state.value = "error"` / + `state.error = <msg>`, and **logs** (computer alias, error message, connection state) so the + failure is observable. Keep the existing connect-time `onError` for the connect promise. +2. **`packages/host-bin/src/main.ts` — boot:** add `process.on("uncaughtException")` and + `process.on("unhandledRejection")` handlers that **log rich detail** (message, stack, + `activeConversations` count, `process.memoryUsage()` snapshot, timestamp) so we know *where* + they happen, then either survive (for non-fatal) or seal in-flight work + exit gracefully. This + is the defense-in-depth that ensures any *future* unhandled error degrades instead of taking + down the whole server and every in-flight turn. + +--- + +## 2. The segfaults — NOT root-caused; 1.3.13 only (may be fixed on 1.3.14) + +### Crash signature +``` +panic(main thread): Segmentation fault at address 0x0 +oh no: Bun has crashed. This indicates a bug in Bun, not your code. +→ Main process exited, code=dumped, status=4/ILL +``` +- **Every segfault was on Bun v1.3.13** (the log prints `Bun v1.3.13 (bf2e2cec)` at each: + 00:12, 00:45, 00:57, 08:50, 09:05). +- Memory peaks ranged widely: **370 MB** (09:05, 1-min uptime) up to 6.2 GB (00:12, 2.5h uptime). +- The 1.3.14 binary first ran at 09:06 JST. **No segfault has been observed on 1.3.14** — the only + 1.3.14 crash is the SSH exit-1 above. The current process has been stable idle. + +### What this means +- The 370 MB segfault is **not memory exhaustion** (24 GB cgroup) — it's a native concurrency/race + bug in Bun's runtime. No amount of memory bounding prevents a native race. +- The 1.3.14 upgrade **may** have fixed it; the sample so far is small and mostly idle, so this is + **not confirmed**. +- **A native segfault cannot be caught by `process.on("uncaughtException")`** (that catches JS + exceptions, not SIGILL/SIGSEGV). So if the segfault recurs on 1.3.14, the only complete fixes + are: leave Bun (port to Node) or root-cause the native trigger (core dump → backtrace → repro → + upstream Bun report). + +### Plan for the segfault +1. Enable core-dump capture (`ulimit -c unlimited` / systemd `COREdump`) so the next segfault + produces a backtrace rather than a bare `address 0x0`. +2. Run 1.3.14 under the real orchestrator workload (concurrent backend+frontend+subagent turns). +3. If no segfault recurs → likely already fixed; close the issue. +4. If one recurs → root-cause via the backtrace; do NOT paper over it with concurrency caps. + +--- + +## 3. What was WRONG in the prior handoff (corrections) + +`notes/memory-leak-investigation-handoff.md` was a careful document but contained three material +errors that this investigation corrected: + +1. **"Telemetry is in journald — `journalctl | grep memory:periodic`."** Wrong. The logger writes + to a **journal sink file** (`DISPATCH_JOURNAL=/var/log/dispatch/app.ndjson`, main.ts:139), + not stderr/journald. The handoff's grep found nothing *by design* — the data was in the file + all along. (Also: the observability-collector that would drain this file into `traces.db` is + **disabled in compiled binaries** — main.ts:150 guards on a source path that doesn't exist in + prod — so the journal just accumulates and rotates; only `app.ndjson` + `.1` are retained.) +2. **"The segfault persists on Bun 1.3.14" (handoff hypothesis #3 disproven).** Wrong. All + segfaults were on **1.3.13**; no segfault has been seen on 1.3.14. The 1.3.14 upgrade may have + fixed it — the handoff's hypothesis #3 is *not* disproven after all. +3. **"~2.5 GB/hour slow memory leak."** Wrong framing. Telemetry shows **idle is flat at 84 MB** + (20+ min, zero reclaimable, zero growth) — there is no background/timer/closure leak. The + "leak" was the live working set of concurrent multi-step turns, which is reclaimed when turns + end. Moreover, raw context size is never GB-scale: 300k tokens ≈ 1-2 MB, so the gigabytes + under load must come from pathological tool payloads, retention, or native bugs — not + "unbounded context." (See §5.) + +--- + +## 4. What was ruled out + +- **Warm-cache probe** — confirmed (Gemini + my read) as a *latent* unhandled-rejection path + (`createWarmService`'s `warm` has no try/catch around its `for await (provider.stream(...))` at + orchestrator.ts:1276; `fireWarm` drops the promise with `void` at warmer.ts:130). **But it has + never fired in production** — zero `cache-warming` activity in the journal, zero enabled + conversations in the `kv` table (53k rows, all conversation settings; warming is opt-in default + OFF). It is a latent risk worth fixing (the `unhandledRejection` guard from §1 catches its + rejection too), but it is **not** the cause of any observed crash. +- **LSP** — exonerated (per handoff §7; caches bounded; disabled as a precaution, unrelated to + crashes). +- **Idle/background leak** — refuted by telemetry (idle flat at 84 MB). +- **`activeConversations` cap as a fix** — rejected. It's a workaround that cripples the + concurrent-agent product; the existing provider-concurrency limiter (3-4) already bounds + provider streams. The root cause is the missing error listener + missing guard. + +--- + +## 5. Memory: what actually drives GB-scale (and what doesn't) + +The user's key insight: **300k tokens ≈ 1-2 MB** — raw context size is never GB-scale (capped at +single-digit MB by the context window). So "unbounded context → GB crash" is numerically wrong. +The only ways a turn reaches GB-scale: + +1. **A pathological tool payload** — a single tool result that's itself hundreds of MB (huge + file/log read). It enters `messages` unbounded and each step re-`JSON.stringify`s it (stream.ts:91) + **plus** the `reqSpan` captures a second copy (stream.ts:112) → ~2× transient per step. + **Tool-output bounding** directly prevents this (opencode caps at 50 KB / 2000 lines, + offloading full text to a managed file — `packages/opencode/src/tool/truncate.ts`). +2. **O(N²) re-serialization across many steps**, only if GC can't keep up or there's retention. +3. **A genuine retention leak** (message arrays / spans / closures held after the step or turn) — + accumulates per-turn over time (the 6.2 GB-over-2.5 h pattern). **Bounding context does NOT + fix this** — the retained reference must be found. (Not yet investigated; the span's + `bodyString` copy is written to the journal sink on disk, so it's likely transient, not + retained — but unverified.) + +**Dispatch vs opencode (context hygiene — NOT crash fixes):** +| Aspect | Dispatch | opencode | +|---|---|---| +| Mid-turn compaction | refuses during active conv (orchestrator.ts:1325) | compacts between steps near context window (llm.ts:210) | +| Tool result size in history | unbounded | bounded 50 KB / 2000 lines (truncate.ts) | +| Step limit | `MAX_STEPS = 0` (unlimited) | configured per agent; `MAX_STEPS_PROMPT` disables tools at last step | +| History sent to provider | full raw `messages` array | projected `toLLMMessages(...)` after compaction | + +These are general hygiene improvements, **not** crash fixes (the live crash is the SSH bug; the +segfault is native). Tool-output bounding is the one with direct crash relevance (prevents +pathological-payload spikes). Compaction/max-steps do not reduce RAM for context the agent +genuinely needs. + +--- + +## 6. Recovery behavior on restart (repair-and-seal, NOT resume) + +When the server crashes and systemd restarts it: +- **`conversation-store/extension.ts:21-27`** — boot-sweep: lists all conversations with + `status: ["active"]` and sets them to `"idle"`. +- **`store.ts:681`** — on load, `reconcileWithReport` repairs partial turns: synthesizes error + results for orphaned tool-calls, strips error-only trailing assistant messages, drops empty + messages. Emits a `reconcile.repair` span when repairs occur. +- **`heartbeat/heartbeat.ts:299`** — sweeps stale "running" runs to "stopped". + +**Result:** the DB is always consistent after a restart (no broken conversations), but in-flight +turns are **sealed, not resumed** — partial assistant output is preserved (reconciled), the +conversation is marked idle, and the user must re-send. This is why the `uncaughtException` guard +matters: without it, one SSH error kills the process → **every** in-flight turn (not just the one +using SSH) is sealed and must re-send. With the guard, only the failing turn seals; the rest +continue. + +--- + +## 7. Data artifacts & quick-reference + +```bash +# Live state +systemctl status dispatch +systemctl show dispatch -p MemoryMax -p MemoryHigh # 24G / 20G (live, applied) +curl http://localhost:24991/health # → {"ok":true} + +# Telemetry lives HERE (not journald): +/var/log/dispatch/app.ndjson # current process +/var/log/dispatch/app.ndjson.1 # rotated (older crash windows rotated away — only .1 kept) +# grep: rg 'memory:periodic|memory:gc' /var/log/dispatch/app.ndjson + +# Journal (crash stacks): +journalctl -u dispatch --since '24 hours ago' | rg 'Main process exited|panic|Bun v' + +# The two investigation docs: +notes/memory-leak-investigation-handoff.md # prior (has the 3 errors in §3) +notes/crash-investigation-findings.md # THIS file (definitive) +crash-review-report.md # independent Gemini review (found the SSH bug) + +# Suspect code: +packages/ssh/src/pool.ts # buildConnection/doConnect — the crash +packages/host-bin/src/main.ts # missing uncaughtException guard +packages/kernel/src/runtime/run-turn.ts # MAX_STEPS=0, streaming retry loop +packages/openai-stream/src/stream.ts # JSON.stringify whole body + span copy +packages/session-orchestrator/src/orchestrator.ts # warm probe (latent), activeConversations +``` + +## 8. Current fix scope + +**In scope now (root-cause, Dispatch code):** +1. `packages/ssh/src/pool.ts` — permanent `'error'` listener on the pooled `ssh2.Client` + logging. +2. `packages/host-bin/src/main.ts` — `uncaughtException` + `unhandledRejection` guards with rich + logging (message, stack, activeConversations, memory snapshot, timestamp). + +**Deferred (separate decisions):** +- Tool-output bounding, mid-turn compaction, MAX_STEPS (general hygiene; not crash fixes). +- Warm-probe try/catch (latent; the `unhandledRejection` guard covers it for now). +- Port to Node (only if segfault recurs on 1.3.14 under real load). +- Core-dump capture setup (operational; do before the next load test). diff --git a/notes/frontend-design.md b/notes/frontend-design.md index 16cb41f..e0687ed 100644 --- a/notes/frontend-design.md +++ b/notes/frontend-design.md @@ -86,7 +86,7 @@ chosen set. Skip the backend's 3-tier ceremony. ## 3. Repo structure (Vite + Svelte 5 SPA — SETTLED; not SvelteKit) ``` -dispatch-web/ (../dispatch-web — NEW repo, own git) +dispatch-web/ (../frontend — NEW repo, own git) AGENTS.md ORCHESTRATOR.md GLOSSARY.md .dispatch/{package-agent.md, rules/frontend-*.md} src/ @@ -416,7 +416,7 @@ slice); DaisyUI styling (F4 follow-up). The slice-1 input decisions were: Or pick a different first surface. 2. **WS-transport boundary:** a NEW `transport-ws` extension vs. augment the existing `transport-http`. (Boundary call.) -3. **Repo scaffold go-ahead:** create `../dispatch-web` (git init + Vite/Svelte/biome/vitest + +3. **Repo scaffold go-ahead:** create `../frontend` (git init + Vite/Svelte/biome/vitest + the §8 harness). Orchestrator can scaffold config + harness; feature code = summoned agents. ### Slice-1 findings + open items (for the next, clean-context agent) diff --git a/notes/heartbeat-setting-handoff.md b/notes/heartbeat-setting-handoff.md new file mode 100644 index 0000000..53382aa --- /dev/null +++ b/notes/heartbeat-setting-handoff.md @@ -0,0 +1,130 @@ +# FE handoff — heartbeat `inactiveOnly` (skip while workspace is busy) + +Courier this to `../frontend` (cross-repo contract change; `lsp references` does not span +repos — ORCHESTRATOR §7). All changes are ADDITIVE — nothing existing breaks. The new +field has a default, so a frontend that never sends it gets the new behavior automatically. + +## What shipped (backend) + +A new per-workspace heartbeat setting, **`inactiveOnly`**: when on (the default), the +heartbeat SKIPS a fire whenever the configured workspace has any active agents — i.e. a +conversation whose persisted status is `"active"` (driving a turn) or `"queued"` (waiting on +the message queue). The heartbeat stays quiet while the user is actively working and only +fires when the workspace is idle. + +The spawned heartbeat conversation lives in the DEDICATED heartbeat workspace (not the +configured one), so a heartbeat run never counts as an "active agent" of the configured +workspace — there is no self-block. + +## The setting + +```ts +// @dispatch/transport-contract (HeartbeatConfig) +interface HeartbeatConfig { + enabled: boolean; + inactiveOnly: boolean; // ← NEW. default: true + systemPrompt: string; + taskPrompt: string; + intervalMinutes: number; + model: string; + reasoningEffort: ReasoningEffort | null; +} +``` + +- **Name:** `inactiveOnly` +- **Type:** `boolean` +- **Default:** `true` (the heartbeat is quiet-by-default while the workspace is busy) +- **Semantics:** + - `true` → before each scheduled fire, the backend checks the configured workspace for + active agents. If any exist, the fire is **silently skipped** (no run is recorded, no + `HeartbeatRun` created). The scheduler re-arms and tries again at the next + `intervalMinutes` interval. + - `false` → the heartbeat fires unconditionally on every interval, regardless of + workspace activity (the pre-existing behavior). +- **What counts as an "active agent":** any conversation in the configured workspace whose + persisted `ConversationStatus` is `"active"` or `"queued"`. The orchestrator sets + `"active"` when a turn starts and `"idle"` when it settles, so this is the live + busy/idle signal. +- **Self-block safety:** heartbeat-spawned conversations are filed in the `heartbeat` + workspace, so an in-flight heartbeat run does NOT keep the configured workspace "busy". + +## API endpoints + +The setting rides on the existing heartbeat config endpoints — no new routes. + +### Read + +`GET /workspaces/:id/heartbeat` → `HeartbeatConfig` (200) + +The response now includes `inactiveOnly`. For a workspace that never configured a +heartbeat, the full default config is returned (`enabled: false`, `inactiveOnly: true`, +`intervalMinutes: 30`, …). + +### Update + +`PUT /workspaces/:id/heartbeat` with a partial `UpdateHeartbeatRequest` body → `HeartbeatConfig` (200) + +```ts +// @dispatch/transport-contract (UpdateHeartbeatRequest) +interface UpdateHeartbeatRequest { + enabled?: boolean; + inactiveOnly?: boolean; // ← NEW. optional; absent = unchanged + systemPrompt?: string; + taskPrompt?: string; + intervalMinutes?: number; + model?: string; + reasoningEffort?: ReasoningEffort | null; +} +``` + +All fields optional (a partial update); only provided fields are applied. Absent +`inactiveOnly` leaves it unchanged (distinct from sending `false`). + +**Validation:** `inactiveOnly` must be a JSON `boolean` when present. A non-boolean +(e.g. `"yes"`, `1`) → HTTP 400 `{ error: "Field 'inactiveOnly' must be a boolean" }`. +The service is NOT called on validation failure. + +**Example:** + +```http +PUT /workspaces/proj/heartbeat +Content-Type: application/json + +{ "inactiveOnly": false } +``` + +```json +200 OK +{ + "enabled": false, + "inactiveOnly": false, + "systemPrompt": "", + "taskPrompt": "", + "intervalMinutes": 30, + "model": "", + "reasoningEffort": null +} +``` + +## Frontend implementation notes + +- Render a **checkbox** (default checked) in the workspace heartbeat settings UI, labelled + something like "Only run when the workspace is idle" / "Pause while agents are active". +- The checkbox reflects and edits `inactiveOnly` on the heartbeat config. +- On toggle, send `PUT /workspaces/:id/heartbeat` with `{ inactiveOnly: <bool> }` (a partial + update — no need to send the rest of the config). +- A skipped fire produces **no `HeartbeatRun`**, so the runs list (`GET + /workspaces/:id/heartbeat/runs`) and the next-run countdown (`GET + /workspaces/:id/heartbeat/next-run`) behave exactly as on any other idle interval: the + next-run timestamp advances to `now + intervalMinutes`. No new event type is emitted on a + skip (it is silent); the backend logs it at `info` level only. +- Legacy workspaces: a config persisted by an older backend (no `inactiveOnly` field) is + read back as `inactiveOnly: true` (the default) — the feature is ON by default for + everyone, including pre-existing workspaces. + +## Versions / deps + +The new field is added to `HeartbeatConfig` / `UpdateHeartbeatRequest` in +`@dispatch/transport-contract`. It is a purely additive field with a default, so no +version bump of the pinned `file:` dependency is strictly required to keep working — but +bump it when convenient to pick up the typed field. diff --git a/notes/memory-leak-investigation-handoff.md b/notes/memory-leak-investigation-handoff.md new file mode 100644 index 0000000..3dc4ad0 --- /dev/null +++ b/notes/memory-leak-investigation-handoff.md @@ -0,0 +1,331 @@ +# Memory-Leak Investigation — Handoff for OpenCode Agent + +> **Purpose:** This document gives an OpenCode harness agent — running outside +> the Dispatch system, with no prior context — everything needed to +> independently investigate the memory leak that is crashing the production +> Dispatch server. Read it top to bottom before touching anything. + +**Last updated:** 2026-06-28 +**Author of this handoff:** umans/umans-glm-5.2 (Dispatch agent) +**Originating investigation:** `notes/server-crash-investigation.md` (commit `b83aa8d`) + +--- + +## 0. TL;DR — what you are here to do + +The production Dispatch server leaks memory at **~2.5 GB/hour** and eventually +hits a **Bun runtime segfault** (native crash, not a JS exception). A prior +investigation already ruled out LSP as the cause. Your job is to **find where +memory is being retained** and propose a fix. There are four undeployed commits +on the `dev` branch that add memory telemetry and a cgroup circuit breaker — +**those need to be built, installed, and observed first** to get the data that +will localize the leak. Do not start code-diving blindly; deploy the telemetry, +collect a crash cycle, then read the logs. + +--- + +## 1. System Overview — what Dispatch is + +Dispatch is an **AI agent orchestration platform**: a backend that runs one or +more AI "agents" through turns (each turn = a step loop of model calls + +tool dispatch), plus a separate frontend that consumes the backend's typed +contracts over HTTP + WebSocket. + +**Repo layout** — all under `/home/tradam/projects/dispatch/`: + +| Path | What | Git remote | Branch | +|---|---|---|---| +| `backend/` | The server (this codebase) | `[email protected]:realtradam/dispatch.git` | `dev` | +| `frontend/` | The web UI (separate repo, same methodology) | `[email protected]:realtradam/frontend.git` | `dev` | +| `worktrees/` | Per-task git worktrees for isolated feature branches | (varies) | (varies) | +| `bin/` *(inside backend)* | Operational shell scripts (build, install, up, secrets, certs) | — | — | + +Both repos use **`dev` as the active development branch**. The backend is a +**Bun + TypeScript** monorepo (project references via `tsc -b`, Biome for +lint/format, Vitest for tests, `bun:sqlite` for storage, the `ai` / +`@ai-sdk/*` packages for model providers). Architecture rules are in +`backend/AGENTS.md` — **read it** before editing (especially the "kernel +touches no I/O" and "no ambient state" rules). + +--- + +## 2. Where things are installed (production) + +| Artifact | Path | Notes | +|---|---|---| +| Production server binary | `/usr/bin/dispatch-server` | Bun **standalone compiled** binary. Currently the OLD one (built Jun 27 21:40). Not you to install — see §6. | +| CLI binary | `/usr/bin/dispatch` | `dispatch send/list/read/...` | +| Frontend static files | `/usr/share/dispatch/web/` | Built by `bin/build` (Vite, `VITE_HTTP_PORT=24991 VITE_WS_PORT=24990`) | +| Config / env file | `/etc/dispatch/env` | `EnvironmentFile` for systemd. **Root-readable only** (`Permission denied` for non-root). Source template in repo: `systemd/dispatch.env` (installed by `bin/setup-env`). | +| Data directory | `/var/lib/dispatch/` | WorkingDirectory of the service. SQLite DBs live here: `dispatch.db`, `dispatch.db-shm`, `dispatch.db-wal`. | +| Logs | the journal | `journalctl -u dispatch` — there are no log files on disk; everything goes to journald. | +| systemd unit (live) | `/etc/systemd/system/dispatch.service` | Installed from the repo template `systemd/dispatch.service` by `bin/install`. | +| systemd unit (repo template) | `backend/systemd/dispatch.service` | **Edit this one**, not the live file — `bin/install` copies it over. | +| Install script | `backend/bin/install` | Builds + installs the binary, unit, env, frontend. Uses `sudo` on privileged lines only (the agent has no sudo — hand scripts to the user). | +| Build script | `backend/bin/build` | `tsc --build` then `bun build --compile` → `dist/dispatch-server`. | +| Memory-limits script | `backend/bin/apply-memory-limits.sh` | Applies `MemoryHigh`/`MemoryMax` to the live service. **User has NOT run it yet.** | + +--- + +## 3. Production ports + +| Service | Port | Confirmed | +|---|---|---| +| HTTP API | **24991** | `BACKEND_PORT=24991` in `systemd/dispatch.env` (set by `bin/setup-env`). Live: `ss -tlnp` shows `dispatch-server` listening on `*:24991`. | +| Surface WebSocket | **24990** | `SURFACE_WS_PORT`. Live: `ss -tlnp` shows `dispatch-server` listening on `*:24990`. | + +The **dev** stack (`bin/up`) uses different ports: **24203** (HTTP) + **24205** (WS) + **24204** (frontend Vite dev server). Don't confuse dev ports with prod ports. + +**Health check:** `curl http://localhost:24991/health` → `{"ok":true}` + +--- + +## 4. How to access the running server + +```bash +# Is it running? +systemctl status dispatch + +# Live logs (follow) +journalctl -u dispatch -f + +# Recent crashes / errors (last 2h) +journalctl -u dispatch --since '2 hours ago' -n 200 + +# Memory telemetry — ONLY visible once the new binary is deployed (see §6). +# The EXACT log tags to grep (NOT "[mem-telemetry]" — that is the module name): +journalctl -u dispatch -f | rg 'memory:periodic|memory:gc' + +# Restart (needs root — hand to the user) +sudo systemctl restart dispatch +``` + +> **The service is named `dispatch`, NOT `dispatch-server`.** The binary is +> `dispatch-server`; the systemd unit is `dispatch.service`. This mismatch +> tripped up the first investigation — don't repeat it. + +The backend checkout at `/home/tradam/projects/dispatch/backend/` is on `dev` +and contains the source. The running binary is compiled, so **editing source +does nothing until you rebuild + install + restart** (§6). + +--- + +## 5. The memory leak — what we know + +### 5.1 Symptom & crash signature + +- The server grows **~2.5 GB/hour** under load, eventually triggering a **Bun + runtime segfault** (native crash). +- **Last crash:** Jun 28 00:12 JST. + ``` + panic(main thread): Segmentation fault at address 0x0 + oh no: Bun has crashed. This indicates a bug in Bun, not your code. + Main process exited, code=dumped, status=4/ILL + ``` + RSS was **6.2 GB** at crash (over 2h31m uptime). A prior session hit + **25.7 GB over 18h**. +- The crash is a **native segfault inside Bun's runtime** (NULL deref in the + allocator/GC), NOT a JS exception. The crash-time memory readout was itself + corrupted (`RSS: 0.02ZB` — impossible), and systemd saw SIGILL while Bun + reported SIGSEGV — both signs of **heap corruption under memory pressure**. +- This is the **only** segfault in 5 days of logs. Earlier crashes (Jun 27 + ~02:44–02:52) were `exit-code` failures = application-level LSP bugs, **now + fixed** (see §7). + +### 5.2 What it is NOT + +- **NOT LSP.** The Language Server Protocol extension was the original prime + suspect; it is exonerated. Its caches are bounded (`MAX_OPEN_DOCUMENTS = 50`, + `MAX_PUSH_DIAGNOSTICS = 100` — see `packages/lsp/src/client.ts:153` and + `diagnostics.ts:47`). 50 docs + 100 diag entries cannot explain gigabytes. +- **NOT the conversation store.** `packages/conversation-store` is + **SQLite-backed** (all reads/writes go through a `StorageNamespace` to + `bun:sqlite`) — it does not hold conversations in an in-memory map. +- **NOT `activeConversations`.** The session-orchestrator's + `activeConversations` is just a `Set<string>` of conversation IDs (tiny). + +### 5.3 Prime suspects (not yet confirmed — that's your job) + +1. **The AI-SDK streaming path** — the `async` generator returned by + `provider.stream(...)` (`@ai-sdk/*`, including the `openai-stream` package). + Streaming response buffers may be retained if the generator is not fully + drained or if the SDK holds internal buffers per request. +2. **Per-turn message arrays** in `session-orchestrator` — the history + + `userMsg` + `providerMessages` arrays assembled before each + `provider.stream(...)` call. For long multi-step agent turns these can be + large; if references outlive the turn (closures, unresolved promises, event + listeners), they leak. +3. **A Bun bug fixed in 1.3.14.** The crash was on Bun **v1.3.13**. Bun 1.3.14 + has been built locally but not installed (§6.3) — deploying it may itself + resolve the segfault even if a leak remains. + +### 5.4 Key files to examine (the leak-suspect code path) + +| File | What's there | Why it matters | +|---|---|---| +| `packages/session-orchestrator/src/orchestrator.ts` | `runTurnDetached()` at **line 541** — kicks off a turn; the `activeConversations.add/delete` around it (556, 979) brackets the turn lifecycle. | Turn lifecycle: where memory should be acquired and released. If a turn's buffers aren't released here, it leaks per-turn. | +| `packages/session-orchestrator/src/orchestrator.ts` | `for await (const event of provider.stream(messages, assembled.tools, providerOpts))` at **line 1276** (and a second one at **1430** for compaction/summary). | **The streaming loop** — the #1 suspect. This is where the AI-SDK async generator is consumed. If the generator is abandoned mid-stream (error, `break`, abort) or its internal buffers are retained, memory grows per turn. | +| `packages/session-orchestrator/src/pure.ts` | The pure turn/step decision logic + the `MemorySample`/`memoryDelta`/`memorySampleAttributes` helpers used by telemetry. | The per-turn before/after sampling wraps the stream boundary (referenced by `mem-telemetry.ts`). | +| `packages/kernel/src/runtime/run-turn.ts` | The **step loop** (`runTurn`) — one agent turn = N steps of (model call → tool dispatch → feed results back). | MAX_STEPS is disabled (`0 = unlimited`, commit `e8b4bf1`), so a single turn can run many steps. Many steps ⇒ many accumulated message arrays ⇒ more retention surface. | +| `packages/kernel/src/runtime/dispatch.ts` | **Tool dispatch** (the `maxConcurrent`/`eager` tool-execution loop). | Tool results accumulate into the turn's message history. A rogue tool returning huge payloads could balloon arrays. | +| `packages/host-bin/src/mem-telemetry.ts` | The periodic telemetry edge effect. | Read this to understand exactly what `memory:periodic` / `memory:gc` log entries contain (rss, heapUsed, heapTotal, external, arrayBuffers, activeConversations, reclaimedRssMB). | + +> **Architecture note (AGENTS.md):** the kernel (`packages/kernel`) touches NO +> I/O and names no concrete feature. Decision logic is pure; effects are +> injected at the edges (host-bin). When investigating, keep this layering in +> mind — a leak "in the kernel" would actually be in a closure captured by an +> edge that feeds the kernel, not in the kernel's own (stateless) turn loop. + +--- + +## 6. What's been done (committed to `dev`, NOT yet deployed) + +The running binary is still the **old one** (built Jun 27 21:40, pre-telemetry, +pre-Bun-1.3.14, pre-LSP-disable). Four commits on `dev` need building + +installing before any of this takes effect. **Verify before you trust:** the +live systemd still reports `MemoryMax=infinity` (confirmed via +`systemctl show dispatch -p MemoryMax`). + +| Commit | What | Status | +|---|---|---| +| `d1de9ed` | `systemd/dispatch.service` gets `MemoryHigh=20G` + `MemoryMax=24G` circuit breaker. | Committed. **Live = infinity (not applied).** | +| `b3b6eb4` | `bin/apply-memory-limits.sh` — applies the limits to the live service. | Committed. **User has NOT run it.** | +| `1cd66da` | Memory telemetry: periodic `process.memoryUsage()` every 15s, per-turn before/after sampling, `activeConversations` tagging, `Bun.gc(true)` every 5 min. Files: `packages/host-bin/src/mem-telemetry.ts` + `packages/session-orchestrator/src/pure.ts`. | Committed. **NOT deployed.** | +| `73ff84c` | **LSP disabled** as a precaution (`main.ts` import commented out + removed from `CORE_EXTENSIONS`; `transport-http` tolerates its absence). Also set telemetry interval 60s→15s. | Committed. **NOT deployed.** LSP stays disabled until the leak is understood. | +| (bun upgrade) | Bun **1.3.13 → 1.3.14** via `bun upgrade`. New binary built at `dist/dispatch-server`. | Rebuilt. **NOT installed** to `/usr/bin/dispatch-server`. | + +### 6.1 The telemetry data you will collect (once deployed) + +The exact log tags to grep in journald are **`memory:periodic`** and +**`memory:gc`** (the module is `mem-telemetry.ts`, but the log *messages* are +those two strings — do not grep for `[mem-telemetry]`). + +- `memory:periodic` — every 15s: `rss`, `heapUsed`, `heapTotal`, `external`, + `arrayBuffers` (bytes), plus `activeConversations` (count). Correlate RSS + growth with active turns: if RSS climbs while `activeConversations` is 0, + the leak is background/idle (timers, watchers, retained closures); if it + climbs only during/after turns, it's the streaming/turn path. +- `memory:gc` — every 5 min: runs `Bun.gc(true)` and logs RSS before/after + + `reclaimedRssMB` (`before - after`). **Interpretation:** a large positive + `reclaimedRssMB` = GC freed it (fragmentation, reclaimable — not a true + leak). Near-zero/negative `reclaimedRssMB` = memory is **LIVE** (retained + objects — a real leak). This single field distinguishes the two failure + modes; check it first when you get data. + +### 6.2 Deploy sequence (hand to the user — you have no sudo) + +The agent cannot run `sudo` or install to `/usr/bin`. Write a script with +`sudo` on the privileged lines and hand it to the user (per the repo's sudo +policy). The sequence, roughly: + +1. From `backend/`: `bun run typecheck && bun run test && bun run check` — + make sure `dev` is green (it should be; these commits are committed). +2. `bin/build` — produces `dist/dispatch-server` (Bun 1.3.14, with telemetry + + LSP-disabled). +3. `bin/install` (or a targeted script) — copies `dist/dispatch-server` → + `/usr/bin/dispatch-server`, the unit template → `/etc/systemd/system/`, + `systemd/dispatch.env` → `/etc/dispatch/env`. (Or run `bin/apply-memory-limits.sh` + separately if you only want the cgroup limits without a full reinstall.) +4. `sudo systemctl daemon-reload && sudo systemctl restart dispatch`. +5. Confirm: `systemctl show dispatch -p MemoryMax` should show `24G`, and + `journalctl -u dispatch -f | rg 'memory:periodic'` should show telemetry + within 15s. + +> ⚠️ **Warning:** deploying restarts the production server. The live server is +> actively serving agent conversations. Coordinate with the user (ceb2 / +> tradam) before restarting — don't just yank it. Also note the running server +> at time of writing is PID 28956 (started Jun 28 08:51). + +--- + +## 7. Background — the original crash investigation (already done) + +You do **not** need to redo this; it's recorded for context. + +- **Original report:** `notes/server-crash-investigation.md` (commit `b83aa8d`). +- The first investigation's task description was **stale**: it claimed an + "uncommitted hot-fix that disables LSP" existed. It did not — the working + tree was clean and LSP was *enabled*. The developer then *chose* to disable + LSP (commit `73ff84c`) as a precaution, but the root cause was already + identified as the Bun segfault + leak, not LSP. +- The **three original LSP suspects** (JSON-parse TypeError, `fs.watch` ENOENT, + unbounded cache leak) were all fixed in commits `05ff256` + `f9d1ca5` and + are correct/tested. LSP being disabled now is a precaution, not a fix for the + crash. Once the leak is understood, LSP can be re-enabled safely. +- **Do not spend time re-investigating LSP.** It is exonerated. + +--- + +## 8. What needs investigation (your tasks, in order) + +1. **Deploy the telemetry + cgroup limits** (§6) — you cannot localize the leak + without the `memory:periodic` / `memory:gc` data. Get a full crash cycle's + worth of logs (let it run until it either hits `MemoryMax=24G` and restarts, + or until the growth pattern is clear). +2. **Read the telemetry first.** The `memory:gc` `reclaimedRssMB` field tells + you live-retained vs fragmentation. If near-zero after GC → real retained + leak; chase which subsystem holds it. +3. **Correlate growth with `activeConversations`.** Idle-growth ⇒ timers / + watchers / retained closures / the LSP file-watcher (if any servers still + spawn — but LSP is disabled). Turn-bound growth ⇒ the streaming/turn path + (suspects in §5.4). +4. **Examine the streaming loop** (`orchestrator.ts:1276`): is the + `provider.stream(...)` async generator fully drained on every path + (success, error, abort, `break`)? Does the AI SDK retain response buffers? + Consider adding a before/after `memoryUsage()` around a single turn to + measure per-turn delta directly. +5. **Examine the message arrays** assembled before `provider.stream` — are + they scoped to the turn and released when the turn completes, or do + closures/promises/event-listeners keep them alive? +6. **Test Bun 1.3.14 in isolation.** Since the upgrade is built but not + installed, see if a memory-stress repro under 1.3.13 vs 1.3.14 behaves + differently — the segfault may be a Bun allocator bug that 1.3.14 fixes. +7. **Propose a fix** (do not implement without coordinating — report up to the + orchestrator). Likely candidates: ensure the streaming generator is always + fully consumed / explicitly closed on error; bound per-turn message + history; release references at `activeConversations.delete` time; or + confirm it's purely a Bun bug requiring the 1.3.14 deploy. + +--- + +## 9. Quick-reference commands + +```bash +# --- status & logs --- +systemctl status dispatch +systemctl show dispatch -p MemoryMax -p MemoryHigh # expect 24G/20G after deploy +journalctl -u dispatch -f # live logs +journalctl -u dispatch -f | rg 'memory:periodic|memory:gc' # telemetry (after deploy) +journalctl -u dispatch --since '2 hours ago' -n 200 # recent history / crashes +curl http://localhost:24991/health # → {"ok":true} +ss -tlnp | rg dispatch # ports 24991 + 24990 + +# --- source (the leak path) --- +cd /home/tradam/projects/dispatch/backend +git log --oneline -n 8 # see the undeployed commits +rg -n 'runTurnDetached|provider\.stream|for await' packages/session-orchestrator/src/orchestrator.ts + +# --- build & verify (NO sudo needed) --- +bun run typecheck && bun run test && bun run check +bin/build # → dist/dispatch-server + +# --- deploy (NEEDS sudo — hand a script to the user) --- +# bin/install (or: sudo cp dist/dispatch-server /usr/bin/dispatch-server) +# bin/apply-memory-limits.sh (applies MemoryMax to live service) +# sudo systemctl daemon-reload && sudo systemctl restart dispatch +``` + +--- + +## 10. Guardrails + +- **Do NOT edit implementation code without reporting to the orchestrator.** + This is an investigation handoff; propose fixes, don't apply them + unilaterally. +- **You have no sudo.** Any step touching `/usr/bin`, `/etc/`, or + `systemctl restart` must be a script handed to the user with `sudo` on the + privileged lines. +- **Do not re-enable LSP** until the leak is understood (it's disabled as a + precaution; re-enabling is a separate decision). +- **Do not merge or push.** Work stays on `dev` locally. +- **The service is `dispatch`, not `dispatch-server`.** diff --git a/notes/review-bugs.md b/notes/review-bugs.md new file mode 100644 index 0000000..85cf625 --- /dev/null +++ b/notes/review-bugs.md @@ -0,0 +1,164 @@ +# Code Review: workspace-star (backend) + +**Branch:** `feature/workspace-star` +**Commit reviewed:** `71c635f feat(workspace-star): starred workspace priority for concurrency limiting` +**Compared to:** `dev` (`414080e`) +**Reviewer:** `umans/umans-kimi-k2.7` +**Date:** 2026-06-28 +**Scope:** Review only — no code changes committed. + +--- + +## Executive Summary + +The workspace-star feature itself (persisted workspace "star" toggle + priority scheduling in the provider concurrency manager) is conceptually sound and mostly correctly threaded through the monorepo: + +- `@dispatch/wire` adds `starred` to `Workspace`. +- `conversation-store` persists and migrates legacy workspace rows. +- `provider-concurrency` queues starred-workspace waiters ahead of non-starred ones and re-sorts on star/unstar. +- `transport-http` exposes `PUT /workspaces/:id/star` and `DELETE /workspaces/:id/star`. + +However, this branch also **reverts a large amount of unrelated crash-investigation work** that lives on `dev`, including production memory telemetry, an `uncaughtException`/`unhandledRejection` guard, and the LSP-disable precaution. That reversion is the single biggest issue and looks unintended given the commit title. There is also a failing test in `provider-wrapper.test.ts` and several smaller edge cases in the star-priority logic. + +**Do not merge this branch as-is.** + +--- + +## Verification Run + +| Command | Result | +|---|---| +| `bun run typecheck` | ✅PASS | +| `bun run check` | ✅PASS (12 pre-existing Biome warnings, 0 errors) | +| `bun test packages/provider-concurrency/src/concurrency-manager.test.ts packages/conversation-store/src/store-workspace.test.ts packages/wire/src/index.test.ts` | ⚠️1/78 failed (see §1) | + +The failing test is covered in detail below. + +--- + +## Detailed Findings + +### 1. BLOCKING — Test suite regression in `provider-wrapper.test.ts` + +**File:** `packages/provider-concurrency/src/provider-wrapper.test.ts` +**Test:** `wrapProviderWithConcurrency > releases the slot even when the stream throws` + +``` +error: +Expected promise +Received: [AsyncFunction] + at packages/provider-concurrency/src/provider-wrapper.test.ts:102:16 +(fail) wrapProviderWithConcurrency > releases the slot even when the stream throws +``` + +- The test asserts that `await expect(async () => { for await... }).rejects.toThrow("stream exploded")` throws when the wrapped async generator throws mid-stream. +- The failure message suggests the async function passed to `expect()` is not being recognized/produced correctly. +- This may be a Vitest/Bun async-generator interop issue, but it makes the test suite red on this branch. +- **Recommendation:** fix or replace the failing assertion before merge. The underlying `try/finally` release logic in `provider-wrapper.ts` looks correct, so this is likely an assertion-level problem. + +### 2. MAJOR — Branch reverts unrelated crash telemetry & production mitigations + +This branch removes or reverts important work that is present on `dev`. Because the commit title is `feat(workspace-star): starred workspace priority for concurrency limiting`, these changes appear to be accidental scope creep or a botched rebase. + +- **Files deleted:** + - `crash-review-report.md` + - `notes/crash-investigation-findings.md` + - `notes/memory-leak-investigation-handoff.md` + - `bin/apply-memory-limits.sh` + - `packages/host-bin/src/mem-telemetry.ts` + - `packages/host-bin/src/mem-telemetry.test.ts` +- **Behavioral reversions:** + - `packages/host-bin/src/main.ts`: removes `process.on("uncaughtException")` and `process.on("unhandledRejection")` guards; removes periodic memory telemetry; re-enables `import { extension as lspExt } from "@dispatch/lsp"`. + - `packages/session-orchestrator/src/orchestrator.ts`: removes `getActiveConversationCount()`, `SessionOrchestratorDeps.sampleMemory`, and per-turn memory telemetry. + - `packages/transport-http/src/extension.ts`: adds `"lsp"` to `dependsOn`, making the LSP extension required again. +- **Risk:** if merged, production loses: the cgroup memory-limit helper script; observability used to diagnose recent crashes; the unhandled-exception guard that prevents a single SSH error from killing the whole process; and the LSP-disable precaution that was added as a stability measure. +- **Recommendation:** strip these reversions out of `feature/workspace-star`. If removing crash telemetry is intentional, do it in a separate, explicitly titled commit/PR with rationale and operational sign-off. + +### 3. MEDIUM — In-memory starred cache leaks IDs for deleted workspaces + +**File:** `packages/provider-concurrency/src/concurrency-manager.ts` + +- `starredWorkspaces` is a `Set<string>` populated by `notifyWorkspaceStarred(true)`. +- `setWorkspaceStarred(false)` deletes the ID from the set. +- However, `deleteWorkspace()` in the conversation store does **not** publish any event/hook to the concurrency manager, so a starred workspace that is deleted remains in the cache until process restart. +- Impact is small (a string per deleted starred workspace), but it is unbounded. +- **Edge case:** if a new workspace later reuses the same slug, it could inherit stale star priority. +- **Recommendation:** add a `notifyWorkspaceStarred(id, false)` call when deleting a workspace, or expose a hook that provider-concurrency can listen to. At minimum, document this minor leak. + +### 4. MEDIUM — Stale in-memory priority when concurrency extension is absent + +**File:** `packages/transport-http/src/app.ts` (lines 1513, 1535) + +```ts +opts.concurrencyService?.notifyWorkspaceStarred(workspaceId, true); +``` + +- `transport-http` does **not** declare `provider-concurrency` in its manifest `dependsOn` (it is loaded opportunistically in `createApp`). +- If the concurrency extension is absent or disabled, the star toggle is persisted, but the in-memory priority cache is never updated. Already-queued agents will keep their old priority until a process restart seeds the cache. +- **Recommendation:** either add `provider-concurrency` to transport-http's `dependsOn` so the service is guaranteed, or emit a warning log when `notifyWorkspaceStarred` is skipped. + +### 5. LOW — `notifyWorkspaceStarred` resolves waiters synchronously inside the HTTP handler + +**File:** `packages/provider-concurrency/src/concurrency-manager.ts`, `notifyWorkspaceStarred` + +- The method sorts every provider's queue and calls `tryGrantNext`, which can resolve queued acquisition promises. +- Those resolves happen while the transport request handler is still on the call stack. +- In practice this is fine because Promise resolution is queued as a microtask, but it is re-entrant and worth noting if future code adds side effects to resolution. +- **Recommendation:** no code change required unless maintainers prefer to defer granting to the next tick. + +### 6. LOW — Star/unstar endpoints follow existing route conventions but introduce scheduling side effects + +**File:** `packages/transport-http/src/app.ts` (lines 1499-1543) + +- `PUT /workspaces/:id/star` and `DELETE /workspaces/:id/star` reuse the same slug validation and 500-error handling as other workspace routes. +- No additional auth/rate-limiting is added, which is consistent with the rest of the workspace API. +- Because these endpoints now affect runtime scheduling priority, a malicious or buggy client could flip-star workspaces rapidly to disturb queue ordering. +- **Recommendation:** acceptable if consistent with existing routes; otherwise gate star toggles behind the same authorization as workspace mutation. + +### 7. LOW — `compareWaiters` does not tie-break equal `promptStartedAt` + +**File:** `packages/provider-concurrency/src/concurrency-manager.ts` + +```ts +function compareWaiters(a: QueuedWaiter, b: QueuedWaiter): number { + const aStarred = isWorkspaceStarred(a.workspaceId); + const bStarred = isWorkspaceStarred(b.workspaceId); + if (aStarred !== bStarred) return aStarred ? -1 : 1; + return a.promptStartedAt - b.promptStartedAt; +} +``` + +- If two agents have the same `promptStartedAt` and the same star status, JavaScript's stable sort preserves insertion order, so FIFO is still guaranteed. +- However, the comparator returns `0` in this case and relies on sort stability, which is true for modern engines but not obvious from the code. +- **Recommendation:** add an explicit tie-breaker (e.g., a monotonic enqueue sequence number) to make ordering deterministic across all runtimes and future proofs. + +### 8. LOW/CONCERN — Default workspace can be starred + +- `setWorkspaceStarred` allows starring `"default"`. +- This is probably intentional (default is just another workspace), but it means all unassigned/legacy conversations inherit star priority. +- **Recommendation:** confirm this is the intended UX. If not, add a guard mirroring `deleteWorkspace`'s prohibition on `"default"`. + +### 9. POSITIVE — Backward compatibility is handled correctly + +- `parseWorkspaceRow` treats absent or non-boolean `starred` as `false`, so legacy workspace rows load safely. +- `isWorkspaceStarred` is optional in `ConcurrencyManagerOpts`; tests verify that omitting it behaves like all workspaces are non-starred. +- `WorkspaceResponse` inherits `starred` through the `Workspace` / `WorkspaceEntry` types. +- The seeded activation path (`extension.ts`) guards `listWorkspaces()` with try/catch and logs warnings, degrading cleanly if the store is unreachable. + +--- + +## Merge Checklist + +Before merging `feature/workspace-star`: + +1. **Fix or disable the failing `provider-wrapper.test.ts` test.** Do not leave the suite red. +2. **Remove the unrelated crash-telemetry/LSP reversions** from this branch, or move them to a separate explicitly labeled commit/PR. +3. Consider: clearing the starred cache when a workspace is deleted. +4. Consider: declaring `provider-concurrency` as a hard dependency of `transport-http` if star scheduling is a first-class feature, or logging a warning when the optional service is absent. +5. Confirm UX intent: can `"default"` be starred? + +--- + +## Note + +This review was generated from `git diff dev..HEAD` on the `feature/workspace-star` worktree. No source code was modified. diff --git a/notes/server-crash-investigation.md b/notes/server-crash-investigation.md new file mode 100644 index 0000000..6e11c4f --- /dev/null +++ b/notes/server-crash-investigation.md @@ -0,0 +1,257 @@ +# Server Crash Investigation — LSP Suspected + +**Date:** 2026-06-27 +**Investigator:** umans/umans-glm-5.2 +**Checkout:** `dispatch/backend` on `dev` (commit `f9d1ca5`, working tree clean) + +## TL;DR + +The crash under investigation (Jun 28 00:12 JST) is **a Bun runtime +segfault**, not an LSP application crash. The three LSP issues the task +suspected (JSON-parse TypeError, fs.watch ENOENT, unbounded cache memory +leak) were **all already fixed** in commits `05ff256` (21:14) and `f9d1ca5` +(21:32), compiled into the binary at 21:40 — which is the binary that +crashed 2.5 h later. A separate, still-live **memory leak** (6.2 GB in 2.5 h +post-fix; 25.7 GB in 18 h pre-fix) fed memory pressure into Bun's +allocator and triggered the native crash. The leak is **not** in the LSP +caches (they are now bounded to ~50 docs + ~100 diag entries) — it is +elsewhere in the runtime (most likely AI-SDK streaming buffers / retained +conversation message arrays during long agent turns). + +The task's premise ("there is an uncommitted hot-fix that disables LSP in +`host-bin`/`transport-http`") does **not** match the checkout. The working +tree is clean; LSP is fully enabled (not disabled); no `test_race.js` +exists. The developer chose to **fix** LSP rather than disable it. + +--- + +## 1. What the logs actually show + +### The crash (the event under investigation) + +`systemctl status dispatch.service` (the service is named `dispatch.service`, +**not** `dispatch-server.service` — the latter does not exist): + +``` +Jun 28 00:12:02 dispatch-server[931590]: Bun v1.3.13 (bf2e2cec) Linux x64 +Jun 28 00:12:02 dispatch-server[931590]: WSL Kernel v6.6.87 | glibc v2.43 +Jun 28 00:12:02 dispatch-server[931590]: Args: "/usr/bin/dispatch-server" +Jun 28 00:12:02 dispatch-server[931590]: Elapsed: 9115307ms | User: 582013ms | Sys: 738525ms +Jun 28 00:12:02 dispatch-server[931590]: RSS: 0.02ZB | Peak: 0.29GB | Commit: 0.02ZB | Faults: 0 | Machine: 33.24GB +Jun 28 00:12:02 dispatch-server[931590]: panic(main thread): Segmentation fault at address 0x0 +Jun 28 00:12:02 dispatch-server[931590]: oh no: Bun has crashed. This indicates a bug in Bun, not your code. +Jun 28 00:12:05 systemd[1]: dispatch.service: Main process exited, code=dumped, status=4/ILL +Jun 28 00:12:05 systemd[1]: dispatch.service: Failed with result 'core-dump'. +Jun 28 00:12:05 systemd[1]: dispatch.service: Consumed 1h 29min CPU over 2h 31min wall, 6.2G memory peak. +``` + +Key signals: + +- **`panic(main thread): Segmentation fault at address 0x0`** — a NULL-pointer + dereference **inside Bun's runtime**, not in dispatch code. Bun's own crash + handler states "This indicates a bug in Bun, not your code." +- **Corrupted crash-time memory report.** `RSS: 0.02ZB` (zettabytes) and + `Faults: 0` are impossible values; `Peak: 0.29GB` inside the dump contradicts + systemd's cgroup accounting of `6.2G memory peak`. The crash corrupted the + runtime state *before* the handler read its own stats — consistent with a + heap corruption / use-after-free, not a clean NULL deref of a known address. +- **Signal mismatch.** Bun reports SIGSEGV ("Segmentation fault") but systemd + recorded `status=4/ILL` (signal 4 = SIGILL, illegal instruction). Two + different signals from one crash ⇒ the runtime was already corrupt when the + trap fired. Classic signature of an allocator/GC corruption under memory + pressure. +- **No coredump retained** (`coredumpctl list` → "No coredumps found"; + `/var/lib/systemd/coredump` empty), so the stack cannot be symbolicated + locally. The redacted report URL is in the journal: + `https://bun.report/1.3.13/l_1bf2e2ce…`. +- **Memory pressure was real.** systemd's cgroup (trustworthy) says the + process peaked at **6.2 GB** over 2 h 31 m before dying — a ~2.5 GB/h growth + rate from a fresh boot. + +### Crash history (last 48 h) — two distinct failure modes + +``` +Jun 26 15:09 Failed exit-code 16h12m wall 3.4G peak (app-level) +Jun 27 02:44 Failed exit-code 1h22m wall 3.1G peak (app-level — crash loop begins) +Jun 27 02:51 Failed exit-code 7m wall 1.1G peak (crash loop) +Jun 27 02:52 Failed exit-code 58s wall 700M peak (crash loop) +Jun 27 20:58 Stopped (manual) 17h58m wall 25.7G peak (huge leak, no crash) +Jun 28 00:12 SIGSEGV/SIGILL dump 2h31m wall 6.2G peak (Bun segfault — THIS event) +``` + +The Jun 27 02:44–02:52 run is a **tight crash loop** (3 exits in 8 min with +collapsing uptime: 1h22m → 7m → 58s). These were `exit-code` failures +(**not** segfaults) — i.e. the application-level LSP crashes (JSON TypeError, +ENOENT). The only segfault in 5 days of logs is the 00:12 event, which +occurred **after** the LSP fixes were deployed. + +--- + +## 2. The task premise vs. the actual checkout + +The task described an **uncommitted hot-fix that disables LSP** in +`packages/host-bin/src/main.ts` and `packages/transport-http/src/extension.ts`, +plus an untracked `packages/lsp/src/test_race.js`. **None of this exists:** + +- `git status` → "nothing to commit, working tree clean". No untracked files. +- `fd test_race` → no results. `test_race.js` does not exist anywhere. +- `git log -- packages/host-bin/src/main.ts packages/transport-http/src/extension.ts` + → no "disable LSP" commit. LSP has only ever been **enabled** here. +- `host-bin/src/main.ts:24` imports `@dispatch/lsp`; line 104 includes `lspExt` + in `CORE_EXTENSIONS`. LSP is **enabled**. +- `transport-http/src/extension.ts:98` calls `host.getService(lspServiceHandle)` + with no try/catch — LSP is wired **straight-through**, not made optional. + +What the developer actually did (commits `05ff256` @ 21:14 and `f9d1ca5` @ +21:32) was **fix** the LSP crashes in place, then rebuild +(`/usr/bin/dispatch-server` mtime `2026-06-27 21:40:04`) and restart +(`Jun 27 21:40:06 Started`). The prior AI review that identified the three +bugs was committed alongside the fixes as `ai-review-report.md`. + +--- + +## 3. The three suspected LSP issues — all already fixed + +A committed prior review (`ai-review-report.md`, part of `05ff256`) names the +exact three issues the task raised. Each is fixed in the current tree: + +### 3a. Unhandled JSON parse / TypeError (`client.ts`) — FIXED + +`rpc.ts:89-99` wraps `JSON.parse` in try/catch (malformed/split-UTF-8 messages +are logged and skipped). The **fatal** bug, though, was the defence-in-depth +boundary in `client.ts`: when the language-server process died, `markBroken()` +set `this.rpc = null`; a final stdout flush then evaluated +`this.rpc?.handleMessage(msg).catch(() => {})` → short-circuits to +`undefined.catch()` → a **synchronous TypeError** that crashed the process. + +**Fix** (`client.ts:310`): a second optional-chaining `?.`: +```ts +void this.rpc?.handleMessage(msg)?.catch(() => {}); +``` +`undefined?.catch()` → `undefined`, no throw. Covered by regression test +`handleBytes does not crash when the server dies and rpc is null (Bug 1)`. + +### 3b. ENOENT from fs.watch on transient `.old_modules` dirs — FIXED + +`extension.ts`'s recursive `node:fs.watch` emitted `'error'` events when +`bun install` deleted transient `.old_modules-*` directories. With no +`.on("error")` listener, Node/Bun escalates the event to an **uncaught +exception** that kills the process. + +**Fix** (`extension.ts:100`): a no-op error listener swallows transient FS +errors: +```ts +watcher.on("error", () => { /* ignore transient FS errors */ }); +``` +Covered by `extension.test.ts` (the `__test__realFileWatcher` re-export lets +the behaviour be exercised with an injected fake watcher). + +### 3c. Unbounded cache memory leak — FIXED (in LSP) + +`LanguageServerClient` previously retained every touched file's text + +diagnostics forever in `openDocuments`, `lastDiagSnapshot`, and +`pushDiagnostics` (the documented "9.5 GB over 12 h" leak). + +**Fixes:** +- `client.ts`: `MAX_OPEN_DOCUMENTS = 50` LRU — overflow evicts the + least-recently-used doc via `textDocument/didClose` + purge + (`evictIfOverCap`, `closeDocument`). +- `diagnostics.ts`: `MAX_PUSH_DIAGNOSTICS = 100` — background-scanned files + (never opened by the agent) are evicted oldest-first + (`evictPushIfOverCap`, delete-then-set for LRU recency). +- `markBroken()` now `clear()`s `openDocuments` + `lastDiagSnapshot` so a + repeatedly-crashed/re-spawned client doesn't accumulate across cycles. +- Initialize timeout leak fixed: the timeout is now passed into + `rpc.sendRequest` (which deletes the pending entry on expiry) instead of a + `Promise.race` that left the original promise lodged in `pending` forever. + +--- + +## 4. Root cause of the 00:12 crash + +**The crash is a Bun runtime segfault, not an LSP bug.** Reasoning: + +1. The binary that crashed (`/usr/bin/dispatch-server`, built 21:40:04) + **contains all three LSP fixes** (commits 21:14 + 21:32 precede the build). + So the known LSP crash paths were already closed. +2. The crash signature is a native `panic(main thread): Segmentation fault at + address 0x0` emitted by **Bun's own crash handler**, which explicitly says + "This indicates a bug in Bun, not your code." Application-level crashes + (the pre-fix TypeError/ENOENT) exit with a JS stack trace and `exit-code`, + not a native `code=dumped, status=4/ILL`. +3. The corrupted memory readout (`RSS: 0.02ZB`, `Faults: 0`, SIGSEGV-vs-SIGILL + mismatch) points to **heap corruption in Bun's allocator/GC**, not a clean + dereference of application state. + +**The trigger is memory pressure from a still-live leak.** The post-fix binary +grew to **6.2 GB in 2.5 h** (the pre-fix session hit **25.7 GB in 18 h**). The +LSP caches are now bounded to ≤50 open documents + ≤100 diagnostic entries — +orders of magnitude too small to account for gigabytes. The leak is +**elsewhere**: + +- `conversation-store` is **SQLite-backed** (`storage.get`/`set` via + `StorageNamespace`), not an in-memory map — not the leak. +- `session-orchestrator`'s `activeConversations` is a `Set<string>` of IDs + (tiny) — not the leak. +- Most likely culprits (not yet confirmed, out of investigation scope): the + **AI SDK streaming buffers** and the **conversation message arrays assembled + in memory** per turn (`orchestrator.ts` `for await (const event of + provider.stream(...))`), which for long multi-step agent turns can hold large + transcripts; plus Bun's standalone-executable memory reclamation under WSL. + +So there are **two problems**, only one of which is fixed: + +| Problem | Status | Failure mode | +|---|---|---| +| LSP app crashes (JSON TypeError, ENOENT) | ✅ Fixed (05ff256, f9d1ca5) | `exit-code`, crash loop | +| Memory leak → Bun segfault | ❌ Not fixed | native `code=dumped`, SIGSEGV/SIGILL | + +--- + +## 5. Proposed fix (do not implement — investigation only) + +The LSP work is done and correct; do **not** disable LSP. The remaining issue +is the memory leak that triggers the native crash. Recommended actions, in +priority order: + +1. **Treat the leak as the primary defect, not LSP.** Open a separate + investigation to localize the 2.5 GB/h growth. First step: add periodic + `process.memoryUsage().rss` + `Bun.gc()` logging on a timer and correlate + growth with active conversations/turns. Suspect the AI-SDK streaming path + and per-turn message assembly in `session-orchestrator/orchestrator.ts`. +2. **Add a memory-pressure circuit breaker** to `dispatch.service` so a leak + degrades gracefully instead of segfaulting: a watchdog that restarts the + process on RSS exceeding a threshold (e.g. 3 GB), or systemd + `MemoryHigh=`/`MemoryMax=` cgroup limits with a managed restart. This turns + the uncontrolled segfault into a controlled recycle. +3. **File the Bun crash** at `https://bun.report/1.3.13/l_1bf2e2ce…` (the URL + is in the journal). The corrupted RSS/signal readout is a Bun bug worth + reporting regardless of our leak — a memory leak should not crash the + runtime with a native segfault; it should OOM-kill cleanly. +4. **Consider a Bun upgrade.** The crash is on `Bun v1.3.13`; allocator/GC + fixes land frequently. Pin and test a newer Bun in the standalone build. +5. **Keep the LSP fixes** (`?.catch()`, `watcher.on("error")`, bounded caches, + `markBroken` clear, sendRequest timeout). They are correct and tested; + disabling LSP would regress the diagnostics tool with no crash benefit, + since the crash is not in LSP. + +### Evidence summary + +| Claim | Evidence | +|---|---| +| LSP fixes deployed before crash | binary mtime 21:40:04 > commits 21:14/21:32; service (re)started 21:40:06 | +| Crash is native, not app-level | `panic(main thread): Segmentation fault`, `code=dumped, status=4/ILL`, no JS stack | +| LSP caches bounded | `MAX_OPEN_DOCUMENTS=50`, `MAX_PUSH_DIAGNOSTICS=100` | +| Leak persists post-fix | 6.2 GB / 2.5 h (post-fix) vs 25.7 GB / 18 h (pre-fix) | +| Conversation store not the leak | SQLite-backed (`StorageNamespace`), not in-memory maps | +| No "disable LSP" hot-fix exists | clean tree; LSP enabled in `main.ts:104` + `transport-http:98`; no `test_race.js` | + +### Contract gaps / change-requests for other units + +- **session-orchestrator:** the per-turn streaming loop (`orchestrator.ts` + `provider.stream(...)`) and message-assembly path are the prime leak suspect. + Request a memory profile of a long multi-step turn to confirm whether + buffers/arrays are retained after the turn completes. No contract change + needed — this is an implementation/leak-localization request. +- **host-bin / systemd unit:** request a `MemoryMax=`/watchdog addition to + `/etc/systemd/system/dispatch.service` (infra, not code). diff --git a/notes/tool-call-in-thinking-bug.md b/notes/tool-call-in-thinking-bug.md new file mode 100644 index 0000000..9a63b6e --- /dev/null +++ b/notes/tool-call-in-thinking-bug.md @@ -0,0 +1,265 @@ +# Bug Investigation: Tool Calls Appearing in Thinking + Turn Ends Abruptly + +**Date:** 2026-06-28 +**Conversation:** `1a997b08-09c2-412c-b296-48703c8aaf58` +**Model:** `umans-flash` (OpenAI-compatible endpoint at `https://api.code.umans.ai/v1`) +**Branch:** predev +**Investigation by:** umans/umans-glm-5.2 + Gemini 3.1 Pro (High) review + +## Executive Summary + +The model's tool calls appear as text inside a `thinking` chunk instead of being +parsed as structured `tool-call` chunks, and the turn ends abruptly. This is a +**regression** caused by a conversation-history reconstruction bug in +`conversation-store` that merges messages from different turns into a single +`user` message, producing a malformed prompt that confuses the model. + +The root cause is that `append()` assigns `msgIdx` as a **local** index (reset to +0 for each append call), but `load()` groups chunks into messages using `msgIdx` +as if it were a **global** identifier. Since every single-message `append()` call +produces `msgIdx=0`, all chunks from consecutive user/assistant messages across +multiple turns collapse into one giant `user`-role message. The model receives a +corrupted conversation where its own prior assistant responses are labeled as +`user` content and its prior `tool-call` chunks are silently stripped (because +`convertUserMessage` only extracts `text` chunks). Bereft of structural context, +the model falls back to emitting tool calls as text inside `reasoning_content`, +which the stream parser routes to the `thinking` channel. Since no structured +`delta.tool_calls` is emitted, the kernel sees zero tool calls and ends the turn. + +A secondary bug in `reconcile.ts` silently drops assistant messages that contain +only `thinking` chunks (no `text` or `tool-call`), causing the buggy output +itself to vanish on the next load. + +## Root Cause Analysis + +### Primary: `msgIdx` collision in conversation-store (`store.ts`) + +**The bug:** `append()` assigns `msgIdx` as the index within the `messages` +array passed to each call: + +```typescript +// store.ts append() — line 585 +for (let msgIdx = 0; msgIdx < messages.length; msgIdx++) { + const msg = messages[msgIdx]; + // ... + const entry: PersistedChunkEntry = { chunk, role: msg.role, msgIdx, chunkIdx }; +``` + +Every `append()` call starts `msgIdx` at 0. The orchestrator calls `append()` +multiple times per turn: +1. `append([userMsg])` — user message → `msgIdx=0` +2. `onStepComplete([assistantMsg, ...toolResults])` — step output → `msgIdx=0,1,2...` + +So every single-message append (the user message, or a text-only assistant +response) produces `msgIdx=0`. Only multi-message appends (assistant + tool +results) produce `msgIdx=1,2`. + +**`load()`** then groups chunks by `msgIdx`: + +```typescript +// store.ts load() — line 684 +if (entry.msgIdx !== currentMsgIdx) { + // start new message + currentRole = entry.role; + currentMsgIdx = entry.msgIdx; +} +currentChunks.push(entry.chunk); +``` + +Since all single-message-appended chunks share `msgIdx=0`, they collapse into ONE +message with the role of the first chunk (usually `user`). + +### Verified with database evidence + +Direct query of `.dispatch-data/dispatch.db` (the dev server's KV store): + +``` +seq=0000000001 role=user msgIdx=0 type=text "hello" +seq=0000000002 role=assistant msgIdx=0 type=thinking "The user simply said hello..." +seq=0000000003 role=assistant msgIdx=0 type=text "Hello! How can I help you today?" +seq=0000000004 role=user msgIdx=0 type=text "Can you read some files in this project" +seq=0000000005 role=assistant msgIdx=0 type=thinking "The user is asking me to read files..." +seq=0000000006 role=assistant msgIdx=0 type=text "I'd be happy to read files..." +seq=0000000007 role=user msgIdx=0 type=text "the caching, there are 3 different percentages displayed" +seq=0000000008 role=assistant msgIdx=0 type=thinking "The user wants to look at files related to caching..." +seq=0000000009 role=assistant msgIdx=0 type=text "Let me explore the project structure..." +seq=0000000010 role=assistant msgIdx=0 type=tool-call run_shell (find ...) +seq=0000000011 role=assistant msgIdx=0 type=tool-call run_shell (grep ...) +seq=0000000012 role=tool msgIdx=1 type=tool-result run_shell (find output) +seq=0000000013 role=tool msgIdx=2 type=tool-result run_shell (empty, error) +seq=0000000014 role=assistant msgIdx=0 type=thinking "<function=run_shell>..." (BUGGY) +``` + +**All of seq 1–11 have `msgIdx=0`** — despite belonging to 6+ different +messages across 3 turns. Only the multi-message step append (seq 12–13) gets +`msgIdx=1,2`. + +### Confirmed with a load() reproduction script + +Running the actual `createConversationStore` + `load()` against the dev DB +produces **3 messages** instead of the expected ~9: + +``` +MSG[0] role=user chunks=11 ← seq 1-11 ALL MERGED (user+assistant+user+assistant+...) +MSG[1] role=tool chunks=1 ← seq 12 +MSG[2] role=tool chunks=1 ← seq 13 +``` + +### What the model received (trace evidence) + +The `provider.request` span in `.dispatch-data/traces.db` captured the verbatim +request body sent to the model for the buggy step (step 1 of turn 3). The +prompt body span confirms the malformed message structure: + +``` +MSG [0] role=user 6 chunks: hello + assistant-thinking + assistant-text + + user-text + assistant-thinking + assistant-text +MSG [1] role=user 1 chunk: "the caching, there are 3 different percentages displayed" +MSG [2] role=assistant 4 chunks: thinking + text + 2× tool-call (step 0 output) +MSG [3] role=tool tool-result (find output) +MSG [4] role=tool tool-result (empty, isError=true) +``` + +**MSG [0] is the corruption:** it has `role=user` but contains assistant +thinking + text chunks from turns 1 and 2. The model sees its own prior +responses as user-authored text. Its prior `tool-call` chunks (seq 10–11) are +in this merged blob with role `user` — and `convertUserMessage()` only extracts +`text` chunks, so the tool-call history is **silently stripped** from the +prompt entirely. + +The model thus has no structural example of how to make tool calls in this +conversation. It falls back to text-based tool-call syntax (`<function=...>`) +inside `reasoning_content`, which the stream parser routes to the `thinking` +channel. + +### Why the turn ended abruptly + +In `run-turn.ts` (line 708–711): + +```typescript +if (stepResult.toolCalls.length === 0) { + finishReason = stepResult.finishReason; + break; +} +``` + +The model emitted `finish_reason: "stop"` (confirmed in the trace: +`step` span with `finishReason:"stop"`). No structured `delta.tool_calls` was +in the SSE stream, so `toolCalls` was empty. The kernel interpreted this as +"the model is done" and sealed the turn with no tool execution and no final +text response. + +### Tools WERE correctly registered + +The request body confirms `toolCount: 9` — all 9 tools were sent in proper +OpenAI format (`{type:"function", function:{name, description, parameters}}`). +The bug is NOT a tool registration issue. + +### Secondary: `reconcile.ts` drops thinking-only assistant messages + +The `reconcile.repair` span in the traces DB confirms: + +```json +{"repairedCount":0,"firstRepairedToolCallId":null,"strippedErrorChunks":0,"droppedEmptyMessages":1} +``` + +`reconcileWithReport()` Phase 2 drops assistant messages that have no `text` or +`tool-call` chunks: + +```typescript +// reconcile.ts line 47-54 +const hasContent = msg.chunks.some( + (chunk) => chunk.type === "text" || chunk.type === "tool-call", +); +if (!hasContent) { + droppedEmptyMessages++; + continue; +} +``` + +An assistant message containing only a `thinking` chunk has +`hasContent === false` and is silently deleted. The buggy seq 14 output +(assistant, thinking-only) would be dropped on the next conversation load, +making the corruption even worse — the evidence of the bug disappears. + +## Which File(s) Need Fixing + +1. **`packages/conversation-store/src/store.ts`** — primary fix. The `msgIdx` + assignment in `append()` and/or the grouping logic in `load()` must produce + correct message boundaries. + +2. **`packages/conversation-store/src/reconcile.ts`** — secondary fix. The + `hasContent` check must recognize `thinking` chunks as valid content so + thinking-only assistant messages are not silently dropped. + +3. **`packages/openai-stream/src/convert-messages.ts`** — hardening (optional). + `convertAssistantMessage` concatenates `thinking` and `text` chunks into a + single `content` string. Wrapping thinking in explicit tags (or omitting it) + would give the model cleaner structural context and reduce confusion. + +## Proposed Fixes (do NOT implement — investigation only) + +### Fix 1: Correct message grouping in `load()` (store.ts) + +**Option A — split on role change too (minimal, pragmatic):** + +```typescript +if (entry.msgIdx !== currentMsgIdx || entry.role !== currentRole) { + // flush previous message, start new one +} +``` + +This splits messages whenever the role changes, even if `msgIdx` is the same. +It handles the common case (user → assistant → user) correctly. + +**Option B — global msgIdx (proper fix):** + +Track a per-conversation global message counter (persisted alongside `seqKey`) +so each message gets a unique, monotonically increasing `msgIdx` across all +append calls. This makes `msgIdx` a true message identifier rather than a +local index. + +### Fix 2: Preserve thinking chunks in reconcile.ts + +```typescript +const hasContent = msg.chunks.some( + (chunk) => + chunk.type === "text" || + chunk.type === "tool-call" || + chunk.type === "thinking", +); +``` + +### Fix 3: (Optional) Wrap thinking in convert-messages.ts + +```typescript +const content = textChunks.map((c) => { + if (c.type === "thinking") return `<think>\n${c.text}\n</think>\n`; + return c.text; +}).join(""); +``` + +## Additional Notes + +- This is a **regression** — the user confirms tool calls worked before recent + merges. The `msgIdx`-based chunk storage was introduced in commit `44e2717` + (June 6, "feat(wire,conversation-store): per-chunk seq sync cursor"). Before + that, whole messages were stored (one per seq), so `load()` simply read each + stored message as-is — no grouping bug was possible. The regression likely + became visible when `onStepComplete` incremental persistence (commit + `519b799`, "feat: incremental seq assignment during generation") started + calling `append()` multiple times per turn (once per step) instead of batching + all messages in a single `append()` call at the end. + +- The model `umans-flash` correctly emitted structured `delta.tool_calls` in + step 0 (seq 10–11 worked fine). The fallback to text-based tool calls only + happened in step 1, after the model received the corrupted history (which + lacked prior tool-call examples). This confirms the bug is in the history + reconstruction, not in the model or the stream parser. + +## Gemini Review + +A Gemini 3.1 Pro (High) review was conducted in parallel. Its full report is at +`ai-review-report.md` (project root). Gemini independently confirmed the same +root cause (`msgIdx` collision) and the `reconcile.ts` thinking-drop issue, and +proposed the same `load()` role-change fix. diff --git a/notes/turn-continuity-design.md b/notes/turn-continuity-design.md index 29ec10d..f33cec6 100644 --- a/notes/turn-continuity-design.md +++ b/notes/turn-continuity-design.md @@ -1,7 +1,7 @@ # Turn continuity — detached turns + multi-client live view > Status: DESIGN (locked) → backend implementation in progress. FE work couriered -> to `../dispatch-web`. See ORCHESTRATOR §7 (cross-repo). +> to `../frontend`. See ORCHESTRATOR §7 (cross-repo). ## Problem (confirmed by code trace) |