Skip to content

ci(experimental): skip or stop superseded runs instead of cancelling them (#1938) - #1942

Merged
JarryShaw merged 4 commits into
mainfrom
ci/1938-superseded-experimental
Oct 19, 2026
Merged

JarryShaw merged 4 commits into
mainfrom
ci/1938-superseded-experimental

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Experimental Tests no longer cancels superseded runs, so commits no longer show a red X when a newer push lands. The concurrency group is gone: even without cancel-in-progress, a group cancels the pending run it replaces.

A new current gate job (util/superseded.py) checks that the commit is still the head of main, or of an open PR. Every other job skips when it is not, and a skipped run is neutral in the commit's status.

The capped pytest command now runs under the script's watcher. Once a newer commit lands, or the PR merges or closes, it stops the session with TERM and then KILL, and exits 0 with a Superseded notice, which frees the runner.

If the API cannot be read, the commit counts as current. A real test failure keeps its status, and every existing cap is unchanged. The behaviour is pinned in tests/project/test_superseded_runs_1938.py, which fails on main. actionlint was not available.

Closes #1938

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 19, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at cd781fae4. The design holds. Skipped jobs report success, per the docs.github.com conditions page. actionlint 1.7.7 is clean, and the tests fail on main. Three things need fixing.

  1. stop() can hang. After SIGKILL it calls child.wait() with no timeout (util/superseded.py:278), and signal_session swallows PermissionError. A Sonnet cross-review reproduced this with a root-owned sudo chain: the signals were refused and the watcher blocked indefinitely. Bound the wait, then report that the stop failed rather than hang.
  2. Signal delivery through sudo. killpg on the watcher's session reaches the tests only if sudo keeps them in that process group. Ubuntu's Defaults use_pty puts the command in its own pty session, so it may not. Stop the systemd scope directly (sudo systemctl kill/stop on the unit), and add a test for the sudo chain.
  3. Gate failure. If the current job itself fails, the downstream if: (no status function, so an implicit success()) skips everything; it does not run the rest as the comment and docs say. Prefix !cancelled() &&, or fix the wording.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: running A cross-review is in flight against the current head - no verdict yet and removed review: running A cross-review is in flight against the current head - no verdict yet review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 19, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at a9d95dff5. A Sonnet re-review listed every wait and subprocess call in util/superseded.py, and each one is bounded. The watcher's --scope pcapkit-tests.scope matches the step's systemd-run --unit= exactly. docs.github.com documents passwordless sudo on hosted runners. The real step script keeps a current run's status, ends a superseded run green, and stops a scoped run through its scope. Exit 70 from a failed stop is a warning, not a test failure. With !cancelled() on the gate, a failed current still runs the rest, and a failed python-versions cannot start an empty matrix. With the round-1 files restored, 14 tests fail; with the fix, 78 pass. actionlint 1.7.7 is clean. The sudoers(5) text quoted in the docs is confirmed: use_pty has no effect without a terminal.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 19, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

for superceded.py, why no requests?

@JarryShaw

Copy link
Copy Markdown
Owner Author

It runs where requests isn't installed. The current gate job checks out only util/superseded.py, with no setup-python and no pip install, so it starts within seconds and runs on the runner's system python3. The watcher wraps the test step on /usr/bin/python3 rather than the leg's interpreter, because under GraalPy every process starts its own VM. Ubuntu 24.04's system Python is externally managed (PEP 668), so pip install requests there needs --break-system-packages. The other util/ scripts that use requests (ci_timing.py, conda_vendor.py, update_python_versions.py) all run after setup-python plus pip install. If you'd rather have requests, there are two options: (1) add setup-python and pip install requests to the gate, at a cost of roughly 10–20 s per run, and run the watcher from that same Python; or (2) keep the stdlib client. I'd keep (2), since the script needs only two GET endpoints and an ETag header. Which do you prefer?

@JarryShaw JarryShaw added the needs: decision Waiting on the maintainer to decide — not blocked by other work label Oct 19, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Going with requests, then. The gate job will run setup-python and pip install requests before the check. In the experimental job, a separate CPython set up with update-environment: false will carry requests and run the watcher, leaving each leg's own interpreter untouched. If that Python's setup fails, the run still goes ahead as current, the same as an API error. I'll post a new verdict once it's reworked and re-reviewed.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed needs: decision Waiting on the maintainer to decide — not blocked by other work review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 19, 2026
…them (#1938)

- Drop experimental.yml's concurrency group: a cancelled check marks its
  commit red, and a group without cancel-in-progress still cancels its
  pending run.
- Add util/superseded.py: a commit is current while it heads main, or its
  open pull request; any other event, or an API error, is current.
- Add a `current` gate job that every other job needs and skips on, so a run
  superseded before it starts ends with skipped checks and a notice.
- Run the capped pytest command under the script's watcher, which checks every
  60 s and, once superseded, stops the session (TERM, then KILL) and exits 0
  with a notice and a summary line; otherwise it keeps the tests' status.
- Describe it in workflows.rst; pin it in tests/project and update the pins
  the gate and watcher move.

Closes #1938
…1938)

- Run the watcher outside the scope and, where the tests run in one, stop
  pcapkit-tests.scope by name with `sudo -n systemctl kill`: the scope's
  processes are out of reach of a signal to the session's root-owned sudo.
  The session's process group stays the path for an unscoped run.
- Bound every wait. A command that outlives SIGKILL gets a `Superseded run
  not stopped` warning and exit 70, not 0, since its tests may run on.
- Open each downstream `if:` with `!cancelled()`, so a failed `current` job
  runs the rest; the legs still need `changes` and `python-versions`.
- Test the scoped stop, the bounded stop and the gate's failure semantics
  through an injectable command runner and signaller, and in the step.

Refs #1938
- Back util/superseded.py's transport with a requests.Session, keeping the
  injectable transport, ETag/If-None-Match, the bounded timeout, and any
  error, a missing requests included, counting as current.
- Gate: read `latest`, set it up and install requests before the check,
  every step continue-on-error.
- Legs: set up `latest` apart from the leg (`update-environment: false`),
  install requests into it alone, and run the watcher on it; where either
  step fails, the tests run unwatched. Each step is capped at 1 minute,
  leaving 3 of the job's 140 uncapped.
- util/update_python_versions.py reads a matrix job's fallback label for
  `latest`, as it does for the tiers.
- Update the docs, the caps pins and the new tests.

Refs #1938
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 19, 2026
@JarryShaw
JarryShaw force-pushed the ci/1938-superseded-experimental branch from a9d95df to 1cce93a Compare October 19, 2026 17:34
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round 3 at 1cce93a4c, rebased onto main: util/superseded.py now fetches with requests. The ETag cache, the injectable transport and the 15 s timeout are unchanged, and a missing requests counts as current. The gate job runs setup-python on latest and installs requests. Each experimental leg sets up a separate latest CPython for the watcher with update-environment: false, so the leg's own interpreter, PATH and pip cache are untouched. If that setup fails, the tests run unwatched.

Two changes outside the ask, both needed to keep existing pins green:

  • test_ktlo_legs_stay_green_1672 budgets capped steps against the job's 140-minute cap. The two new 1-minute steps fit only by cutting the spare margin from 5 to 3 minutes. Raising the job cap to 142 is the alternative.
  • util/update_python_versions.py now reads the matrix.os || fallback label for latest, as it already did for the other tiers.

A Sonnet re-review is running.

@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 19, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 1cce93a4c, prose only. A Sonnet re-review confirmed the design. A missing or failing requests reads as current, and the 304/ETag path still works. update-environment: false leaves the leg's environment alone (per setup-python's find-python.ts), and the new pins fail without their fixes. The one change: the #1938 test docstrings carry review-round labels ("Round 2:", "Round 3:"), and a KTLO comment narrates the old margin. Both are being restated as standing facts in a follow-up commit.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 19, 2026
- Reword the #1938 tests' docstrings and comments without the review-round
  prefixes, each still stating what its test checks.
- State the experimental leg's uncapped margin as what it covers and the
  measured maximum, not as what it was.

Refs #1938
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 19, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at bbc1cad65. A Sonnet delta review confirmed the new commit edits only wording in the two test files, with no assertion or constant changed. No review-round labels or history wording remain in the PR, and the KTLO comment's list of uncapped steps matches experimental.yml. Both test files pass (58 passed).

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 19, 2026
@JarryShaw
JarryShaw merged commit 6a4a72f into main Oct 19, 2026
8 checks passed
@JarryShaw
JarryShaw deleted the ci/1938-superseded-experimental branch October 19, 2026 17:54
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 19, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ci(experimental): skip or stop superseded runs instead of cancelling them, so commits are not marked red

1 participant