Fix target registry: thread-safety, single creation policy, wait-for-target in get()

.github/workflows/build.yml

@@ -8,6 +8,9 @@ jobs:
   test:
     runs-on: ${{ matrix.os }}
     strategy:
+      # Don't cancel the other OS legs when one flakes, so a single intermittent
+      # failure doesn't hide the real per-OS result.
+      fail-fast: false
       matrix:
         os: [ ubuntu-latest, windows-latest, macos-latest ]
     steps:
@@ -19,7 +22,7 @@ jobs:
           distribution: 'temurin'
           java-version: 21
       - name: Install dependencies and run JVM tests
-        run: ./gradlew jvmTest
+        run: ./gradlew jvmTest --info
       - name: Run macOS native tests
         if: runner.os == 'macOS'
         run: ./gradlew macosArm64Test

core/src/commonMain/kotlin/dev/kdriver/core/browser/Browser.kt

@@ -40,7 +40,7 @@ interface Browser {
      *
      * If you want to tabs, consider using [tabs] property instead.
      */
-    val targets: MutableList<Connection>
+    val targets: List<Connection>
 
     /**
      * The WebSocket URL for the browser's debugger.

core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt

@@ -15,6 +15,7 @@
 import kotlinx.coroutines.*
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
+import kotlin.concurrent.Volatile
 import kotlin.time.Duration.Companion.seconds
 
 /**
@@ -36,16 +37,25 @@
 
     override var info: ContraDict? = null
 
-    override val targets: MutableList<Connection> = mutableListOf()
+    // The canonical registry: mutated only while holding [updateTargetInfoMutex]. After each
+    // mutation an immutable copy is published to [targetsSnapshot] so the non-suspend getters
+    // below can read a consistent view without the lock (ISSUE-5).
+    private val mutableTargets = mutableListOf<Connection>()
+
+    @Volatile
+    private var targetsSnapshot: List<Connection> = emptyList()
+
+    override val targets: List<Connection>
+        get() = targetsSnapshot
 
     override val websocketUrl: String
         get() = info?.webSocketDebuggerUrl ?: error("Browser not yet started. Call start() first")
 
     override val mainTab: Tab?
-        get() = targets.filterIsInstance<Tab>().maxByOrNull { it.type == "page" }
+        get() = targetsSnapshot.filterIsInstance<Tab>().maxByOrNull { it.type == "page" }
 
     override val tabs: List<Tab>
-        get() = targets.filterIsInstance<Tab>().filter { it.type == "page" }
+        get() = targetsSnapshot.filterIsInstance<Tab>().filter { it.type == "page" }
 
     /*
     override val cookies: CookieJar
@@ -65,6 +75,9 @@
 
     companion object {
 
+        private const val TARGET_WAIT_TIMEOUT_MS = 10_000L
+        private const val TARGET_POLL_INTERVAL_MS = 50L
+
         /**
          * The entry point for creating a new Browser instance.
          *
@@ -96,6 +109,67 @@
     protected open fun createTab(websocketUrl: String, targetInfo: Target.TargetInfo): Tab =
         DefaultTab(websocketUrl, coroutineScope, targetInfo, this)
 
+    /**
+     * Adds the target if unknown — page targets become [Tab]s, everything else a plain connection —
+     * or updates the [Target.TargetInfo] of the existing entry. Dedupes by `targetId` so a target
+     * discovered by both [updateTargets] and a `targetCreated` event can't appear twice, and so page
+     * targets are consistently [Tab]s (ISSUE-6).
+     *
+     * Must be called while holding [updateTargetInfoMutex].
+     */
+    private fun upsertTarget(targetInfo: Target.TargetInfo) {
+        val existing = mutableTargets.firstOrNull { it.targetId == targetInfo.targetId }
+        if (existing != null) {
+            existing.targetInfo = targetInfo
+        } else {
+            val wsUrl = "ws://${config.host}:${config.port}/devtools/${targetInfo.type}/${targetInfo.targetId}"
+            val created = if (targetInfo.type == "page") createTab(wsUrl, targetInfo)
+            else createConnection(wsUrl, targetInfo)
+            mutableTargets.add(created)
+            logger.debug("target added => $created")
+        }
+        publishTargetsSnapshot()
+    }
+
+    /** Publishes an immutable copy of [mutableTargets]. Must be called while holding the mutex. */
+    private fun publishTargetsSnapshot() {
+        targetsSnapshot = mutableTargets.toList()
+    }
+
+    private fun findPageTab(predicate: (Tab) -> Boolean): Tab? =
+        targetsSnapshot.filterIsInstance<Tab>().firstOrNull { it.type == "page" && predicate(it) }
+
+    /**
+     * Waits for a page [Tab] matching [predicate] to appear in the registry, instead of assuming it
+     * is already present. The matching `Tab` is only registered once the asynchronous `targetCreated`
+     * event is processed, which races `createTarget()` returning (ISSUE-8).
+     */
+    private suspend fun awaitPageTab(timeoutMillis: Long, predicate: (Tab) -> Boolean): Tab =
+        withTimeoutOrNull(timeoutMillis) {
+            var found = findPageTab(predicate)
+            while (found == null) {
+                delay(TARGET_POLL_INTERVAL_MS)
+                found = findPageTab(predicate)
+            }
+            found
+        } ?: throw IllegalStateException("Timed out waiting for a page target after ${timeoutMillis}ms")
+
+    /**
+     * Best-effort wait, during [start], for the initial page [Tab] to be registered, so `mainTab`
+     * (and tests built on `browser.mainTab`) are ready when `start()` returns instead of racing the
+     * asynchronous target discovery (ISSUE-6 residual). Re-runs [updateTargets] each poll so it
+     * works whether or not auto-discovery events are enabled. Does NOT throw on timeout — a browser
+     * may legitimately have no page yet, in which case `mainTab` stays null as before.
+     */
+    internal suspend fun awaitInitialPageTab() {
+        withTimeoutOrNull(TARGET_WAIT_TIMEOUT_MS) {
+            while (findPageTab { true } == null) {
+                delay(TARGET_POLL_INTERVAL_MS)
+                updateTargets()
+            }
+        }
+    }
+
     override suspend fun wait(timeout: Long): Browser {
         delay(timeout)
         return this
@@ -121,11 +195,11 @@
                 newWindow = newWindow,
                 enableBeginFrameControl = true
             )
-            targets.filterIsInstance<Tab>().first { it.type == "page" && it.targetId == targetId.targetId }.also {
+            awaitPageTab(TARGET_WAIT_TIMEOUT_MS) { it.targetId == targetId.targetId }.also {
                 if (it is OwnedConnection) it.owner = this
             }
         } else {
-            targets.filterIsInstance<Tab>().first { it.type == "page" }.also {
+            awaitPageTab(TARGET_WAIT_TIMEOUT_MS) { true }.also {
                 it.page.navigate(url)
                 if (it is OwnedConnection) it.owner = this
             }
@@ -224,49 +298,27 @@
         }
 
         updateTargets()
+        // Ensure the initial page target is registered before returning, so mainTab is ready
+        // (closes the residual ISSUE-6 startup race that flakes browser.mainTab ?: error(...)).
+        awaitInitialPageTab()
         return this
     }
 
     private suspend fun handleTargetUpdate(event: Any) = updateTargetInfoMutex.withLock {
         when (event) {
-            is Target.TargetInfoChangedParameter -> {
-                val targetInfo = event.targetInfo
-                val currentTab = targets.firstOrNull { it.targetId == targetInfo.targetId } ?: run {
-                    logger.warn("TargetInfoChangedParameter: Target with ID ${targetInfo.targetId} not found in current targets.")
-                    return
-                }
-                val currentTarget = currentTab.targetInfo
-
-                logger.debug("target #${targets.indexOf(currentTab)} has changed")
-                /*
-                if (logger.isLoggable(Level.FINE)) {
-                    val changes = compareTargetInfo(currentTarget, targetInfo)
-                    val changesString = changes.joinToString("\n") { (key, old, new) ->
-                        "$key: $old => $new"
-                    }
-                    logger.fine("target #${targets.indexOf(currentTab)} has changed: \n$changesString")
-                }
-                */
-
-                currentTab.targetInfo = targetInfo
-            }
-
-            is Target.TargetCreatedParameter -> {
-                val targetInfo = event.targetInfo
-                val wsUrl = "ws://${config.host}:${config.port}/devtools/${targetInfo.type}/${targetInfo.targetId}"
-
-                val newTarget = createTab(wsUrl, targetInfo)
-                targets.add(newTarget)
-                logger.debug("target ${targets.size - 1} created => $newTarget")
-            }
+            // Both create and info-changed go through the same dedupe-by-targetId upsert, so an
+            // info-changed for an as-yet-unregistered target registers it rather than being dropped.
+            is Target.TargetInfoChangedParameter -> upsertTarget(event.targetInfo)
+            is Target.TargetCreatedParameter -> upsertTarget(event.targetInfo)
 
             is Target.TargetDestroyedParameter -> {
-                val currentTab = targets.firstOrNull { it.targetId == event.targetId } ?: run {
+                val current = mutableTargets.firstOrNull { it.targetId == event.targetId } ?: run {
                     logger.warn("TargetDestroyedParameter: Target with ID ${event.targetId} not found in current targets.")
-                    return
+                    return@withLock
                 }
-                logger.debug("target removed. id ${targets.indexOf(currentTab)} => $currentTab")
-                targets.remove(currentTab)
+                logger.debug("target removed => $current")
+                mutableTargets.remove(current)
+                publishTargetsSnapshot()
             }
 
             is Target.TargetCrashedParameter -> {
@@ -318,16 +370,10 @@
     }
 
     override suspend fun updateTargets() {
-        getTargets().forEach { t ->
-            val existingTab = this.targets.firstOrNull { it.targetInfo?.targetId == t.targetId }
-
-            if (existingTab != null) {
-                existingTab.targetInfo = t.copy() // ou update manuellement les champs si nécessaire
-            } else {
-                val wsUrl = "ws://${config.host}:${config.port}/devtools/page/${t.targetId}"
-                val newConnection = createConnection(wsUrl, t)
-                this.targets.add(newConnection)
-            }
+        // Fetch outside the lock (it's a network round-trip), then apply atomically (ISSUE-5).
+        val targetInfos = getTargets()
+        updateTargetInfoMutex.withLock {
+            targetInfos.forEach { upsertTarget(it) }
         }
 
         yield() // equivalent to asyncio.sleep(0)

core/src/jvmTest/kotlin/dev/kdriver/core/browser/TargetRegistryTest.kt

@@ -0,0 +1,146 @@
+package dev.kdriver.core.browser
+
+import dev.kdriver.cdp.CommandMode
+import dev.kdriver.cdp.Serialization
+import dev.kdriver.cdp.domain.Target
+import dev.kdriver.core.connection.DefaultConnection
+import dev.kdriver.core.tab.DefaultTab
+import dev.kdriver.core.tab.Tab
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.advanceUntilIdle
+import kotlinx.coroutines.test.runCurrent
+import kotlinx.coroutines.test.runTest
+import kotlinx.io.files.Path
+import kotlinx.serialization.json.JsonElement
+import kotlinx.serialization.json.encodeToJsonElement
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNotNull
+import kotlin.test.assertNull
+import kotlin.test.assertTrue
+
+/**
+ * Target-registry behavior of [DefaultBrowser] (audit ISSUE-6 / ISSUE-8): page targets are
+ * registered as [Tab]s and deduped, and `get()` waits for the target to appear rather than
+ * assuming it is already present.
+ *
+ * Driven through a fake [connection] (a [DefaultConnection] whose `callCommand` is stubbed), so no
+ * real browser is required.
+ */
+class TargetRegistryTest {
+
+    private fun pageInfo(id: String) = Target.TargetInfo(
+        targetId = id,
+        type = "page",
+        title = "",
+        url = "about:blank",
+        attached = true,
+        canAccessOpener = false,
+    )
+
+    /** Fake connection: answers `Target.getTargets` with [targetInfos]; everything else is a no-op. */
+    private class FakeConnection(
+        scope: CoroutineScope,
+        var targetInfos: List<Target.TargetInfo>,
+    ) : DefaultConnection("ws://stub/devtools/browser/stub", scope) {
+        override suspend fun callCommand(method: String, parameter: JsonElement?, mode: CommandMode): JsonElement? =
+            when (method) {
+                "Target.getTargets" ->
+                    Serialization.json.encodeToJsonElement(Target.GetTargetsReturn(targetInfos = targetInfos))
+                else -> null
+            }
+    }
+
+    /** A [DefaultTab] whose CDP calls (e.g. `page.navigate`) are stubbed out. */
+    private class FakeTab(
+        scope: CoroutineScope,
+        targetInfo: Target.TargetInfo,
+    ) : DefaultTab("ws://stub/devtools/page/stub", scope, targetInfo) {
+        override suspend fun callCommand(method: String, parameter: JsonElement?, mode: CommandMode): JsonElement? =
+            when (method) {
+                // navigate() decodes a non-null NavigateReturn; everything else is unused here.
+                "Page.navigate" -> Serialization.json.parseToJsonElement("""{"frameId":"frame1"}""")
+                else -> null
+            }
+    }
+
+    private class TestBrowser(scope: CoroutineScope) : DefaultBrowser(
+        scope,
+        Config(browserExecutablePath = Path("/fake/chrome"), host = "127.0.0.1", port = 9222, autoDiscoverTargets = false),
+    ) {
+        override fun createTab(websocketUrl: String, targetInfo: Target.TargetInfo): Tab = FakeTab(coroutineScope, targetInfo)
+    }
+
+    @Test
+    fun updateTargets_registersPageTargetAsTab() = runTest {
+        val browser = TestBrowser(this)
+        browser.connection = FakeConnection(this, listOf(pageInfo("t1")))
+
+        browser.updateTargets()
+
+        assertEquals(1, browser.targets.size)
+        assertEquals(1, browser.tabs.size, "a page target must be registered as a Tab (visible to tabs/mainTab)")
+        assertTrue(browser.targets.first() is Tab)
+    }
+
+    @Test
+    fun updateTargets_dedupesByTargetId() = runTest {
+        val browser = TestBrowser(this)
+        browser.connection = FakeConnection(this, listOf(pageInfo("t1")))
+
+        browser.updateTargets()
+        browser.updateTargets() // same target discovered again
+
+        assertEquals(1, browser.targets.size, "repeat discovery must not duplicate a target")
+    }
+
+    @Test
+    fun get_waitsForPageTabToAppear_insteadOfThrowing() = runTest {
+        val browser = TestBrowser(this)
+        val connection = FakeConnection(this, emptyList()) // no page tab registered yet
+        browser.connection = connection
+
+        val outcome = CompletableDeferred<Result<Tab>>()
+        launch {
+            outcome.complete(runCatching { browser.get("https://example.com", newTab = false, newWindow = false) })
+        }
+
+        // Let get() start and reach its wait (no page tab exists yet).
+        runCurrent()
+
+        // The page target now appears (e.g. the initial page registering after start-up).
+        connection.targetInfos = listOf(pageInfo("t1"))
+        browser.updateTargets()
+
+        advanceUntilIdle()
+
+        val result = outcome.await()
+        assertTrue(result.isSuccess, "get() should wait for the page target, not throw NoSuchElementException")
+    }
+
+    @Test
+    fun awaitInitialPageTab_makesMainTabReadyOncePageAppears() = runTest {
+        val browser = TestBrowser(this)
+        val connection = FakeConnection(this, emptyList()) // no page target yet
+        browser.connection = connection
+
+        // Without waiting, mainTab is null right after discovery — the startup race that flakes
+        // `browser.mainTab ?: error(...)`.
+        browser.updateTargets()
+        assertNull(browser.mainTab)
+
+        // start()'s tail waits for the page; run it concurrently.
+        val job = launch { browser.awaitInitialPageTab() }
+        runCurrent()
+        assertNull(browser.mainTab, "still no page registered while waiting")
+
+        // The initial page now appears.
+        connection.targetInfos = listOf(pageInfo("t1"))
+        advanceUntilIdle()
+
+        assertNotNull(browser.mainTab, "mainTab must be ready once awaitInitialPageTab returns")
+        job.join()
+    }
+}