Skip to content

feat: Add auto-termination of stuck PENDING container executions after configured timeout#292

Open
yuechao-qin wants to merge 1 commit into
masterfrom
ycq/feat-terminate-pending-container-after-timeout
Open

feat: Add auto-termination of stuck PENDING container executions after configured timeout#292
yuechao-qin wants to merge 1 commit into
masterfrom
ycq/feat-terminate-pending-container-after-timeout

Conversation

@yuechao-qin

@yuechao-qin yuechao-qin commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes https://github.com/Shopify/oasis-backend/issues/422

  • Adds a configurable pending_execution_timeout to the orchestrator that auto-terminates container executions stuck in PENDING state beyond the threshold (default: disabled, oasis-backend sets 12h)
  • Terminates the K8s pod, uploads logs best-effort, marks the execution as SYSTEM_ERROR with a user-visible error message, and skips downstream nodes
  • Prevents zombie PENDING pods from degrading orchestrator throughput by consuming polling cycles indefinitely

Changes

  • orchestrator_sql.py: New _terminate_timed_out_pending_execution method + _format_duration helper
  • orchestrator_main_oasis.py: Explicitly passes pending_execution_timeout=timedelta(hours=12)
  • tests/test_orchestrator_sql.py: New test file covering timeout termination, no-op when not expired, disabled mode, and RUNNING not affected

Context

Closes https://github.com/Shopify/oasis-backend/issues/422

When a K8s pod fails to schedule (e.g., resource exhaustion, node pool issues), the container execution remains PENDING forever. The orchestrator polls it every sweep cycle but never takes action, wasting batch budget and delaying real executions behind it in the queue.

Test plan

  • Unit tests for expired, not-expired, disabled (None), and RUNNING scenarios
  • Unit tests for _format_duration at boundary values (seconds/minutes/hours)
  • All 443 existing tests pass

yuechao-qin commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

@yuechao-qin yuechao-qin marked this pull request as ready for review July 6, 2026 18:42
@yuechao-qin yuechao-qin requested a review from Ark-kun as a code owner July 6, 2026 18:42
@morgan-wowk

Copy link
Copy Markdown
Collaborator

🤖 This is a clean, well-scoped fix — really nice work. The teardown path is self-contained and easy to follow (terminate → SYSTEM_ERROR → record message → skip downstream → commit), the guards read clearly, and I like that it's opt-in per deployment with RUNNING left untouched (nice test for that) and _format_duration covering the boundaries.

On the 12h default — it's a reasonable starting point, and I think it's worth grounding in what we've actually seen rather than picking it by feel. The status-change history is already recorded in each execution's extra_data, so we can back the number out of real data: pull the retroactive traces for execution status transitions and look at how long historical tasks legitimately dwell in PENDING before they start (the PENDING→RUNNING gap), so the threshold clears the tail of that distribution. Worth keeping in mind that created_at includes Kueue queue time, so that gap is exactly what this deadline clips.

One suggestion for this first version: the termination message is generic ("stuck in PENDING for X"), so from the SYSTEM_ERROR alone an operator can't tell a gcsfuse mount wedge (CreateContainerConfigError) from an image-pull stall or an unschedulable pod. Since we're already holding the pod status here, folding the main container's waiting.reason/message into the message would make each termination self-explanatory in the UI. There's a small property that does exactly that pull in #289 if it's handy as a reference — feel free to lift it or leave it. Even just appending the reason string would go a long way.

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.

2 participants