Skip to content

recipes: bash step timeout ignores on_error: continue and fails the whole recipe #406

Description

@Joi

A bash step that hits its timeout: fails the whole recipe even when the step says on_error: continue.

Where. microsoft/amplifier-bundle-recipes (issues are disabled there, so filing here), modules/tool-recipes/amplifier_module_tool_recipes/executor.py, _execute_bash_step (main at bf5f88c). The OSError branch a few lines above checks step.on_error and returns a failed BashResult under continue; its comment says a step failure "honours on_error exactly like a non-zero exit". The non-zero-exit branch below does the same. The timeout branch between them does not:

except asyncio.TimeoutError:
    # Kill the process on timeout
    process.kill()
    await process.wait()
    raise ValueError(
        f"Step '{step.id}': command timed out after {effective_timeout}s"
    ) from None

Repro.

name: timeout-continue
steps:
  - id: slow-optional
    type: bash
    timeout: 2
    command: sleep 10
    on_error: continue
  - id: after
    type: bash
    command: echo reached

Expected: slow-optional is recorded as failed and after runs. Actual: Recipe execution failed: Step 'slow-optional': command timed out after 2s; after never runs.

Why it matters. on_error: continue is how a recipe marks a step optional. A slow optional source is the most common way such a step fails, and it is the one case the flag does not cover. One of my nightly recipes lost its whole run twice to an optional 60s step whose command sometimes takes 110s.

Suggested fix. In the timeout branch, build the same message, then follow the other two branches: raise ValueError for fail, SkipRemainingError for skip_remaining, and for continue log the warning and return a BashResult with the message in stderr and a non-zero exit code (124 would match coreutils timeout). process.kill() also only kills the shell, not the process group, so a command's children can outlive the step; starting the subprocess with start_new_session=True and using os.killpg would close that.

Happy to send a PR if that direction is right.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions