fix: disambiguate signal-killed errors from context cancellation - #896
Merged
Conversation
CommandDevContainer now attaches ctx.Err() to a killed docker-exec error so cancellation-driven kills (e.g. a lifecycle hook cut short by a canceled/timed-out context) are no longer indistinguishable from an opaque "signal: killed". RunWithResult's ctx.Done() branch used to unconditionally return (t.result, nil), silently reporting success even when the tunnel was canceled before any result ever arrived. It now only does that when a result was actually received via SendResult, otherwise it surfaces ctx.Err(). Access to the shared result field is now mutex-guarded since it's written concurrently by the gRPC server's SendResult handler.
✅ Deploy Preview for devsydev canceled.
|
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSetup, build, and workspace startup now use centralized tunnel result reporting. Tunnel execution preserves stored results during cancellation. Docker command failures now include context cancellation errors. ChangesResult reporting and cancellation handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant ReportResult
participant ResultCallback
participant TunnelClient
Command->>ReportResult: execute callback
ReportResult->>ResultCallback: run setup, build, or up operation
ResultCallback-->>ReportResult: return result and execution error
ReportResult->>TunnelClient: send serialized result
TunnelClient-->>ReportResult: return transport status
ReportResult-->>Command: return callback or send error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
✅ Deploy Preview for images-devsy-sh canceled.
|
CommandDevContainer now attaches ctx.Err() to a killed docker-exec error, so a cancellation-driven kill reads as "context canceled" or "context deadline exceeded" instead of an opaque "signal: killed". Add tunnelserver.ReportResult, a shared helper that guarantees a remote agent command's outcome is sent over the tunnel via SendResult exactly once, success or failure, using a context independent of the command's own (possibly cancelled) context. up, build, and setup all now funnel through it instead of each maintaining its own copy of this logic, closing gaps where an early-return path (setup's prepareWorkspace) or a command with no result at all (build) never reported completion. With every RunWithResult caller now participating in this explicit-completion contract, its ctx.Done() branch can honestly report ctx.Err() when no result ever arrived, instead of silently treating an interrupted run as success. Access to the tunnel server's shared result field is now mutex-guarded, since it's written concurrently by the gRPC server's SendResult handler while RunWithResult reads it.
skevetter
marked this pull request as ready for review
August 7, 2026 05:33
skevetter
marked this pull request as draft
August 7, 2026 05:35
CommandDevContainer (docker exec, used for lifecycle hooks) was the only call site wrapping a killed docker subprocess with ctx.Err(). The machine-provider e2e flake showed the same bare "signal: killed" coming from a different, unwrapped call site (docker pull/create/ start), which happens before any container exists and never goes through CommandDevContainer. Move the disambiguation to DockerHelper's actual choke point: every plain cmd.Run() (Pull, RunWithDir, RunWithEnv, GetContainerLogs) now goes through a shared runCmd helper that attaches ctx.Err() when present. CommandDevContainer's own wrapping is simplified since the context is now supplied by Docker.Run itself, avoiding double-wrapping.
skevetter
marked this pull request as ready for review
August 7, 2026 06:36
skevetter
marked this pull request as draft
August 7, 2026 06:37
skevetter
marked this pull request as ready for review
August 7, 2026 06:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/internal/agentworkspace/build.go (1)
88-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
cancelCtxinstead ofctxfor consistency.Line 83 creates the runner from
cancelCtx, and Line 70 initializes the workspace withcancelCtx. The build callback now runs under the outerctx. The two contexts behave the same today becausecancelis only deferred, but the mismatch invites a divergence later.♻️ Proposed change
_, err = tunnelserver.ReportResult( - ctx, + cancelCtx, tunnelClient, func(ctx context.Context) (*config2.Result, error) { return nil, buildAndPushImages(ctx, runner, workspaceInfo) }, )🤖 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 `@cmd/internal/agentworkspace/build.go` around lines 88 - 93, Update the ReportResult callback in the build flow to pass cancelCtx to buildAndPushImages instead of ctx. Keep the existing runner and workspace initialization context usage consistent with this callback.pkg/agent/tunnelserver/result_reporter.go (2)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ReportResultmutates the caller's result struct.Line 26 aliases
result, and Line 31 writestoSend.Error. Iffnreturns a non-nil result together with an error and an emptyErrorfield,ReportResultmodifies the struct that the caller still owns and receives back. Copy the struct before you set the synthesized error to keep the function free of side effects.♻️ Proposed change to avoid mutating the caller's struct
- toSend := result - if toSend == nil { - toSend = &config.Result{} - } - if err != nil && toSend.Error == "" { + toSend := &config.Result{} + if result != nil { + copied := *result + toSend = &copied + } + if err != nil && toSend.Error == "" { toSend.Error = err.Error() }🤖 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/agent/tunnelserver/result_reporter.go` around lines 26 - 32, Update ReportResult to copy the non-nil result value before assigning the synthesized error, while retaining the existing empty Result allocation for nil inputs. Apply the err-derived Error update only to the copy so the caller-owned result remains unchanged.
16-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA panic in
fnbypasses the reporting guarantee.The doc comment states that the outcome is sent "exactly once ... whether fn succeeds or fails". A panic inside
fnskips the send, so the host receives no result and waits for the tunnel to close. Add a deferred recover that converts the panic into an error, or narrow the doc comment to returned errors only.🤖 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/agent/tunnelserver/result_reporter.go` around lines 16 - 24, Update ReportResult to recover panics from fn, convert the recovered value into an error, and continue through the existing SendResult path so exactly one outcome is reported before returning. Preserve the current handling for normal results and returned errors, and keep the documented guarantee accurate.pkg/agent/tunnelserver/tunnelserver_test.go (1)
77-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the server-error branch of
RunWithResult.The new tests cover the
ctx.Done()branch in both states. TheerrChanbranch at Line 151 oftunnelserver.goalso returns a stored result now. A test that closes the pipe to forces.Serveto return, with a result already set, would lock in that behavior.🤖 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/agent/tunnelserver/tunnelserver_test.go` around lines 77 - 129, Add a test for the server-error path in RunWithResult, alongside the existing cancellation tests, that sets a result via srv.setResult, closes the pipe to make Serve return, and verifies RunWithResult returns the stored result together with the server error.
🤖 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 `@cmd/internal/agentcontainer/setup.go`:
- Around line 111-121: Update the callback passed to tunnelserver.ReportResult
so its context is not silently ignored: either assign the callback’s ctx to the
scoped context used by prepareWorkspace and finalizeSetup through sctx, or
explicitly name the callback parameter _ if sctx.ctx is intentionally
authoritative. Ensure setup cancellation and timeout behavior remains consistent
on callback errors.
In `@pkg/docker/helper_test.go`:
- Around line 283-297: Update TestRunCmd_AttachesCtxErrOnFailure so the fake
Docker script signals readiness and then blocks, allowing the test to wait for
that signal before canceling the context. Keep h.Run running until cancellation,
then assert the returned error includes context.Canceled, thereby exercising the
command-killed path rather than pre-start cancellation.
In `@pkg/docker/helper.go`:
- Around line 245-249: Update the command execution flow around cmd.Run to track
whether context cancellation actually terminated the process, and only wrap the
command error with ctx.Err() when that cancellation path caused termination. Do
not use a post-run ctx.Err() check alone; preserve the original Docker error
when cancellation occurs concurrently with an independent command failure.
In `@pkg/driver/docker/lifecycle_test.go`:
- Around line 213-222: Update the readiness-wait goroutine around ready so it
records whether os.Stat observed the readiness marker before the deadline; if
readiness is not observed, fail the test immediately instead of only calling
cancel(), while preserving cancellation after successful readiness detection.
---
Nitpick comments:
In `@cmd/internal/agentworkspace/build.go`:
- Around line 88-93: Update the ReportResult callback in the build flow to pass
cancelCtx to buildAndPushImages instead of ctx. Keep the existing runner and
workspace initialization context usage consistent with this callback.
In `@pkg/agent/tunnelserver/result_reporter.go`:
- Around line 26-32: Update ReportResult to copy the non-nil result value before
assigning the synthesized error, while retaining the existing empty Result
allocation for nil inputs. Apply the err-derived Error update only to the copy
so the caller-owned result remains unchanged.
- Around line 16-24: Update ReportResult to recover panics from fn, convert the
recovered value into an error, and continue through the existing SendResult path
so exactly one outcome is reported before returning. Preserve the current
handling for normal results and returned errors, and keep the documented
guarantee accurate.
In `@pkg/agent/tunnelserver/tunnelserver_test.go`:
- Around line 77-129: Add a test for the server-error path in RunWithResult,
alongside the existing cancellation tests, that sets a result via srv.setResult,
closes the pipe to make Serve return, and verifies RunWithResult returns the
stored result together with the server error.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 774a9afb-0047-4978-a623-114f8ff44b11
📒 Files selected for processing (11)
cmd/internal/agentcontainer/setup.gocmd/internal/agentworkspace/build.gocmd/internal/agentworkspace/up.gopkg/agent/tunnelserver/result_reporter.gopkg/agent/tunnelserver/result_reporter_test.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/tunnelserver/tunnelserver_test.gopkg/docker/helper.gopkg/docker/helper_test.gopkg/driver/docker/lifecycle.gopkg/driver/docker/lifecycle_test.go
Comment on lines
+213
to
+222
| go func() { | ||
| deadline := time.Now().Add(time.Second) | ||
| for time.Now().Before(deadline) { | ||
| if _, err := os.Stat(ready); err == nil { | ||
| break | ||
| } | ||
| time.Sleep(time.Millisecond) | ||
| } | ||
| cancel() | ||
| }() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when the Docker exec command does not start.
The timeout path calls cancel() even when ready was never created. The test can then pass with context.Canceled from an earlier operation and does not prove cancellation occurred during Docker.Run.
Signal test failure when readiness is not observed before the deadline.
🤖 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/driver/docker/lifecycle_test.go` around lines 213 - 222, Update the
readiness-wait goroutine around ready so it records whether os.Stat observed the
readiness marker before the deadline; if readiness is not observed, fail the
test immediately instead of only calling cancel(), while preserving cancellation
after successful readiness detection.
skevetter
marked this pull request as draft
August 7, 2026 07:09
- runCmd now hooks cmd.Cancel to record whether os/exec itself acted on this ctx, instead of checking ctx.Err() after cmd.Run() returns, which could race an unrelated concurrent cancellation against the command's own independent failure. - ReportResult no longer mutates the caller-owned *config.Result when synthesizing an Error field; it copies first. - ReportResult recovers a panic in its job function so its "exactly once" completion guarantee holds even when the job crashes outright. - build.go passes cancelCtx (matching its sibling calls) instead of the outer ctx to ReportResult. - setup.go's ReportResult callback no longer shadows an unused ctx parameter; sctx.ctx is already the authoritative context there.
Code should be self-documenting; per CodeRabbit review feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CommandDevContainer(docker exec, used for lifecycle hooks) now gets itsctx.Err()disambiguation fromDocker.Runitself rather than wrapping locally — a cancellation-driven kill reads as "context canceled/deadline exceeded" instead of an opaquesignal: killed.DockerHelper(Pull,RunWithDir,RunWithEnv,GetContainerLogs) via a sharedrunCmdhelper, since a separate e2e flake showed the identical baresignal: killedcoming from an unwrapped call site (image pull / container create-start) that happens before any container exists and never goes throughCommandDevContainer.tunnelserver.ReportResult, a shared helper that guarantees a remote agent command's outcome is sent over the tunnel viaSendResultexactly once, success or failure, using a context independent of the command's own (possibly cancelled) context.up,build, andsetupnow funnel through it instead of each maintaining its own copy of this logic, closing gaps where an early-return path (setup'sprepareWorkspace) or a command with no result at all (build) never reported completion.RunWithResultcaller now participating in this explicit-completion contract, itsctx.Done()branch honestly reportsctx.Err()when no result ever arrived, instead of silently treating an interrupted run as success.Run()(used only by the fire-and-forget services tunnel, which never sends a result by design) treats that same cancellation as expected shutdown.SendResulthandler whileRunWithResultreads it.Investigated via
superpowers:systematic-debuggingstarting from flaky CI failures (devsy up failed: ... start workspace: signal: killed) recurring across different e2e suites (build, run-user-commands, machine-provider); reviewed with CodeRabbit CLI (0 findings).Summary by CodeRabbit
Bug Fixes
Reliability