Add 1ES CI for DTFx.AS v2 #1139

davidmrdavid · 2024-07-16T23:12:59Z

The primary goal of this PR is to add a public 1ES ADO testing CI for DTFx.AS v2. This pipeline's results (and a messy development history) can be found here: https://dev.azure.com/azfunc/public/_build?definitionId=881&_a=summary

While migrating our CI pipeline, I encountered previously stable tests becoming flaky, as well as the fact that some tests were previously not running (and were failing once enabled) plus other 1ES-specific idiosyncrasies. This PR aims to resolve them all.

To help with reviews, I'll add comments throughout the diff to clarify why some part of the code had to be changed. I feel comfortable about most changes except for one: the addition of System.Runtime.CompilerServices.Unsafe v6.0.0 as an explicit dependency in DTFx.Core.

Without this change, some DTFx.Core would fail with the following exception trace:

Could not load file or assembly 'System.Runtime.CompilerServices.Unsafe, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. Could not find or load a specific file. (0x80131621)
File name: 'System.Runtime.CompilerServices.Unsafe, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
at System.Diagnostics.ActivityTraceId.SetToRandomBytes(Span1 outBytes)    at System.Diagnostics.ActivitySpanId.CreateRandom()    at DurableTask.Core.TaskOrchestrationDispatcher.ProcessScheduleTaskDecision(ScheduleTaskOrchestratorAction scheduleTaskOrchestratorAction, OrchestrationRuntimeState runtimeState, Boolean includeParameters, Activity parentTraceActivity) in /_/src/DurableTask.Core/TaskOrchestrationDispatcher.cs:line 1016    at DurableTask.Core.TaskOrchestrationDispatcher.OnProcessWorkItemAsync(TaskOrchestrationWorkItem workItem) in /_/src/DurableTask.Core/TaskOrchestrationDispatcher.cs:line 412    at DurableTask.Core.TaskOrchestrationDispatcher.OnProcessWorkItemAsync(TaskOrchestrationWorkItem workItem) in /_/src/DurableTask.Core/TaskOrchestrationDispatcher.cs:line 598    at DurableTask.Core.TaskOrchestrationDispatcher.OnProcessWorkItemSessionAsync(TaskOrchestrationWorkItem workItem) in /_/src/DurableTask.Core/TaskOrchestrationDispatcher.cs:line 211    at DurableTask.Core.WorkItemDispatcher1.ProcessWorkItemAsync(WorkItemDispatcherContext context, Object workItemObj) in /_/src/DurableTask.Core/WorkItemDispatcher.cs:line 374

This exception occurs in the Distributed Tracing path, as you can see by the mention of System.Diagnostics.ActivitySpanId in the stack, which is a type we get from our System.Diagnostics.DiagnosticSource dependency.

I tried researching this, and found this GH thread with the following excerpt:

Unsafe package isn't referenced anymore on netstandard2.0 or net462 which means that consumers of this package now get the transitive Unsafe dependency of System.Memory which is super old (4.5.3 vs 6.0.0).

