Conversation
| { | ||
| try | ||
| { | ||
| var cancellationToken = _jobContext?.CancellationToken ?? CancellationToken.None; |
There was a problem hiding this comment.
this relies on IsActive always implying _jobContext != null which is true right now but feels fragile. if that invariant breaks later this silently hangs the runner. can we just bail early?
| var cancellationToken = _jobContext?.CancellationToken ?? CancellationToken.None; | |
| if (_jobContext == null) | |
| { | |
| Trace.Warning("No job context available, skipping completion pause."); | |
| break; | |
| } | |
| var cancellationToken = _jobContext.CancellationToken; |
| // Pause for debugger inspection, then tear down the DAP session. | ||
| // OnJobCompletedAsync pauses first, then sends terminated/exited | ||
| // events and stops the transport. | ||
| if (_dapDebugger != null) |
There was a problem hiding this comment.
nit: init-failure path nulls _dapDebugger after cleanup but this doesn't. doesn't matter since JobExtension is per-job and StopAsync() handles the real teardown, just looks inconsistent
| catch (Exception ex) | ||
| { | ||
| Trace.Error($"Failed to start DAP debugger: {ex.Message}"); | ||
| AddDebuggerConnectionTelemetry(jobContext, $"Failed: {ex.Message}"); |
There was a problem hiding this comment.
ex.Message going into JobTelemetry, probably fine since the host token goes in auth headers not URLs, but I don't see HostToken registered with SecretMasker either. safer to just log the type:
| AddDebuggerConnectionTelemetry(jobContext, $"Failed: {ex.Message}"); | |
| AddDebuggerConnectionTelemetry(jobContext, $"Failed: {ex.GetType().Name}"); |
full message is already in Trace.Error
salmanmkc
left a comment
salmanmkc
left a comment
There was a problem hiding this comment.
couple tests that'd be nice to have:
- cancellation during
WaitForCommandAsync(not just pre-cancelled) - exception in
OnJobCompletedAsyncduring FinalizeJob
This adds the DAP server setup to the Setup job step, to both help with messaging for users and keep the step as in progress to communicate we're waiting on a debugger to connect.
We also add a final pause during the Complete job step to allow one final inspection after all steps have completed.
https://github.com/github/c2c-actions/issues/9828