diff --git a/velocity/src/main/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinator.kt b/velocity/src/main/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinator.kt index 9f993a5..594f71f 100644 --- a/velocity/src/main/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinator.kt +++ b/velocity/src/main/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinator.kt @@ -30,6 +30,7 @@ internal class ResourcePackCoordinator( private val delivery = Any() private val sent = ConcurrentHashMap() private val targetAttributions = LinkedHashMap(16, 0.75f, true) + private val pendingTargetAttributions = HashMap() private var currentSettings: ResourcePackSettings? = null private var closed = false @@ -77,20 +78,28 @@ internal class ResourcePackCoordinator( synchronized(delivery) { sent.remove(playerId) targetAttributions.keys.removeIf { it.playerId == playerId } + pendingTargetAttributions.keys.removeIf { it.playerId == playerId } } } internal fun targetId(playerId: UUID, packId: UUID?): String? = synchronized(delivery) { packId - ?.let { targetAttributions[PlayerPack(playerId, it)] } + ?.let { + val key = PlayerPack(playerId, it) + pendingTargetAttributions[key] ?: targetAttributions[key] + } ?.let { it as? TargetAttribution.Exact } ?.targetId } internal fun ownsPack(playerId: UUID, packId: UUID?): Boolean = synchronized(delivery) { - packId != null && targetAttributions.containsKey(PlayerPack(playerId, packId)) + if (packId == null) false + else { + val key = PlayerPack(playerId, packId) + pendingTargetAttributions.containsKey(key) || targetAttributions.containsKey(key) + } } internal fun clear() = @@ -99,6 +108,7 @@ internal class ResourcePackCoordinator( currentSettings = null sent.clear() targetAttributions.clear() + pendingTargetAttributions.clear() } private fun dispatchLocked( @@ -113,28 +123,35 @@ internal class ResourcePackCoordinator( sent.compute(player.uniqueId) { _, existing -> if (existing == prepared.fingerprint) existing else { + val provisionalAttributions = + prepared.packIds.associate { packId -> + val key = PlayerPack(player.uniqueId, packId) + val previous = + targetAttributions.entries.firstOrNull { it.key == key }?.value + key to + when (previous) { + null -> TargetAttribution.Exact(prepared.targetId) + is TargetAttribution.Exact -> + if (previous.targetId == prepared.targetId) previous + else TargetAttribution.Ambiguous + TargetAttribution.Ambiguous -> TargetAttribution.Ambiguous + } + } + pendingTargetAttributions.putAll(provisionalAttributions) try { sender.send(player, prepared.request) } catch (failure: Exception) { if (isolateSendFailure) return@compute existing throw failure + } finally { + provisionalAttributions.keys.forEach(pendingTargetAttributions::remove) } + targetAttributions.putAll(provisionalAttributions) try { deliveryObserver.sent(player, prepared) } catch (_: Exception) { // Diagnostics must not change the delivery result. } - prepared.packIds.forEach { packId -> - val key = PlayerPack(player.uniqueId, packId) - targetAttributions[key] = - when (val previous = targetAttributions[key]) { - null -> TargetAttribution.Exact(prepared.targetId) - is TargetAttribution.Exact -> - if (previous.targetId == prepared.targetId) previous - else TargetAttribution.Ambiguous - TargetAttribution.Ambiguous -> TargetAttribution.Ambiguous - } - } while (targetAttributions.size > MAX_STATUS_ATTRIBUTIONS) { targetAttributions.entries.iterator().run { next() diff --git a/velocity/src/test/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinatorTest.kt b/velocity/src/test/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinatorTest.kt index b3a71eb..d7b62d3 100644 --- a/velocity/src/test/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinatorTest.kt +++ b/velocity/src/test/kotlin/gg/grounds/resourcepacks/velocity/ResourcePackCoordinatorTest.kt @@ -202,6 +202,54 @@ class ResourcePackCoordinatorTest { assertEquals(2, attempts) } + // Break caught: a synchronous Velocity status emitted by sendResourcePacks can arrive before + // the coordinator records ownership and disappear from diagnostics and metrics. + @Test + fun `pack ownership is visible while the request is being sent`() { + val settings = settings() + val state = readyState(settings, snapshot(settings)) + val player = player("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa") + val packId = UUID.fromString("11111111-1111-1111-1111-111111111111") + var ownedDuringSend = false + lateinit var coordinator: ResourcePackCoordinator + coordinator = + ResourcePackCoordinator( + { settings }, + { state }, + OnlinePlayerView { emptyList() }, + PackSender { sentPlayer, _ -> + ownedDuringSend = coordinator.ownsPack(sentPlayer.uniqueId, packId) + }, + VelocityPackRequestFactory(), + ) + + coordinator.onLogin(player) + + assertTrue(ownedDuringSend) + } + + // Break caught: provisional ownership can survive a non-Exception send failure and make a + // pack that was never delivered look owned by the coordinator. + @Test + fun `non exception send failure clears provisional attribution`() { + val settings = settings() + val state = readyState(settings, snapshot(settings)) + val player = player("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa") + val packId = UUID.fromString("11111111-1111-1111-1111-111111111111") + val coordinator = + ResourcePackCoordinator( + { settings }, + { state }, + OnlinePlayerView { emptyList() }, + PackSender { _, _ -> throw AssertionError("send failed") }, + VelocityPackRequestFactory(), + ) + + assertFailsWith { coordinator.onLogin(player) } + + assertNull(coordinator.targetId(player.uniqueId, packId)) + } + // Break caught: target attribution can be recorded before a failed send or survive the // player's disconnect indefinitely. @Test