From 14f553757db9d728dc349effa554ecc040c17841 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 20 Oct 2026 07:06:40 +0200 Subject: [PATCH 1/3] fix(provider-tck): the review findings that merged unaddressed - jackson-databind 2.22.1 to 2.22.3. The TCK exposes it at compile scope so adopters inherit it, and tools/flagd-api-testkit already pins 2.22.3 -- the suite was behind its own repo. - A Compose stack that threw from start() leaked its containers: startCompose() had not returned into the field stopSuite() reads. It stops the stack before rethrowing. - POST /start sent config=null for the configuration overHttp() documents as nullable, and concatenated the name unencoded, so a space made URI.create throw. The query is omitted when there is no configuration and URL-encoded otherwise. - TckLifecycle.start() rethrew after stopSuite(), so a teardown failure replaced the reason the suite would not start. It is suppressed onto the original instead. - A failed start was not recorded, so every later scenario retried it: for a containerized suite, one Compose timeout per scenario. It is cached for the suite and cleared in beginSuite() and endSuite(). The last two are in code added late in #1830 and had no review pass before it merged. Found by CodeRabbit on #1830. 257 tests, three of them new. Signed-off-by: Simon Schrottner --- tools/tck/pom.xml | 2 +- .../tck/ContainerizedProviderTckTest.java | 12 +++++++- .../contrib/tools/tck/HttpBackendControl.java | 7 ++++- .../tck/internal/steps/TckLifecycle.java | 24 ++++++++++++++-- .../tools/tck/HttpBackendControlTest.java | 20 +++++++++++++ .../tck/internal/steps/TckLifecycleTest.java | 28 +++++++++++++++++++ 6 files changed, 87 insertions(+), 6 deletions(-) diff --git a/tools/tck/pom.xml b/tools/tck/pom.xml index 70d6531289..c71c481ad1 100644 --- a/tools/tck/pom.xml +++ b/tools/tck/pom.xml @@ -20,7 +20,7 @@ 3.27.7 4.3.0 - 2.22.1 + 2.22.3 2.0.17 2.0.4 1.3.0 diff --git a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/ContainerizedProviderTckTest.java b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/ContainerizedProviderTckTest.java index cd2004b726..7e7bf0dfbd 100644 --- a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/ContainerizedProviderTckTest.java +++ b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/ContainerizedProviderTckTest.java @@ -186,7 +186,17 @@ private ComposeContainer startCompose() { stack.withStartupTimeout(startupTimeout()); log.info("Starting Compose stack {} (started once per suite, never restarted)", composeFile.getAbsolutePath()); - stack.start(); + try { + stack.start(); + } catch (RuntimeException e) { + // stopSuite() reaches the stack through a field this method has not returned into yet + try { + stack.stop(); + } catch (RuntimeException cleanup) { + e.addSuppressed(cleanup); + } + throw e; + } return stack; } } diff --git a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/HttpBackendControl.java b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/HttpBackendControl.java index a8229e3f80..1c054408d4 100644 --- a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/HttpBackendControl.java +++ b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/HttpBackendControl.java @@ -2,9 +2,11 @@ import java.io.IOException; import java.net.URI; +import java.net.URLEncoder; import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; import java.time.Duration; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -145,7 +147,10 @@ public void awaitReady(Duration timeout) { /** Starts the backend, seeding the baseline flag state. */ private void start() { - post("/start?config=" + backendConfiguration); + post( + backendConfiguration == null + ? "/start" + : "/start?config=" + URLEncoder.encode(backendConfiguration, StandardCharsets.UTF_8)); backendStopped = false; } diff --git a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java index 18b3b74aef..c98f4e6df7 100644 --- a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java +++ b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java @@ -31,6 +31,8 @@ final class TckLifecycle { private static volatile boolean suiteRunning; + private static volatile RuntimeException startFailure; + private final ProviderTckHarness harness; private final BackendControl backendControl; @@ -44,12 +46,14 @@ static void beginSuite() { ProviderTckHarness harness = discoverHarness(); log.info("Provider TCK harness: {}", harness.getClass().getName()); Capability.requireDeclarable(harness.capabilities()); + startFailure = null; suiteRunning = true; } /** Stops every lifecycle this suite started. */ static void endSuite() { suiteRunning = false; + startFailure = null; List toStop; synchronized (started) { toStop = new ArrayList<>(started); @@ -82,7 +86,17 @@ static Optional startForExecutingSuite() { if (!suiteRunning) { return Optional.empty(); } - return Optional.of(start(discoverHarness())); + RuntimeException failed = startFailure; + if (failed != null) { + // one Compose timeout per suite, not one per scenario + throw new IllegalStateException("the suite's backend already failed to start", failed); + } + try { + return Optional.of(start(discoverHarness())); + } catch (RuntimeException e) { + startFailure = e; + throw e; + } } /** Starts the given harness and registers the result for {@link #endSuite()}. */ @@ -99,8 +113,12 @@ static TckLifecycle start(ProviderTckHarness harness) { started.add(lifecycle); return lifecycle; } catch (RuntimeException e) { - // release anything a partial start allocated, e.g. containers - harness.stopSuite(); + // release anything a partial start allocated, e.g. containers, without losing why + try { + harness.stopSuite(); + } catch (RuntimeException cleanup) { + e.addSuppressed(cleanup); + } throw e; } } diff --git a/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/HttpBackendControlTest.java b/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/HttpBackendControlTest.java index 98284ce171..09eda6959d 100644 --- a/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/HttpBackendControlTest.java +++ b/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/HttpBackendControlTest.java @@ -62,6 +62,26 @@ void fallsBackToStartAndCachesTheAnswer() throws Exception { } } + @Test + @DisplayName("a null configuration posts /start with no query, rather than config=null") + void aNullConfigurationOmitsTheQuery() throws Exception { + try (StubControlApi stub = StubControlApi.start().answering("/reset", 404)) { + new HttpBackendControl(stub.baseUrl(), null).prepareScenario(); + + assertThat(stub.requests()).containsExactly("POST /reset", "POST /start"); + } + } + + @Test + @DisplayName("a configuration name is encoded, so a space cannot break the URI") + void theConfigurationNameIsEncoded() throws Exception { + try (StubControlApi stub = StubControlApi.start().answering("/reset", 404)) { + new HttpBackendControl(stub.baseUrl(), "two words").prepareScenario(); + + assertThat(stub.requests()).containsExactly("POST /reset", "POST /start?config=two+words"); + } + } + @ParameterizedTest @ValueSource(ints = {404, 501}) @DisplayName("both documented not-implemented statuses trigger the fallback") diff --git a/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycleTest.java b/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycleTest.java index eeb047ca9e..d88b8d0aa5 100644 --- a/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycleTest.java +++ b/tools/tck/src/test/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycleTest.java @@ -119,6 +119,34 @@ public void stopSuite() { .isZero(); } + @Test + @DisplayName("a teardown failure is attached to the startup failure, not substituted for it") + void cleanupFailureDoesNotReplaceTheCause() { + class FailsBothWays implements ProviderTckHarness { + @Override + public FeatureProvider createProvider() { + return new NoOpProvider(); + } + + @Override + public BackendControl backendControl() { + return null; + } + + @Override + public void stopSuite() { + throw new IllegalStateException("teardown also failed"); + } + } + + assertThatThrownBy(() -> TckLifecycle.start(new FailsBothWays())) + .as("the adopter needs the reason the suite would not start") + .hasMessageContaining("backendControl() returned null") + .satisfies(thrown -> assertThat(thrown.getSuppressed()) + .as("and the teardown failure is kept, not dropped") + .hasSize(1)); + } + @Test @DisplayName("no suite running means no lifecycle, so extension-only glue registers nothing") void noSuiteMeansNoLifecycle() { From 5835de4f15682b81d7efe609e8b2160a13459a85 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 20 Oct 2026 07:17:22 +0200 Subject: [PATCH 2/3] fix(provider-tck): stop shipping a second Cucumber ObjectFactory TckObjectFactory needs org.picocontainer's container, which the TCK was getting transitively through io.cucumber:cucumber-picocontainer. That artifact also registers Cucumber's own PicoFactory through META-INF/services, so an adopter inherited two ObjectFactory services and Cucumber refused to choose between them in the adopter's own Cucumber suites. That is how flagd's ConfigCucumberTest failed as soon as it rebased onto a base that ships the TCK. Depending on org.picocontainer:picocontainer directly leaves only the TCK's own factory on an adopter's classpath. dependency:tree confirms it: picocontainer 2.15.2 at compile scope, cucumber-picocontainer absent. Inside this repository the root pom hands every module cucumber-picocontainer at test scope, so the adoptions here still see both factories and still name the one they want. This is about what the published artifact imposes on everyone else. 257 tests, unchanged. Signed-off-by: Simon Schrottner --- tools/tck/pom.xml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/tck/pom.xml b/tools/tck/pom.xml index c71c481ad1..700c66a1de 100644 --- a/tools/tck/pom.xml +++ b/tools/tck/pom.xml @@ -66,10 +66,16 @@ cucumber-junit-platform-engine - + - io.cucumber - cucumber-picocontainer + org.picocontainer + picocontainer + 2.15.2 compile From 194255733b6f35e807ddd12a53aabd3e30030b81 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 20 Oct 2026 07:22:56 +0200 Subject: [PATCH 3/3] fix(provider-tck): keep the first startup failure, not the last The cached failure was a plain volatile, so two threads failing to start their own backend could each write it and the adopter saw whichever landed last. It is an AtomicReference set with compareAndSet, so the first failure is the one reported. Not synchronized beyond that, on purpose: making a second thread wait for the first thread's Compose startup is the serialisation per-thread backends exist to avoid. Threads that race before a failure is cached each pay one attempt, in parallel, and everyone arriving after it fails fast. Raised by CodeRabbit on #1942, which asked for the whole check-and-start to be synchronized; that half would have undone the parallel design. Signed-off-by: Simon Schrottner --- .../tools/tck/internal/steps/TckLifecycle.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java index c98f4e6df7..066a689cac 100644 --- a/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java +++ b/tools/tck/src/main/java/dev/openfeature/contrib/tools/tck/internal/steps/TckLifecycle.java @@ -11,6 +11,7 @@ import java.util.Optional; import java.util.ServiceLoader; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,7 +32,7 @@ final class TckLifecycle { private static volatile boolean suiteRunning; - private static volatile RuntimeException startFailure; + private static final AtomicReference startFailure = new AtomicReference<>(); private final ProviderTckHarness harness; private final BackendControl backendControl; @@ -46,14 +47,14 @@ static void beginSuite() { ProviderTckHarness harness = discoverHarness(); log.info("Provider TCK harness: {}", harness.getClass().getName()); Capability.requireDeclarable(harness.capabilities()); - startFailure = null; + startFailure.set(null); suiteRunning = true; } /** Stops every lifecycle this suite started. */ static void endSuite() { suiteRunning = false; - startFailure = null; + startFailure.set(null); List toStop; synchronized (started) { toStop = new ArrayList<>(started); @@ -86,7 +87,11 @@ static Optional startForExecutingSuite() { if (!suiteRunning) { return Optional.empty(); } - RuntimeException failed = startFailure; + // Deliberately not synchronized: making a second thread wait for the first thread's + // Compose startup is the serialisation per-thread backends exist to avoid. Threads that race + // before a failure is cached each pay one attempt, in parallel; everyone after fails fast, + // and compareAndSet keeps the first failure rather than whichever landed last. + RuntimeException failed = startFailure.get(); if (failed != null) { // one Compose timeout per suite, not one per scenario throw new IllegalStateException("the suite's backend already failed to start", failed); @@ -94,7 +99,7 @@ static Optional startForExecutingSuite() { try { return Optional.of(start(discoverHarness())); } catch (RuntimeException e) { - startFailure = e; + startFailure.compareAndSet(null, e); throw e; } }