Since we recently changed DTFx.Core to use only netstandard2.0 (as per https://github.com/Azure/durabletask/pull/1125/files), it's possible this is the reason why this error is only showing up now. Unfortunately, I'm unable to reproduce this locally, but this reproduces pretty consistently on the 1ES CI.

As a relevant aside: Durable Functions v2 has been incompatible with Azure Functions v1 for a while now. We never got to the bottom of that investigation, but I have a rough recollection that the reason for it was an exception like this being thrown in Functions V1. Perhaps that's worth revisiting.

In any case - I need some .NET expertise to help me validate what's the problem here and if the right solution is indeed to add the explicit dependency to CompilerServices.Unsafe v6.x.

Other than that, I'll annotate the PR below. Thanks

See the CI passing for the latest commit here: https://dev.azure.com/azfunc/public/_build/results?buildId=172877&view=results

src/DurableTask.AzureStorage/DurableTask.AzureStorage.csproj

src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs

davidmrdavid · 2024-07-16T23:18:43Z

test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs

@@ -2252,6 +2252,7 @@ public async Task ScheduledStart_Activity_GetStatus_Returns_ScheduledStart(bool
        /// End-to-end test which validates a simple orchestrator function that calls an activity function
        /// and checks the OpenTelemetry trace information
        /// </summary>
+        [TestCategory("DisabledInCI")]


these disabled tests have not been running for a long time, and they're failing in the main branch as well. They weren't running because they're conditionally compiled not to run in NET462, and our previous testing task in ADO was configured to run tests found in the net462 dll.

Now, the test task does discover these tests, but I've disabled them as they aren't passing. @bachuv can you please add this as a To-Fix before Dist. Tracing GA?

Yes, I'll add this task to the list.

davidmrdavid · 2024-07-16T23:18:58Z

test/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj

    <PackageReference Include="Microsoft.WindowsAzure.ConfigurationManager" version="3.2.1" />
    <PackageReference Include="WindowsAzure.Storage" version="8.7.0" />
  </ItemGroup>

  <ItemGroup Condition="'$(TargetFramework)' != 'net462'">
    <PackageReference Include="Azure.Monitor.OpenTelemetry.Exporter" Version="1.0.0-beta.3" />
-    <PackageReference Include="Microsoft.Extensions.Logging" Version="3.1.6" />
-    <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />


newtonsoft.json is inherited by the DTFx.Core project

davidmrdavid · 2024-07-16T23:20:24Z

test/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj

+    <PackageReference Include="Microsoft.Extensions.Logging" Version="2.2.0" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.2.0" />


I added this Logging.Console dependency because I've configured the tests to emit traces to STDOUT, which makes debugging failed ADO tests much easier. I decided to unify the dependencies to version 2.2.0 for simplicy sake and to avoid conditional compilation.

2.2.0 is quite old. Can we standardize to 6.0.0 as part of the v3 change?

I had some issues getting the code to build on 1ES with more recent versions. I'm open to upgrading, but since this is the test project, I'd like to push that to be a separate PR.

davidmrdavid · 2024-07-16T23:20:35Z

test/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj

-    <None Update="testhost.dll.config" Condition="'$(TargetFramework)' == 'netcoreapp2.1'">
-      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
-    </None>


we don't have a netcoreapp2.1 TFM

davidmrdavid · 2024-07-16T23:21:35Z

test/DurableTask.AzureStorage.Tests/TestHelpers.cs

-                // TODO: Add a logger provider so we can collect these logs in memory.
-                LoggerFactory = new LoggerFactory(),
+                LoggerFactory = new LoggerFactory().AddConsole(LogLevel.Trace),
            };
+#pragma warning restore CS0618 // Type or member is obsolete


as mentioned, I've added logging to STDOUT on our tests to facilitate debugging. The current logging library we use is pretty old, which is why we have to disable the warning here. I tried using a newer library, but then had issues with the NET462 TFM. For speed, I decided to use the old library instead, which works.

davidmrdavid · 2024-07-16T23:22:10Z

test/DurableTask.Core.Tests/DurableTask.Core.Tests.csproj

-  <ItemGroup Condition="'$(TargetFramework)' == 'net462'">
-    <PackageReference Include="EnterpriseLibrary.SemanticLogging.EventSourceAnalyzer" Version="2.0.1406.1" />
-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.9.0" />
-    <PackageReference Include="MSTest.TestAdapter" Version="1.4.0" />
-    <PackageReference Include="MSTest.TestFramework" Version="1.4.0" />
-  </ItemGroup>
-


there was no reason for separate dependencies anymore

davidmrdavid · 2024-07-16T23:22:44Z

test/DurableTask.Core.Tests/RetryInterceptorTests.cs

-        [TestMethod]
+        [DataTestMethod]


this was throwing a warning. TestMethods cannot have DataRows, for that you need DataTestMethod instead. The VSTesting task is forgiving enough here, but I changed this to remove the warning

davidmrdavid · 2024-07-16T23:23:32Z

test/DurableTask.Emulator.Tests/DurableTask.Emulator.Tests.csproj

-    <PackageReference Include="MSTest.TestAdapter" Version="3.1.1" />
-    <PackageReference Include="MSTest.TestFramework" Version="3.1.1" />
+    <PackageReference Include="MSTest.TestAdapter" Version="1.4.0" />
+    <PackageReference Include="MSTest.TestFramework" Version="1.4.0" />


We have an inconsistent use of MSTest, some use version 1.4.0 and others use versoin 3.1.1. The use of different frameworks led to warnings. I've downgraded to the common denominator, as I encountered issues upgrading them all to 3.1.1 (can't remember which unfortunately).

davidmrdavid · 2024-07-16T23:23:59Z

test/DurableTask.Test.Orchestrations/SimpleOrchestrations.cs

+            // before sending the event, wait a few seconds to ensure the sub-orchestrator exists
+            // otherwise, we risk a race condition where the event is dicarded because the instances table
+            // does not yet have the sub-orchestrator instance in it.
+            await context.CreateTimer<object>(context.CurrentUtcDateTime.AddSeconds(10), state: null);
+


this test suddenly became flaky. So added this to make it less flakey

This is the right thing to do given the timeline we're working with, but I'm curious to better understand why this particular pattern isn't working reliably.

I think I know the root cause - if the ExternalEvent arrives before the ExecutionStarted event, then the IsExecutableInstance check will return false. We do that here:

durabletask/src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

Lines 1038 to 1060 in d4b98d4

bool IsExecutableInstance(OrchestrationRuntimeState runtimeState, IList<TaskMessage> newMessages, out string message)

{

if (runtimeState.ExecutionStartedEvent == null && !newMessages.Any(msg => msg.Event is ExecutionStartedEvent))

{

var instanceId = newMessages[0].OrchestrationInstance.InstanceId;

if (DurableTask.Core.Common.Entities.AutoStart(instanceId, newMessages))

{

message = null;

return true;

}

else

{

// A non-zero event count usually happens when an instance's history is overwritten by a

// new instance or by a ContinueAsNew. When history is overwritten by new instances, we

// overwrite the old history with new history (with a new execution ID), but this is done

// gradually as we build up the new history over time. If we haven't yet overwritten *all*

// the old history and we receive a message from the old instance (this happens frequently

// with canceled durable timer messages) we'll end up loading just the history that hasn't

// been fully overwritten. We know it's invalid because it's missing the ExecutionStartedEvent.

message = runtimeState.Events.Count == 0 ? "No such instance" : "Invalid history (may have been overwritten by a newer instance)";

return false;

}

So we return "no such instance" as seen above, and we end up discarding the message as it is for an instanceID that never existed. That code is here:

durabletask/src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

Lines 782 to 836 in d4b98d4

if (!this.IsExecutableInstance(session.RuntimeState, orchestrationWorkItem.NewMessages, out string warningMessage))

{

// If all messages belong to the same execution ID, then all of them need to be discarded.

// However, it's also possible to have messages for *any* execution ID batched together with messages

// to a *specific* (non-executable) execution ID. Those messages should *not* be discarded since

// they might be consumable by another orchestration with the same instance id but different execution ID.

var messagesToDiscard = new List<MessageData>();

var messagesToAbandon = new List<MessageData>();

foreach (MessageData msg in session.CurrentMessageBatch)

{

if (msg.TaskMessage.OrchestrationInstance.ExecutionId == session.Instance.ExecutionId)

{

messagesToDiscard.Add(msg);

}

else

{

messagesToAbandon.Add(msg);

}

}

// If no messages have a matching execution ID, then delete all of them. This means all the

// messages are external (external events, termination, etc.) and were sent to an instance that

// doesn't exist or is no longer in a running state.

if (messagesToDiscard.Count == 0)

{

messagesToDiscard.AddRange(messagesToAbandon);

messagesToAbandon.Clear();

}

// Add all abandoned messages to the deferred list. These messages will not be deleted right now.

// If they can be matched with another orchestration, then great. Otherwise they will be deleted

// the next time they are picked up.

messagesToAbandon.ForEach(session.DeferMessage);

var eventListBuilder = new StringBuilder(orchestrationWorkItem.NewMessages.Count * 40);

foreach (MessageData msg in messagesToDiscard)

{

eventListBuilder.Append(msg.TaskMessage.Event.EventType.ToString()).Append(',');

}

this.settings.Logger.DiscardingWorkItem(

this.azureStorageClient.QueueAccountName,

this.settings.TaskHubName,

session.Instance.InstanceId,

session.Instance.ExecutionId,

orchestrationWorkItem.NewMessages.Count,

session.RuntimeState.Events.Count,

eventListBuilder.ToString(0, eventListBuilder.Length - 1) /* remove trailing comma */,

warningMessage);

// The instance has already completed or never existed. Delete this message batch.

await this.DeleteMessageBatchAsync(session, messagesToDiscard);

await this.ReleaseTaskOrchestrationWorkItemAsync(orchestrationWorkItem);

return null;

}

To be clear, the piece where we end up sending external events to the discard list is this particular section:

durabletask/src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

Lines 801 to 810 in d4b98d4

// If no messages have a matching execution ID, then delete all of them. This means all the

// messages are external (external events, termination, etc.) and were sent to an instance that

// doesn't exist or is no longer in a running state.

if (messagesToDiscard.Count == 0)

{

messagesToDiscard.AddRange(messagesToAbandon);

messagesToAbandon.Clear();

}

So it's an ordering issue. In theory, I think we could be more conservative and not discard events immediately, but that can creating poison messages. Please let me know if I'm missing something.

davidmrdavid · 2024-07-16T23:25:09Z

eng/ci/public-build.yml

-            testAssembly: '**\bin\**\netcoreapp3.1\DurableTask.Core.Tests.dll'
+            # TODO: for some reason we're unable to run the net462 tests in the CI
+            testAssembly: '**\bin\Debug\netcoreapp3.1\DurableTask.Core.Tests.dll'


In theory, I want to have the pattern \**\bin\**\DurableTask.Core.Tests.dll here. However, when I input that, 1ES complains that there's not tests to be found in the net462 dll. This makes no sense to me since I can run those tests locally. If I may, I'd like to push for this to remain a "ToDo"

Is the netcoreapp3.1 part of the path necessary? I see you didn't include it in line 90 and 109, further below.

I had netcoreapp3.1 simply because there is a subset of tests in net462 that fail to run on 1ES (again, not locally). It's the tests in test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs.

I've now changed the pattern back to '**\bin\**\DurableTask.Core.Tests.dll' and simply excluded the tests in test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs through and #if !NET462 exclusion. It's hacky - but otherwise the tests fail with the errors seen here: https://dev.azure.com/azfunc/public/_build/results?buildId=173021&view=ms.vss-test-web.build-test-results-tab

cgillum

Overall LGTM. Left a few comments.

I don't know how to explain the need for explicitly adding System.Runtime.CompilerServices.Unsafe, unless perhaps it's related to differences in how binplacing works on these machines. When running locally, do you see this assembly anywhere in your build output on the file system?

cgillum · 2024-07-17T14:35:31Z

test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs

+            // configure logging so traces are emitted during tests.
+            // This facilitates debugging when tests fail.
+
+#pragma warning disable CS0618 // Type or member is obsolete


Can we add comments here with reminders to update these dependencies?

Yes, just did here -> bc9a5ea

cgillum · 2024-07-17T14:39:24Z

test/DurableTask.Test.Orchestrations/SimpleOrchestrations.cs

+            // before sending the event, wait a few seconds to ensure the sub-orchestrator exists
+            // otherwise, we risk a race condition where the event is dicarded because the instances table
+            // does not yet have the sub-orchestrator instance in it.
+            await context.CreateTimer<object>(context.CurrentUtcDateTime.AddSeconds(10), state: null);
+


This is the right thing to do given the timeline we're working with, but I'm curious to better understand why this particular pattern isn't working reliably.

cgillum · 2024-07-17T14:41:13Z

eng/ci/public-build.yml

-            testAssembly: '**\bin\**\netcoreapp3.1\DurableTask.Core.Tests.dll'
+            # TODO: for some reason we're unable to run the net462 tests in the CI
+            testAssembly: '**\bin\Debug\netcoreapp3.1\DurableTask.Core.Tests.dll'


Is the netcoreapp3.1 part of the path necessary? I see you didn't include it in line 90 and 109, further below.

bachuv · 2024-07-17T17:08:48Z

eng/templates/test.yml

Can you explain the changes that were made in this file?

Yes. So previously I had:

- task: VSTest@2 displayName: 'Run tests' inputs: testAssemblyVer2: | ${{ parameters.testAssembly }} !**\obj\** testFiltercriteria: 'TestCategory!=DisabledInCI' vsTestVersion: 17.0 distributionBatchType: basedOnExecutionTime platform: 'any cpu' configuration: 'Debug' diagnosticsEnabled: True collectDumpOn: always rerunFailedTests: true rerunFailedThreshold: 30 rerunMaxAttempts: 3

As you can see, everything below testAssemblyVer2 is at the same level indentation. That's an issue, since testFiltercriteria and the params below are not an input to testAssemblyVer2, instead they're properties that are supposed to be at the same level. So I've indented them back/out so that they're properly parsed by 1ES.

So now I have something equivalent to this (notice the indent)

- task: VSTest@2 displayName: 'Run tests' inputs: testAssemblyVer2: | ${{ parameters.testAssembly }} !**\obj\** testFiltercriteria: 'TestCategory!=DisabledInCI' vsTestVersion: 17.0 distributionBatchType: basedOnExecutionTime platform: 'any cpu' configuration: 'Debug' diagnosticsEnabled: True collectDumpOn: always rerunFailedTests: true rerunFailedThreshold: 30 rerunMaxAttempts: 3

jviau

I believe your assembly load issue is a combo of .netstandard and that we reference System.Diagnostics.DiagnosticSource/5.0.0, but then Azure Storage brings in a higher version. That package depends on the unsafe package, so I guess that is not transitively being upleveled?

Either way, I don't know if referencing Unsafe directly is the answer vs taking this chance to upgrade DiagnosticSource package.

jviau · 2024-07-17T20:33:09Z

src/DurableTask.Core/DurableTask.Core.csproj

@@ -39,6 +39,7 @@
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.2.0" />
    <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
    <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="5.0.1" />
+    <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="6.0.0" />


If we are going v3 with DTFx, I think we should try and just upgrade some packages here:

Microsoft.Extensions.Logging.Abstractions -> 6.0.0
System.Diagnostics.DiagnosticSource -> 6.0.0

See if that fixes your problem

Ah, maybe that's it. I'll try to diagnostics source change first, that could be it

No luck with just upgrading DiagnosticsSource -> https://dev.azure.com/azfunc/public/_build/results?buildId=173059&view=ms.vss-test-web.build-test-results-tab .

Let me try to rev the logging abstractions package as well next

Still no luck that failed too -> https://dev.azure.com/azfunc/public/_build/results?buildId=173069&view=ms.vss-test-web.build-test-results-tab

I'll keep the packages on the 6.x version, and also include the compilerServices.Unsafe pkg

I increased the System.Diagnostics.DiagnosticSource pkg to 6.0.1 and that indeed fixed the issue :-) .
I did opt against increasing the logging abstractions package just because I want to avoid too many transitive dependency changes that may create issues unifying with the rest of bundles. If we have reason to think this is fine, I'm happy to add that in a follow up PR. For now though, I'd like to proceed as-is to unblock a few PRs.

jviau · 2024-07-17T20:34:28Z

test/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj

+    <PackageReference Include="Microsoft.Extensions.Logging" Version="2.2.0" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.2.0" />


2.2.0 is quite old. Can we standardize to 6.0.0 as part of the v3 change?

jviau · 2024-07-17T20:35:24Z

test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs


            this.client = new TaskHubClient(service);
        }

