Propagate signal cancellation through the native run context - #148
Conversation
Wire a signal-derived context at the command boundary (SIGINT/SIGTERM) into bench.Run, workload setup/iteration/teardown, driver readiness waits, and load workers as one cancellation tree. - First signal cancels the root context for graceful teardown. - A second signal forces immediate exit via a bounded escape hatch. - Exit statuses: 130 on graceful cancellation, 1 on forced exit. - Replace the unused NewQuitSignal helper with shutdown.NotifyContext. Refs #135
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSIGINT and SIGTERM now cancel command execution and trigger graceful workload teardown. A second signal forces exit with status 2. Cancellation propagates through Go workloads, benchmark execution, workers, and SQL operations. Exit statuses distinguish signal cancellation from other errors. ChangesSignal cancellation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change propagates cancellation through command execution and documents graceful and forced interruption behavior; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant OS
participant RootCommand
participant RunCommand
participant bench.Run
participant WorkloadTeardown
OS->>RootCommand: SIGINT or SIGTERM
RootCommand->>RunCommand: Cancel command context
RunCommand->>bench.Run: Propagate canceled context
bench.Run->>WorkloadTeardown: Run detached teardown
WorkloadTeardown-->>bench.Run: Return teardown result
bench.Run-->>RootCommand: Return combined error
RootCommand-->>OS: Exit with signal-derived status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Run workload Teardown exactly once from a defer under a fresh timeout context, so graceful cancellation still performs schema cleanup. - Prevent the use-after-stop race: drain buffered signals and gate force on a stopped flag so a fast double-Ctrl-C cannot turn a clean exit into a forced one. - Derive the graceful exit status from the first signal (130 SIGINT, 143 SIGTERM) and use 2 as the distinct forced-exit status. - Fix exit-status read ordering so the signal is queried after the run returns. - Add exit-code mapping and teardown-on-cancel tests; move the changelog line to Changed. Refs #135
Extract magic-number constants (128 signal base, 2 signal buffer size), name NotifyContext results, annotate the bounded int->int32 signal conversion, and reflow test blank lines for wsl.
Arbitrate forced exit against shutdown atomically, keep generic cancellations at exit status 1, preserve caller values during teardown, and allow successful Execute callers to run deferred cleanup.
Closes #135
What
The native command path installed no signal-derived cancellation context:
pkg/common/shutdown.NewQuitSignalhad no production caller, so SIGINT/SIGTERM never reached Cobra,bench.Run, workload setup/iterations, driver queries/transactions, or insert drains.This change creates one cancellation context at the command boundary and passes it through every operation as one tree.
Approach
cmd/stroppy/commands/root.gowires a signal-derived context inExecute(). It usesshutdown.NotifyContextand runs Cobra viarootCmd.ExecuteContext.pkg/common/shutdowngainsNotifyContext(parent, force)replacing the disconnectedNewQuitSignal. The first SIGINT/SIGTERM cancels the context (graceful teardown); a second signal after cancellation invokesforce(ForcedExitCode)as a bounded escape hatch.stop()releases the OS handler and cancels the context.cmd/stroppy/commands/run/run.gopassescmd.Context()throughrunGoWorkloadintobench.Run, instead ofcontext.Background().The context already threads through
bench.Run→driver.Dispatch→ each driver'sNewDriver/WaitForDB, and through workloadSetup/Iterate/Teardowninto query/tx APIs andcommon.RunParallelBatchload workers, so the command boundary was the only missing link.Exit statuses
130— graceful cancellation after the first SIGINT/SIGTERM (128 + SIGINT).1— forced exit after a second signal during teardown.Documented in
stroppy run --help(Signals section) and the changelog.Tests
pkg/common/shutdown/signal_test.go— cancel-then-force loop, stop releases the handler, and real SIGINT/SIGTERM delivery.pkg/driver/sqldriver/cancel_test.go— cancellation during the driver readiness wait (WaitForDB) and during a blocked query (RunQuery).pkg/bench/cancel_test.go— cancellation reaching workloadSetup(schema/load) and stopping a fixed-duration (constant-vus) scenario without leaked workers (timeout-guarded).All fake drivers / contexts, no live DB.
Acceptance criteria
bench.Run, workload methods, query/transaction APIs, driver readiness waits, and all load workers.Summary by CodeRabbit
New Features
Documentation
Bug Fixes