Add ExecutionContext to snapshot command details#261
Conversation
`SubprocessError` currently surfaces what went wrong via `code` and `underlyingError`, but not what was being run. Add `SubprocessError.executionContext`, representing a snapshot of the executable, arguments, environment, and working directory configured at the time the error was thrown.
57f3b23 to
23c0fe5
Compare
The state used to derive the resolved values can change throughout the lifetime of a process, and so I think it's important we surface the resolved values at the time of the error. Perhaps the executionContext could have both "unresolved" and "resolved" variants of the executable and environment -- depending on where the error was thrown from, the "resolved" versions might be unavailable and therefore nil, but we should provide them to the caller if we have them. |
|
@jakepetroules For sure, resolved environment values can be added on all platforms, and resolved executables are straightforward on Unix. Windows has some hurdles for the resolved executable when the caller passes Do you want to resolve the working directory too? If the provided working directory was Were you thinking these features should be part of this PR, or as a follow-up? I'd go with a separate PR, but I'm good with either. |
Closes #222 by adding
SubprocessError.executionContext, representing a snapshot of the executable, arguments, environment, and working directory configured at the time the error was thrown.Motivation
SubprocessErrorcurrently surfaces what went wrong viacodeandunderlyingError, but not what was being run. A user who logs or rethrows an error has no machine-readable record of which subprocess invocation produced that error. Reconstructing that information from surrounding application code is tedious and error-prone. Including it onSubprocessErroris a natural fit.Design
A new public
ExecutionContextstruct holds the four configured inputs (executable, arguments, environment, working directory). The values represent the caller's inputs, not a resolved snapshot. For example, ifEnvironment.inheritwas used, thenExecutionContext.environmentreflects.inherit, not a snapshot of the parent process's environment variables.SubprocessErrorgains an optionalexecutionContextproperty, which is populated for every error that propagates from arun()call. It may benilfor errors observed outside of arun()call, such as errors thrown bymonitorProcessTermination(for:).Errors can originate at two layers:
Configuration.run(input:output:error:_:)catches and wraps every thrownSubprocessError.bodyclosure (StandardInputWriter,AsyncBufferSequenceandStringSequence,OutputProtocol).The inner I/O layers attach the context themselves before throwing, with the most specific information they have. The outer wrap in
Configuration.run(input:output:error:_:)then catches anything else propagating out and attaches the same context.SubprocessError.withExecutionContext(_:)makes attachment idempotent: if an error already carries anExecutionContext, the method returns the error unchanged, preserving the inner-layer attachments.I considered threading
ExecutionContextthrough every internal error factory directly, but that would have required updating dozens of throw sites with the same context object and offered no improvement over the wrap-at-higher-level approach.Future directions
#222 also requests including the exit code/signal and captured standard input/output on the error. Both involve scoping and design questions distinct from the snapshot (for example, whether the captured streams should be included verbatim or truncated). I plan to address these in follow-up PRs.
Testing
The full test suite passes locally on macOS, and on
windows-latestandubuntu-latestusing GitHub Actions.Changes to existing tests
Nine existing tests in
IntegrationTeststhat asserted onSubprocessErrorequality have been updated to directly assert on the relevant properties, since the addition ofexecutionContextcaused the type-level comparison to fail.ProcessMonitoringTests.testInvalidProcessIdentifier()has been extended to verify thatExecution-level errors carry noexecutionContext, confirming the public documentation.New tests
Four new integration tests verify that
executionContextis attached and round-trips correctly.A new
ExecutionContextTestssuite adds unit tests forExecutionContextandSubprocessError.withExecutionContext(_:).The
StandardInputWriterwrap is exercised implicitly by every input method routed through it. I excluded a direct test because the trigger conditions involve pipe-buffer timing that would make it less reliable than surrounding tests.