Skip to content

Wire runtime idle threshold through runtime-api chart#637

Open
neubig wants to merge 3 commits into
mainfrom
fix/runtime-idle-terminal-timeout-cap
Open

Wire runtime idle threshold through runtime-api chart#637
neubig wants to merge 3 commits into
mainfrom
fix/runtime-idle-terminal-timeout-cap

Conversation

@neubig
Copy link
Copy Markdown
Contributor

@neubig neubig commented May 16, 2026

Summary

  • Move RUNTIME_IDLE_SECONDS into the shared runtime-api chart env block so the deployment and cleanup job see the same idle threshold
  • Remove the duplicate cleanup-only env entry
  • Bump the runtime-api chart and parent openhands chart dependency/version

Validation

  • git diff --check
  • Helm dependency update was attempted locally, but runtime-api:0.3.4 is not published yet; CI preview publishing should provide the alpha dependency for PR validation.

This PR was created by an AI agent (OpenHands) on behalf of the user.

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
@neubig neubig marked this pull request as ready for review May 16, 2026 15:06
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟡 Acceptable

The core change is sound - moving RUNTIME_IDLE_SECONDS to a shared template reduces duplication and ensures consistency between deployment and cleanup job. However, there are configuration issues and behavioral changes that need attention.


[CRITICAL ISSUES]
None

[IMPROVEMENT OPPORTUNITIES]

  • [charts/runtime-api/templates/_env.yaml, Line 7] Configuration Mismatch: Template default (1800 seconds) doesn't match values.yaml default (1200 seconds). This creates confusion about the actual default value users will get. Either change this to default 1200 to match values.yaml, or update values.yaml to 1800.
  • [charts/runtime-api/values.yaml, Line 212] Configuration Mismatch: The values.yaml default (1200) doesn't match the template default (1800). These should be aligned to avoid ambiguity.

[TESTING GAPS]

  • [charts/runtime-api/templates/deployment.yaml, Line 63] Behavioral Change Not Validated: The deployment pod now receives RUNTIME_IDLE_SECONDS environment variable for the first time (via the shared runtime-api.env template). The PR description indicates this is intentional, but provides no runtime evidence that the deployment code handles this correctly. Need confirmation that:
    1. The runtime-api service code actually uses this environment variable
    2. The deployment has been tested with this env var present
    3. Both deployment and cleanup job interpret the value consistently

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

This PR changes runtime configuration by adding an environment variable to the deployment that was previously only available to the cleanup job. While the intent is to ensure consistency, there's no evidence the change was tested end-to-end. The configuration default mismatch could also lead to unexpected behavior. The workflow changes for preview chart dependencies are correct and necessary.

Recommendation: Before merging, verify that the runtime-api service code properly handles RUNTIME_IDLE_SECONDS and align the default values between values.yaml and the template.


VERDICT:
Worth merging (after addressing default value mismatch and confirming deployment behavior)

KEY INSIGHT:
Moving shared configuration to a common template is the right pattern, but requires validating that all consumers (deployment + cronjob) handle the configuration correctly and consistently.


Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Comment thread charts/runtime-api/templates/_env.yaml Outdated
Co-authored-by: openhands <openhands@all-hands.dev>
@neubig neubig requested a review from all-hands-bot May 16, 2026 15:21
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟢 Good taste

Clean refactoring that eliminates duplication and ensures consistency. The shared template approach is the right design, and the default value alignment (1200s) resolves the previous configuration mismatch.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

Configuration-only changes with proper defaults. The workflow updates correctly enable preview chart publishing by pointing dependencies to the PR-specific OCI registry. No breaking changes to runtime behavior.

VERDICT:
Worth merging: Solid refactoring with no issues found.

KEY INSIGHT:
Moving RUNTIME_IDLE_SECONDS to the shared env template ensures all runtime-api components (deployment, cleanup jobs, init containers) see the same idle threshold, eliminating configuration drift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants