From 068bf52e708bcf511399fd321b5a506aed3662c0 Mon Sep 17 00:00:00 2001 From: "naiyuantian@microsoft.com" Date: Thu, 16 Jul 2026 20:36:19 -0700 Subject: [PATCH 1/3] initial commit --- azurefunctions/build.gradle | 1 + .../middleware/ActivityMiddleware.java | 331 ++++++++++++++++++ ...nctions.internal.spi.middleware.Middleware | 3 +- .../middleware/ActivityMiddlewareTest.java | 286 +++++++++++++++ .../TestExceptionPropertiesProvider.java | 23 ++ .../PROTO_SOURCE_COMMIT_HASH | 2 +- .../protos/orchestrator_service.proto | 2 +- 7 files changed, 645 insertions(+), 3 deletions(-) create mode 100644 azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java create mode 100644 azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java create mode 100644 azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/TestExceptionPropertiesProvider.java diff --git a/azurefunctions/build.gradle b/azurefunctions/build.gradle index 4d99103..5563b5b 100644 --- a/azurefunctions/build.gradle +++ b/azurefunctions/build.gradle @@ -40,6 +40,7 @@ dependencies { compileOnly "com.microsoft.azure.functions:azure-functions-java-spi:1.1.0" // Test dependencies + testImplementation "com.microsoft.azure.functions:azure-functions-java-spi:1.1.0" testImplementation 'org.mockito:mockito-core:5.21.0' testImplementation 'org.mockito:mockito-junit-jupiter:5.21.0' testImplementation platform('org.junit:junit-bom:5.14.2') diff --git a/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java b/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java new file mode 100644 index 0000000..5d6721c --- /dev/null +++ b/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java @@ -0,0 +1,331 @@ +/** + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for + * license information. + */ + +package com.microsoft.durabletask.azurefunctions.internal.middleware; + +import com.microsoft.azure.functions.internal.spi.middleware.Middleware; +import com.microsoft.azure.functions.internal.spi.middleware.MiddlewareChain; +import com.microsoft.azure.functions.internal.spi.middleware.MiddlewareContext; +import com.microsoft.durabletask.ExceptionPropertiesProvider; + +import java.lang.reflect.InvocationTargetException; +import java.util.Iterator; +import java.util.Map; +import java.util.ServiceLoader; +import java.util.function.Supplier; +import java.util.logging.Logger; + +/** + * Durable Function Activity Middleware. + * + *

When an activity function throws, this middleware gives a registered + * {@link ExceptionPropertiesProvider} the chance to attach custom properties to the failure. If the + * provider returns any properties, the exception is reshaped into a serialized + * {@code TaskFailureDetails} JSON payload (matching the protobuf JSON shape) so the Durable Task + * host extension can surface the structured properties on {@code FailureDetails.Properties}. This + * mirrors the {@code durable-functions} JavaScript SDK's activity handler wrapper. + * + *

If no provider is registered, or it yields no properties for the thrown exception, the original + * exception is re-thrown untouched so the legacy failure behavior is preserved. + * + *

The provider is discovered via {@link ServiceLoader} (SPI): an application registers its + * implementation in {@code META-INF/services/com.microsoft.durabletask.ExceptionPropertiesProvider}. + * + *

This class is internal and is hence not for public use. Its APIs are unstable and can change + * at any time. + */ +public class ActivityMiddleware implements Middleware { + + private static final String ACTIVITY_TRIGGER = "DurableActivityTrigger"; + private static final int MAX_INNER_FAILURE_DEPTH = 10; + private static final Logger LOGGER = Logger.getLogger(ActivityMiddleware.class.getName()); + + private static final Object PROVIDER_LOCK = new Object(); + private static volatile boolean providerLoaded = false; + private static ExceptionPropertiesProvider cachedProvider; + + // Visible for testing only. When non-null, this supplier replaces SPI discovery so unit tests + // can exercise the reshaping and pass-through behavior without registering a real provider. + private static Supplier providerSupplierOverride; + + @Override + public void invoke(MiddlewareContext context, MiddlewareChain chain) throws Exception { + String parameterName = context.getParameterName(ACTIVITY_TRIGGER); + if (parameterName == null) { + chain.doNext(context); + return; + } + + try { + chain.doNext(context); + } catch (Exception e) { + ExceptionPropertiesProvider provider = getProvider(); + if (provider == null) { + throw e; + } + + Throwable userException = unwrap(e); + Map properties = safeGetProperties(provider, userException); + if (properties == null || properties.isEmpty()) { + // No custom properties for this failure - preserve the original behavior. + throw e; + } + + throw new StructuredActivityFailure(buildFailureDetailsJson(userException, provider)); + } + } + + private static ExceptionPropertiesProvider getProvider() { + if (!providerLoaded) { + synchronized (PROVIDER_LOCK) { + if (!providerLoaded) { + cachedProvider = providerSupplierOverride != null + ? providerSupplierOverride.get() + : discoverProvider(); + providerLoaded = true; + } + } + } + return cachedProvider; + } + + private static ExceptionPropertiesProvider discoverProvider() { + // The provider is registered via SPI in the function app's jar. Depending on how the + // Azure Functions Java worker dispatches invocations, the thread context class loader may + // be the worker's class loader (which cannot see the app's META-INF/services registration) + // rather than the app class loader. Try several candidate class loaders and use the first + // one that yields a provider. The class loader that loaded this middleware is bundled with + // the app (durabletask-azure-functions is an app dependency), so it can see the app's SPI + // registration and is the most reliable fallback. + return discoverProvider(new ClassLoader[] { + Thread.currentThread().getContextClassLoader(), + ActivityMiddleware.class.getClassLoader(), + ExceptionPropertiesProvider.class.getClassLoader(), + }); + } + + // Visible for testing. Iterates the candidate class loaders in order and returns the first + // provider discovered via SPI, skipping nulls and duplicates. This is the seam that guards + // against the worker-thread class loader regression: discovery must not stop at the (possibly + // provider-blind) thread context class loader. + static ExceptionPropertiesProvider discoverProvider(ClassLoader[] candidates) { + ClassLoader previous = null; + for (ClassLoader classLoader : candidates) { + if (classLoader == null || classLoader == previous) { + continue; + } + previous = classLoader; + try { + ServiceLoader loader = + ServiceLoader.load(ExceptionPropertiesProvider.class, classLoader); + Iterator iterator = loader.iterator(); + if (iterator.hasNext()) { + return iterator.next(); + } + } catch (Throwable t) { + // Discovery failures must not break activity execution; the feature is opt-in. + LOGGER.warning("Failed to load ExceptionPropertiesProvider via ServiceLoader using " + + classLoader + ": " + t); + } + } + return null; + } + + // Visible for testing. Overrides SPI discovery with the given supplier (may be {@code null} to + // simulate "no provider registered") and clears the cached provider so the next lookup re-runs. + static void setProviderSupplierForTesting(Supplier supplier) { + synchronized (PROVIDER_LOCK) { + providerSupplierOverride = supplier; + providerLoaded = false; + cachedProvider = null; + } + } + + // Visible for testing. Restores real SPI discovery and clears any cached provider so tests do + // not leak state into one another (the provider is cached in a static field). + static void resetProviderCacheForTesting() { + synchronized (PROVIDER_LOCK) { + providerSupplierOverride = null; + providerLoaded = false; + cachedProvider = null; + } + } + + private static Throwable unwrap(Throwable e) { + Throwable current = e; + while (current instanceof InvocationTargetException && current.getCause() != null) { + current = current.getCause(); + } + return current; + } + + private static Map safeGetProperties( + ExceptionPropertiesProvider provider, + Throwable exception) { + if (!(exception instanceof Exception)) { + return null; + } + try { + return provider.getExceptionProperties((Exception) exception); + } catch (Exception providerException) { + // Don't let a misbehaving provider mask the original failure. + LOGGER.warning("ExceptionPropertiesProvider threw while extracting properties: " + providerException); + return null; + } + } + + // Builds the single-line JSON payload that mirrors the protobuf TaskFailureDetails shape + // consumed by the Durable Task host extension. + private static String buildFailureDetailsJson(Throwable exception, ExceptionPropertiesProvider provider) { + StringBuilder sb = new StringBuilder(256); + appendFailure(sb, exception, provider, 0); + return sb.toString(); + } + + private static void appendFailure( + StringBuilder sb, + Throwable exception, + ExceptionPropertiesProvider provider, + int depth) { + sb.append('{'); + sb.append("\"errorType\":"); + appendString(sb, exception.getClass().getName()); + sb.append(",\"errorMessage\":"); + appendString(sb, exception.getMessage() != null ? exception.getMessage() : ""); + sb.append(",\"stackTrace\":"); + appendString(sb, getFullStackTrace(exception)); + sb.append(",\"isNonRetriable\":false"); + + Map properties = safeGetProperties(provider, exception); + if (properties != null && !properties.isEmpty()) { + sb.append(",\"properties\":"); + appendValue(sb, properties); + } + + Throwable cause = exception.getCause(); + if (cause != null && cause != exception && depth < MAX_INNER_FAILURE_DEPTH) { + sb.append(",\"innerFailure\":"); + appendFailure(sb, cause, provider, depth + 1); + } + + sb.append('}'); + } + + @SuppressWarnings("unchecked") + private static void appendValue(StringBuilder sb, Object value) { + if (value == null) { + sb.append("null"); + } else if (value instanceof String) { + appendString(sb, (String) value); + } else if (value instanceof Boolean) { + sb.append(((Boolean) value) ? "true" : "false"); + } else if (value instanceof Double || value instanceof Float) { + double d = ((Number) value).doubleValue(); + if (Double.isNaN(d) || Double.isInfinite(d)) { + appendString(sb, value.toString()); + } else { + sb.append(value.toString()); + } + } else if (value instanceof Number) { + sb.append(value.toString()); + } else if (value instanceof Map) { + sb.append('{'); + boolean first = true; + for (Map.Entry entry : ((Map) value).entrySet()) { + if (!first) { + sb.append(','); + } + first = false; + appendString(sb, String.valueOf(entry.getKey())); + sb.append(':'); + appendValue(sb, entry.getValue()); + } + sb.append('}'); + } else if (value instanceof Iterable) { + sb.append('['); + boolean first = true; + for (Object item : (Iterable) value) { + if (!first) { + sb.append(','); + } + first = false; + appendValue(sb, item); + } + sb.append(']'); + } else if (value instanceof Object[]) { + sb.append('['); + Object[] array = (Object[]) value; + for (int i = 0; i < array.length; i++) { + if (i > 0) { + sb.append(','); + } + appendValue(sb, array[i]); + } + sb.append(']'); + } else { + appendString(sb, value.toString()); + } + } + + private static void appendString(StringBuilder sb, String value) { + sb.append('"'); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + switch (c) { + case '"': + sb.append("\\\""); + break; + case '\\': + sb.append("\\\\"); + break; + case '\n': + sb.append("\\n"); + break; + case '\r': + sb.append("\\r"); + break; + case '\t': + sb.append("\\t"); + break; + case '\b': + sb.append("\\b"); + break; + case '\f': + sb.append("\\f"); + break; + default: + if (c < 0x20) { + sb.append(String.format("\\u%04x", (int) c)); + } else { + sb.append(c); + } + break; + } + } + sb.append('"'); + } + + private static String getFullStackTrace(Throwable e) { + StackTraceElement[] elements = e.getStackTrace(); + StringBuilder sb = new StringBuilder(elements.length * 64); + for (StackTraceElement element : elements) { + sb.append("\tat ").append(element.toString()).append(System.lineSeparator()); + } + return sb.toString(); + } + + /** + * Internal exception whose message carries the serialized {@code TaskFailureDetails} JSON + * payload. It intentionally has no cause so the Java worker reports its message verbatim. + */ + private static final class StructuredActivityFailure extends RuntimeException { + private static final long serialVersionUID = 1L; + + StructuredActivityFailure(String message) { + super(message, null, false, false); + } + } +} diff --git a/azurefunctions/src/main/resources/META-INF/services/com.microsoft.azure.functions.internal.spi.middleware.Middleware b/azurefunctions/src/main/resources/META-INF/services/com.microsoft.azure.functions.internal.spi.middleware.Middleware index 0ba98d0..a7cf3ad 100644 --- a/azurefunctions/src/main/resources/META-INF/services/com.microsoft.azure.functions.internal.spi.middleware.Middleware +++ b/azurefunctions/src/main/resources/META-INF/services/com.microsoft.azure.functions.internal.spi.middleware.Middleware @@ -1,2 +1,3 @@ com.microsoft.durabletask.azurefunctions.internal.middleware.OrchestrationMiddleware -com.microsoft.durabletask.azurefunctions.internal.middleware.EntityMiddleware \ No newline at end of file +com.microsoft.durabletask.azurefunctions.internal.middleware.EntityMiddleware +com.microsoft.durabletask.azurefunctions.internal.middleware.ActivityMiddleware \ No newline at end of file diff --git a/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java new file mode 100644 index 0000000..0a07212 --- /dev/null +++ b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java @@ -0,0 +1,286 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +package com.microsoft.durabletask.azurefunctions.internal.middleware; + +import com.microsoft.azure.functions.internal.spi.middleware.MiddlewareChain; +import com.microsoft.azure.functions.internal.spi.middleware.MiddlewareContext; +import com.microsoft.durabletask.ExceptionPropertiesProvider; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link ActivityMiddleware}, covering exception reshaping, pass-through behavior, + * and the cross-class-loader SPI discovery that guards against the worker-thread regression. + */ +public class ActivityMiddlewareTest { + + private static final String ACTIVITY_TRIGGER = "DurableActivityTrigger"; + + /** A MiddlewareChain whose {@code doNext} throws the supplied exception. */ + private static MiddlewareChain throwingChain(Exception toThrow) { + return context -> { + throw toThrow; + }; + } + + /** A test exception representing a user's activity failure. */ + private static final class BusinessException extends Exception { + private static final long serialVersionUID = 1L; + + BusinessException(String message) { + super(message); + } + } + + private MiddlewareContext activityContext() { + MiddlewareContext context = mock(MiddlewareContext.class); + when(context.getParameterName(anyString())).thenReturn("input"); + return context; + } + + @BeforeEach + void resetBefore() { + ActivityMiddleware.resetProviderCacheForTesting(); + } + + @AfterEach + void resetAfter() { + ActivityMiddleware.resetProviderCacheForTesting(); + } + + @Test + @DisplayName("Reshapes a failing activity into structured TaskFailureDetails JSON when the " + + "provider yields properties") + void reshapesFailureWhenProviderYieldsProperties() { + ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + Map properties = new LinkedHashMap<>(); + properties.put("code", "E123"); + properties.put("count", 7); + return properties; + }); + + BusinessException original = new BusinessException("boom"); + ActivityMiddleware middleware = new ActivityMiddleware(); + + Exception thrown = assertThrows(Exception.class, + () -> middleware.invoke(activityContext(), throwingChain(original))); + + // The original exception is replaced by a structured-failure carrier whose message is JSON. + assertNotSameInstance(original, thrown); + String message = thrown.getMessage(); + assertNotNull(message); + assertTrue(message.startsWith("{"), "message should be a JSON object, was: " + message); + assertTrue(message.contains("\"errorType\":\"" + BusinessException.class.getName() + "\""), + message); + assertTrue(message.contains("\"errorMessage\":\"boom\""), message); + assertTrue(message.contains("\"code\":\"E123\""), message); + assertTrue(message.contains("\"count\":7"), message); + } + + @Test + @DisplayName("Rethrows the original exception unchanged when the provider returns no properties") + void rethrowsOriginalWhenProviderReturnsEmpty() { + ActivityMiddleware.setProviderSupplierForTesting( + () -> exception -> Collections.emptyMap()); + + BusinessException original = new BusinessException("boom"); + ActivityMiddleware middleware = new ActivityMiddleware(); + + Exception thrown = assertThrows(Exception.class, + () -> middleware.invoke(activityContext(), throwingChain(original))); + + assertSame(original, thrown); + } + + @Test + @DisplayName("Rethrows the original exception unchanged when the provider returns null") + void rethrowsOriginalWhenProviderReturnsNull() { + ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> null); + + BusinessException original = new BusinessException("boom"); + ActivityMiddleware middleware = new ActivityMiddleware(); + + Exception thrown = assertThrows(Exception.class, + () -> middleware.invoke(activityContext(), throwingChain(original))); + + assertSame(original, thrown); + } + + @Test + @DisplayName("Rethrows the original exception unchanged when no provider is registered") + void rethrowsOriginalWhenNoProvider() { + ActivityMiddleware.setProviderSupplierForTesting(() -> null); + + BusinessException original = new BusinessException("boom"); + ActivityMiddleware middleware = new ActivityMiddleware(); + + Exception thrown = assertThrows(Exception.class, + () -> middleware.invoke(activityContext(), throwingChain(original))); + + assertSame(original, thrown); + } + + @Test + @DisplayName("Rethrows the original exception unchanged when the provider itself throws") + void rethrowsOriginalWhenProviderThrows() { + ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + throw new IllegalStateException("provider is broken"); + }); + + BusinessException original = new BusinessException("boom"); + ActivityMiddleware middleware = new ActivityMiddleware(); + + Exception thrown = assertThrows(Exception.class, + () -> middleware.invoke(activityContext(), throwingChain(original))); + + assertSame(original, thrown); + } + + @Test + @DisplayName("Does not invoke the provider for non-activity triggers") + void passesThroughNonActivityTrigger() throws Exception { + AtomicInteger providerCalls = new AtomicInteger(); + ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + providerCalls.incrementAndGet(); + return Collections.singletonMap("k", "v"); + }); + + MiddlewareContext context = mock(MiddlewareContext.class); + when(context.getParameterName(anyString())).thenReturn(null); // not an activity + MiddlewareChain chain = mock(MiddlewareChain.class); + ActivityMiddleware middleware = new ActivityMiddleware(); + + middleware.invoke(context, chain); + + verify(chain, times(1)).doNext(context); + assertEquals(0, providerCalls.get(), "provider must not be consulted for non-activities"); + } + + @Test + @DisplayName("Reshapes nested causes into a nested innerFailure payload") + void reshapesNestedCauses() { + ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + // Attach a property only to the outer exception so we can assert nesting shape. + if ("outer".equals(exception.getMessage())) { + return Collections.singletonMap("layer", "outer"); + } + return null; + }); + + BusinessException cause = new BusinessException("inner"); + Exception outer = new RuntimeException("outer", cause); + ActivityMiddleware middleware = new ActivityMiddleware(); + + Exception thrown = assertThrows(Exception.class, + () -> middleware.invoke(activityContext(), throwingChain(outer))); + + String message = thrown.getMessage(); + assertNotNull(message); + assertTrue(message.contains("\"innerFailure\":{"), message); + assertTrue(message.contains("\"errorType\":\"" + BusinessException.class.getName() + "\""), + message); + assertTrue(message.contains("\"layer\":\"outer\""), message); + } + + // --- Cross-class-loader SPI discovery (the worker-thread regression guard) --- + + @Test + @DisplayName("discoverProvider returns null when no candidate class loader exposes the SPI file") + void discoverProviderReturnsNullWhenNoServiceFileVisible() { + ClassLoader blind = getClass().getClassLoader(); + assertNull(ActivityMiddleware.discoverProvider(new ClassLoader[] {blind})); + } + + @Test + @DisplayName("discoverProvider falls back past a provider-blind class loader to one that " + + "exposes the SPI file") + void discoverProviderFallsBackToClassLoaderThatSeesServiceFile() throws Exception { + ClassLoader blind = getClass().getClassLoader(); + URLClassLoader appLike = newClassLoaderExposingProvider(blind); + try { + // The first (blind) class loader mirrors the Azure Functions worker thread's context + // class loader, which cannot see the app's META-INF/services registration. Discovery + // must not stop there; it must fall back to the class loader that does. + ExceptionPropertiesProvider provider = + ActivityMiddleware.discoverProvider(new ClassLoader[] {blind, appLike}); + + assertNotNull(provider, "provider should be discovered via the fallback class loader"); + assertInstanceOf(ExceptionPropertiesProvider.class, provider); + assertEquals(TestExceptionPropertiesProvider.class.getName(), + provider.getClass().getName()); + } finally { + appLike.close(); + } + } + + @Test + @DisplayName("discoverProvider skips null and duplicate candidate class loaders") + void discoverProviderSkipsNullAndDuplicateCandidates() throws Exception { + ClassLoader blind = getClass().getClassLoader(); + URLClassLoader appLike = newClassLoaderExposingProvider(blind); + try { + ExceptionPropertiesProvider provider = ActivityMiddleware.discoverProvider( + new ClassLoader[] {null, blind, blind, appLike, appLike}); + assertNotNull(provider); + assertEquals(TestExceptionPropertiesProvider.class.getName(), + provider.getClass().getName()); + } finally { + appLike.close(); + } + } + + /** + * Builds a URLClassLoader that exposes a {@code META-INF/services} registration for + * {@link TestExceptionPropertiesProvider}. The provider class itself is loaded via the parent + * (so it resolves to the same {@link ExceptionPropertiesProvider} type), while the service file + * is served from this loader's own URL root — mirroring how an app jar carries its SPI file. + */ + private URLClassLoader newClassLoaderExposingProvider(ClassLoader parent) throws IOException { + Path root = Files.createTempDirectory("amw-spi-"); + root.toFile().deleteOnExit(); + Path servicesDir = root.resolve("META-INF").resolve("services"); + Files.createDirectories(servicesDir); + Path serviceFile = servicesDir.resolve(ExceptionPropertiesProvider.class.getName()); + Files.write(serviceFile, + (TestExceptionPropertiesProvider.class.getName() + System.lineSeparator()) + .getBytes(StandardCharsets.UTF_8)); + serviceFile.toFile().deleteOnExit(); + servicesDir.toFile().deleteOnExit(); + root.resolve("META-INF").toFile().deleteOnExit(); + + URL rootUrl = root.toUri().toURL(); + return new URLClassLoader(new URL[] {rootUrl}, parent); + } + + private static void assertNotSameInstance(Object unexpected, Object actual) { + assertFalse(unexpected == actual, + "expected a different instance than the original exception"); + } +} diff --git a/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/TestExceptionPropertiesProvider.java b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/TestExceptionPropertiesProvider.java new file mode 100644 index 0000000..c594f12 --- /dev/null +++ b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/TestExceptionPropertiesProvider.java @@ -0,0 +1,23 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +package com.microsoft.durabletask.azurefunctions.internal.middleware; + +import com.microsoft.durabletask.ExceptionPropertiesProvider; + +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * Public {@link ExceptionPropertiesProvider} used only by {@link ActivityMiddlewareTest} to verify + * SPI discovery across class loaders. It must be {@code public} with a public no-arg constructor so + * {@link java.util.ServiceLoader} can instantiate it. + */ +public class TestExceptionPropertiesProvider implements ExceptionPropertiesProvider { + + @Override + public Map getExceptionProperties(Exception exception) { + Map properties = new LinkedHashMap<>(); + properties.put("discoveredVia", "serviceLoader"); + return properties; + } +} diff --git a/internal/durabletask-protobuf/PROTO_SOURCE_COMMIT_HASH b/internal/durabletask-protobuf/PROTO_SOURCE_COMMIT_HASH index dcc8b2f..a981c68 100644 --- a/internal/durabletask-protobuf/PROTO_SOURCE_COMMIT_HASH +++ b/internal/durabletask-protobuf/PROTO_SOURCE_COMMIT_HASH @@ -1 +1 @@ -98e138452d57586e3109545b94055448f2f6cc24 \ No newline at end of file +3145f9337fca9de57d2f89a6ff6f07150d34f1c2 \ No newline at end of file diff --git a/internal/durabletask-protobuf/protos/orchestrator_service.proto b/internal/durabletask-protobuf/protos/orchestrator_service.proto index e7e1252..3d9194a 100644 --- a/internal/durabletask-protobuf/protos/orchestrator_service.proto +++ b/internal/durabletask-protobuf/protos/orchestrator_service.proto @@ -377,7 +377,7 @@ message OrchestratorResponse { // Zero-based position of the current chunk within a chunked completion sequence. // This field is omitted for non-chunked completions. - google.protobuf.Int32Value chunkIndex = 9 [deprecated=true];; + google.protobuf.Int32Value chunkIndex = 9 [deprecated=true]; } message CreateInstanceRequest { From d0df7f894009239175b9812a519201b67e17c160 Mon Sep 17 00:00:00 2001 From: "naiyuantian@microsoft.com" Date: Thu, 16 Jul 2026 20:53:34 -0700 Subject: [PATCH 2/3] update middleware private --- .../middleware/ActivityMiddleware.java | 79 +++++++++++++------ .../middleware/ActivityMiddlewareTest.java | 53 ++++++++++--- 2 files changed, 98 insertions(+), 34 deletions(-) diff --git a/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java b/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java index 5d6721c..4f1087a 100644 --- a/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java +++ b/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java @@ -47,10 +47,15 @@ public class ActivityMiddleware implements Middleware { private static volatile boolean providerLoaded = false; private static ExceptionPropertiesProvider cachedProvider; - // Visible for testing only. When non-null, this supplier replaces SPI discovery so unit tests - // can exercise the reshaping and pass-through behavior without registering a real provider. + // Test-only override. When non-null, this supplier replaces SPI discovery so tests can inject a + // provider (or {@code null}) without registering a real one. Set/cleared via reflection. private static Supplier providerSupplierOverride; + /** + * Runs the activity and, if it fails and a provider supplies custom properties, replaces the + * failure with a structured {@code TaskFailureDetails} JSON payload; otherwise the original + * exception is rethrown unchanged. Non-activity invocations pass straight through. + */ @Override public void invoke(MiddlewareContext context, MiddlewareChain chain) throws Exception { String parameterName = context.getParameterName(ACTIVITY_TRIGGER); @@ -78,6 +83,11 @@ public void invoke(MiddlewareContext context, MiddlewareChain chain) throws Exce } } + /** + * Lazily resolves and caches the {@link ExceptionPropertiesProvider}, using the test override + * when present and otherwise discovering it via SPI. The result (including {@code null}) is + * cached for the lifetime of the worker. + */ private static ExceptionPropertiesProvider getProvider() { if (!providerLoaded) { synchronized (PROVIDER_LOCK) { @@ -92,14 +102,12 @@ private static ExceptionPropertiesProvider getProvider() { return cachedProvider; } + /** + * Discovers the app-registered {@link ExceptionPropertiesProvider} via SPI, trying the thread + * context, middleware, and interface class loaders in turn (the worker thread's context loader + * may not see the app's {@code META-INF/services} registration). + */ private static ExceptionPropertiesProvider discoverProvider() { - // The provider is registered via SPI in the function app's jar. Depending on how the - // Azure Functions Java worker dispatches invocations, the thread context class loader may - // be the worker's class loader (which cannot see the app's META-INF/services registration) - // rather than the app class loader. Try several candidate class loaders and use the first - // one that yields a provider. The class loader that loaded this middleware is bundled with - // the app (durabletask-azure-functions is an app dependency), so it can see the app's SPI - // registration and is the most reliable fallback. return discoverProvider(new ClassLoader[] { Thread.currentThread().getContextClassLoader(), ActivityMiddleware.class.getClassLoader(), @@ -107,11 +115,12 @@ private static ExceptionPropertiesProvider discoverProvider() { }); } - // Visible for testing. Iterates the candidate class loaders in order and returns the first - // provider discovered via SPI, skipping nulls and duplicates. This is the seam that guards - // against the worker-thread class loader regression: discovery must not stop at the (possibly - // provider-blind) thread context class loader. - static ExceptionPropertiesProvider discoverProvider(ClassLoader[] candidates) { + /** + * Returns the first {@link ExceptionPropertiesProvider} found by {@link ServiceLoader} across + * the given class loaders (nulls and duplicates skipped), or {@code null} if none is found. + * This is the seam that guards against the worker-thread class loader regression. + */ + private static ExceptionPropertiesProvider discoverProvider(ClassLoader[] candidates) { ClassLoader previous = null; for (ClassLoader classLoader : candidates) { if (classLoader == null || classLoader == previous) { @@ -134,9 +143,11 @@ static ExceptionPropertiesProvider discoverProvider(ClassLoader[] candidates) { return null; } - // Visible for testing. Overrides SPI discovery with the given supplier (may be {@code null} to - // simulate "no provider registered") and clears the cached provider so the next lookup re-runs. - static void setProviderSupplierForTesting(Supplier supplier) { + /** + * Test-only. Overrides SPI discovery with the given supplier ({@code null} simulates "no + * provider registered") and clears the cache so the next lookup re-runs. Invoked via reflection. + */ + private static void setProviderSupplierForTesting(Supplier supplier) { synchronized (PROVIDER_LOCK) { providerSupplierOverride = supplier; providerLoaded = false; @@ -144,9 +155,11 @@ static void setProviderSupplierForTesting(Supplier } } - // Visible for testing. Restores real SPI discovery and clears any cached provider so tests do - // not leak state into one another (the provider is cached in a static field). - static void resetProviderCacheForTesting() { + /** + * Test-only. Restores real SPI discovery and clears the cached provider so tests do not leak + * state into one another (the provider is cached in a static field). Invoked via reflection. + */ + private static void resetProviderCacheForTesting() { synchronized (PROVIDER_LOCK) { providerSupplierOverride = null; providerLoaded = false; @@ -154,6 +167,10 @@ static void resetProviderCacheForTesting() { } } + /** + * Unwraps reflective {@link InvocationTargetException} layers to reach the user exception that + * actually caused the activity to fail. + */ private static Throwable unwrap(Throwable e) { Throwable current = e; while (current instanceof InvocationTargetException && current.getCause() != null) { @@ -162,6 +179,11 @@ private static Throwable unwrap(Throwable e) { return current; } + /** + * Invokes the provider defensively, returning {@code null} if the failure is not an + * {@link Exception} or the provider itself throws, so a misbehaving provider never masks the + * original failure. + */ private static Map safeGetProperties( ExceptionPropertiesProvider provider, Throwable exception) { @@ -177,14 +199,20 @@ private static Map safeGetProperties( } } - // Builds the single-line JSON payload that mirrors the protobuf TaskFailureDetails shape - // consumed by the Durable Task host extension. + /** + * Builds the single-line JSON payload that mirrors the protobuf {@code TaskFailureDetails} shape + * consumed by the Durable Task host extension. + */ private static String buildFailureDetailsJson(Throwable exception, ExceptionPropertiesProvider provider) { StringBuilder sb = new StringBuilder(256); appendFailure(sb, exception, provider, 0); return sb.toString(); } + /** + * Recursively appends one failure level (error type/message/stack trace, any custom properties, + * and the cause as a nested {@code innerFailure}) to the JSON buffer. + */ private static void appendFailure( StringBuilder sb, Throwable exception, @@ -214,6 +242,11 @@ private static void appendFailure( sb.append('}'); } + /** + * Serializes a single property value as JSON, handling strings, booleans, numbers (non-finite + * doubles fall back to strings), maps, iterables, and arrays; anything else is written as its + * {@code toString()}. + */ @SuppressWarnings("unchecked") private static void appendValue(StringBuilder sb, Object value) { if (value == null) { @@ -270,6 +303,7 @@ private static void appendValue(StringBuilder sb, Object value) { } } + /** Appends {@code value} as a JSON string literal, escaping quotes, backslashes, and control characters. */ private static void appendString(StringBuilder sb, String value) { sb.append('"'); for (int i = 0; i < value.length(); i++) { @@ -308,6 +342,7 @@ private static void appendString(StringBuilder sb, String value) { sb.append('"'); } + /** Formats the throwable's stack trace as newline-separated {@code \tat ...} frames. */ private static String getFullStackTrace(Throwable e) { StackTraceElement[] elements = e.getStackTrace(); StringBuilder sb = new StringBuilder(elements.length * 64); diff --git a/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java index 0a07212..0eaabef 100644 --- a/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java +++ b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java @@ -11,6 +11,7 @@ import org.junit.jupiter.api.Test; import java.io.IOException; +import java.lang.reflect.Method; import java.net.URL; import java.net.URLClassLoader; import java.nio.charset.StandardCharsets; @@ -20,6 +21,7 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -65,21 +67,48 @@ private MiddlewareContext activityContext() { return context; } + // --- Reflection bridges to ActivityMiddleware's private test seams --- + // The seams are private (they are not part of the middleware's API), so tests reach them via + // reflection rather than widening visibility. + + private static void setProviderSupplier(Supplier supplier) { + invokeStatic("setProviderSupplierForTesting", new Class[] {Supplier.class}, supplier); + } + + private static void resetProviderCache() { + invokeStatic("resetProviderCacheForTesting", new Class[] {}); + } + + private static ExceptionPropertiesProvider discoverProvider(ClassLoader[] candidates) { + return (ExceptionPropertiesProvider) invokeStatic( + "discoverProvider", new Class[] {ClassLoader[].class}, (Object) candidates); + } + + private static Object invokeStatic(String name, Class[] paramTypes, Object... args) { + try { + Method method = ActivityMiddleware.class.getDeclaredMethod(name, paramTypes); + method.setAccessible(true); + return method.invoke(null, args); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException("Failed to invoke ActivityMiddleware." + name, e); + } + } + @BeforeEach void resetBefore() { - ActivityMiddleware.resetProviderCacheForTesting(); + resetProviderCache(); } @AfterEach void resetAfter() { - ActivityMiddleware.resetProviderCacheForTesting(); + resetProviderCache(); } @Test @DisplayName("Reshapes a failing activity into structured TaskFailureDetails JSON when the " + "provider yields properties") void reshapesFailureWhenProviderYieldsProperties() { - ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + setProviderSupplier(() -> exception -> { Map properties = new LinkedHashMap<>(); properties.put("code", "E123"); properties.put("count", 7); @@ -107,7 +136,7 @@ void reshapesFailureWhenProviderYieldsProperties() { @Test @DisplayName("Rethrows the original exception unchanged when the provider returns no properties") void rethrowsOriginalWhenProviderReturnsEmpty() { - ActivityMiddleware.setProviderSupplierForTesting( + setProviderSupplier( () -> exception -> Collections.emptyMap()); BusinessException original = new BusinessException("boom"); @@ -122,7 +151,7 @@ void rethrowsOriginalWhenProviderReturnsEmpty() { @Test @DisplayName("Rethrows the original exception unchanged when the provider returns null") void rethrowsOriginalWhenProviderReturnsNull() { - ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> null); + setProviderSupplier(() -> exception -> null); BusinessException original = new BusinessException("boom"); ActivityMiddleware middleware = new ActivityMiddleware(); @@ -136,7 +165,7 @@ void rethrowsOriginalWhenProviderReturnsNull() { @Test @DisplayName("Rethrows the original exception unchanged when no provider is registered") void rethrowsOriginalWhenNoProvider() { - ActivityMiddleware.setProviderSupplierForTesting(() -> null); + setProviderSupplier(() -> null); BusinessException original = new BusinessException("boom"); ActivityMiddleware middleware = new ActivityMiddleware(); @@ -150,7 +179,7 @@ void rethrowsOriginalWhenNoProvider() { @Test @DisplayName("Rethrows the original exception unchanged when the provider itself throws") void rethrowsOriginalWhenProviderThrows() { - ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + setProviderSupplier(() -> exception -> { throw new IllegalStateException("provider is broken"); }); @@ -167,7 +196,7 @@ void rethrowsOriginalWhenProviderThrows() { @DisplayName("Does not invoke the provider for non-activity triggers") void passesThroughNonActivityTrigger() throws Exception { AtomicInteger providerCalls = new AtomicInteger(); - ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + setProviderSupplier(() -> exception -> { providerCalls.incrementAndGet(); return Collections.singletonMap("k", "v"); }); @@ -186,7 +215,7 @@ void passesThroughNonActivityTrigger() throws Exception { @Test @DisplayName("Reshapes nested causes into a nested innerFailure payload") void reshapesNestedCauses() { - ActivityMiddleware.setProviderSupplierForTesting(() -> exception -> { + setProviderSupplier(() -> exception -> { // Attach a property only to the outer exception so we can assert nesting shape. if ("outer".equals(exception.getMessage())) { return Collections.singletonMap("layer", "outer"); @@ -215,7 +244,7 @@ void reshapesNestedCauses() { @DisplayName("discoverProvider returns null when no candidate class loader exposes the SPI file") void discoverProviderReturnsNullWhenNoServiceFileVisible() { ClassLoader blind = getClass().getClassLoader(); - assertNull(ActivityMiddleware.discoverProvider(new ClassLoader[] {blind})); + assertNull(discoverProvider(new ClassLoader[] {blind})); } @Test @@ -229,7 +258,7 @@ void discoverProviderFallsBackToClassLoaderThatSeesServiceFile() throws Exceptio // class loader, which cannot see the app's META-INF/services registration. Discovery // must not stop there; it must fall back to the class loader that does. ExceptionPropertiesProvider provider = - ActivityMiddleware.discoverProvider(new ClassLoader[] {blind, appLike}); + discoverProvider(new ClassLoader[] {blind, appLike}); assertNotNull(provider, "provider should be discovered via the fallback class loader"); assertInstanceOf(ExceptionPropertiesProvider.class, provider); @@ -246,7 +275,7 @@ void discoverProviderSkipsNullAndDuplicateCandidates() throws Exception { ClassLoader blind = getClass().getClassLoader(); URLClassLoader appLike = newClassLoaderExposingProvider(blind); try { - ExceptionPropertiesProvider provider = ActivityMiddleware.discoverProvider( + ExceptionPropertiesProvider provider = discoverProvider( new ClassLoader[] {null, blind, blind, appLike, appLike}); assertNotNull(provider); assertEquals(TestExceptionPropertiesProvider.class.getName(), From 26c98e326743eeb57e36b8544ba651e13576fd1c Mon Sep 17 00:00:00 2001 From: "naiyuantian@microsoft.com" Date: Sun, 19 Jul 2026 20:56:56 -0700 Subject: [PATCH 3/3] fx copilot comment --- .../middleware/ActivityMiddleware.java | 23 ++++++++++++------- .../middleware/ActivityMiddlewareTest.java | 11 +++++---- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java b/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java index 4f1087a..84e6fc8 100644 --- a/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java +++ b/azurefunctions/src/main/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddleware.java @@ -16,6 +16,7 @@ import java.util.Map; import java.util.ServiceLoader; import java.util.function.Supplier; +import java.util.logging.Level; import java.util.logging.Logger; /** @@ -79,7 +80,7 @@ public void invoke(MiddlewareContext context, MiddlewareChain chain) throws Exce throw e; } - throw new StructuredActivityFailure(buildFailureDetailsJson(userException, provider)); + throw new StructuredActivityFailure(buildFailureDetailsJson(userException, properties, provider)); } } @@ -136,8 +137,9 @@ private static ExceptionPropertiesProvider discoverProvider(ClassLoader[] candid } } catch (Throwable t) { // Discovery failures must not break activity execution; the feature is opt-in. - LOGGER.warning("Failed to load ExceptionPropertiesProvider via ServiceLoader using " - + classLoader + ": " + t); + LOGGER.log(Level.WARNING, + "Failed to load ExceptionPropertiesProvider via ServiceLoader using " + classLoader, + t); } } return null; @@ -194,7 +196,9 @@ private static Map safeGetProperties( return provider.getExceptionProperties((Exception) exception); } catch (Exception providerException) { // Don't let a misbehaving provider mask the original failure. - LOGGER.warning("ExceptionPropertiesProvider threw while extracting properties: " + providerException); + LOGGER.log(Level.WARNING, + "ExceptionPropertiesProvider threw while extracting properties; ignoring provider output.", + providerException); return null; } } @@ -203,9 +207,12 @@ private static Map safeGetProperties( * Builds the single-line JSON payload that mirrors the protobuf {@code TaskFailureDetails} shape * consumed by the Durable Task host extension. */ - private static String buildFailureDetailsJson(Throwable exception, ExceptionPropertiesProvider provider) { + private static String buildFailureDetailsJson( + Throwable exception, + Map properties, + ExceptionPropertiesProvider provider) { StringBuilder sb = new StringBuilder(256); - appendFailure(sb, exception, provider, 0); + appendFailure(sb, exception, properties, provider, 0); return sb.toString(); } @@ -216,6 +223,7 @@ private static String buildFailureDetailsJson(Throwable exception, ExceptionProp private static void appendFailure( StringBuilder sb, Throwable exception, + Map properties, ExceptionPropertiesProvider provider, int depth) { sb.append('{'); @@ -227,7 +235,6 @@ private static void appendFailure( appendString(sb, getFullStackTrace(exception)); sb.append(",\"isNonRetriable\":false"); - Map properties = safeGetProperties(provider, exception); if (properties != null && !properties.isEmpty()) { sb.append(",\"properties\":"); appendValue(sb, properties); @@ -236,7 +243,7 @@ private static void appendFailure( Throwable cause = exception.getCause(); if (cause != null && cause != exception && depth < MAX_INNER_FAILURE_DEPTH) { sb.append(",\"innerFailure\":"); - appendFailure(sb, cause, provider, depth + 1); + appendFailure(sb, cause, safeGetProperties(provider, cause), provider, depth + 1); } sb.append('}'); diff --git a/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java index 0eaabef..e671197 100644 --- a/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java +++ b/azurefunctions/src/test/java/com/microsoft/durabletask/azurefunctions/internal/middleware/ActivityMiddlewareTest.java @@ -9,6 +9,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import java.io.IOException; import java.lang.reflect.Method; @@ -45,6 +46,10 @@ public class ActivityMiddlewareTest { private static final String ACTIVITY_TRIGGER = "DurableActivityTrigger"; + /** Auto-cleaned temp directory root for SPI class loader fixtures. */ + @TempDir + Path tempDir; + /** A MiddlewareChain whose {@code doNext} throws the supplied exception. */ private static MiddlewareChain throwingChain(Exception toThrow) { return context -> { @@ -292,17 +297,13 @@ void discoverProviderSkipsNullAndDuplicateCandidates() throws Exception { * is served from this loader's own URL root — mirroring how an app jar carries its SPI file. */ private URLClassLoader newClassLoaderExposingProvider(ClassLoader parent) throws IOException { - Path root = Files.createTempDirectory("amw-spi-"); - root.toFile().deleteOnExit(); + Path root = Files.createTempDirectory(tempDir, "amw-spi-"); Path servicesDir = root.resolve("META-INF").resolve("services"); Files.createDirectories(servicesDir); Path serviceFile = servicesDir.resolve(ExceptionPropertiesProvider.class.getName()); Files.write(serviceFile, (TestExceptionPropertiesProvider.class.getName() + System.lineSeparator()) .getBytes(StandardCharsets.UTF_8)); - serviceFile.toFile().deleteOnExit(); - servicesDir.toFile().deleteOnExit(); - root.resolve("META-INF").toFile().deleteOnExit(); URL rootUrl = root.toUri().toURL(); return new URLClassLoader(new URL[] {rootUrl}, parent);