[feat] Detect a stuck-substitution sandbox before the first model call and rebuild it - #6370
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
size:L
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
66bf2da to
c5f0f39
Compare
c5f0f39 to
ca7d734
Compare
mmabrouk
changed the title
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Review summary
- Risky:
services/runner/src/engines/sandbox_agent/credential-preflight.ts:129- a healthy sandbox can be classified as stuck when Daytona response scrubbing rewrites an echoed real credential back to adtn_secret_placeholder. - Missing tests:
services/runner/tests/unit/credential-preflight.test.tsneeds a healthy, scrubbed-response regression case.
Reviewed exact head ca7d73402fd2e616cbaada5a9b0c26752c5965af. I recommend fixing this false-positive path before merging.
| return "ok"; | ||
| } | ||
| const elapsedMs = now() - startedAt; | ||
| if (!body || !body.includes("dtn_")) { |
There was a problem hiding this comment.
High severity: This condition cannot distinguish a raw placeholder request from a healthy request whose real credential was echoed by the endpoint and then scrubbed by Daytona. The reproduction notes already establish that Daytona rewrites real credential values in responses back to dtn_secret_.... For a custom OpenAI-compatible endpoint that echoes the Authorization header in its {} error response, every healthy probe therefore contains dtn_, both acquisition attempts are destroyed, and the first turn fails even though the real model call would work.
Please only convict on the proven provider-specific masked raw-placeholder signature, such as LiteLLM's specific Received=dtn_**** shape, and fail open for other dtn_ bodies. Add a regression test where a healthy echoed credential has been scrubbed into a full placeholder.
There was a problem hiding this comment.
You are right, and the module doc was arguing against itself — it cited the scrubbing behaviour and then convicted on a bare dtn_. Fixed in a8bfc83.
The preflight now convicts only on a MASKED placeholder echo, which is what scrubbing cannot forge: masking removes the real value, so there is nothing left for the scrubber to match, and a dtn_ beside a mask can only mean the raw placeholder really went out. The regex covers both proven shapes (LiteLLM Received=dtn_****, OpenAI dtn_secr*****) plus LiteLLM naming a dtn_ key as what it received, mirroring PLACEHOLDER_CREDENTIAL in errors.ts. * is the only mask character trusted — a ... or … truncation could equally be a cut-off scrubbed value, so it fails open.
An unmasked dtn_ body now returns ok on the first probe and logs why it convicts nothing, so a stuck sandbox behind an echoing endpoint surfaces as the 401 rather than as two destroyed acquire attempts.
Regression test added for exactly your case (fails open when a HEALTHY echoed key was scrubbed into a full placeholder), plus one asserting both masked shapes still convict. Detection is unchanged in practice: every incident in production and in the 20-sandbox probe carried a masked echo.
ca7d734 to
74bc668
Compare
74bc668 to
89e1508
Compare
…rst call The guard for the placeholder-401 race (#6362): Daytona applies a new Secret's substitution rule asynchronously with no confirmation signal, and ~3% of fresh sandboxes' first model calls on EU cloud carried the raw dtn_ placeholder and failed the user's turn. On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner now probes the credential's own /chat/completions from INSIDE the sandbox: a response echoing dtn_ means the placeholder went through raw, so wait 2s and probe again (budget 25s); anything else means the header was substituted and the run proceeds. The probe starts right after sandbox create and is awaited at the END of acquire, so it runs concurrently with the ~10s of mounts/workspace/session work and the common case pays nothing. Fail open everywhere: an exec error, an empty body, or a spent budget logs and lets the run proceed — the worst outcome is the pre-existing behavior, now at least classified as credential_delivery_failed. The raw key value never appears runner-side: the shell expands the env var inside the sandbox. Every observed incident was a first call of a fresh sandbox (7 of 7), so once one probe substitutes, the sandbox is good for its lifetime. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
The reproduction proved the fault is binary per sandbox and permanent: a healthy sandbox substitutes on its FIRST request (~2s), a stuck one never does, and a fresh sandbox on the same Secret works immediately. So the preflight's original wait-25s-then-proceed design was wrong on both ends — the wait can never help, and proceeding sends the turn into a guaranteed 401. The preflight now returns a verdict: 'ok' on the first non-placeholder echo (or any ambiguity — ambiguity still fails open), 'stuck' after 4 consecutive raw-placeholder echoes. A stuck verdict throws through the acquire's shared teardown (sandbox destroyed, Secrets deleted) and the acquire wrapper retries ONCE with a brand-new sandbox. At the measured 25% stuck rate that turns a failed first turn into a slower one, with ~6% residual for the honest credential_delivery_failed error. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
…ight Their support (2026-08-31) confirmed the per-sandbox wiring failure and stated the operating rule: a sandbox whose retry starts working within ~30s may be kept; one still sending the placeholder after that must be recreated. So the conviction moves from a fixed 4-probe count (~10s) to their 30s bound, probing every 2.5s. Our own 20 samples saw nothing land between 3s and 180s, but their number is authoritative for their system, and the grace runs concurrently with acquire setup, so a healthy sandbox still pays nothing. Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
Product-owner call (2026-08-31): every healthy sandbox in the 20-sample measurement answered on its FIRST probe, Daytona's wiring fix is in progress, and a 30s grace only holds a stuck user's turn for a recovery nobody has observed. The module doc records the decision and the signal that would reopen it (a 'substitution confirmed after N probes' log with N > 1 at a late offset). Claude-Session: https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt
80ae98f to
1413a5f
Compare
Daytona's egress proxy scrubs responses, rewriting a real credential back into `dtn_secret_<id>`. A bare `dtn_` in the probe body was therefore not evidence: an endpoint that echoes the Authorization header returns that shape on a perfectly healthy sandbox, so both acquire attempts were destroyed and the first turn failed even though the real model call would have worked. Convict only on a masked echo, which scrubbing cannot forge because the masked string no longer holds the real value to match. That covers both proven shapes (LiteLLM's `Received=dtn_****`, OpenAI's `dtn_secr*****`) and every incident observed so far, so the narrower signature costs no detection. An unmasked echo now logs and fails open. Review comment from @mmabrouk on #6370. Claude-Session: https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g
89e1508 to
a8bfc83
Compare
Daytona's egress proxy scrubs responses, rewriting a real credential back into `dtn_secret_<id>`. A bare `dtn_` in the probe body was therefore not evidence: an endpoint that echoes the Authorization header returns that shape on a perfectly healthy sandbox, so both acquire attempts were destroyed and the first turn failed even though the real model call would have worked. Convict only on a masked echo, which scrubbing cannot forge because the masked string no longer holds the real value to match. That covers both proven shapes (LiteLLM's `Received=dtn_****`, OpenAI's `dtn_secr*****`) and every incident observed so far, so the narrower signature costs no detection. An unmasked echo now logs and fails open. Review comment from @mmabrouk on #6370. Claude-Session: https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g
a8bfc83 to
4187659
Compare
Vault-backed fixture connections (the #6371 review catch), and the stuck-substitution rebuild that can fake an eviction in a sandbox-counting warm case after #6370. Claude-Session: https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g
1 participant
Context
The placeholder-401 fault (#6362), now fully reproduced (2026-08-30, 20 fresh-Secret samples, production create shape, target eu): Daytona's Secret substitution is binary per sandbox. A healthy sandbox substitutes on its very first request, 1.5 to 2.9 seconds after Secret creation. A stuck sandbox never substitutes: the raw
dtn_secret_placeholder reaches the provider for as long as anyone watches, a twin sandbox created against the same Secret works immediately, and stop+start does not repair it. Measured 5 stuck of 20 that day; the earlier production window showed ~3%, so the rate varies. The delete-then-create eviction ordering is not a factor.Two instrument facts worth keeping: Daytona also scrubs responses (real values are rewritten back to placeholders before entering the sandbox), so an echo service cannot observe substitution; a provider whose error body echoes a masked key can. And a raw-placeholder echo stays visible, which is what this preflight reads.
Changes
On a freshly created Daytona sandbox whose model key rides a Secret and whose connection declares an endpoint base URL, the runner probes the credential's own
/chat/completionsfrom inside the sandbox, concurrently with the rest of acquire.Before (the first version of this PR): wait up to 25 seconds for substitution, then proceed regardless. Both halves were wrong for a binary fault: the wait can never help, and proceeding sends the turn into a guaranteed 401.
After: the preflight returns a verdict. The first non-placeholder echo (or anything ambiguous, which still fails open) is
ok. A raw placeholder still echoing at the 10 second grace convicts the sandbox asstuck(deliberately below Daytona's ~30s keep-or-recreate bound: every healthy sandbox we measured answered on its first probe, their wiring fix is in progress, and a longer grace only holds a stuck user's turn); the acquire's shared teardown destroys it and deletes its Secrets, and the acquire wrapper retries once with a brand-new sandbox, which their support confirms is the correct recovery (restart never helps; a new sandbox on the same Secret does). At a 25% stuck rate, one retry turns a failed first turn into a slower one, leaving ~6% for the honestcredential_delivery_failederror. Their side has a priority fix PR in progress; this guard stays as the belt afterwards.Tests
[credential-preflight] STUCK ... rebuilding freshwhere placeholder 401s used to be, and the LiteLLM proxy log'sReceived=dtn_count trends to the ~6% residual of the varying stuck rate.The reproduction record lives in
docs/design/daytona-secret-propagation/README.md(stacked below, #6369).Stacked on #6369.
https://claude.ai/code/session_014s6jqKCsVKsNMmnrJVJkZt