Repository navigation
feat: run tasks on a workload's context in shared sandboxes - #494
Conversation
A synchronous /runs request can name a workloadId. Moca checks that the session token's user owns the ready workload, downloads its frozen revision from Context Service once, and copies it into the shared sandbox through SandboxExec as raw bytes. The sandbox verifies the sha256 and publishes it read-only; it is removed after the run by default, and each user's copy is cached separately. Shared, running sandboxes cannot mount the workload's PVC, so phase 1 copies; phase 2 (rossoctl#476) mounts it instead. Part of the work split out of rossoctl#455. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
The authorization model looks right. Workload runs are sync-only and require a session token whose sub owns a ready workload. The record is keyed by sha256(owner), so another user's workload is a 404, a caller-supplied internalContext is stripped at the boundary, and async is 501. The overlay scripts validate (regex) and single-quote every interpolated value, verify the digest before tar, publish via a same-directory rename under flock, and clean up in finally with retention defaulting to 0. The context lives at /workspace/.moca-context/runs/<sid>, outside the leaf worktree, and only prompt leaves take it, so there's no converge or solve-patch interaction.
No blockers. Inline suggestions: the in-sandbox sha256 check doesn't tie the bytes to the pinned revision, gzip-bomb / uncompressed size is unbounded on a shared sandbox, peak memory grows with concurrent large downloads, one comment overclaims per-owner isolation, and the async error code in the docs doesn't match the code path.
Branch is 35 commits behind main but git merge-tree against current main is clean, and nothing the PR relies on (routes.ts, assertContextServiceSettings, the 501/500 workload codes) changed there. CI green, DCO signed. The overlay tests are string-level on the generated scripts (they never run tar/flock for real), which is fine given k8s-kind-e2e passed, but a real-shell test of populate→acquire→cleanup would be worth adding later.
| } | ||
| chunks.push(Buffer.from(value)); | ||
| } | ||
| const bundle = Buffer.concat(chunks, received); |
There was a problem hiding this comment.
suggestion — nothing ties the downloaded bytes to the pinned revision. The overlay computes sha256(bundle) from whatever fetchBundle returned, and the sandbox checks the archive against that same digest. That proves the relay copy is intact, not that Context Service served revision {sha256}. If the revision is the bundle digest, check sha256(bundle) === revision here before caching. If it's a tree/content digest, say in context-service.md that the in-sandbox check is transport integrity only. The PR text currently reads like an end-to-end pin.
| `trap 'chmod -R u+w "$STAGE" 2>/dev/null || true; rm -rf "$STAGE" "$ARCHIVE"' EXIT`, | ||
| `printf '%s %s\\n' ${sq(sha256)} "$ARCHIVE" | sha256sum -c --status`, | ||
| 'chmod -R u+w "$STAGE" 2>/dev/null || true; rm -rf "$STAGE"; mkdir -p "$STAGE"', | ||
| 'tar -xzf "$ARCHIVE" --no-same-owner --no-same-permissions -C "$STAGE"', |
There was a problem hiding this comment.
suggestion — MOCA_CONTEXT_BUNDLE_MAX_BYTES (256 MiB) bounds only the compressed size. assertCompleteGzip inflates the whole stream with no count, and tar -xzf extracts with no size or inode limit into /workspace on a sandbox shared by several users, so one user's gzip bomb can fill the disk for everyone's runs. Count the inflated bytes in the assertCompleteGzip Writable and abort past a limit (the cheap fix, since it already inflates everything), and/or cap entries before extracting.
| throw tooLarge(); | ||
| } | ||
| if (!response.body) throw new ContextServiceError('Context Service returned no bundle body'); | ||
| const chunks: Buffer[] = []; |
There was a problem hiding this comment.
suggestion — every distinct download buffers up to 256 MiB, and Buffer.concat briefly doubles that. Bundles over the cache limit are never cached, so each run of a large workload downloads again, all in the same process. N concurrent large-workload runs need roughly N × 512 MiB at peak. Consider a global in-flight byte budget or streaming to a temp file.
| * Delivers a workload's frozen Context Service revision into a shared P6 sandbox, read-only, for one | ||
| * run. The sandbox keeps one extracted copy per owner and revision under `cache/`; each run | ||
| * registers a ref on it and gets its own view at `runs/<session>`. The bundle crosses the relay only | ||
| * on a miss. Keying the cache by owner means two users never share a copy, so one user's agent |
There was a problem hiding this comment.
suggestion — "one user's agent cannot alter what another user's run reads" overclaims. All agents run as the same UID, cache/ is writable, and chmod -R u+w undoes 0555/0444. ownerScope is sha256(owner).slice(0,32), which anyone who knows the owner string can compute. The acquire hit path trusts any existing cache/<scope>-<rev> dir without re-verifying it, so an agent could plant a poisoned copy for a known revision. context-service.md already says shared sandboxes are not a boundary, so at minimum align this comment. If you want the property, use an HMAC'd scope or re-verify a manifest on hit.
| a session token (scope `turn:write`). The token's user must own the workload: another user's | ||
| workload, like a missing one, is `404 workload_not_found`. A workload that is not `ready` answers | ||
| `409 workload_not_ready`, and another kind of run `400 workload_run_requires_prompt`. A run with no | ||
| token is `401 token_required`. Asynchronous runs on a workload, and every workload run while the |
There was a problem hiding this comment.
suggestion — a workload run needs a token, and a tokened async request is refused earlier by the async gate in handler with 501 async_runs_unavailable. workload_runs_unavailable is only reachable for an anonymous async request, which should arguably be 401 token_required. Either check workloadId before the async gate, or fix this sentence and the matching harness-openapi.yaml entries (~412/467).
|
|
||
| it('keeps asynchronous workload runs unavailable', async () => { | ||
| await seed(alice, 'demo', 'ready', REVISION); | ||
| expect((await run(alice, prompt('demo', { async: true }))).status).toBe(501); |
There was a problem hiding this comment.
nit — this asserts only the status, which is why the code mismatch above slipped through. Assert the error body as well.
| assertContextServiceSettings(); | ||
| } catch (err) { | ||
| console.error('workloads are misconfigured:', err); | ||
| res.writeHead(500, JSON_HEADERS).end(JSON.stringify({ error: 'workloads_misconfigured' })); |
There was a problem hiding this comment.
nit — /runs can now return 500 workloads_misconfigured, but the createRun 500 response in harness-openapi.yaml still lists only internal_error. Add it next to the new 400/404/409 entries.
| internalContext: { | ||
| revision, | ||
| ownerScope, | ||
| loadBundle: () => downloadContextBundle(contextName, revision, owner), |
There was a problem hiding this comment.
nit — DELETE /workloads/{id} doesn't evict the deleted workload's bundle from the in-memory bundleCache, so it stays until LRU eviction. Only the owner can reach it, so this is retention, not a leak. Consider evicting the owner\0contextName\0* entries on delete.
Follow-up to #494 from review: - Refuse bundles that inflate past 1.25 GiB, and more than 10,000 entries in the sandbox before extracting. - Share a 512 MiB in-flight download budget across concurrent downloads. - Name sandbox cache folders with an HMAC of the owner under a per-process secret. - Answer 501 workload_runs_unavailable for any async run naming a workload. - Document that the sandbox sha256 check is transport integrity only; list 500 workloads_misconfigured. - Assert error bodies in tests; evict a deleted workload's cached download. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
Part of the work split out of #455 (the original context-delegation PR). Follows #482 (
/workloadsphase 1).What this adds. A
/runsrequest can now name aworkloadId, and the agent sees that workload's files. Moca checks that the session token's user owns the workload and that it has been activated. It then gives the task a read-only copy of the workload's frozen revision: the exact upload pinned at activation, identified by its sha256.docs/context-service.mdhas the updated diagram.How the context gets there, and why it's a copy. Phase 1 runs tasks on the operator's shared, already-running sandboxes. They can't mount the workload's PVC: Kubernetes can't add a volume to a running pod, and mounting one user's PVC into a pod other users' tasks share would expose it. So Moca downloads the frozen revision from Context Service once, keeps it in a bounded cache, and copies it into the sandbox through SandboxExec (the relay's command API) as raw bytes. The sandbox checks it against its sha256 and publishes it read-only. After the run it's removed by default, and each user's copy is cached separately, so users never share one. Bundles are limited to 256 MiB.
Why only synchronous. Workload runs require a logged-in user, and Moca doesn't offer asynchronous runs to logged-in users yet: running a task later would mean storing the user's token in the queue. Async workload runs answer
501until owned async runs exist.Multiple users. Several users can create workloads and run tasks at once: names, ownership, Contexts and sandbox caches are per user. The sandboxes are shared, though, and are not a security boundary between users: tasks from different users can run on the same sandbox, as the same Linux user, one after another.
Limits. Read-only is enforced by file permissions only, since the agent runs as the same user as the files. Only sandboxes reached through the relay are supported.
Next steps
mocactlsubagent commands in Coding-agent subagents A2: mocactl subagent commands (spawn, subagents, get, watch, approve, follow-up) over today's API #381 with what Maia's demo CLI has proven. Thenmocactlsupport for workloads.moca-delegate-taskskill moves to that CLI, with a demo that delegates and fans out many tasks from one local agent.🤖 Generated with Claude Code