Repository navigation
Conversation
Nothing on the agent-shim path handled signals, so Ctrl-C, SIGTERM, or SIGHUP during setup terminated the process before the failure event was logged or uploaded, and USER_ABORTED was unreachable. Cancel the context on the first signal instead so the in-flight step aborts and the event is reported. Co-authored-by: Isaac <no-reply@databricks.com>
agent-shim needs to cancel on the first signal and then restore the previous disposition, so a second one gets its usual handling. environments setup-local and air each carry a near-copy of that logic. Add one helper in libs/process, modelled on signal.NotifyContext, and use it in place of agent-shim's own copy. The second-signal regression test from environments is copied alongside it, now covering SIGINT, SIGTERM, and SIGHUP. Moving environments and air over to the helper is left to a follow-up. Co-authored-by: Isaac <no-reply@databricks.com>
A signal that arrived while a step ignored ctx (warm bootstrap, file I/O) was swallowed and the agent launched anyway. Restore the default signal handling before committing to the launch, so a later signal kills the shim as it would without a handler, then check whether a signal already came in. Checking first would leave a window in which a signal is consumed by the handler and the agent still launches. stopSignals cancels ctx, so process.NotifyContext now cancels with a *SignalError cause on a signal, and its stop still reports a signal that arrived before it. The success event uses the caller's context. probeAIGateway now returns the cancellation before classifying its results, so an interrupted probe is no longer reported as a disabled gateway or a transient error to retry. Co-authored-by: Isaac <no-reply@databricks.com>
Cover a probe run on a cancelled context, and a signal during a warm-path bootstrap step that swallows the cancellation: the shim must not exec the agent and must report USER_ABORTED. Isolate HOME and provide the exec ID and runtime detection cmd/root sets up, so a regression fails by launching the stub agent rather than by panicking earlier. Co-authored-by: Isaac <no-reply@databricks.com>
394ff7b to
2006cfc
Compare
Integration test reportCommit: 2006cfc
Top 7 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Two cancellation paths still lose or misclassify USER_ABORTED telemetry. Both reproduced against 2006cfc. The affected package tests and focused race-detector checks pass.
| // late signals get the default handling again. | ||
| parentCtx := ctx | ||
| ctx, stopSignals := process.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP) | ||
| defer stopSignals() |
There was a problem hiding this comment.
[P2] Join the signal handler before classifying failures
The failure telemetry defer runs before stopSignals(), so it can read ctx.Err() while the signal goroutine is still processing an interrupt. If an installer exits at the same time, cmd.Run() can return first and the event gets the install failure category.
I reproduced this with a fake uv that runs kill -INT "$PPID" followed by exit 1. With GOMAXPROCS=1, iteration 77 uploaded UNITY_GATEWAY_CLI_INSTALL_FAILED instead of USER_ABORTED. The handler is joined after the incorrect event has already been buffered.
We should stop/join the handler before building the failure outcome, then use the signal cause and parent context to distinguish an interrupt from the cancellation caused by stopSignals() itself.
| // the launch is committed, | ||
| // late signals get the default handling again. | ||
| parentCtx := ctx | ||
| ctx, stopSignals := process.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP) |
There was a problem hiding this comment.
[P2] Cancel installer descendants before waiting for output
The shim now intercepts the first SIGTERM/SIGHUP, but cancellation in runCommand still uses exec.CommandContext's default behavior, which kills only the direct child. If uv tool install has a descendant holding stdout or stderr open, cmd.Run() keeps waiting for EOF with no WaitDelay. The shim doesn't return or upload the USER_ABORTED event until that descendant exits.
I reproduced this with a fake uv that backgrounds a sleep, sends SIGTERM to the shim, then waits. The shim stayed blocked after cancellation and returned only when I manually killed the grandchild.
We should cancel the installer process group and bound the wait, including cleanup of surviving descendants. The existing process.WithProcessGroup() support covers this behavior.
This branch has not been deployed
4 participants
Changes
Addresses this comment: #6883
Previously
agent-shimtelemetry did not accurately report the error category when a user cancelled viaCtrl-CThis PR also adds
libs/process/signal.gowhich is the existing signal handling code used byairandenvironments setup-localmoved to a shared place. Refactoring to makeairandenvironments setup-localuse the new shared lib lives in #7053Why
So error categories are accurate
Tests
Added tests for new signal handling