[6/15] Make settings resolution thread-safe

core/src/main/kotlin/com/lightningkite/lightningserver/settings/ServerSettings.kt

@@ -89,7 +89,20 @@ public class ServerSettings private constructor(
     private var usingDefaults: Boolean = false
 
     private val serializable: MapRegistry<ServerSetting<*, *>, Any?> = MapRegistry()
-    private val goal: MapRegistry<ServerSetting<*, *>, Any?> = MapRegistry()
+
+    // Transformed results, published as an immutable snapshot. Reads are lock-free: once a setting is resolved,
+    // its value is visible here for good (the @Volatile guarantees other threads see the new snapshot). Writes
+    // replace the whole map under [resolveLock] (copy-on-write) — cheap because each setting is resolved at most
+    // once, and in production every setting is pre-resolved single-threaded during ready(). An immutable Map
+    // (not a ConcurrentHashMap) is used so a null result is stored naturally, with no sentinel.
+    @Volatile
+    private var goal: Map<ServerSetting<*, *>, Any?> = emptyMap()
+
+    // Guards resolution so each setting is transformed exactly once, even if several threads first request it
+    // at once. A single reentrant lock — not per-setting — is deliberate: a setting's getter may resolve its
+    // dependencies by calling get() again on the same thread, which re-enters this lock harmlessly, whereas
+    // per-setting locks could deadlock between two mutually-dependent settings.
+    private val resolveLock = Any()
 
     // Track settings currently being resolved to detect circular dependencies at runtime
     private val resolving: ThreadLocal<MutableSet<ServerSetting<*, *>>>? =
@@ -137,7 +150,8 @@ public class ServerSettings private constructor(
      */
     public infix fun <RESULT> ServerSetting<*, RESULT>.setStatic(value: RESULT) {
         if (ready) throw IllegalStateException("Settings are marked as ready.")
-        goal.register(this, value as Any?)
+        // Pre-ready configuration is single-threaded, so a plain copy-on-write assignment is enough.
+        goal = goal + (this to value)
     }
 
     /**
@@ -250,15 +264,24 @@ public class ServerSettings private constructor(
     public fun <SERIALIZABLE, RESULT> get(key: ServerSetting<SERIALIZABLE, RESULT>): RESULT {
         if (!ready) throw IllegalStateException("Settings not ready yet.")
 
-        return goal.getOrRegister(key) {
+        // Lock-free fast path: once a setting is resolved its value lives in [goal] forever, read without locking.
+        // containsKey (not a null check) because a resolved value may legitimately be null.
+        goal.let { if (it.containsKey(key)) return it[key] as RESULT }
+
+        // Slow path: resolve under [resolveLock] so each setting is transformed at most once, even when several
+        // threads first request it concurrently. The per-thread [resolving] set still detects circular deps.
+        synchronized(resolveLock) {
+            // Re-check now that we hold the lock; another thread may have resolved it while we waited.
+            goal.let { if (it.containsKey(key)) return it[key] as RESULT }
+
             // Check for circular dependency during resolution
             val currentlyResolving = resolving?.get()
             if (currentlyResolving != null && key in currentlyResolving) {
                 throw CircularOverrideException(currentlyResolving.toList() + key)
             }
 
             currentlyResolving?.add(key)
-            try {
+            val result = try {
                 overrides[key]?.let { defer ->
                     serializable[key]
                         ?.let { key.get(it as SERIALIZABLE) } // specified in settings file
@@ -282,7 +305,11 @@ public class ServerSettings private constructor(
             } finally {
                 currentlyResolving?.remove(key)
             }
-        } as RESULT
+            // Publish by re-reading [goal] (never a snapshot captured before resolution): the getter above may
+            // have recursively resolved dependencies and published them, and those additions must be preserved.
+            goal = goal + (key to result)
+            return result as RESULT
+        }
     }
 
     // For some dumb fucking reason kotlin made this internal, and it's what getOrElse should do in the first place!
@@ -328,7 +355,8 @@ public class ServerSettings private constructor(
         overrides: Map<ServerSetting<*, *>, Runtime<*>> = this.overrides,
     ) = ServerSettings(settings, overrides).also {
         it.serializable.include(this.serializable)
-        it.goal.include(this.goal)
+        // Safe to share the reference: [goal] is immutable, and copy-on-write writes replace it rather than mutate.
+        it.goal = this.goal
     }
 
     public operator fun plus(requirement: ServerSetting<*, *>): ServerSettings = copy(settings + requirement)
@@ -347,9 +375,6 @@ public class ServerSettings private constructor(
  * 2. The missing settings check in `ready()` doesn't distinguish between truly required settings
  *    and optional settings with no default. The error message could be more helpful.
  *
- * 3. The `get()` function performs lazy transformation but the caching isn't thread-safe.
- *    If called concurrently during initial ready phase, could transform the same setting twice.
- *    (Though this is documented in ServerSetting.kt TODO, worth noting here too.)
  *
  * 4. `readyUsingDefaults()` bypasses all validation with a warning, but doesn't actually
  *    enforce that defaults exist for all settings. Could still throw during get().

core/src/test/kotlin/com/lightningkite/lightningserver/definition/ServerSettingThreadSafetyTest.kt

@@ -1,6 +1,9 @@
 package com.lightningkite.lightningserver.definition
 
+import com.lightningkite.lightningserver.runtime.ServerRuntime
+import com.lightningkite.lightningserver.settings.ServerSettings
 import kotlinx.coroutines.*
+import kotlinx.serialization.builtins.serializer
 import org.junit.Test
 import java.util.concurrent.atomic.AtomicInteger
 import kotlin.test.assertEquals
@@ -59,4 +62,52 @@ class ServerSettingThreadSafetyTest {
         // The computation should only happen once despite 100 concurrent accesses
         assertEquals(1, executionCount.get(), "Runtime should only execute once, got ${executionCount.get()}")
     }
+
+    /**
+     * Hammers [ServerSettings.get] from many threads for several settings simultaneously and asserts that
+     * each setting's transformer runs exactly once. Guards against the double-transform race that existed
+     * when the goal registry was populated without synchronization.
+     */
+    @Test
+    fun `ServerSettings get transforms each setting exactly once under concurrent first-access`() = runBlocking {
+        val counters = (0 until 8).map { AtomicInteger(0) }
+        val settings = counters.mapIndexed { i, counter ->
+            ServerSetting("setting$i", 0, Int.serializer()) {
+                counter.incrementAndGet()
+                Thread.sleep(20) // Widen the race window so an unsynchronized cache would double-transform.
+                "result-$i"
+            }
+        }
+        val serverSettings = ServerSettings(settings)
+        serverSettings.readyUsingDefaults()
+
+        val runtime = stubRuntime()
+
+        // Every coroutine resolves every setting; without per-setting synchronization the transformers race.
+        (1..100).map {
+            async(Dispatchers.Default) {
+                with(runtime) { settings.map { serverSettings.get(it) } }
+            }
+        }.awaitAll()
+
+        counters.forEachIndexed { i, counter ->
+            assertEquals(1, counter.get(), "Transformer for setting$i should run exactly once, got ${counter.get()}")
+        }
+    }
+
+    /** Minimal [ServerRuntime] stub whose only usable capability is acting as the settings/transformation context. */
+    private fun stubRuntime(): ServerRuntime = object : ServerRuntime {
+        override val server get() = throw NotImplementedError()
+        override val settings get() = throw NotImplementedError()
+        override val internalSerialization get() = throw NotImplementedError()
+        override val externalSerialization get() = throw NotImplementedError()
+        override val serverId get() = ""
+        override val serverVersion get() = ""
+        override val projectName get() = ""
+        override val sharedResources get() = throw NotImplementedError()
+        override suspend fun <T> Task<T>.invoke(input: T) = throw NotImplementedError()
+        override suspend fun <PATH : com.lightningkite.lightningserver.pathing.PathSpec, T> sendWebSocketSubscriptionMessage(
+            event: com.lightningkite.lightningserver.websockets.WebSocketSubscriptionMessage<PATH, T>,
+        ) = throw NotImplementedError()
+    }
 }

core/src/test/kotlin/com/lightningkite/lightningserver/settings/ServerSettingsTest.kt

@@ -172,4 +172,25 @@ class ServerSettingsTest {
             assertEquals("A", b()) // should defer to 'a' as configured in the server
         }
     }
+
+    @Test
+    fun `resolving a setting caches its reentrantly-resolved dependencies`() {
+        // Guards get()'s copy-on-write publish: resolving `derived` reentrantly resolves `base`, and publishing
+        // derived must not clobber base's freshly-cached entry. If it did, `base` would be transformed twice.
+        val baseTransforms = java.util.concurrent.atomic.AtomicInteger(0)
+        object : ServerBuilder() {
+            val base = setting("base", "x", getter = { baseTransforms.incrementAndGet(); it })
+            val derived = setting("derived", "y")
+
+            init {
+                derived bind base
+            }
+        }.let { server ->
+            server.test({}) {
+                assertEquals("x", server.derived()) // reentrantly resolves and caches base
+                assertEquals("x", server.base())    // must hit the cache, not re-transform
+                assertEquals(1, baseTransforms.get(), "base should be transformed exactly once")
+            }
+        }
+    }
 }