fix(workload): let Ctrl-C stop the intercept session cleanly#6031
fix(workload): let Ctrl-C stop the intercept session cleanly#6031devantler wants to merge 12 commits into
Conversation
SIGINT previously hit the default disposition and killed the process with status 130, so the tunnel session and its local connections never unwound through their cleanup paths and the in-cluster agent was left to notice the dead peer on its own. Wrap the steering session in a signal-aware context (signal.NotifyContext, SIGINT/SIGTERM), mirroring the mirror command's fix, so an interrupt cancels the session context: ServeIntercepted maps the cancellation to a clean stop, every pump is torn down and drained, and the command exits 0 as its help text now documents. Fixes #6010 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesIntercept signal shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
…nt-unwind # Conflicts: # pkg/svc/chat/docs_generated.go
✅MegaLinter analysis: Success✅ Linters with no issuesactionlint, bash-exec, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint Notices📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining See detailed reports in MegaLinter artifacts
|
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in the branch remains at 66%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
@coderabbitai full review |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/svc/chat/docs_generated.go`:
- Line 7: Escape the literal pipe characters that occur inside Markdown table
cells in the declarative configuration source, including entries such as
“kind.yaml |” and “true=Enabled |”, so they no longer terminate cells; then
regenerate the embedded documentation via `go generate ./pkg/svc/chat/...`,
updating `generatedDocumentation` accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55b8a60f-8cf3-4ae7-b1a3-3348ddfc67a9
📒 Files selected for processing (4)
docs/src/content/docs/cli-flags/workload/workload-intercept.mdxpkg/cli/cmd/workload/intercept.gopkg/cli/cmd/workload/intercept_test.gopkg/svc/chat/docs_generated.go
Resolved the Docstring Coverage pre-merge failure in
@coderabbitai review |
|
✅ Action performedReview finished.
|
@coderabbitai full review Please recompute the pre-merge checks on current head |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/cmd/workload/intercept.go (1)
274-301: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCreate the signal-aware context before setup
resolveInjectionPointandensureSteerstill run oncmd.Context(), so Ctrl-C during--wait-timeoutcan bypass clean shutdown and kill the process abruptly. Movesignal.NotifyContextearlier and thread it through setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/cmd/workload/intercept.go` around lines 274 - 301, Create the signal-aware context with signal.NotifyContext before resolveInjectionPoint and ensureSteer in the intercept command flow, then pass that context through both setup calls and runInterceptSession. Use the derived context consistently instead of cmd.Context(), and keep the deferred stop cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/svc/chat/docs_generated.go`:
- Line 7: Fix the truncated documentation table cells in the source declarative
configuration page for `distributionConfig`, `NodeAutoscalerConfig.enabled`, and
`NodeAutoscalerConfig.expander`, ensuring each description is complete and
properly closed. Then rerun `go generate ./pkg/svc/chat/...` so the
`generatedDocumentation` constant in `docs_generated.go` is regenerated from the
corrected source.
---
Outside diff comments:
In `@pkg/cli/cmd/workload/intercept.go`:
- Around line 274-301: Create the signal-aware context with signal.NotifyContext
before resolveInjectionPoint and ensureSteer in the intercept command flow, then
pass that context through both setup calls and runInterceptSession. Use the
derived context consistently instead of cmd.Context(), and keep the deferred
stop cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd4e4acb-387d-414a-a849-03bd0ba89d49
📒 Files selected for processing (4)
docs/src/content/docs/cli-flags/workload/workload-intercept.mdxpkg/cli/cmd/workload/intercept.gopkg/cli/cmd/workload/intercept_test.gopkg/svc/chat/docs_generated.go
Resolved the outside-diff interrupt finding in eef181b. The signal-aware context is now installed before any client or cluster setup, then propagated through pod resolution, steering injection and readiness, and the tunnel session. Signal-driven context cancellation is normalized to the documented clean exit. The regression test reproduced the pre-fix failure as signal: interrupt when SIGINT arrived during setup, then passed after the fix. Validation passed: 1,193 affected-package tests, two focused signal tests, scoped golangci-lint, the Go build, and the Astro docs build with link validation. @coderabbitai review |
|
✅ Action performedReview finished.
|
@coderabbitai full review Please recompute the pre-merge checks on current head cb2c1ce. Every changed or new named function now has a descriptive comment immediately before its declaration, including the process-wide signal tests whose nolint directives were moved above their doc comments. |
|
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 10 minutes. |
The signal-aware context was installed only around the steering session, so Ctrl-C during injection-point resolution or the steering agent's --wait-timeout wait hit the default disposition and killed the process abruptly. Install signal.NotifyContext before the first Kubernetes call and thread it through setup via cmd.SetContext, so an interrupt during setup cancels cleanly. Red/green-proven with a setup-phase interrupt test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…into claude/intercept-sigint-unwind # Conflicts: # pkg/cli/cmd/workload/intercept.go # pkg/cli/cmd/workload/intercept_test.go
Resolution record for the @coderabbitai review |
|
✅ Action performedReview finished.
|
goconst flags the third occurrence of the "windows" literal introduced by the setup-interrupt regression tests; fold the identical three-line skip block into one skipIfInterruptUnsupported helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documented the touched test functions with concise intent comments. A file-wide AST audit now finds exact-name Go documentation on 56 of 69 named functions (81.16%). Validation: go test ./docs ./pkg/cli/cmd/workload -count=1 (581 tests), scoped golangci-lint, and git diff --check all pass. @coderabbitai full review |
|
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 6 minutes. |
Adaptive-review cooldown elapsed; re-requesting the full review for head 2526247 after the documentation-only gate fix. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/cli/cmd/workload/intercept_test.go (1)
204-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding SIGTERM coverage alongside the SIGINT (Ctrl-C) test.
Issue
#6010calls for handling both SIGINT and SIGTERM through the signal-aware context, andrunInterceptCommandregisterssyscall.SIGTERMalongsideos.Interrupt. The new regression tests only exerciseos.Interrupt; a companion test raisingsyscall.SIGTERM(mirroring this stub) would close the coverage gap for the other registered signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/cmd/workload/intercept_test.go` around lines 204 - 227, Add a companion test for SIGTERM alongside TestInterceptCmdInterruptStopsSessionCleanly, using the existing stubInterruptingInterceptSession setup and command assertions; trigger syscall.SIGTERM and verify cmd.Execute returns no error, confirming runInterceptCommand handles both registered termination signals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/configuration/declarative-configuration.mdx`:
- Line 416: The maxNodesTotal description ends with an incomplete sentence.
Update the source description for maxNodesTotal to say “This value should be <=
serverLimit.”, then regenerate the declarative configuration MDX documentation
rather than editing the generated file directly.
In `@pkg/apis/cluster/v1alpha1/types.go`:
- Line 94: Update the jsonschema_description on DistributionConfig to include
kwok/, gke.yaml, and aks.yaml alongside the existing supported paths, then
regenerate the schema and documentation artifacts using the repository’s
standard generation workflow rather than editing generated files manually.
In `@pkg/cli/cmd/workload/intercept_test.go`:
- Around line 168-192: Replace the fixed 100ms wait in
stubInterruptingInterceptSetup and the analogous setup at the referenced
location with bounded synchronization on the cancellation signal, using the
select/polling pattern from stubInterruptingInterceptSession. Wait for
ctx.Done() or a bounded timeout before returning context.Canceled, preserving a
timeout fallback so the test cannot hang and cleanInterceptCancellation observes
the canceled context deterministically.
---
Nitpick comments:
In `@pkg/cli/cmd/workload/intercept_test.go`:
- Around line 204-227: Add a companion test for SIGTERM alongside
TestInterceptCmdInterruptStopsSessionCleanly, using the existing
stubInterruptingInterceptSession setup and command assertions; trigger
syscall.SIGTERM and verify cmd.Execute returns no error, confirming
runInterceptCommand handles both registered termination signals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af3306c0-aee0-4d1a-99a7-263e211ae8a9
⛔ Files ignored due to path filters (1)
web/ui/src/generated/ksail-config.tsis excluded by!**/generated/**
📒 Files selected for processing (11)
docs/fielddocs_test.godocs/src/content/docs/cli-flags/workload/workload-intercept.mdxdocs/src/content/docs/configuration/declarative-configuration.mdxpkg/apis/cluster/v1alpha1/autoscaler.gopkg/apis/cluster/v1alpha1/types.gopkg/cli/cmd/workload/intercept.gopkg/cli/cmd/workload/intercept_test.gopkg/cli/cmd/workload/mirror.gopkg/cli/cmd/workload/mirror_test.gopkg/svc/chat/docs_generated.goschemas/ksail-config.schema.json
Replace timing-based setup cancellation with a context-driven readiness wait, cover SIGINT and SIGTERM through one table, and remove duplicate schema changes now owned by #6036.
Resolved all current review findings on head 986ec04:
Validation passed: make generate; 1,128 affected tests; 561 full workload tests; scoped golangci-lint; Astro build plus link validation; git diff --check. @coderabbitai full review |
|
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 20 minutes. |
@coderabbitai run pre-merge checks Please recompute only the pre-merge checks at current head |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
@coderabbitai review The current head |
|
✅ Action performedReview finished.
|
@coderabbitai run pre-merge checks The Out of Scope Changes warning refers to autoscaler/schema/fielddocs changes that are no longer on this branch — the diff is now exactly the five files needed for the #6010 interrupt fix (intercept.go, its tests, the generated intercept CLI reference, mirror.go help text, and the generated chat doc). The class-wide schema/doc migration lives in #6036. Please recompute against head. |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
Live-cluster evidence for this PR from the #5971 validation pass (Kind/Docker, 2026-07-11): on current |
@coderabbitai The Out of Scope Changes warning (re-evaluated 02:20Z) cites "config schema/docs and fielddoc tests", but none of those are in the current 5-file diff: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
gh pr diff 6031 --name-only 2>/dev/null || trueLength of output: 426
There is no config-schema, fielddocs, or autoscaler file in the current diff. I'll re-run the review now so the pre-merge checks and Out of Scope Changes result are recomputed against this head. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/svc/chat/docs_generated.go`:
- Line 7: Fix the upstream jsonschema descriptions for the fields represented by
distributionConfig, NodeAutoscalerConfig.enabled, and
NodeAutoscalerConfig.expander so they contain complete, properly escaped
table-safe text rather than truncating at pipe characters; locate the
corresponding schema/type definitions by their field names and verify all
descriptions are complete. Then regenerate the artifact with go generate
./pkg/svc/chat/... and confirm the generated configuration table contains the
full descriptions; do not edit generatedDocumentation directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c02bba6-afb8-4d3e-a723-131bee6f6969
📒 Files selected for processing (2)
pkg/cli/cmd/workload/intercept_test.gopkg/svc/chat/docs_generated.go
Resolved the current-main conflict in 09a9945. The only merge conflict was generated pkg/svc/chat/docs_generated.go, rebuilt with make generate rather than hand-edited. Scoped lint also exposed and fixed one whitespace-only WSL finding in pkg/svc/mirror/replay_test.go. Validation passed: focused SIGINT/SIGTERM intercept tests; workload, chat, mirror, API-type, and docs tests; scoped golangci-lint with 0 issues; generator idempotence; gofmt; and git diff --check. The draft is mergeable again and fresh CI is running. CodeRabbit remains fail-closed until a current-head review/pre-merge result is available. |
Resolution record for the two review-body findings, verified against current head
No code change needed; both were addressed by later commits on this branch. |
1 participant
Why
workload intercept tells the user to press Ctrl-C to stop, but the interrupt killed the process outright. The tunnel never drained and the in-cluster steering agent was left with a dead peer instead of an orderly teardown.
What
SIGINT and SIGTERM now cancel one signal-aware context installed before cluster setup. Resolution, steering-agent injection, readiness waits, and the tunnel all unwind through that context, and signal cancellation maps to the documented exit status 0. Regression coverage raises both termination signals and proves setup cancellation finishes before its readiness timeout. Generated command help documents the clean shutdown contract.
Scope
This PR is limited to the #6010 intercept shutdown fix. The separate schema-description generator bug discovered during review is tracked by #6035 and implemented in draft #6036; duplicate schema, autoscaler, and generated-reference hunks were removed from this branch.
Fixes #6010 · Part of #4521
Summary by CodeRabbit
New Features
workload interceptnow stops cleanly when interrupted with Ctrl-C, SIGINT, or SIGTERM.Documentation
Bug Fixes