fix(sdk): bound session runtime memory - #18
Draft
matej21 wants to merge 45 commits into
Draft
Conversation
Replay after eviction now costs one event-log pass instead of two, so a shorter window buys less than it did. Ten minutes covers a longer human pause without a rebuild. Eviction is still idle-only, with no cap on the number of resident runtimes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
resolveServicePort walked getStats(), which after this branch lists every persisted session rather than the resident ones, and called getSession() on each. Every proxied asset request therefore refreshed lastAccessAt on all of them — neutering the idle sweep — and cold-loaded any evicted session, running its onSessionReady side effects: orphan-PID reconciliation, autoStart service spawn, worker relaunch. listResidentSessions() exposes the ready entries without loading and without touching lastAccessAt. A session with a live service holds a runtime lease, so it is resident by construction and the narrower sweep still finds it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
Both agent debounce paths attached lease release to control flow rather than to a finally, so two reachable paths held a scheduled lease forever — and a permanently held lease is the unbounded retention this branch exists to fix. The callback-based debounce armed an async setTimeout whose body had no try/catch and no .catch(). A throw from hasPluginPendingMessages() or from the preset-supplied debounceCallback (user code) left an unhandled rejection, scheduled stuck true, and cancelSchedule() unreached. The body moves into runDebounceCheck(), which never rejects. continue() also released the scheduled and retry leases below its two guards. The isClosed() path is reachable: performDisposal awaits onSessionClose hooks before agent.shutdown(), so a timer firing in that window returned early with the lease still held. Release now happens first, via releaseTimerLeases(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
uploadAsync acquired the runtime lease and attached it to the lifecycle, then ran three unguarded statements before lifecycle.start(): ctx.notify, getEntryAgentId, and startAsyncUpload. A throw from any of them left the lifecycle unsettled, so the lease was never released and onSessionClose — which awaits every active upload's completion — hung forever. ctx.notify is the realistic trigger: Session.createNotify calls the embedder-supplied onUserOutput with no try/catch, and in standalone and sandbox that serialises and writes to sockets. TestHarness gains an onUserOutput sink so a hostile embedder can be simulated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
performClose walked services sequentially with no try/catch, and forceStopForClose throws when a group cannot be signalled (EPERM/EIO, or "survived SIGKILL"). One such service aborted the whole close: the remaining services were never stopped, and the tail never ran — cancelAllDeferredStarts, the port release loop, services.clear, and the waiter drain were all skipped. PortPool is process-global, so a skipped release loses those ports for the life of the server, and in-flight waitForReady callers only unblocked on their own timeout. Each stop is now guarded, the tail runs in a finally, and the collected failures are rethrown at the end, so close still rejects on a process that would not die. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
sessions.create accepts a caller-supplied sessionId, and sessionIdSchema was z.string().brand() with SessionId() a bare cast — no format check anywhere. The value is interpolated into join(basePath,'sessions',sessionId), mkdir-ed recursively, used as a FileLogger target and for .events writes. path.join collapses `..`, so a crafted id created directories and wrote attacker-influenced JSONL outside the data root; eventStore.exists() was evaluated on the same traversed path, so the collision guard did not help. The schema now pins the id shape and createSession validates before deriving any path. Generated ids are UUIDv7 and both platform consumers supply crypto.randomUUID(), so the accepted shape covers existing callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
attachment_terminal_materialized, the unmaterialized projection and the onMaterialized callback cost a permanently-persisted event type, a second state map and a second event append per upload, to carry information the code already had. unmaterialized had two readers. One was `metadata.error = unmaterialized?.error ?? …`, but createFinalMetadata already writes result.error to meta.json before the terminal event on every path, so the fallback was the same value. The other was the emit whose only consumer was the reducer branch that cleared unmaterialized — self-referential bookkeeping. The durability guarantee is carried entirely by terminalEventPersisted, which loadAttachments uses to read extracted content back from meta.json. Reconciliation on session ready now keys off terminal[uploadId] plus terminalEventPersisted, which was already idempotent. The post-persist callback survives as onPersisted so notifications still follow the durable write. This lands before the event type ever ships — in a published SDK an event type is forever. Removing it now is safe: no consumer in roj, roj-platform or webmaster references it, and reducers ignore unknown event types, so branch-era logs that still carry it replay unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
loadConfig() is a public export and returned a 600s idle timeout whenever SESSION_IDLE_TIMEOUT_MS was unset, so "programmatic SDK usage keeps eviction disabled by default" only held for callers hand-building createSystem options. packages/sandbox-runtime calls loadConfig() and sets no such variable, so it would have switched to 10-minute eviction on upgrade — with every onSessionReady side effect that a reload brings — without anyone choosing it. The field is now left unset unless the variable is given, and standalone-server applies its own default. Runtimes opt in; the env var still overrides, and 0 still disables. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
SESSION_IDLE_TIMEOUT_MS appeared in no markdown in the repo while README keeps an explicit env-var table, and CreateSystemOptions.sessionIdleTimeoutMs was the only option in that interface without a doc comment. The plugin-authoring reference was the bigger gap. SessionContext gained runtimeActivity, getSessionState and reserveMailboxMessageSequence and listed none of them, while still describing onSessionClose as firing "once on termination" — it now also means "runtime evicted", which for standalone is every 10 idle minutes. A third-party plugin that starts async work without a lease has its runtime disposed underneath it, and one that releases an external lock in onSessionClose now does so on every eviction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
Four seams that the follow-up retention units all build on, landed together so their signatures are frozen before anything consumes them. SessionRuntimeActivity gains tryAcquire() and run() on the interface. Only the throwing acquire() was plugin-facing, so a plugin structurally could not avoid the throw that plugins.md tells it to wrap — and four in-tree call sites do not wrap it. run(reason, fn) acquires, retains the in-flight promise and releases in a finally, which is the ten lines every plugin was otherwise rewriting; it throws synchronously when the runtime is not ready, because the alternative — running fn unleased — is the one option that silently drops work against a tearing-down runtime. The controller gains drain(), which loops until quiescent so work scheduled during the drain is still awaited. onSessionClose gains a reason: performClose and an idle eviction ran the same hook with no discriminator, while the SDK's own hooks SIGKILL process groups and cancel running workers on it. session.ts passes a literal 'closed' for now; the manager threads the real value once it owns that call site. parseSessionId returns a Result instead of the unchecked brand cast that every transport route calls. The cast sat directly under the new SESSION_ID_PATTERN, whose own doc comment states the invariant it violates. FileStore.write now returns Err like read/stat/list/remove. It was the only Result-returning method that threw, which is why prepareUpload's abandon() branch never ran: the throw unwound past it. Five of the eight call sites already handled the Err; three were adapted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
sessionIdSchema gained a strict regex on this branch, and that schema is not only an input schema — it is the reader for persisted metadata. One unparseable meta.json therefore threw out of getAllSessionMetadata and took down the whole listing while every session individually still loaded. The blast radius grew here too: getStats() became a new caller of listSessionsWithMetadata(), so /status started failing for the whole store, which a downstream consumer reports as an unhealthy host. A non-conforming id is not even required — writeMetadata is a plain non-atomic writeFile, so a truncated file does the same. Reads are now tolerant per record: safeParse in readMetadata, skip-and-log in getAllSessionMetadata. The strict pattern stays on the write boundary. This matches the house pattern already used by loadAllSessions, which catches per session; getStats was the outlier. The same change closes the O(n) read: readMetadata is now the single parse point, so it also caches. /status is a polling endpoint and every poll was a readdir plus one readFile per session directory that had ever existed. writeMetadata is the only writer in the process, so the cache is write-through and cannot go stale; a full listing prunes it to the readdir set, which keeps deletion, out-of-band creation and fork-by-copy self-healing. An unparseable record caches its verdict so it warns once rather than once per poll; ENOENT is deliberately not cached, so probing arbitrary ids over RPC cannot grow the map. FileEventStore takes an optional logger, wired in bootstrap so the skip is not silent in production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
prepareUpload registered the lifecycle in activeUploads before any fs work and then wrote with no guard. FileStore.write used to throw, so the !writeResult.ok branch that calls lifecycle.abandon() never ran: the orphaned lifecycle's completion never resolved, onSessionClose awaited it with no timeout, and under eviction that became a session whose unloadPromise never settled — permanently unreachable and never freed, the exact opposite of what this branch is for. It was reachable from the public surface, since the upload route passes the multipart filename through and validateUploadInput checks only size and MIME. The seam commit fixed the throw. This closes the bookkeeping: everything after reserveUploadLifecycle is guarded so every failure path reaches abandon(), and the close drain is bounded — on expiry it warns with the close reason and the still unsettled upload ids and proceeds, rather than hanging disposal on one stuck lifecycle. Leases still release when the wedged work finally settles. Three smaller retention defects in the same plugin: A failed terminal-event append no longer swallows the notification. The refactor split onFinal into onTerminal and onPersisted, and processUpload awaited the former first, so an append failure unwound past the notify — leaving the client's attachment stuck in `processing` with no polling fallback until the next rebuild. The append is guarded and the notification is unconditional; the unset persisted flag makes onSessionReady re-emit. extractedContent is gone from the status-changed payload — no producer set it any more and it could carry megabytes. The orphan sweep is skipped for forked sessions. forkSession copies the event log but not the uploads dir, so the first upload of a fork's own made the sweep emit deletions for every inherited id. closeDrainTimeoutMs joins the plugin config so the bound is testable without waiting the default out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
Pause was implemented by spinning, not stopping — the loop kept turning on isPaused(), so execute() never returned and the lease released in its finally was held forever. onSessionReady re-established it on every rebuild by relaunching workers whose status was running *or* paused. That relaunch existed for a reason: resume and sendCommand both looked the worker up in runningWorkers and failed without an entry, so a paused worker had to keep spinning just to remain resumable. Pause now stops the run and resume launches a worker that is not running, which removes the entry requirement and the lease with it. sendCommand deliberately does not launch — handleCommand needs a live context, and launching from a message-passing method would silently undo an explicit pause; it now names the reason instead of reporting "not running". Re-entering execute() becomes a stated contract. It was already happening on every rebuild while definition.ts said "the worker runs until completion, failure, or cancellation", with nothing telling the worker it was a resumption. WorkerContext gains `resumed`, and cancel gained the bounded exit its twin ServiceExecutor already had — worker_cancel is an LLM-facing tool, so an agent could pin a runtime with a worker that ignores the abort signal. The relaunch loop no longer bypasses MAX_CONCURRENT_WORKERS. It was the one path running unattended at boot for every session at once, so the cap moved into launchWorker, with a compensating worker_failed for the narrow window where a start is persisted and the launch then refuses. Both stop bounds are real now. The 5 s close bail-out had an unbounded waitForEffects() drain behind it, so a stalled event-store write hung dispose() regardless; stop and drain get separate deadlines, both configurable, because close must still wait for in-flight writes after giving up on the body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
A session id reached the filesystem before anything could validate it. Hono routes on the raw pathname, so %2F survived route matching and decoded only at c.req.param; the value went straight into resolve(dataPath, 'sessions', id), and preventTraversal then guarded the already-escaped root and never fired: GET /sessions/..%2F../files/secret.txt => 200 <contents outside the data root> No session had to exist, and the same payload worked through standalone's /api/v1/instances/:id prefix. That is arbitrary host-file read on a listener the docs describe as unauthenticated by design — an acceptance that covers the data root, not the filesystem. The earlier fix on this branch applied isValidSessionId at createSession only, so every route still called acquireSessionLease(SessionId(param)) as its first statement — an unchecked brand cast, ahead of any Zod schema. parseSessionId now runs at all nine transport entries, including the uploads download route that fed a hostile id into a scoped() call that throws. Path resolution goes through the guarded FileStore rather than a bare resolve. An invalid id is now a 400 rather than a 200 session_not_found envelope. Also in the request path: A batch RPC no longer discards committed work. The loop returned results only at the end, so a genuine throw — and a session load now happens at request time — unwound past it and answered 500 with no indication which earlier items had already run, while their side effects were in the log and a retry repeated them. Each item is isolated; an unexpected throw becomes an envelope and stops the batch with the partial results intact. upload-from-url and inject-resource fetched a caller-supplied URL with no scheme, host or redirect restriction, on a server that also proxies local dev services and answers CORS preflight. A shared guard rejects non-http(s), loopback, link-local and private ranges, and re-checks every redirect hop; REMOTE_FETCH_ALLOWED_HOSTS opens specific hosts back up. Both twins use it. Session and workspace files are served with nosniff and a sandboxed CSP, and the Content-Disposition filename is escaped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
The supervision timer held its lease backwards. scheduleSupervisionTick acquired before setTimeout and the callback released at the top, so the lease covered the idle interval and the actual work ran unleased — exactly inverted. The acquire now sits inside the callback around trigger(), via tryAcquire so a tick landing on an unloading runtime skips instead of throwing. It also never stopped re-arming. _supervisionTick rescheduled whenever getDirectChildren() was non-empty, and that helper filters on parentId with no status check while agents are never removed from session state — so once any child had ever spawned, the timer re-armed for the life of the process, long after every child finished. The two re-arm sites now require a working descendant, walking the subtree so a child idling on its own children still counts. The snapshot content is unchanged: a parent still sees paused and errored children as the prompt promises, and still gets one final snapshot when the last child goes idle. An accepted consequence, per the owner's decision: supervision holds no lease while waiting, so the session becomes evictable and supervision stops after an eviction until something touches it. The assertion that pinned the session resident with a timer armed was added by the eviction commit itself and encoded the defect, so it is inverted rather than dropped — a new test asserts the runtime does reach zero. Two lease holes in the agent loop, same class: continue() opened by releasing both the scheduled and the retry lease, but errorRetryTimer stayed armed and scheduleErrorRetry guards on the timer existing, so it never re-acquired. Armed timer, zero leases. Only the scheduled lease is dropped now, and the retry lease has coverage for the first time. Disposal could clear agents and plugin contexts while a turn was still inside a tool call that ignores the abort signal, leaving hooks to dereference a cleared map. Agent gains waitForIdle(), which awaits every in-flight continue() including work started while draining; scheduleProcessing refuses to re-arm after shutdown, without which the drain is unsound. shutdown() stays synchronous — it is published API and its caller drops a returned promise. The session-side await lands with the manager. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
This is why eviction did not fire for the workload that motivated the branch.
isLiveStatus counted ready as live, so finishStartAttempt's release was
unreachable for a healthy dev server and the lease was held for its whole life:
CONTROL loaded=0 evictions=1 leases=[]
AUTOSTART loaded=1 evictions=0 leases=[{"service:auto-start":1}]
pid-registry.ts documents the motivating incident verbatim — four dev servers at
~500 MB across three sessions on 4 GB — which is exactly the set of sessions that
could never evict. needsRuntime now keeps the lease only while a start or stop is
genuinely in flight, so an idle session with a ready service evicts and its service
is stopped rather than parked in memory.
paused is gone from ServiceStatus, and pause()/resume() with it. They were SIGSTOP
and SIGCONT, reachable only from tests — there is no pause RPC method and no agent
tool — and SIGSTOP leaves the entire RSS resident, so it was never an answer to a
memory problem. The unimplemented autoPause declaration goes too: once eviction
stops services, a second idle timer over the same process has nothing to add.
A service now records who stopped it. The agent stopping a service and eviction
stopping one both landed as `stopped`, but they must not behave alike on the next
rebuild: autoStart decided purely from the executor's in-memory status, which is
always null on a fresh runtime, so it restarted a service the agent had deliberately
stopped and told the user about. stoppedBy carries the distinction through the event
and the projection, and legacy events default to 'agent' — the only default that
cannot resurrect something meant to stay down.
Two stop paths that stranded state:
forceStopForClose persisted `stopping` and then threw, unlike its twin stopInternal
which restores and re-emits. `stopping` fell through both recovery paths and the
orphan sweep skips records owned by the live process, so the service survived
holding its port while performClose had already returned that port to the
process-global pool. It now restores a reap-able status before throwing, and
`stopping` is recognised by the reconcile and the restart reducer.
hasScheduledRestart ignored portConflictTimers, so a lease could drop while a
port-conflict retry was still armed and close() would then cancel it.
Both direct process.kill sites now go through the injectable kill seam and log the
failure instead of swallowing it, and ServiceExecutor.shutdown — which no production
code called — is deleted in favour of the shipped close path its tests now exercise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
The SSRF guard landed reading REMOTE_FETCH_ALLOWED_HOSTS straight from process.env on every call, which made it the one setting in the SDK that bypassed Config — invisible to validateConfig, unsettable by an embedder that never exports the variable, and untestable without mutating the process environment. It now arrives as Config.remoteFetchAllowedHosts and is passed down as FetchGuardOptions. Omitting the option yields an empty allowlist, so a caller that forgets it fails closed rather than open. validateConfig rejects an entry carrying a slash or whitespace: a whole URL pasted in place of a bare host never matches the guard's normalised hostname, and would otherwise leave the fetch silently blocked. Normalisation of the entries stays in the guard, next to the matching logic, so config keeps no knowledge of IPv6 brackets or trailing dots. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
The services state machine exists twice — once in the sdk plugin, once in the client-side projection — because shared already references sdk for types and the sdk cannot import back. The two drifted: - `stoppedBy` never reached the projection at all, so a client could not tell a service parked by an idle eviction from one the agent shut down and expects to stay down. - `session_restarted` reset only `starting` and `ready`. A service whose runtime died mid-stop stayed `stopping` forever in every client, with nothing else on that path to clear it. - Two older divergences the mirror exposes: a first-seen `starting` event dropped its port, and a restart merged the old port instead of clearing it, leaving a stale port on screen after a portless restart. Rather than fix the instance, close the class: a conformance test replays identical event sequences through both reducers and compares after every event, the same pattern agent-status already uses. Against the previous projection 7 of its 8 cases fail, so it does discriminate. `ServiceStoppedBy` and `ServiceStopSource` are exported from the sdk so the projection can name the type instead of restating the union. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
Session sequencing was hardcoded to one counter: a private field seeded from mailbox state in the constructor. Anything else needing gap-free ids across an eviction had nowhere to put them, and a naive counter would restart at its initial value on the rebuilt runtime and mint ids that collide with the ones already in the log. reserveSequence(name, seed) makes the counter generic and takes the seed as a thunk instead of a constructor-time value. The thunk runs at most once per resident runtime, on the first reservation after it is built, so the starting point always comes from replayed state rather than from whatever the previous runtime happened to leave behind. Callers that never reserve pay nothing. reserveMailboxMessageSequence stays as the mailbox-shaped convenience — it has call sites across the workers and mailbox plugins — and now delegates, so both share one closed-runtime guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
SessionCloseReason shipped with three values and one producer, which passed the literal 'closed'. So 'evicted' was unreachable — and the one branch that reads it, in the services plugin, is what decides whether a service keeps its graceful stop window. With the discriminator wired to a constant, `parking` was always false and `forcedGraceMs` always 0, so an idle eviction took a dev server down with no window at all: the exact opposite of what its own doc comment promises. Making ready services evictable is what put that path in reach, so the regression arrives with this branch. The manager already owned the real cause. It now maps its four onto the hook's three and passes one down through dispose(): idle, disposed -> evicted parked, rebuilt on next access closed -> closed the session is over shutdown -> shutdown the process is going down idle and disposed deliberately collapse: the idle sweep and getSession's stale-runtime purge race on the same runtime, and a plugin must not see a different reason depending on which one won. dispose() defaults to 'evicted' for the same reason its docstring gives — it disposes runtime resources without changing persisted state — so the bare calls left (failed load, failed reopen, tests) get the non-destructive guess. run() and drain() go, rather than getting fixed. They had no production caller: what the follow-up commits actually consume is tryAcquire, which predates the seam commit. Both were also wrong. run() was declared Promise<T> but threw synchronously, invisible to the type checker and missed by the .catch() idiom used all around it. drain() waited only on run()'s own promises and ignored every acquire() lease — i.e. all of them — so it read as a quiescence barrier while returning immediately under live work. Repairing either would mean freezing a plugin-facing signature, and picking a drain timeout, with no caller to constrain the choice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
The write-through cache landed with no rule beyond "whoever touched the map last wins", and readers are slower than writers. readMetadata sampled the cache, awaited readFile, then stored unconditionally — so a write that landed during the await was overwritten by the stale record. That record then fed updateMetadataFromEvents, which computed from it and persisted the regressed count: four events on disk, totalEvents 2 in meta.json, permanently. A name written in the same window was lost the same way, and reconcileMetadata preserves name/tags/custom from the stored record, so it could not recover them either. totalEvents is the polling cursor and the reconciliation trigger, so wrong there means a wrong toIndex handed to clients and endless reconcile churn. Writes now win: a read stores its result only if the cache entry has not moved under it, and returns the writer's record when it has. Four more holes around the same cache: - writeMetadata cached whatever it was handed, so the cache served records readMetadata would reject — and updateMetadata built those records through a cast that hid a null merge producing no presetId and no createdAt. Same bytes on disk then listed one session in a warm process and zero in a fresh one. The merge is validated now and the cast is gone. - The unparseable verdict was cached forever for any directory that still existed, so a repaired meta.json stayed unreadable until restart, where before every poll self-healed. It is keyed to the file's mtime and size now; warn-once survives. - listSessions() still trusted every directory name, so a name its own sibling skipped was replayed into a live runtime by loadAllSessions. - Non-ENOENT read errors were never deduped, so one unreadable directory re-logged at error level on every poll — the spam the cache existed to stop. Reporting moved to the single parse point and is deduped per error kind; the read itself still retries, so a fixed permission heals. Reads also hand back a copy again: the pre-cache contract was that every read returned freshly parsed JSON, and a mutating caller would otherwise corrupt the store's own truth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
… a host The session-id hardening fixed one path segment and left the next two on the same line unvalidated. `uploadId` and `filename` came straight from the URL, `%2F` survives Hono routing and decodes at `c.req.param`, and the store's containment is lexical — so the download route read any file under the data root, including another session's whole event log, and escaping further reached a `scoped()` that throws and surfaced as a 500. It also had no symlink check at all, unlike the files route beside it, and the agent can write into the upload directory. A symlink there was served with a 200. `resolveCanonicalPath` moves out of the files route into `path-containment.ts` so both routes share one implementation: realpath the root and the target, then check containment. `uploadId` and `filename` are validated at the boundary, and `scoped()` is gone from the transport layer entirely — every path now goes through the Result-returning `realPath()`, so no request-shaped input can reach a throw. Lifting containment into SessionFileStore was the other option and was rejected: `realPath`/`resolvePath` are synchronous and used by the image processor, the filesystem plugin and four preprocessors, so making them realpath-aware would turn the store async for every caller. The SSRF guard checked IP literals and let any hostname through, which one DNS record defeats — a name pointing at loopback or the metadata endpoint was allowed. It now resolves and applies the range rules to every address returned, for the initial URL and each redirect hop, failing closed on a resolver error or an empty answer. The residual gap is rebinding, which needs socket-level pinning; the comment says so instead of conflating the two. An allowlisted host still skips resolution — naming a host that lives in a blocked range is the entire point of the allowlist. Also: 100.64/10, 192.0.0.0/24, 198.18/15, 224/4 and 240/4 were reachable, as were IPv4-compatible, IPv4-translated, NAT64 and 6to4 IPv6 forms and a host with more than one trailing dot. Allowlist entries take an optional port, so an entry that can never match is a config error rather than silently dead. Content-Disposition emits an ASCII fallback plus RFC 5987, because a non-Latin-1 header value makes `new Response` throw — every upload with a CJK or emoji filename was answering 500. And both remote fetches stream against their size cap instead of buffering a body that declares no length. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
…sult resume checked runningWorkers, then awaited an emit, then launched — so two concurrent resumes both passed the check and the second launch overwrote the first entry, exactly what the comment above it forbids. The orphan is unreachable: onSessionClose iterates the map, so it never gets invalidate(), its lease is never released, and its body keeps looping against a disposed session's context. One worker id, two live bodies, and a runtime that can never evict — the retention this branch exists to remove. Reachable from two parallel worker_resume calls, an /rpc batch, or two clients. The slot is claimed synchronously now, before the first await. cancel's guard let a worker through on map presence alone, and that entry outlives the terminal event for the whole of waitForEffects(). So cancelling a worker that had just completed overwrote its projection and threw away the recorded result, while returning ok. pause was already gated on the projection; cancel is the twin that was not. A failed relaunch compensated with worker_failed, whose reducer rebuilds the entry without state or config — so a transient cap collision or a forced unload during the emit turned a healthy paused worker into one that can never be resumed again. It compensates back to paused instead, which keeps both. The two dead ends in the onSessionReady relaunch loop now park the worker the same way, rather than leaving the projection claiming `running` with nothing running behind it. Two bounds that were missing: executeWorker's own drain in the finally had no deadline, which is the construct the previous commit claimed to have fixed, and force-release only ever dropped the worker lease and never the outstanding effect leases — so bounding one without the other would still have pinned the runtime. And a worker whose stop outlived its deadline is now refused by resume until its body actually returns, instead of being relaunched alongside the one still running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
… name it The upload routes answered 400 for every plugin error, so a storage fault raised as a 500 still reached the client as "your request was invalid" — and a client honouring 4xx will never retry a fault that is entirely server-side. The three plugin-error sites now read the error's own httpStatus. The route still speaks only 400 and 500, and says so, rather than forwarding an arbitrary number into the response. The seams this branch added were also unreachable from outside the package: parseSessionId, SessionCloseReason and the runtime-activity types were absent from index.ts, while the unchecked SessionId constructor was exported. A plugin that receives runtimeActivity in its context could not name its type, and one that reads ctx.reason had nothing to import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
isAgentWorking() returned false for `errored` before looking at anything else. But decide() treats `errored` with a preserved dequeue token as resume_from_error: the agent holds a live retry timer and keeps going. So the one state where a child is provably still working read as idle, and neither re-arm site fired again. Nothing recovers from that — a child going idle to busy on its own triggers none of the four scheduling call sites, and the parent has nothing to infer about because it is waiting on that child. A parent told to watch for a stuck child stopped watching exactly when the child needed it. The status now defers to the queue, the way decide() does; `paused` is the only state that really waits on something external. scheduleErrorRetry was the sibling timer supervision was fixed away from: it took its lease around the wait rather than inside the callback, so a session against a failing provider held a lease continuously and could never be evicted — and every acquire bumped lastActivityAt, so the idle clock never advanced either. It arms unleased now and acquires in the callback, with scheduleProcessing() taking over before the retry lease drops. It also refuses to re-arm after abort(), which scheduleProcessing already did: on a forced unload the store is not closed, so a draining turn could arm a 60s timer against a controller about to be disposed. Two smaller ones. The re-arm gate read a state snapshot captured before the tick's own await, so a child that started working while the snapshot was being sent was invisible — it reads live state now. And a paused parent was still sent snapshots it cannot consume, one per interval, piling up until it resumes; it keeps its timer but not the sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
…un on Agent.waitForIdle() was added to close a stated race — disposal clearing agents and plugin contexts while a turn is still inside a tool call — and then nothing ever called it. shutdown() only aborts; the turn keeps running against the maps cleared on the next lines. performDisposal awaits the drain now, bounded, because a wedged turn must not hold disposal open any more than a wedged upload or worker does. The rebuild tests are characterisation, not a fix. Moving the error-retry lease inside its timer callback made a session evictable mid-backoff, which raised the question of whether anything re-arms an agent once the runtime is rebuilt. It does: mail appended straight to the log with no runtime loaded is consumed after the reopen, with no in-process scheduling that could account for it. I had written the wake-every-agent-on-ready loop before checking, and the negative control against the previous commit refuted it — both tests pass without it. So the loop is gone and the tests stay, to pin behaviour nobody had asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
…faults
uploadIdSchema was `z.string().brand('UploadId')` — a brand promising
something it never checked. Since every uploads method interpolates the id
straight into a path, that was not merely lax: `uploads.markUsed` with an
id of `../../<other-session>/uploads/<id>` normalises to a path still
inside the data root, so lexical containment passes, and markUsed — unlike
delete and loadAttachments — never compares meta.sessionId. One session
could write into another session's upload metadata, which silently drops
that upload from the victim's listPending and makes the victim's own
delete fail. It now carries a pattern, with parseUploadId alongside it in
the same shape sessionIdSchema uses, enforced through the method input
schemas and on the metadata read in onSessionReady — that meta.json sits
in a directory the agent can write, and the id flows back to the agent as
a path.
The write path never checked the filename either. `nested/inner.txt` was
accepted and the store's mkdir built the subtree, so an upload could put
files anywhere below the upload dir. The check lives in
validateUploadInput, so it covers both entry points and any non-HTTP
caller rather than only the route.
Also, from the review of the retention fix itself:
- A store fault came back as a 400 carrying the raw error text, which for
an embedder-supplied object store means bucket names, endpoints and
request ids reaching the client — and a 4xx tells every well-behaved
caller not to retry a fault that is entirely server-side. Both arms now
log the cause and return a generic message at 500.
- A failed terminal append was left to "the next load", which for the
default config never comes: eviction is off unless an embedder opts in.
The attachment then never reached the agent while the caller was told
201. The append retries, and the comment no longer promises a recovery
the configuration does not provide.
- The close-drain warning named neither of the two things it claimed:
tracked mutations were not counted at all, so it typically fired naming
nothing. It logs both sets now.
- Post-expiry, state that in-flight work still reads was cleared
unconditionally, letting a settling upload write a terminal meta.json
over one being deleted. Only the clean path clears it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
Tightening uploadIdSchema turned uploads.delete's input into a branded UploadId, which is the same shape agentIdSchema already has and which client-react already handles for AgentId and ChatMessageId two lines above — except UploadId was reachable from neither @roj-ai/shared nor the sdk's own entry point, so the client had nothing to call and ts:build broke on the plain string. shared/lib/ids.ts gets the mirror, alongside its three siblings and with the same caveat: structurally compatible with the sdk's declaration, nothing asserting the two agree. The sdk also exports UploadId with parseUploadId and isValidUploadId next to it, so an out-of-tree consumer gets the checked constructor and not only the unchecked one. Caught by ts:build, which is the only gate that spans packages — the unit gates are per-package and all of them were green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
Two assertions read the consumption events straight after waitForIdle(). Consumption commits after inference_completed on purpose — a crash in that gap re-delivers the message rather than losing it — so the agent reads idle a moment before the events land. On an idle machine waitForIdle's double-check covers the gap; under contention it does not. Measured on the file: 2 of 8 runs failed with the box saturated, 0 of 10 on an idle one. After the change, 0 of 10 under the same saturation. This matters beyond the file: the whole-suite run is the gate every unit on this branch was verified against, and a gate that fails 25% of the time under load cannot certify anything. The production ordering is not touched — the tests wait for it instead of assuming it has already happened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125RabEobHrqouaQHHxtF46
The manager rebuilds an evicted session from the log, so anything that outlives disposal — an abandoned worker, a timer, a settled effect — could append behind the replacement runtime. The write landed durably while only the dead projection applied it, and the live runtime never caught up. `SessionStore.detach()` now runs at the end of disposal, after the close hooks and the agent drain that may still emit legitimately, and every later append fails with `SessionRuntimeDetachedError` instead of writing. Reopen goes with it: the manager already refuses to re-register a disposed runtime, so reopening through one produced a handle whose runtime was dead. It now returns a domain error telling the caller to load the session again.
`update` read the committed state, validated a full merge against the size cap and the configured refinements, then appended only the patch. Two calls could interleave across that append: both validated against the same base, both passed, and the merged result landed above the 256 KiB limit or broke a cross-field rule that each patch satisfied on its own. The response also reported a state that was never committed. Updates now run one at a time per session and re-read the committed state inside the critical section, so every merge is validated against what the previous one actually wrote.
Both spawn paths derived the agent id from `agentCounters` in the projection, which only advances once the `agent_spawned` event is appended. Two spawns of one definition that overlapped across that append minted the same id: the second overwrote the first in the agent map while both callers were told they had spawned an agent. They now reserve through `SessionContext.reserveSequence` — the same synchronous seam the mailbox uses — so the id is taken before anything awaits.
A ready service no longer pins its session resident, and the proxy took no lease of its own, so idle eviction could stop the dev server under live preview traffic. A streamed response — HMR, SSE — is the worst case: it outlives the idle window without ever producing a request that would refresh the clock. The lookup now leases the runtime of the session that owns the service and holds it until the body ends, fails, or the consumer walks away. A session whose runtime is already unloading is skipped rather than served. The proxy takes the two members it uses instead of the whole `SessionManager`, which is also what makes it testable without a live runtime.
A lifecycle transition holds the lease idle eviction waits on, so an embedder callback or a PID-registry write that never settles pinned the session resident forever — the exact bound the lease exists to enforce. The `availableWhen`, `cwd`, `command` and `env` resolvers and both registry operations now run under a 30 s deadline. The operation itself cannot be cancelled, but the transition stops waiting on it and reports a recoverable failure, which releases the lease. A registry write that misses the deadline also kills the child it could not record: nothing would have been able to reap it after a crash.
`callPluginMethod` returns `Result<unknown, …>`, and the concurrent-spawn test reached into it through a cast, which also erased the `AgentId` brand and broke the SDK test type-check. Parsing the output through `agentIdSchema` keeps the branded ids the state comparison expects.
The package compiled everything under `src`, so a `*.test.ts` there landed in `dist` and shipped with the published package. Excluding tests from the build mirrors the SDK, and the new `tsconfig.test.json` — also mirroring the SDK — keeps them type-checked. CI now runs both test projects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
Session runtimes stayed cached indefinitely. Several plugin projections also retained consumed or terminal payloads, including large extracted upload content and worker results. Background operations did not share one lifecycle contract, so safe eviction could race writes or stop live work.
Behavior and compatibility
loadConfig()leaves the timeout unset, so programmatic SDK usage — includingpackages/sandbox-runtime— keeps eviction disabled unless the embedder opts in. The default belongs tostandalone-server.SESSION_IDLE_TIMEOUT_MSoverrides it;SESSION_IDLE_TIMEOUT_MS=0disables eviction explicitly.onSessionReady/onSessionClosenow fire once per resident runtime lifetime, not once per session — an evicted runtime closes and reopens. Documented inskills/roj/references/plugins.md; a plugin doing durable teardown inonSessionCloseneeds to move it to thesession_closedevent.ctx.runtimeActivitylease.sessions.createnow rejects a caller-suppliedsessionIdoutside^[A-Za-z0-9_-]{1,64}$. Generated ids are UUIDv7 and both platform consumers supplycrypto.randomUUID().Follow-up review fixes on this branch
A multi-agent review of the branch produced 56 findings. The first wave is included here:
LEASE-2/LEASE-3— the agent debounce timer leaked its lease on any throw fromdebounceCallback, andcontinue()returned above the release on the reachable closed-session path. Both were the unbounded retention this PR exists to fix, reintroduced.LEASE-4—uploadAsyncleaked its lease and hungonSessionCloseifctx.notifythrew.PERF-1— the standalone preview proxy walked every persisted session and calledgetSession()on each, per proxied asset request: it reset every runtime's idle clock and cold-loaded evicted sessions, re-running orphan-PID reconciliation, service spawn and worker relaunch. It now sweeps resident runtimes only.CORRECT-4—performCloseaborted on the first service that could not be signalled, skipping the port-release loop;PortPoolis process-global, so those ports were lost for the life of the server.PRE-1(pre-existing) —sessionIdwas interpolated into filesystem paths with no format check.ARCH-4— removedattachment_terminal_materializedand theunmaterializedprojection before they ship. In a published SDK an event type is forever, and this one carried informationterminalEventPersistedalready had.Remaining findings are tracked separately; the largest open one is that a session with an auto-start service, armed supervision, or a paused worker holds a runtime lease indefinitely, so the bundled configuration does not actually evict those.
Downstream forward compatibility
/statusreports metadata-derived metrics for an evicted session, and metadata carries nollmErrors,toolErrors,cacheReadTokens,cacheWriteTokensorcompactions. Both consumers upsert those into D1 and would have overwritten real totals with zeros. Consumer-side fixes land first:@roj-ai/sdkis pinned with a caret there, so a lockfile refresh picks the change up).0.1.28, so it moves on a deliberate bump.The SDK-side
metricsSourcechange is not in this PR.Verification
cpu-lease run -n 2 -- bun run ts:buildcpu-lease run -n 2 -- bun run lintcpu-lease run -n 2 -- bunx tsc -p packages/sdk/tsconfig.test.json --noEmitcpu-lease run -n 2 -- bun run test— 1206 passed, 40 skipped, 0 failedVerification gap
The branch has deterministic lifecycle and retention tests, but no long-running production-like heap snapshot or forced-GC soak measurement. Memory bounds are verified through ownership, projection, replay, and disposal behavior rather than quantified RSS or heap deltas. Measuring is only meaningful once the lease-pinning findings above are closed — until then a soak would mostly measure pinned runtimes.