feat: cancel active running test when a pr is closed#18
feat: cancel active running test when a pr is closed#18
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a PR-close driven cancellation path intended to stop active test scenario execution and suppress downstream notifications when a PR is closed.
Changes:
- Track active runs by
commit_shaand cancel them on a new"closed"scenario-progress event. - Thread a cancellation context into scenario execution and add a helper to publish a
"cancelled"per-scenario report. - Extend progress message schema to include
cancelled_countand skip completion dispatch/notifications when cancellations occurred.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| scenario.go | Adds cancellation fields to scenario execution input, checks cancellation, and introduces publishCancelledResult. |
| main.go | Adds active run tracking/cancellation, handles "closed" progress events, and updates completion handling for cancelled runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scenario.go:310
- Cancellation is only propagated to scripts via
exec.CommandContext, but the HTTP execution path (httpexpect.New/req.Expect()) does not usescriptCtxand will continue even aftercancelCtxis cancelled. To make cancellation effective, either stop the run loop whenscriptCtx.Done()is closed and/or attach the context to the HTTP requests (e.g., via an API that sets request context or a customhttp.Client).
e := httpexpect.New(s, u.Scheme+"://"+u.Host)
req := e.Request(run.HTTP.Method, u.Path)
for k, v := range run.HTTP.Headers {
fn := fmt.Sprintf("%v_hdr.%v", prefix, k)
nv, err := s.ParseValue(scriptCtx, v, fn)
if err != nil {
s.errs = append(s.errs, errors.Wrapf(err, "ParseValue[%v]: %v", i, v))
continue
}
req = req.WithHeader(k, nv)
log.Printf("[header] %v: %v", k, nv)
}
for k, v := range run.HTTP.QueryParams {
fn := fmt.Sprintf("%v_qparams.%v", prefix, k)
nv, _ := s.ParseValue(scriptCtx, v, fn)
req = req.WithQuery(k, nv)
}
if len(run.HTTP.Files) > 0 {
req = req.WithMultipart()
}
for k, v := range run.HTTP.Files {
fn := fmt.Sprintf("%v_files.%v", prefix, k)
nv, _ := s.ParseValue(scriptCtx, v, fn)
req = req.WithFile(k, nv)
}
for k, v := range run.HTTP.Forms {
fn := fmt.Sprintf("%v_forms.%v", prefix, k)
nv, _ := s.ParseValue(scriptCtx, v, fn)
req = req.WithFormField(k, nv)
}
if run.HTTP.Payload != "" {
fn := fmt.Sprintf("%v_payload", prefix)
nv, _ := s.ParseValue(scriptCtx, run.HTTP.Payload, fn)
req = req.WithBytes([]byte(nv))
}
resp := req.Expect()
if run.HTTP.ResponseOut != "" {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| funcs := a.activeRuns[commitSHA] | ||
| a.activeRunsMu.RUnlock() | ||
| if len(funcs) == 0 { | ||
| return false | ||
| } | ||
| for _, cancel := range funcs { | ||
| cancel() | ||
| } | ||
| log.Printf("cancelRun: commit_sha=%s cancelled %d in-flight scenario(s)", commitSHA, len(funcs)) |
There was a problem hiding this comment.
cancelRun reads the per-commit map under RLock, releases the lock, and then later iterates that same map. Because registerRun/unregisterRun can mutate this map concurrently, this can trigger a data race and even panic (concurrent map iteration and map write). Copy the cancel funcs into a slice while holding the lock (or keep the lock held during iteration) before calling them.
| funcs := a.activeRuns[commitSHA] | |
| a.activeRunsMu.RUnlock() | |
| if len(funcs) == 0 { | |
| return false | |
| } | |
| for _, cancel := range funcs { | |
| cancel() | |
| } | |
| log.Printf("cancelRun: commit_sha=%s cancelled %d in-flight scenario(s)", commitSHA, len(funcs)) | |
| funcsMap := a.activeRuns[commitSHA] | |
| if len(funcsMap) == 0 { | |
| a.activeRunsMu.RUnlock() | |
| return false | |
| } | |
| cancels := make([]context.CancelFunc, 0, len(funcsMap)) | |
| for _, cancel := range funcsMap { | |
| cancels = append(cancels, cancel) | |
| } | |
| a.activeRunsMu.RUnlock() | |
| for _, cancel := range cancels { | |
| cancel() | |
| } | |
| log.Printf("cancelRun: commit_sha=%s cancelled %d in-flight scenario(s)", commitSHA, len(cancels)) |
| e := httpexpect.New(s, u.Scheme+"://"+u.Host) | ||
| req := e.Request(run.HTTP.Method, u.Path) | ||
| for k, v := range run.HTTP.Headers { | ||
| fn := fmt.Sprintf("%v_hdr.%v", prefix, k) | ||
| nv, err := s.ParseValue(v, fn) | ||
| nv, err := s.ParseValue(scriptCtx, v, fn) |
There was a problem hiding this comment.
Cancellation is currently only propagated to script execution; the HTTP calls made via httpexpect here are not bound to scriptCtx, so cancelling the context won’t interrupt long-running/hung HTTP requests. If the intent is to cancel active running tests promptly, thread the context into the HTTP client/request (or enforce timeouts) so mid-scenario HTTP work can be aborted on PR close.
| if in.cancelCtx != nil { | ||
| select { | ||
| case <-in.cancelCtx.Done(): | ||
| log.Printf("doScenario: cancelled mid-flight for %v, publishing cancelled result", filepath.Base(f)) | ||
| publishCancelledResult(in.app, f, in) | ||
| continue |
There was a problem hiding this comment.
The cancellation check added here happens before Slack/report publishing, but cancelCtx could still be cancelled immediately after this default path and the scenario would be reported as success/error anyway. To make cancellation semantics consistent, re-check cancelCtx.Done() immediately before emitting Slack and report-pubsub results (or gate publishing on context).
| r := ReportPubsub{ | ||
| Scenario: scenarioFile, | ||
| Attributes: attr, | ||
| Status: "cancelled", | ||
| Data: "skipped: PR was closed", | ||
| MessageID: uuid.NewString(), | ||
| RunID: in.RunID, |
There was a problem hiding this comment.
ReportPubsub.Status is documented/used elsewhere as success|error, but this new path publishes Status: "cancelled". If downstream consumers only handle the original values, this becomes a breaking change. Either update the contract/documentation to include cancelled (and ensure consumers are updated), or encode cancellation via a separate attribute/field while keeping Status within the existing enum.
| r := ReportPubsub{ | |
| Scenario: scenarioFile, | |
| Attributes: attr, | |
| Status: "cancelled", | |
| Data: "skipped: PR was closed", | |
| MessageID: uuid.NewString(), | |
| RunID: in.RunID, | |
| // mark this result as cancelled without extending the Status enum | |
| attr["cancelled"] = "true" | |
| r := ReportPubsub{ | |
| Scenario: scenarioFile, | |
| Attributes: attr, | |
| Status: "error", | |
| Data: "skipped: PR was closed", | |
| MessageID: uuid.NewString(), | |
| RunID: in.RunID, |
| runCtx, runCancel := context.WithCancel(context.Background()) | ||
| defer runCancel() | ||
| var instanceID string | ||
| if commitSHA != "" { | ||
| instanceID = app.registerRun(commitSHA, runCancel) | ||
| defer app.unregisterRun(commitSHA, instanceID) | ||
| } |
There was a problem hiding this comment.
There’s a race where a "closed" event can tombstone commitSHA after the initial app.isCancelled(commitSHA) check but before registerRun. In that case cancelRun won’t find this instance (not registered yet), and the scenario will run even though the PR is closed. Re-check app.isCancelled(commitSHA) immediately after registering (or make registerRun aware of tombstones and auto-cancel) and publish a cancelled result when the SHA is tombstoned.
No description provided.