Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ internal class BootstrapCoordinator(
syncDefault(client, scope, binding)
null
} catch (error: Exception) {
refreshScheduler.requireDefaultSync(scope)
error
}
refreshScheduler.subscribeToChanges(listener, scope)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ internal class ConfigScopeSynchronizer(
private val refreshScheduler =
RefreshScheduler(
logger = logger,
objectMapper = objectMapper,
snapshotApplier = snapshotApplier,
refreshExecutorFactory = refreshExecutorFactory,
refreshWorkerExecutorFactory = refreshWorkerExecutorFactory,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package gg.grounds.config.internal.sync

import gg.grounds.config.client.ConfigDefaultData
import gg.grounds.config.client.SnapshotResult
import gg.grounds.config.internal.binding.ConfigBinding
import gg.grounds.config.internal.scope.AppEnvScope
import gg.grounds.config.nats.ConfigChangeListener
import java.util.concurrent.ConcurrentHashMap
Expand All @@ -9,9 +11,11 @@ import java.util.concurrent.ScheduledExecutorService
import java.util.concurrent.ScheduledFuture
import java.util.concurrent.TimeUnit
import org.slf4j.Logger
import tools.jackson.databind.ObjectMapper

internal class RefreshScheduler(
private val logger: Logger,
private val objectMapper: ObjectMapper,
private val snapshotApplier: SnapshotApplier,
private val refreshExecutorFactory: () -> ScheduledExecutorService,
private val refreshWorkerExecutorFactory: () -> ExecutorService,
Expand All @@ -20,6 +24,7 @@ internal class RefreshScheduler(
private val withLifecycleReadLock: ((() -> Unit) -> Unit),
) {
private val trackedScopes = ConcurrentHashMap.newKeySet<AppEnvScope>()
private val scopesRequiringDefaultSync = ConcurrentHashMap.newKeySet<AppEnvScope>()
private var refreshExecutor: ScheduledExecutorService? = null
private var refreshFuture: ScheduledFuture<*>? = null
private var refreshWorkerExecutor: ExecutorService? = null
Expand All @@ -42,6 +47,7 @@ internal class RefreshScheduler(
refreshWorkerExecutor?.shutdownNow()
refreshWorkerExecutor = null
trackedScopes.clear()
scopesRequiringDefaultSync.clear()
}

fun close() {
Expand All @@ -53,6 +59,10 @@ internal class RefreshScheduler(
trackedScopes.add(scope)
}

fun requireDefaultSync(scope: AppEnvScope) {
scopesRequiringDefaultSync.add(scope)
}

fun subscribeToChanges(listener: ConfigChangeListener, scope: AppEnvScope) {
if (!scope.markSubscriptionStarted()) {
return
Expand Down Expand Up @@ -91,6 +101,10 @@ internal class RefreshScheduler(
forceFullSnapshot: Boolean,
) {
val requiresFullSnapshot = forceFullSnapshot || scope.hasUninitializedBindings()
if (scopesRequiringDefaultSync.contains(scope)) {
syncUninitializedDefaults(client, scope)
scopesRequiringDefaultSync.remove(scope)
}
val response =
if (requiresFullSnapshot) {
executeRetryableCall(
Expand All @@ -116,6 +130,34 @@ internal class RefreshScheduler(
applyResponse(scope, response)
}

private fun syncUninitializedDefaults(
client: gg.grounds.config.client.ConfigSyncClient,
scope: AppEnvScope,
) {
val defaults =
scope.bindingsSnapshot().values.filterNot(ConfigBinding<*>::initialized).map { binding

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Retry defaults even when the cache initialized the binding

When the initial default sync fails but degraded bootstrap successfully restores this binding from a cached snapshot, initialized() is true, so this filter produces no defaults. refreshScope then removes the scope's pending-default marker, meaning subsequent refreshes never retry the failed seed even after service-config recovers. Track and retry the bindings whose default sync failed rather than deriving that set from runtime initialization state.

Useful? React with 👍 / 👎.

->
ConfigDefaultData(
namespace = binding.definition.namespace,
configKey = binding.definition.key,
defaultContentJson =
objectMapper.writeValueAsString(binding.definition.defaultValue),
)
}
if (defaults.isEmpty()) {
return
}
executeRetryableCall(
logger = logger,
scope = scope,
operation = "sync_defaults",
maxAttempts = REFRESH_MAX_ATTEMPTS,
sleepMillis = sleepMillis,
) {
client.syncDefaults(scope.app, scope.env, defaults)
}
}

private fun applyResponse(scope: AppEnvScope, response: SnapshotResult) {
if (response.changed) {
snapshotApplier.applySnapshot(scope, response.version, response.documents)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import java.util.concurrent.Executors
import java.util.concurrent.Future
import java.util.concurrent.ScheduledExecutorService
import java.util.concurrent.TimeUnit
import java.util.concurrent.atomic.AtomicBoolean
import java.util.concurrent.atomic.AtomicInteger
import kotlin.io.path.createTempDirectory
import kotlin.test.Test
Expand Down Expand Up @@ -439,6 +440,67 @@ class ConfigScopeSynchronizerTest {
}
}

@Test
fun `periodic refresh seeds an uninitialized degraded binding after the service recovers`() {
val serviceAvailable = AtomicBoolean(false)
val refreshCompleted = CountDownLatch(1)
var onChangeReceived: (() -> Unit)? = null
val client =
RecordingConfigSyncClient(
syncDefaultsHandler = {
if (!serviceAvailable.get()) {
throw ConfigServiceException("service unavailable", 503)
}
SyncDefaultsResult(version = 1, createdKeys = emptyList())
},
getSnapshotHandler = {
check(syncDefaultCalls >= 4) { "defaults were not re-synced" }
refreshCompleted.countDown()
defaultSnapshotResponse(version = 1, value = "recovered")
},
)
val executors = mutableListOf<ScheduledExecutorService>()
val synchronizer =
ConfigScopeSynchronizer(
logger = LoggerFactory.getLogger("ConfigScopeSynchronizerRecoveryTest"),
configClientFactory = { client },
natsListenerFactory = {
RecordingConfigChangeListener { callback -> onChangeReceived = callback }
},
refreshExecutorFactory = {
Executors.newSingleThreadScheduledExecutor().also { executor ->
executors += executor
}
},
sleepMillis = {},
)
val scope = AppEnvScope(app = "test-app", env = "dev")
val binding = ConfigBinding(TestStringConfig)
scope.putBindingIfAbsent(
ConfigKey(TestStringConfig.namespace, TestStringConfig.key),
binding,
)

try {
synchronizer.start("dns:///config", "nats://localhost:4222")
assertEquals(
ConfigRegistrationStatus.NOT_READY,
synchronizer.bootstrap(scope, binding, ConfigStartupMode.DEGRADED).status,
)

serviceAvailable.set(true)
checkNotNull(onChangeReceived).invoke()

assertTrue(refreshCompleted.await(1, TimeUnit.SECONDS))
assertTrue(binding.initialized())
assertEquals("recovered", binding.get())
assertEquals(4, client.syncDefaultCalls)
} finally {
synchronizer.close()
executors.forEach { executor -> executor.shutdownNow() }
}
}

@Test
fun `bootstrap leaves binding uninitialized when snapshot is missing document`() {
val client =
Expand Down