Repository navigation
feat(muse-proxy): warm sessions on a persistent muse serve (MSP) host (#480) - #769
Conversation
…#480) One persistent 'muse serve' process hosts durable MSP sessions keyed by sha256(model + system + first user message), persisted to warm-sessions.json in the workdir (LRU 64, 2h TTL) and resumed after proxy/host restarts. Keyed hits send only the new user turn; the provider prompt cache then serves the ~26k scaffolding tokens as cache reads (measured: turn 2 input 26,455 tokens, 26,225 of them cacheReadTokens). Diverged histories re-bootstrap as a fresh session whose first turn is the flattened transcript (today's exec behaviour); any warm failure before the response is committed falls back to the spawn-per-request exec path, and MUSE_PROXY_WARM=0 disables the warm path. Raw MSP (NDJSON JSON-RPC 2.0) over stdlib only - no SDK runtime dependency. Also publishes endpoint.json like the sibling proxies and pins NO_PROXY loopback for child processes.
|
Caution Review failedThe pull request is closed. Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a61e39bb23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const midUsers = suffix.slice(0, -1).filter(m => m.role === 'user').length; | ||
| if (prefixOk && suffix.length > 0 && last.role === 'user' && midUsers === 0) { | ||
| turnText = messageText(last); |
There was a problem hiding this comment.
When a client edits an assistant reply or forks a conversation after the same opening user message, this condition still reuses the original session: entry.hashes covers only the previous request's inputs, so the newly supplied assistant reply is outside the checked prefix. Sending only messageText(last) then silently discards that reply and answers against the original session's history instead. The same condition also discards intervening tool results. Store and validate the generated assistant response, and re-bootstrap whenever the supplied suffix contains history that is not already represented in the session.
Useful? React with 👍 / 👎.
| let fullContent = result.deltasText; | ||
| let unstreamedSuffix = ''; | ||
| if (typeof result.fullText === 'string' && result.fullText.length >= fullContent.length) { | ||
| if (result.fullText.startsWith(fullContent)) unstreamedSuffix = result.fullText.slice(fullContent.length); | ||
| fullContent = result.fullText; |
There was a problem hiding this comment.
When a warm turn emits intermediate agent-message commentary followed by a shorter final answer, deltasText contains the accumulated commentary while fullText contains the selected finalAnswer. This length check rejects the authoritative answer, so even a non-streaming response returns commentary concatenated with the answer instead of the final answer alone. A stub turn with accumulated deltas Long intermediate commentary. Done and final text Done reproduces this. Select the authoritative text unconditionally for non-streaming responses; keep prefix checks only for deciding what can safely be appended to an already-started stream.
Useful? React with 👍 / 👎.
|
|
||
| let result; | ||
| try { | ||
| result = await enqueueWarm(key, job); |
There was a problem hiding this comment.
When a client aborts a request, the warm path neither cancels its active turn nor prevents its queued job from starting, unlike the exec path's response-close handler. This is reproducible by delaying one warm turn, queuing another request for the same conversation, and aborting the second request before the first finishes: the canceled request still starts another turn. Abandoned requests therefore continue consuming inference and holding the per-conversation queue, potentially delaying subsequent live requests for the full turn timeout. Observe response closure, skip canceled queued jobs, and cancel active work without terminating unrelated sessions.
Useful? React with 👍 / 👎.
| child.on('error', err => this._onExit(`spawn error: ${err.message}`)); | ||
| child.on('close', (code, signal) => this._onExit(`serve host exited (code ${code}, signal ${signal})`)); |
There was a problem hiding this comment.
If initialization fails while another request is queued, _start() kills the failed child and clears its reference before that child's asynchronous close event arrives. The queued request can start a replacement host in that interval, but the old child's callback then invokes _onExit(), which unconditionally clears the replacement's state and rejects its pending RPCs. This makes the replacement initialization fail because of the previous process's exit. Bind these callbacks to their originating child and ignore them when that child is no longer the active host.
Useful? React with 👍 / 👎.
| const enqueueWarm = (key, job) => { | ||
| const prev = warmChains.get(key) || Promise.resolve(); | ||
| const run = prev.then(job, job); | ||
| warmChains.set(key, run.catch(() => {})); |
There was a problem hiding this comment.
For a long-lived proxy serving distinct conversations, warmChains permanently retains one promise per conversation key. Because run.catch(() => {}) preserves successful fulfillment values, those promises also retain each completed turn's entire result, including fullText and deltasText; the 64-session LRU never removes these entries. Extracting this implementation and completing 256 unique conversations left all 256 result objects alive after repeated garbage collection, whereas clearing the map released them. Remove settled chains when they are still the latest chain for their key so response data does not accumulate without bound.
Useful? React with 👍 / 👎.
| _write(obj) { | ||
| if (!this.child || !this.child.stdin.writable) throw new Error('serve host stdin is not writable'); | ||
| this.child.stdin.write(JSON.stringify(obj) + '\n'); |
There was a problem hiding this comment.
If the serve host exits while a buffered RPC write is pending, child.stdin emits an asynchronous error event that neither the surrounding write try/catch nor child.on('error') handles. This terminates the entire proxy instead of falling back to exec, dropping unrelated requests as well. A stub host that exits after reading the beginning of a 4 MiB turn/start request reproduces an uncaught Error: write EPIPE and proxy process exit. Register an error handler on the stdin stream and route the failure through the host/RPC rejection path.
Useful? React with 👍 / 👎.
What
muse-proxy no longer pays a fresh
muse execspawn (~26k scaffolding tokens, ~8-20s) per chat completion when it can avoid it:muse servehost (MSP = newline-delimited JSON-RPC 2.0 over stdio, schema viamuse schema generate-json-schema), driven with Node stdlib only — no@muse-code/sdkruntime dependency (the SDK exists, v1.4.13, zero-dep, but raw MSP proved tractable in a spike, so the proxy stays dependency-free and~/toolscopies run standalone).warm-sessions.jsonin the workdir (LRU 64, 2h idle TTL, endpoint.json-style atomic write). Sessions are durable in the CLI's own store, so after a proxy/host restart the map re-attaches withsession/resume.turn/start). Measured on the sandbox with the real CLI: turn 2 input was 26,455 tokens of which 26,225 were cacheReadTokens (~230 uncached) vs 26,708 fully re-read input tokens for the same turn via exec; wall clock 8.2s vs 19.9s.MUSE_PROXY_WARM=0disables the warm path. All existing behaviour preserved: strictparseProxyPort, placeholder apiKey acceptance (no auth check added), child env (META_API_KEY deleted, egress proxy + CA pinned, NO_PROXY loopback added), streaming + terminal-text reconciliation, SSE error framing.endpoint.jsonlike the sibling proxies (the muse proxy was missing it).Investigation evidence
muse exec --help:--session-id <UUID>exists, but no--resume/--continue;muse resumeis the interactive TUI. Exec-resume (option B) was therefore not the best available path once serve worked.muse serve --help+ schema export: full MSP method set (initialize,session/start|resume|leave,turn/start,session/tokenUsage,item/delta,turn/completed). Gotchas found by spiking:commandIdmust be UUIDv7 (v4 rejected,invalidParams), session objects carrysessionId(notid), usage arrives onsession/tokenUsage(not onturn/completed).Tests
node --test bin/muse-proxy.test.mjs— 9/9 pass. The stubmusenow also speaks minimal MSP: warm tests assert one session per conversation, turn 2 sends onlybeta two(not the history), MSP usage flows into the response, streaming works over MSP deltas, and a dying serve host falls back to the exec answer. Exec-path tests pinMUSE_PROXY_WARM=0and are unchanged in intent.Deployment
Sandbox only (per #480): repo file +
proxy-port.mjscopied to~/tools/muse-proxy/, system service restarted,/healthverified, before/after timings in the issue. No laptop unit/provider created (separate workstream).Need help on this PR? Tag
@codesmith-botwith what you need.