Repository navigation
feat(sandbox): dedicated short timeout for background status polls - #2765
Merged
Merged
Conversation
Contributor
|
🌿 Preview your docs: https://nvidia-preview-fix-dedup-command-submit-on-retry.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
Background-command status polls are sub-second GETs, but they inherit the general connection request timeout, which is tuned for long submits and creates (often minutes). Against a sandbox whose backend has become unreachable, every poll then hangs for that full budget per retry, so a dead sandbox occupies a rollout slot for retries x minutes before the typed unreachable error fires. Add operations.status_poll_timeout_s (default 10s: a healthy poll answers in milliseconds and even the slow legitimate path, endpoint re-resolution plus a connect retry, is a few seconds) as the per-request budget for status polls only; None falls back to the shared budget, restoring the old behavior. The submit call and the final logs fetch keep their existing timeouts (log payloads can be large). A per-call timeout surfaces as TimeoutError, which the shared retry classifier deliberately treats as terminal so a timed-out submit is never replayed into a double-run. With the short poll budget that would let a single slow poll kill a running command, so _await_sdk_operation now accepts an is_retryable predicate (default unchanged for every other caller) and the status poll passes one that treats TimeoutError as retryable: re-polling a status is an idempotent GET, and the retry budget still bounds the total time against a truly dead backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemild@nvidia.com>
Since Python 3.11 asyncio.TimeoutError IS builtin TimeoutError, so the wait_for-based hard-cap wrapper around exec dispatch also caught any TimeoutError raised inside the dispatch — for example a status poll exhausting its per-request budget and retries — and relabeled that minutes-scale failure as 'exceeded hard cap of <hours>s; the command wedged', which is factually wrong and corrupts failure taxonomy. Replace wait_for with asyncio.timeout and attach the wedged message only when the cap itself expired (Timeout.expired()); a TimeoutError raised inside the dispatch now propagates with its original message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemild@nvidia.com>
hemildesai
force-pushed
the
fix/dedup-command-submit-on-retry
branch
from
August 26, 2026 02:39
f8de5b5 to
cdcd0f7
Compare
hemildesai
changed the title
bxyu-nvidia
marked this pull request as ready for review
August 26, 2026 15:44
Contributor
|
/ok to test cdcd0f7 |
khazic
pushed a commit
to khazic/Gym-lao
that referenced
this pull request
Oct 7, 2026
…Mo#3993) A timeout fetching a finished background command's logs currently aborts the operation. Retry that idempotent GET for the same execution using the existing retry budget, extending the status-read behavior from NVIDIA-NeMo#2765. Command submission semantics and per-call timeouts stay unchanged. Persistent log-read failures can now consume the configured retry budget before surfacing. The regression covers recovery, exhausted retries, and exactly one command submission. A fresh-sandbox fault-injection probe recovered after the first log-read timeout, with two read attempts and exactly one submission; a file append also confirmed one execution. A subsequent real-model terminal rollout completed with two accepted final declarations and successful verification. The model smoke also included NVIDIA-NeMo#3992's verifier-directory preparation. The failure and focused regression are contained in this PR, so a separate issue is unnecessary. Signed-off-by: Frederic Lafrance <flafrance@nvidia.com>
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
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
Background-command status polls are sub-second idempotent GETs, but they inherit the general
request_timeout_sbudget (tuned for long submits/creates, often minutes). Against a sandbox whose pod has become unreachable (a blackholed IP that swallows SYNs), each poll then hangs for a full TCP connect timeout before failing — retries × minutes per dead sandbox before the typed unreachable error fires, tying up a rollout slot the whole time.feat: dedicated short timeout for background status polls
operations.status_poll_timeout_s: float | None = 10.0: per-request budget for background-command status polls only. A healthy poll answers in milliseconds and even the slow legitimate path (endpoint re-resolution plus a connect retry) is a few seconds.Nonerestores the previous behavior; submits keep theirtimeout_s + 60headroom and the logs fetch keeps the shared budget.TimeoutError, which the default retry classifier deliberately treats as terminal (a long timeout has already burned its budget — andTimeoutErrorsubclassesOSError, which would otherwise be retried). With a 10s budget that would make a single slow poll rollout-fatal, so_await_sdk_operationgains a pluggableis_retryablepredicate (default unchanged for all other callers) and the status-poll path passes one that retries timeouts — safe because a status poll is an idempotent GET, unlike a submit where a timeout must stay terminal.fix: stop relabeling inner timeouts as the exec hard cap
Pre-existing bug the short poll budget makes frequent: since Python 3.11
asyncio.TimeoutErrorIS builtinTimeoutError, so thewait_for-based hard-cap wrapper around exec dispatch also caught anyTimeoutErrorraised inside the dispatch (e.g. a status poll exhausting its budget and retries) and relabeled that minutes-scale failure asexceeded hard cap of <hours>s; the command wedged— factually wrong, corrupting failure taxonomy. Now usesasyncio.timeoutand attaches the wedged message only when the cap itself expired (Timeout.expired()); inner timeouts propagate with their original message.Note on the branch's original scope
This PR originally implemented duplicate-submission protection for background commands (adopting an already-scheduled execution after a lost submit response). Per-request tracking deployed at production scale (>115k requests across multiple 1024-way-parallel runs, with body-hash-level duplicate detection) observed zero duplicate submissions in practice, so that machinery was withdrawn to keep the provider simple; this PR now ships only the status-poll timeout work above.
Tests
51 passed on the provider suite: parametrized per-operation timeout routing (poll gets the short budget; submit and logs keep theirs),
Nonefallback, poll-timeout retryability, config validation, inner-timeout propagation with original message (fails against the oldwait_forcode), and a genuinely wedged dispatch still receiving the hard-cap label.🤖 Generated with Claude Code