-        [TestCleanup]
-        public async Task TestCleanup()
+        [TestCleanup()]


nit: undo this change

You caught this PR as I was testing some weird stuff here. It's been reverted.

jviau · 2024-07-17T20:36:01Z

test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs

        {
-            await this.worker!.StopAsync(true);
+            this.worker!.StopAsync(true).RunSynchronously();


Where is RunSynchronously from? Typically you sync wait for tasks with .GetAwaiter().GetResult();

As mentioned - You caught this PR as I was testing some weird stuff here. It's been reverted.

davidmrdavid · 2024-07-18T04:16:10Z

src/DurableTask.Core/TaskActivityDispatcher.cs

@@ -23,6 +23,7 @@ namespace DurableTask.Core
    using DurableTask.Core.Logging;
    using DurableTask.Core.Middleware;
    using DurableTask.Core.Tracing;
+    using ActivityStatusCode = Tracing.ActivityStatusCode;


after upgrading DTFx.Core's System.Diagnostics.DiagnosticSource to 6.x, there's some ambiguous definitions here. So I needed to add this alias to disambiguate

davidmrdavid · 2024-07-18T04:17:04Z

src/DurableTask.Core/Tracing/TraceHelper.cs

-                name: activityName,
+                activityName,


after upgrading DTFx.Core's System.Diagnostics.DiagnosticSource to 6.x, these named parameters create ambiguous method invocations here (there's multiple versions of this method with the same parameters in different orders). Using a positional argument here disambiguates the call.

