Fixes for C# concurrency bugs in 7492

[t0yv0]

Description

• reproduces the problem in a failing test
• tentative fix that passes the test
• fix existing test failures due to wrapping of exception in AggregateException
• add a test that early termination behavior is preserved on Exception
• add a test that description logging behavior is preserved

Fixes #7492 and #6377

Checklist

• I have added tests that prove my fix is effective or that my feature works

• I have updated the CHANGELOG-PENDING file with my change

• Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

[t0yv0]

@orionstudt early preview, I still have some tests to write here.

[mikhailshilkov]

Probably, also fixes #6377 ?

[t0yv0]

Removed the AggregatedException wrapper, it would seem that the special fancy type checks on Exception would not be able to detect what they want to detect if seeing AggregateException that originated from the new code, in LogExceptionToErrorStream (formerly HandleExceptionAsync). Seems like a good idea to preserve the behavior. Let me try to run the CI test suites a few times and see how this last version does on the test suite and determinism.

[t0yv0]

OK folks finally a green checkmark from CI (though you need to restart it a few times to get past Python etc flakes...). Ready for final review!

[mikhailshilkov]

I'm curious why we even allow deploying two stacks.

What does this test prove exactly? It seems that RegisterTask could be a no-op and it would still pass. Do we have a test that makes sure that all the tasks are awaited appropriately?

[t0yv0]

This test failed on master. This is our first test to reproduce the original problem. It was non-deterministic but N=100 was enough to trigger it pretty consistently. Consider it a regression test? I an add a comment about that.

The RunAsync<EmptyStack>() clause indirectly invokes WhileRunningAsync() which is where the bug was. Should I instead make that internal and invoke that directly?

[t0yv0]

Ok this is pretty interesting I investigated a bit further here.

The RunAsync<EmptyStack> is essential to the repro. In fact it seems that I can only repro the exception when more than one stack is being deployed against the same Runner object (and same Deployment object). So to reproduce the issue we need a race of two concurrent WhileRunningAsync() threads on the same runner. In fact even with these changes here, we will not see exceptions, but we might have race conditions in this case of mis-attributing exceptions (or spurious waits). If the single runner is servicing two stacks, it's going to mixup their exceptions.

So taking a big step back. Do we believe it's an error to be running two stacks simultaneously against the single Deployment? If it's an error, how can we best prevent that from happening?

[orionstudt]

I missed this on my review. To @mikhailshilkov 's point I didn't even know this was possible. I don't think a consumer would be able to do this because the access modifiers would prevent them from accessing the Runner. So to answer your question @t0yv0 I don't think you need to prevent it from happening because a consumer currently cannot do this. Their only entrypoint into pulumi execution is the static Deployment.RunAsync(...) methods which ensures that a new Deployment is instantiated.

[orionstudt]

Conceptually I think it is an error. Currently a Deployment is an instance of a pulumi update action against a single stack, and the Runner is an implementation detail of that Deployment - and it is 1:1. I don't think it would ever make sense the way this it is currently architected to have 2 update actions executing against a single Deployment.

[orionstudt]

So the issue is that we need to find a way to repro the original issue which was a flakey race condition caused by access to the inner task dictionary of the Runner? It's always difficult to try to repro a flakey issue but maybe we could instantiate a Runner for testing purposes and just throw N tasks at it? Maybe you don't need a Deployment to repro since this issue was isolated to the Runner

[t0yv0]

Hrm. That could just be a bad test case. Maybe this situation is unrealistic but under some other situations this condition can arise as well. I'm leaning a little to accepting changes here as they make the locking a little more foolproof?

Let me have one more look at CI failures that manifested with the exception we're fixing here.

Also. We could put a little check into WhileRunningAsync to make sure it's never invoked concurrently (one instance at a time), and fail loudly if it does. If we believe it should never happen, but if we manage to do it somehow anyway, that'd catch it.

[orionstudt]

Probably wouldn't hurt anything. Yea when I made that original Automation API PR I specifically refactored the core SDK so that all pulumi execution enters right here, even the Automation API enters there. And you'll see the first thing it does is instantiate a Deployment

[t0yv0]

I've put a PR here with the defensive check #7597 - locally it does not seem to detect it, let's see on CI... Also I locally sporadically get "key not found" in the tests, without triggering the concurrent WRA. So basically I have a poor repro test here, there must be other ways to repro the situation.

[t0yv0]

Yeah that PR basically passes! So far so good.

[t0yv0]

Ah the duplication of exceptions is still present in CI (and can't for the life of me get it locally). OK I need to work through this.

   xUnit.net 00:00:02.08]     Pulumi.Tests.DeploymentRunnerTests.TerminatesEarlyOnException [FAIL]
  X Pulumi.Tests.DeploymentRunnerTests.TerminatesEarlyOnException [47ms]
  Error Message:
   The collection was expected to contain a single element, but it contained more than one element.
  Stack Trace:
     at Pulumi.Tests.DeploymentRunnerTests.TerminatesEarlyOnExceptionStack.<>c__DisplayClass4_0.<.ctor>b__0(Task`1 t) in


   xUnit.net 00:00:17.76]     Pulumi.Tests.DeploymentRunnerTests.TerminatesEarlyOnException [FAIL]
  Failed Pulumi.Tests.DeploymentRunnerTests.TerminatesEarlyOnException [90 ms]
  Error Message:
   Assert.IsType() Failure
Expected: Pulumi.RunException
Actual:   System.AggregateException
  Stack Trace:
     at Pulumi.Tests.DeploymentRunnerTests.TerminatesEarlyOnException() in /Users/runner/work/pulumi/pulumi/sdk/dotnet/Pulumi.Tests/Deployment/DeploymentRunnerTests.cs:line 47
--- End of stack trace from previous location where exception was thrown ---

[t0yv0]

Allright folks I've played with timeouts until I could reliably reproduce exception duplication locally, and it's an artifact of badly written test. Here's what was happening exactly:

1. Stack had outputs, and was participating in void IDeploymentInternal.RegisterResourceOutputs(Resource resource, Output<IDictionary<string, object?>> outputs), which is a registered task also (with the runner)

2. RegisterResourceOutputs task was failing with "Deliberate test error", so it was reported via the runner twice, once from the inner task and once from RegisterResourceOutputs

3. Normally, error deduplication works just fine to remove the dup reporting here; however since the test was using EmptyStack, it introduced a race with two WhileRunningAsync instances active on the same runner; under certain circumstances the two instances grabbed one copy of the exception each, so deduplication did not apply

4. Based on conversation here and aux PR we believe WhileRunningAsync never runs more than one copy, so it was a borked test

5. Last version of test does not have this problem

[t0yv0]

Allright, sorry for many detours here.. So my repro was bad, but I think this change still helps. On master I get sporadic test failure. With the change the tests are deterministic for me.

@mikhailshilkov please take another look? Appreciate it.

[mikhailshilkov]

Your changes LGTM. Just one question about the test.