fix(sandbox): reclaim, fair reconcile, pins, scope, safe rollback - #3175
Draft
larryro wants to merge 5 commits into
Draft
fix(sandbox): reclaim, fair reconcile, pins, scope, safe rollback#3175larryro wants to merge 5 commits into
larryro wants to merge 5 commits into
Conversation
dockerRm never rejects (runDocker resolves with exit 124 on a host-side timeout), so removeContainer's .catch never fired and stopSession returned "existed" while the container kept running; sweepExpired then dropped the registry entry and the live container was orphaned until a restart re-adopted it. removeContainer now judges the rm result and throws on any outcome other than a clean exit or "no such container", which is the stop/destroy throw-on-hiccup contract the reaper already retries on. The create-failure cleanups and the legacy one-shot sweeps log a failed rm and count only verified removals.
handleCreate rolled back a create whose endpoint could not be resolved with destroySession — the data-deleting verb — even when the create had RESUMED a stopped session onto its preserved workspace. On Kubernetes resolveEndpoint reads the Pod IP and can blip, so a transient API error on a resume deleted the user's PVC. createSession now reports whether the workspace pre-existed and the route rolls a resume back with stopSession (compute released, data kept for the retry); only a fresh create destroys the half-made workspace it provisioned itself, mirroring both backends' own failed-create cleanup.
The registry's pinned flag was in-memory only, and adoptExisting rebuilt every entry unpinned with expiresAtMs = createdAt + maxLifetime — so a deploy or crash forgot every pin and the first sweep TTL/idle-stopped the user's always-on session; nothing platform-side re-pushes the pin until the user toggles it again. The pin is now recorded on the backend object's durable state — a marker under the host session root (outside the workspace) on Docker, a tale.dev/pinned Pod annotation on Kubernetes — listSessions reports it back, adoptExisting carries it into the registry, and GET /v1/sessions/:id exposes it. A new create always starts unpinned (the platform row is the truth) and stop/destroy clear the record. The spawner Role gains patch on pods.
The sandbox watchdog's reconcile probed the 25 globally-oldest rows by created_at_ms every tick with no rotation: long-lived healthy sessions (a pinned always-on agent never expires) sat at the head forever, so a younger phantom active row behind them was never probed and held one of the org's few slots until its 24h TTL — indefinitely when pinned. And an automation run's terminal door only hibernated its per-execution sessions (stopped — a LIVE status the Sandboxes page lists and the spawner keeps a workspace for): nothing ever destroyed them, so every agent-node run left a dead row and a host workspace behind, forever. Migration 0070 adds sandbox_sessions.last_reconciled_at_ms; every spawner-facing pass now walks least-recently-visited first and stamps the rows it visited, so every row is reached within ceil(rows / batch) ticks. A RECLAIM pass destroys the sessions of ended (or purged) runs past a 10-minute grace through the spawner's if_idle door — a late node still executing is left for the next tick, a spawner error leaves the row alone — and settles the row only once the spawner confirmed the compute gone. The spawner verbs are injectable, so the unit layer and the real-Postgres probe drive both passes with a scripted spawner.
resolveKnowledgeToolAccess mapped every org-wide automation run to the hub-only scope (projectIds: []) and ignored the automation's bound projects, while resolveSessionActionContext confined the same run's task and document actions to exactly those projects. rag_search, rag_fetch and document_find on an unpinned run of a two-project automation therefore answered not_found for the very documents the run was deployed over; the bridge's document_find additionally listed only projectIds[0]. The scope now spans every bound project that still exists (its team, shared teams, archived label) plus the hub — one helper shared with the project-session binding, fail-closed when a bound project is gone — and the listing door takes the whole authorized project set.
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.
Six verified sandbox-session defects from the backend deep review (mediums), fixed on
fedc8cc15(post-#3163/#3168 — none were covered by those). Two spawner-side leaks, one dead-end, one data-loss path, one reaper fairness gap, one authz dead-end.Per-finding outcome
stoppedsession rows foreverwatchdogs.tsRECLAIM pass:workflow_runrowsstopped/expiredwhose run is terminal (or purged) past a 10-min grace go through the spawner'sif_idledestroy; a busy session or a spawner error leaves the row for the next tick; the row settles only after the spawner confirmed the compute gone. Integration probe: ended →destroyed, purged-run orphan →destroyed, live run's active and hibernated rows untouched, busy leftstopped.0070addssandbox_sessions.last_reconciled_at_ms(+ walk-order index); both spawner-facing passes order by least-recently-visited (NULLS FIRST) and stamp every visited row, so every row is reached withinceil(rows / batch)ticks. Integration probe: batch of 2 over 3 rows reaches all 3 by tick 2 (base never reached the third).shim.tsorg_run with bindings → scope over every bound project that exists (team, shared teams, archived label) + hub, via the same helper the project binding uses (fail-closed when a bound project is gone);document_findlists the whole authorized project set (listDocumentsForAgent.projectIds), notprojectIds[0]. Integration probe through the realPOST /api/tools/executedoor: org run lists both bound projects' docs, no unbound leak; pinned run lists only its own.DockerSessionBackend.stopSessionswallowsdocker rmfailure, orphaning a running containerdockerRmreturns the CLI result (it never rejected — a timeout is exit 124);removeContainerthrows on any outcome other than clean exit / "no such container", which is the stop/destroy throw-on-hiccup contract the reaper already retries on. Create-failure cleanups and the legacy sweeps log failed rms and count only verified removals.adoptExistingdropspinned, always-on sessions reaped after a spawner restartSessionBackend.setPinnedrecords the pin on durable backend state — Docker:<hostSessionRoot>/.pins/<id>.pinned(outside the workspace; cleared on create/stop/destroy), Kubernetes:tale.dev/pinnedPod annotation (merge-patch; Role gainspatchon pods) —listSessionsreports it,adoptExistingcarries it,GET /v1/sessions/:idexposes it.handleCreaterollback destroys a preserved workspace on a resumecreateSessionreturns{ resumed }; the route rolls a resume back withstopSession(workspace kept for the retry) and only a fresh create withdestroySession— mirroring both backends' own failed-create cleanup.Tests (all red on base, green on branch)
services/sandbox(bun): 251 pass (+11 new) — fake-docker(viaDOCKER_BIN) stop/destroy honour the rm result (4), durable pin marker lifecycle +listSessions(2), k8s pin annotation patch + read-back (1), create rollback stop-vs-destroy (2), pin survivesadoptExisting+ sweep (1), pin toggle recorded on the backend (folded into the existing pinned test).services/platform(vitest server):watchdogs.test.ts(5: fair order + stamp, stamp-on-error, reclaim outcomes, reclaim SQL guards, skipReconcile),shim.knowledge-scope.test.ts(5),workspace_tools_bridge.test.tsbounddocument_findupdated to the project-set shape. Full server+pii suite: 72,718 pass; the 3app/routes/*.test.tsxload failures under theserverproject are pre-existing on base (Denied ID … @fontsource … woff2?url).backend:integrationon a throwaway tale-db + MinIO: base 375/375, branch 377/377 — two new probes (fair reconcile + ended-run reclaim with a scripted spawner; org-rundocument_find+ knowledge scope over two bound projects).Verification
bun test,tsc --noEmit,oxlint --type-aware,oxfmt --check— clean.tsc --noEmit, workspace-wideoxlint --type-aware,oxfmt --check— clean; commitlint on all commits.patchverb lands inservices/sandbox/docs/kubernetes.md(the deployed Helm Role lives outside this repo — needs the same verb, otherwise the pin falls back to in-memory with a warning).Notes
0070is the next contiguous number onmain; a sibling draft (fix(platform): SSO team-sync provenance, 2FA grace anchor, SCIM guards #3173) also claims0070— whichever merges second renumbers.expirednon-automation session (TTL flips the row, the spawner only stops); a phantom heal re-provisions a pinned session's deterministic id as an unpinned new row (pinnedis not carried into the healed incarnation);handleSetPinned's durable record is best-effort on backends whose Role lackspatch.