davidmrdavid · 2024-07-18T04:17:30Z

test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs

@@ -10,6 +10,7 @@
 //  See the License for the specific language governing permissions and
 //  limitations under the License.
 //  ----------------------------------------------------------------------------------
+#if !NET462 // for some reasons these tests are not discoverable on 1ES, leading to the test getting aborted. TODO: Needs investigation


FYI @cgillum, @jviau

Do you mean the .net462 flavor of the tests are not discoverable? The .NET Core flavor is working?

test/DurableTask.Core.Tests/app.config

davidmrdavid · 2024-07-18T04:22:16Z

As mentioned here, upgrading System.Diagnostics.DiagnosticSource to 6.0.1 did fix the DLL loading issue.

I'm going to take a bit of optimistic liberty here to speed up some blocked PRs and merged on the basis of the previous approval. Goes without saying I'm happy to incorporate any follow-up feedback that may arise after this. I just would like to unblock some of the PRs that are waiting for this CI to be in place.

So merging now, but please reach out for feedback / follow up requests. Thanks!

cgillum · 2024-07-18T17:13:10Z

The latest changes look good to me. ✅

I had one clarifying question about some tests that were disabled, but it's non-blocking.

davidmrdavid added 6 commits July 13, 2024 07:55

increase timeouts

6807f98

