Repository navigation
fix: harden workload context delivery - #516
Conversation
Follow-up to rossoctl#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>
ingpaolodettori-dev
left a comment
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Thanks, this closes the #494 follow-ups cleanly. The budget reserve/release paths, the inflate cap, and the async-refusal reordering look right and are well tested. Three suggestions, none blocking: (1) the default cache secret is per forked worker, so a W-worker supervisor splits each user's sandbox cache W ways; (2) the download budget is released before the buffer is delivered to the sandbox, so it doesn't bound resident bundles; (3) please confirm the 10k entry cap counts the same thing as Context Service's limit (tar -t counts directories too). Plus one docs nit about the require-auth 401 ordering.
Author: moonlight16 (MEMBER — maintainer) · Agent/IDE config: none · Commits: 1, signed-off · CI: passing
| * process: replicas with different secrets only miss each other's copies. | ||
| */ | ||
| export function sandboxCacheScope(owner: string): string { | ||
| cacheSecret ??= process.env.MOCA_CONTEXT_CACHE_SECRET?.trim() || randomBytes(32); |
There was a problem hiding this comment.
suggestion: The random-per-process default is per worker, not per replica. The supervisor forks SH_WORKERS (default availableParallelism()) processes, each with its own randomBytes(32), and routes leastInFlight. So by default one user's revision gets a different cache name on each worker: runs landing on different workers re-transfer and re-extract the bundle onto the shared sandbox disk, up to W copies when RETAIN_MINUTES>0. Every worker restart also orphans the cached copies. Suggest a stable default that agents still never see, e.g. derive it from CONTEXT_SERVICE_TOKEN (HMAC(token, 'moca-context-cache-scope'); it is already required whenever workloads run), or have the supervisor generate one and pass it to its forks. At minimum, fix the comment at L86 and the docs table ("random per process" → per worker process, set it for W>1).
| await assertCompleteGzip(bundle, maxExpanded); | ||
| return bundle; | ||
| } finally { | ||
| release(); |
There was a problem hiding this comment.
suggestion: The reservation is released when the download finishes, but loadBundle's Buffer then lives on through the whole relay delivery (8 MiB SandboxExec chunks plus the sandbox sha256 check), which is usually the slow part. So the budget bounds download buffering, not what's resident: N concurrent runs of distinct large bundles (different users or revisions; same-revision runs already share a download) still hold N buffers during delivery. Either hold the reservation until overlayContext finishes with the bytes (e.g. return the release with the buffer and release after the populate step), or narrow the comment at L257 and the docs ("Downloads in flight share a byte budget") so they don't promise a memory bound.
| `STAGE=${sq(`${BASE}/cache/.stage.${rev}.${sid}`)}`, | ||
| `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`, | ||
| `ENTRIES="$(tar -tzf "$ARCHIVE" | wc -l)"; [ "$ENTRIES" -le ${maxEntries} ] || { echo "context bundle has $ENTRIES entries, over ${maxEntries}" >&2; exit 1; }`, |
There was a problem hiding this comment.
suggestion: tar -tzf | wc -l counts every member, directories (and a ./ root, if the export has one) included. If Context Service's 10,000 limit counts files, a bundle it accepted with 10,000 files in a few dirs would be refused here, which breaks the "a bundle Context Service accepted is never refused" principle the expanded cap follows. Please confirm what Context Service's limit counts and what its export contains. If the limit counts files, count regular files only (tar -tvzf … | grep -c '^-') or give the default headroom as the expanded cap does. Also, the PR body says "more than 10,000 files", but the code counts entries.
| routes are off, answer `501 workload_runs_unavailable`. The sandbox tier is the session's, chosen as | ||
| for any other run. | ||
| `409 workload_not_ready`, and another kind of run `400 workload_run_requires_prompt`. A synchronous | ||
| run with no token is `401 token_required`. Asynchronous runs on a workload, with or without a token, |
There was a problem hiding this comment.
nit: "with or without a token" holds only when SH_REQUIRE_AUTH is off. On a require-auth deployment, resolveTurnAuth answers a token-less async request with 401 token_required before the new check runs. The same applies to openapi L469 "whether or not it carries a token" and leaf-and-envelope.md. Suggest qualifying it as "(a token-less caller on a require-auth deployment still gets 401 first)".
Move the context workflows list to docs/concepts/ with a Preview line and index entries, use the MOCA spelling, state phase 1 limits and that shared sandboxes are not a security boundary, link Context Service's capture/restore for workflow 9, and say "workflow N" for cross-references. Also correct that under SH_REQUIRE_AUTH an unauthenticated workload run is 401 token_required first (missed #516). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
4 participants
Follow-up to #494 (run tasks on a workload's context in shared sandboxes), from Paolo's review.
MOCA_CONTEXT_DOWNLOAD_BUDGET_BYTES).501 workload_runs_unavailable.500 workloads_misconfigured. The docs and comments say the sandbox's sha256 check proves transport integrity only, and that per-owner cache folders aren't isolation, since every agent runs as the same Linux user.Isolation between users remains #476 (phase 2: per-workload sandboxes and storage).
🤖 Generated with Claude Code