Allow logs to fully flush before disposing of DcpExecutor.

[afscrome]

Description

Updated DcpExecutor to wait for log streams to fully flush during StopAsync, rather than immediately cancelling them. Log streams should shut down once the associated resource is shut down and the logs have been processed.

Previous behaviour could cause logs from the very end of a process to be lost as the log streams could be shut down before the logs were forwarded from DCP to ILogger. This was particularly problematic for DistributedApplicationBuilderTests failing whilst waiting for a resource to enter a particular state as the final lines of error are usually the ones you want to work out why a container / executable exited / failed to start.

Fixes #8206

Checklist

• Is this feature complete?
  • Yes. Ready to ship.
  • No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • Yes
  • No
• Did you add public API?
  • Yes
    • If yes, did you have an API Review for it?
      • Yes
      • No
    • Did you add <remarks /> and <code /> elements on your triple slash comments?
      • Yes
      • No
  • No
• Does the change make any security assumptions or guarantees?
  • Yes
    • If yes, have you done a threat model and had a security review?
      • Yes
      • No
  • No
• Does the change require an update in our Aspire docs?
  • Yes
    • Is this introducing a breaking change?
      • Yes
        • Link to aspire-docs issue (please use this breaking-change template):
      • [] No
        • Link to aspire-docs issue (please use this doc-idea template):
  • No

[danmoseley]

Seems reasonable but it's not my area. Maybe Karol can review

[karolz-ms]

Maybe I am not understanding the change, but I am not sure it has the effect you intended.

If you want to stop DCP and then stop the log streams, you should do just that, i.e. move the call that waits on the log stream tasks to end so that it happens after the call to stop DCP.

The cancellation token passed to StopAsync() is a cancellation token for the whole stopping operation. It may be cancelled after a long time, or never (CancellationToken.None), meaning that with your proposed change the log streams may never be cancelled, depending on what the cancellation token is.

[afscrome]

Sorry my bad, I meant to abandon this PR - see #8206 (comment). Whilst I got my failing test passing, I broke others as I stopped Aspire from shutting down any still running resources - unintended effects as you said.

I assume this needs some changes on DCP side to ensure when DCP is shutting down, it fully flushes logs before finishing the shutdown process (although without seeing DCP code it's hard for me to tell).

[karolz-ms]

Absolutely no worries. We are in huge debt to you for all the help you have given us with making Aspire better.

I think though you were on the right track with this change. Log capture from services and streaming them to clients is asynchronous in DCP, so there is a bit of lag, but it will keep going until the API server shuts down, which is after the resource cleanup is complete. So at least part of the fix IMO should be to stop cancelling the log streams before app host requests DCP shutdown.

I also agree though that the second part might be fine-tuning how we handle the logs during shutdown inside DCP. Specifically, in the current implementation, there is no delay between the end of resource cleanup and the shutdown of the DCP/DCP API server. So there might be cases when logs are captured, resource (Executable or Container) is stopped, but a portion of the logs are not streamed to client before the stream is closed from DCP side because DCP is going away.