Skip to content

Add timeout-minutes to jobs that have none (PM_GH_REPO_024) - #217

Open
anish-sujanani wants to merge 1 commit into
mainfrom
pmsec/job-timeouts
Open

anish-sujanani wants to merge 1 commit into
mainfrom
pmsec/job-timeouts

Conversation

@anish-sujanani

Copy link
Copy Markdown

Add timeout-minutes to jobs that have none

Addresses PM_GH_REPO_024 — Workflow jobs must declare a timeout.

A job with no timeout-minutes inherits GitHub's default of 360 minutes, so a hung job
holds a runner for six hours before the platform stops it. This bounds 2 jobs;
worst case for this repo drops from 720 runner-minutes to 23.

Proposed values

Workflow Job Successful runs sampled p50 p90 max timeout-minutes
ci.yml all-checks 18 0.1m 0.1m 0.9m 3
docker-build.yml build 49 5.0m 6.7m 7.7m 20

How these were calculated

timeout-minutes = min(350, max(3, ceil(m x slowest SUCCESSFUL run)))
    m = 2.0 under 30m   1.5 for 30-120m   1.3 above 120m
  • Source: GET /repos/{owner}/{repo}/actions/runs/{id}/jobs, per-job started_at to
    completed_at over the 50 most recent completed runs of each workflow. That is the same
    clock timeout-minutes uses: it starts when the job begins executing and excludes queue
    and runner-acquisition time, so it will read lower than the run duration shown in the
    Actions tab.
  • Only successful runs were counted. Cancelled and failed runs measure when someone hit
    cancel or when a job died, not how long the work takes.
  • Floor of 5 rather than 3 for any job that installs dependencies or runs a build or test
    suite. A 3-minute floor killed a legitimate job in CI earlier in this rollout.
  • 350 cap because GitHub-hosted runners hard-stop at 360, so anything above it is decorative.

Verification

  • The parsed YAML is identical before and after with timeout-minutes stripped from both
    trees, so nothing else moved.
  • The diff adds only timeout-minutes: lines and deletes nothing.
  • actionlint reports the same findings before and after.
  • Re-running the tool refuses every job, so the change is idempotent.

Break risk

A timeout can only stop a job that exceeds it. If any value is tighter than you want, say so
and I will raise it — a loose bound still fixes the finding.

Sized from observed per-job durations of successful runs where available, otherwise from a budget the job itself declares or from what the steps do. Cancelled and failed runs were excluded. See the batch notes for per-job reasoning.

@nitpickybot nitpickybot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adds timeout-minutes to the two jobs that lacked it. Values look sane: the aggregation-only all-checks job gets 3 minutes, and the Docker build gets 20 against a 7.7m observed max. No correctness or security concerns; nothing else in the workflows moved.

Review Scope

  • Mode: Full pull request
  • Range: Full pull request through 1692f19afebec0e6e4d837f53582a3315fdfc6c6
  • Current range coverage: Complete
  • Cumulative pull request coverage: Complete

This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.

This branch has not been deployed

No deployments
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.

1 participant