Fix infinite wait in sandbox.wait_for_sandbox

[i-vainn]

Fixed a bug where wait_for_sandbox() could hang indefinitely if the sandbox never became ready

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved sandbox initialization reliability with enhanced timeout handling and better error messaging when startup delays occur.

• Refactor

  • Refined sandbox wait mechanism for more robust polling with clearer timeout boundaries.

[greptile-apps]

_{No files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce a more robust sandbox startup wait mechanism by replacing a simple timeout parameter with dual timeout parameters: wait_timeout for the total wait duration and http_timeout for individual HTTP check attempts. The implementation adds explicit error handling via RuntimeError when the sandbox fails to become ready within the wait timeout. A call site is updated to use the new parameter names and increased wait timeout.

Changes

Cohort / File(s)	Summary
Sandbox Timeout Mechanism nemo_skills/code_execution/sandbox.py, nemo_skills/evaluation/evaluator/code.py	Updated wait_for_sandbox() method signature to accept separate wait_timeout and http_timeout parameters with improved polling logic and timeout-based error handling. Call site updated from positional argument wait_for_sandbox(50) to keyword argument wait_for_sandbox(wait_timeout=240), increasing max wait time during sandbox initialization.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client Code
    participant Sandbox as Sandbox
    participant CheckReady as _check_ready()
    participant Endpoint as HTTP Endpoint

    Client->>Sandbox: wait_for_sandbox(wait_timeout=240, http_timeout=5)
    
    rect rgba(100, 150, 200, 0.5)
    Note over Sandbox: Polling Loop (until wait_timeout)
    
    loop While elapsed < wait_timeout
        Sandbox->>CheckReady: _check_ready(http_timeout=5)
        
        alt HTTP request succeeds
            CheckReady->>Endpoint: GET /readiness
            Endpoint-->>CheckReady: 200 OK
            CheckReady-->>Sandbox: True (sandbox ready)
            Sandbox-->>Client: Success (return)
        else HTTP request times out
            CheckReady->>Endpoint: GET /readiness (timeout after 5s)
            Endpoint--xCheckReady: Timeout
            CheckReady-->>Sandbox: False (not ready yet)
            Sandbox->>Sandbox: Continue polling...
        end
    end
    end
    
    rect rgba(255, 100, 100, 0.5)
    alt wait_timeout exceeded
        Sandbox->>Sandbox: Raise RuntimeError
        Sandbox-->>Client: RuntimeError (timeout)
    end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Suggested reviewers

• gwarmstrong
• Kipok

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and clearly addresses the main change: fixing an infinite wait bug in sandbox.wait_for_sandbox by implementing timeout parameters.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch imoshkov/fix-sandbox-timeout

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Kipok]

should we do the same for llm wait as well? @i-vainn

[gwarmstrong]

@Kipok are you saying like waiting for the llm server?

[Kipok]

yes, I think we should have the same timeout logic for wait for llm server as we do for sandbox server. Currently llm will also block forever if something is wrong with the address

[gwarmstrong]

how long do you think we should poll for? some models can take over 30 minutes to be ready--we could make the wait time configurable, but sometimes there is a decent amount of variance in the load times. Would we rather preemptively kill jobs that are ok, or fail misconfigured jobs with a job reaper (which will have a delay, but otherwise be more accurate signal).

[Kipok]

we can make it configurable. I guess by default we should set a fairly high timeout, like 2 hours. But just thinking that we should have a consistency in logic, if we support timeout for sandbox, we should also support it for llm. But this isn't very high priority