pass 1ES CI

7bbfb20

resolve warning

5341508

Fix indent

dd49cf6

test removing pkg

4bc59ce

re-add dep

b2bd2f9

davidmrdavid commented Jul 16, 2024

View reviewed changes

src/DurableTask.AzureStorage/DurableTask.AzureStorage.csproj Show resolved Hide resolved

davidmrdavid commented Jul 16, 2024

View reviewed changes

src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs Show resolved Hide resolved

davidmrdavid commented Jul 16, 2024

View reviewed changes

cgillum approved these changes Jul 17, 2024

View reviewed changes

davidmrdavid added 3 commits July 17, 2024 09:16

add TODOs

bc9a5ea

testing the removal of netcoreapp3.1 in test

aa77bd5

test method rename

56984ce

bachuv reviewed Jul 17, 2024

View reviewed changes

davidmrdavid added 5 commits July 17, 2024 10:23

try renaming clean up

eef1d03

try conditional compilation

b45e013

simplify test assembly paths

5b7b1d8

test

aa060c2

adapt test clean up and initializers to work better with CI

941decc

jviau reviewed Jul 17, 2024

View reviewed changes

clean out NET462

1e1275d

davidmrdavid added 20 commits July 17, 2024 14:19

try mitigation

15f44df

rev deps

1c0c4d2

rev logging abstractions

b1eec32

add back compilerServices.Unsafe 6.x

345fc27

remove binding redirect in app.config

e791819

keep diagnosticsource low

01ba1b2

increase diagnostics source

896fcc5

clean up

5df3df2

diagnostic source to 6.0.01

dd7c604

rev diagnostic source

f1b0f72

rev Azure.Core

de3761a

rev logging abstractions

4bf3039

rev logging abstractions in azstorage

2e7d199

decrease logging change, try again to run all tests

1dda1ab

add parens

be16e51

simplify signature

cb72151

try v3

1f56a2d

remove parallelism in DT.Core

bd915ec

get tests to pass

4d4dda0

re-enable all partition manager tests

275b3dd

davidmrdavid commented Jul 18, 2024

View reviewed changes

test/DurableTask.Core.Tests/app.config Show resolved Hide resolved

