Skip to content

sig_term_handled: a process the runtime killed at the deadline is reported as killed - #2860

Merged
collivier merged 1 commit into
mainfrom
fix/sigterm-killed-at-deadline
Oct 30, 2026
Merged

collivier merged 1 commit into
mainfrom
fix/sigterm-killed-at-deadline

Conversation

@martin-mat

@martin-mat martin-mat commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Why

sig_term_handled (since #2835) checks the judged processes once a second until the grace period runs out, and the runtime kills the container just after it. Whether the last check falls before or after that kill is timing, and it decides the message:

Run Container stopped in Reported
#2853's branch (slow runner) 12–22 s survivors: [], killed: ["11132 (sleep, killed with SIGKILL)"] → "killed instead of stopped … exit code 137"
#2845's branch (fast runner) 10.7 s survivors: ["11790"], killed: [] → "still running 10s after SIGTERM"

The verdict is FAILED either way, but the spec that asserts the wording ("should fail if PID 1 ignores SIGTERM and keeps running") failed on #2845 for that reason alone, and can fail on any PR that gets a fast runner.

Change

Once the container has exited with 137, a process still alive at the deadline whose strace ends in SIGKILL is counted as killed, not as a survivor: the runtime ended it at the deadline, which is what "killed instead of stopped" says. StopOutcome.survivors_after_runtime_kill holds the rule; a container that did not exit with 137, or a survivor without that trace, is reported as before. No verdict changes, only which of the two failure messages a runtime-killed process gets.

Spec (sig_term shard)

spec/utils/sigterm_outcome_spec.cr: a deadline survivor killed by the runtime counts as killed; with any other exit code, or none, the survivors are kept.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Shutdown results now exclude processes that were forcibly killed after a runtime exit with status 137. For other exit statuses, reported survivors remain unchanged.
  • Tests

    • Added coverage for shutdown outcomes involving forced termination and other runtime exit statuses.

…orted as killed

The test checks the judged processes once a second until the grace
period runs out, and the runtime kills the container just after it.
Whether the last check comes before or after that kill is timing: on a
slow runner the container stops after 12 to 22 s and the process
is already gone ("killed instead of stopped ... exit code 137"); on a
fast one it stops at 10.7 s, the check at 10 s still sees it, and the
same process is reported as "still running 10s after SIGTERM". The
verdict is FAILED either way, but the message changed with the runner,
and the spec that asserts it ("should fail if PID 1 ignores SIGTERM and
keeps running") failed on #2845's run for that reason alone.

Once the container has exited with 137, a process still alive at the
deadline whose trace ends in SIGKILL is counted as killed, not as a
survivor (StopOutcome.survivors_after_runtime_kill): the runtime ended
it at the deadline, which is what "killed instead of stopped" says. A
container that did not exit with 137, or a survivor without that trace,
is reported as before. No verdict changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Martin Matyas <martin.matyas@tieto.com>
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 685c636a-8a56-49bf-aae4-f1d0afdee1bc
Commits

Reviewing files that changed from the base of the PR and between 6a0fd58 and 5b4b528.

Files selected for processing: 3
  • spec/utils/sigterm_outcome_spec.cr
  • src/tasks/utils/stop_outcome.cr
  • src/tasks/workload/microservice.cr

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

The SIGTERM test now filters observed survivors when the container exits with status 137. The new utility removes survivors recorded as killed by SIGKILL and retains the input list for other exit codes.

Changes: SIGTERM outcome handling

Layer / File(s) Summary
Runtime-kill survivor filtering
src/tasks/utils/stop_outcome.cr, spec/utils/sigterm_outcome_spec.cr
Adds StopOutcome.survivors_after_runtime_kill and specs. For exit status 137, it excludes survivors whose recorded ending has killed_by set to SIGKILL. For status 0 or nil, it retains the supplied survivor IDs.
Microservice test integration
src/tasks/workload/microservice.cr
The SIGTERM test passes observed survivors, traced endings, and the container exit code to the new utility.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: collivier, rich-l

Merge Risk: Minimal · up to 5b4b5

The change adjusts SIGTERM survivor reporting; no merge-blocking issue was established.

Architecture risk: Low · up to 5b4b5

The change affects 2 systems.

Changed systems: src, spec

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 2 changed files map to changed impact.
  • observed — spec (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in spec/utils/sigterm_outcome_spec.cr: Adds SIGTERM-tagged expectations for survivors_after_runtime_kill: status 137 excludes "11" and retains "12", while status 0 or nil retains the supplied survivor IDs. The accompanying comment describes the deadline/runtime-kill scenario.
  • observed — Modified behavior in src/tasks/utils/stop_outcome.cr: Adds the StopOutcome module and its public method. The method preserves the input survivors unless the container exit code is 137; in that case, it removes entries whose ending has killed_by equal to "SIGKILL". The accompanying comments describe the runtime-kill interpretation.
  • observed — Modified behavior in src/tasks/workload/microservice.cr: Adds the stop_outcome.cr dependency.
  • observed — Modified behavior in src/tasks/workload/microservice.cr: The SIGTERM test now recalculates survivors using StopOutcome.survivors_after_runtime_kill, passing the observed survivors, traced endings, and container exit code.
Pre-merge checks: Passed 4
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the main behavior change: a process that the runtime killed at the grace-period deadline is now reported as killed. The summary and objectives describe the same change in StopOutcome.s…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main behavior change: a process that the runtime killed at the grace-period deadline is now reported as killed. The summary and objectives describe the same change in StopOutcome.survivors_after_runtime_kill, which filters SIGKILL survivors when the container exit code is 137. The title is specific and matches the changeset. It does not name the exit code 137 condition, but it does not need to cover every detail.

Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

martin-mat added a commit that referenced this pull request Oct 29, 2026
…rt, and let no cleanup hide a failure

The prometheus_traffic and open_metrics specs failed now and then on
unrelated PRs (#2859, #2860; green on rerun), always on the same line:
the ensure block's "helm delete prometheus" assertion. Three defects
made that the only visible symptom:

- The specs ran helm repo add and helm install of an unpinned chart
  from GitHub through ShellCmd.run and never looked at the exit status,
  so a network blip left no Prometheus and nothing said so.
- KubectlClient::Wait.resource_wait_for_install did not wait for a
  Deployment, StatefulSet, DaemonSet or ReplicaSet that did not exist
  yet: replica_count raised NotFoundError at once. The example ended a
  second after cnf_install, which is what every failed run shows.
- The cleanup asserted that helm delete succeeded; with no release it
  failed, and its assertion replaced the example's own failure.

The specs now install Prometheus through one helper that uses the
suite's Helm.helm_repo_add and Helm.install (transient network errors
retried, a failed install raised with Helm's message), refreshes the
repository index (repo add leaves an existing repository as it is, and
the self-hosted runners keep it between jobs), pins the chart to
29.39.0 for Renovate, and waits for the server as an assertion.
Alertmanager persistence is turned off with the chart's actual key
(alertmanager.persistence.enabled; the old persistentVolume key left a
2 GiB claim behind), and the no-op pushgateway key is gone. Each example
runs inside with_prometheus, which removes the release afterwards: a
failed removal fails an example that passed, and is only printed after
one that failed, so it can no longer replace the real failure. resource_ready? treats a workload that is not there yet as not
ready, as it already did for the other kinds: a caller waiting for a
Deployment an operator creates a moment later waits for it instead of
ending on an exception.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Martin Matyas <martin.matyas@tieto.com>
martin-mat added a commit that referenced this pull request Oct 29, 2026
…rt, and let no cleanup hide a failure

The prometheus_traffic and open_metrics specs failed now and then on
unrelated PRs (#2859, #2860; green on rerun), always on the same line:
the ensure block's "helm delete prometheus" assertion. Three defects
made that the only visible symptom:

- The specs ran helm repo add and helm install of an unpinned chart
  from GitHub through ShellCmd.run and never looked at the exit status,
  so a network blip left no Prometheus and nothing said so.
- KubectlClient::Wait.resource_wait_for_install did not wait for a
  Deployment, StatefulSet, DaemonSet or ReplicaSet that did not exist
  yet: replica_count raised NotFoundError at once. The example ended a
  second after cnf_install, which is what every failed run shows.
- The cleanup asserted that helm delete succeeded; with no release it
  failed, and its assertion replaced the example's own failure.

The specs now install Prometheus through one helper that uses the
suite's Helm.helm_repo_add and Helm.install (transient network errors
retried, a failed install raised with Helm's message), refreshes the
repository index (repo add leaves an existing repository as it is, and
the self-hosted runners keep it between jobs), pins the chart to
29.39.0 for Renovate, and waits for the server as an assertion.
Alertmanager persistence is turned off with the chart's actual key
(alertmanager.persistence.enabled; the old persistentVolume key left a
2 GiB claim behind), and the no-op pushgateway key is gone. Each example
runs inside with_prometheus, which removes the release afterwards: a
failed removal fails an example that passed, and is only printed after
one that failed, so it can no longer replace the real failure.
The delete passes --ignore-not-found, and the example that needs no
Prometheus asserts that any release left behind was removed. resource_ready? treats a workload that is not there yet as not
ready, as it already did for the other kinds: a caller waiting for a
Deployment an operator creates a moment later waits for it instead of
ending on an exception.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Martin Matyas <martin.matyas@tieto.com>
@collivier
collivier merged commit 1ce4fac into main Oct 30, 2026
188 of 199 checks passed
@collivier
collivier deleted the fix/sigterm-killed-at-deadline branch October 30, 2026 09:43
@github-project-automation github-project-automation Bot moved this from In progress to Done in CNF Test Suite Oct 30, 2026
collivier pushed a commit that referenced this pull request Oct 30, 2026
…rt, and let no cleanup hide a failure

The prometheus_traffic and open_metrics specs failed now and then on
unrelated PRs (#2859, #2860; green on rerun), always on the same line:
the ensure block's "helm delete prometheus" assertion. Three defects
made that the only visible symptom:

- The specs ran helm repo add and helm install of an unpinned chart
  from GitHub through ShellCmd.run and never looked at the exit status,
  so a network blip left no Prometheus and nothing said so.
- KubectlClient::Wait.resource_wait_for_install did not wait for a
  Deployment, StatefulSet, DaemonSet or ReplicaSet that did not exist
  yet: replica_count raised NotFoundError at once. The example ended a
  second after cnf_install, which is what every failed run shows.
- The cleanup asserted that helm delete succeeded; with no release it
  failed, and its assertion replaced the example's own failure.

The specs now install Prometheus through one helper that uses the
suite's Helm.helm_repo_add and Helm.install (transient network errors
retried, a failed install raised with Helm's message), refreshes the
repository index (repo add leaves an existing repository as it is, and
the self-hosted runners keep it between jobs), pins the chart to
29.39.0 for Renovate, and waits for the server as an assertion.
Alertmanager persistence is turned off with the chart's actual key
(alertmanager.persistence.enabled; the old persistentVolume key left a
2 GiB claim behind), and the no-op pushgateway key is gone. Each example
runs inside with_prometheus, which removes the release afterwards: a
failed removal fails an example that passed, and is only printed after
one that failed, so it can no longer replace the real failure.
The delete passes --ignore-not-found, and the example that needs no
Prometheus asserts that any release left behind was removed. resource_ready? treats a workload that is not there yet as not
ready, as it already did for the other kinds: a caller waiting for a
Deployment an operator creates a moment later waits for it instead of
ending on an exception.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Martin Matyas <martin.matyas@tieto.com>
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