davidmrdavid merged commit e73e75f into azure-storage-v12 Jul 18, 2024
42 checks passed

		<PackageReference Include="Microsoft.Extensions.Logging" Version="2.2.0" />
		<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.2.0" />

	bool IsExecutableInstance(OrchestrationRuntimeState runtimeState, IList<TaskMessage> newMessages, out string message)
	{
	if (runtimeState.ExecutionStartedEvent == null && !newMessages.Any(msg => msg.Event is ExecutionStartedEvent))
	{
	var instanceId = newMessages[0].OrchestrationInstance.InstanceId;

	if (DurableTask.Core.Common.Entities.AutoStart(instanceId, newMessages))
	{
	message = null;
	return true;
	}
	else
	{
	// A non-zero event count usually happens when an instance's history is overwritten by a
	// new instance or by a ContinueAsNew. When history is overwritten by new instances, we
	// overwrite the old history with new history (with a new execution ID), but this is done
	// gradually as we build up the new history over time. If we haven't yet overwritten all
	// the old history and we receive a message from the old instance (this happens frequently
	// with canceled durable timer messages) we'll end up loading just the history that hasn't
	// been fully overwritten. We know it's invalid because it's missing the ExecutionStartedEvent.
	message = runtimeState.Events.Count == 0 ? "No such instance" : "Invalid history (may have been overwritten by a newer instance)";
	return false;
	}

	if (!this.IsExecutableInstance(session.RuntimeState, orchestrationWorkItem.NewMessages, out string warningMessage))
	{
	// If all messages belong to the same execution ID, then all of them need to be discarded.
	// However, it's also possible to have messages for any execution ID batched together with messages
	// to a specific (non-executable) execution ID. Those messages should not be discarded since
	// they might be consumable by another orchestration with the same instance id but different execution ID.
	var messagesToDiscard = new List<MessageData>();
	var messagesToAbandon = new List<MessageData>();
	foreach (MessageData msg in session.CurrentMessageBatch)
	{
	if (msg.TaskMessage.OrchestrationInstance.ExecutionId == session.Instance.ExecutionId)
	{
	messagesToDiscard.Add(msg);
	}
	else
	{
	messagesToAbandon.Add(msg);
	}
	}

	// If no messages have a matching execution ID, then delete all of them. This means all the
	// messages are external (external events, termination, etc.) and were sent to an instance that
	// doesn't exist or is no longer in a running state.
	if (messagesToDiscard.Count == 0)
	{
	messagesToDiscard.AddRange(messagesToAbandon);
	messagesToAbandon.Clear();
	}

	// Add all abandoned messages to the deferred list. These messages will not be deleted right now.
	// If they can be matched with another orchestration, then great. Otherwise they will be deleted
	// the next time they are picked up.
	messagesToAbandon.ForEach(session.DeferMessage);

	var eventListBuilder = new StringBuilder(orchestrationWorkItem.NewMessages.Count * 40);
	foreach (MessageData msg in messagesToDiscard)
	{
	eventListBuilder.Append(msg.TaskMessage.Event.EventType.ToString()).Append(',');
	}

	this.settings.Logger.DiscardingWorkItem(
	this.azureStorageClient.QueueAccountName,
	this.settings.TaskHubName,
	session.Instance.InstanceId,
	session.Instance.ExecutionId,
	orchestrationWorkItem.NewMessages.Count,
	session.RuntimeState.Events.Count,
	eventListBuilder.ToString(0, eventListBuilder.Length - 1) /* remove trailing comma */,
	warningMessage);

	// The instance has already completed or never existed. Delete this message batch.
	await this.DeleteMessageBatchAsync(session, messagesToDiscard);
	await this.ReleaseTaskOrchestrationWorkItemAsync(orchestrationWorkItem);
	return null;
	}

Add 1ES CI for DTFx.AS v2 #1139

Add 1ES CI for DTFx.AS v2 #1139

Conversation

davidmrdavid commented Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmrdavid Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

jviau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmrdavid commented Jul 18, 2024

cgillum commented Jul 18, 2024

davidmrdavid commented Jul 16, 2024 •

edited

Loading

davidmrdavid Jul 17, 2024 •

edited

Loading