diff --git a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift index aa1bd58f9e..7788dc3642 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/DeviceRegistryStore.swift @@ -76,6 +76,20 @@ public struct DeviceRegistryStore: Sendable { } } + /// Stamp a row as seen right now. Called when a strap actually connects or drops, because nothing + /// else in the BLE path writes `lastSeenAt` — before #1527 it was only ever set when the row was + /// created or promoted to active, so the Devices card reported time-since-ADDED and a strap syncing + /// every day could read "Last seen 45 d ago". + /// + /// Archived rows are excluded: "Removed - data kept" is a deliberate resting state, and a stray + /// connect must not quietly resurrect one into looking live. + public func touchLastSeen(_ id: String, at ts: Int) throws { + try dbQueue.write { db in + try db.execute(sql: "UPDATE pairedDevice SET lastSeenAt = ? WHERE id = ? AND status != 'archived'", + arguments: [ts, id]) + } + } + /// Adopt (or clear) the stable BLE identity for a registry row. `peripheralId` is the /// CBPeripheral.identifier.uuidString on iOS/Mac; passing nil un-adopts it. public func setPeripheralId(_ id: String, peripheralId: String?) throws { diff --git a/Strand/BLE/SourceCoordinator.swift b/Strand/BLE/SourceCoordinator.swift index d7500aa2d0..5b5e87d4cd 100644 --- a/Strand/BLE/SourceCoordinator.swift +++ b/Strand/BLE/SourceCoordinator.swift @@ -489,6 +489,16 @@ final class SourceCoordinator: ObservableObject { // MARK: - Identity adoption + /// Stamp the row belonging to the strap that just DISCONNECTED, resolved by the identity it adopted + /// rather than by whatever is active now. On a multi-WHOOP install those are not the same row: make + /// another strap active while this one is live, and stamping "the active row" would record a sighting + /// of a strap that was never connected — the same mis-mapping the peripheralId guard above exists to + /// prevent. A uuid no row has adopted stamps nothing. (#1527) + private func touchLastSeen(forStrap uuid: String) { + guard let device = registry.device(forPeripheralId: uuid) else { return } + registry.touchLastSeen(device.id) + } + /// The BLE engine connected to a WHOOP peripheral (`uuid`). Persist that stable identity onto the /// CURRENTLY ACTIVE device when it's a WHOOP and hasn't adopted one yet — so the legacy "my-whoop" /// learns its strap's id on first connect, and a freshly-paired WHOOP confirms its identity. @@ -508,8 +518,16 @@ final class SourceCoordinator: ObservableObject { // Track the live strap's uuid for the WHOOP->WHOOP adopt-in-place skip (#74). nil is a // disconnect/never-connected republish: clear it so a later make-active can't wrongly match a stale // link, then fall through to the existing ignore. + let previouslyConnectedTo = connectedWhoopUuid connectedWhoopUuid = uuid - guard let uuid else { return } + guard let uuid else { + // Disconnect edge. The strap was in hand right up to this instant, so stamp it NOW rather than + // leaving the connect-time value: after a ten-hour overnight session that would have the card + // reading "Last seen 10 h ago" the moment the link dropped. Only when we were actually + // connected — a nil republish with no prior link is not a sighting. (#1527) + if let previouslyConnectedTo { touchLastSeen(forStrap: previouslyConnectedTo) } + return + } let activeId = registry.activeDeviceId guard isWhoop(activeId), @@ -519,8 +537,9 @@ final class SourceCoordinator: ObservableObject { case .none: // First connect for this WHOOP row → adopt the strap's stable identity. registry.setPeripheralId(activeId, peripheralId: uuid) + registry.touchLastSeen(activeId) case .some(uuid): - break // already adopted this exact strap → nothing to do + registry.touchLastSeen(activeId) // already adopted this exact strap → only the sighting is new case .some(let existing): // A DIFFERENT strap connected under this WHOOP row. Re-adopt ONLY when this is the #52 stale-pin // handoff — i.e. the engine is genuinely encrypted-bonded to the strap whose id just arrived. @@ -531,6 +550,7 @@ final class SourceCoordinator: ObservableObject { if live.encryptedBond { live.append(log: "Multi-WHOOP (#52): active device \(activeId) was pinned to strap \(existing) which refused to bond — re-adopting the working strap \(uuid).") registry.setPeripheralId(activeId, peripheralId: uuid) + registry.touchLastSeen(activeId) } else { live.append(log: "Multi-WHOOP: active device \(activeId) is registered to strap \(existing) but \(uuid) connected — not overwriting.") } diff --git a/Strand/Data/DeviceRegistry.swift b/Strand/Data/DeviceRegistry.swift index 84b23a44b0..5060fae5d7 100644 --- a/Strand/Data/DeviceRegistry.swift +++ b/Strand/Data/DeviceRegistry.swift @@ -125,6 +125,13 @@ final class DeviceRegistry: ObservableObject { reload() } + /// Stamp a device as seen right now — a real connect or disconnect, not every inbound packet, which + /// would be a write per second for no more truth. Refreshes the published list. Best-effort. (#1527) + func touchLastSeen(_ id: String, at ts: Int = Int(Date().timeIntervalSince1970)) { + try? store.touchLastSeen(id, at: ts) + reload() + } + /// Find the paired device that has adopted a given BLE peripheral, if any. A plain read of the /// store (no reload) — returns nil on any error or when no row has adopted that peripheral yet. func device(forPeripheralId peripheralId: String) -> PairedDevice? { diff --git a/android/app/src/main/java/com/noop/ble/SourceCoordinator.kt b/android/app/src/main/java/com/noop/ble/SourceCoordinator.kt index 5b0573684c..f56d4de32b 100644 --- a/android/app/src/main/java/com/noop/ble/SourceCoordinator.kt +++ b/android/app/src/main/java/com/noop/ble/SourceCoordinator.kt @@ -212,6 +212,23 @@ class SourceCoordinator( } } + /** Stamp the row belonging to the strap that just DISCONNECTED, resolved by the identity it adopted + * rather than by whatever is active now. On a multi-WHOOP install those are not the same row: make + * another strap active while this one is live, and stamping "the active row" would record a sighting of + * a strap that was never connected — the same mis-mapping the peripheralId guard below exists to + * prevent. An address no row has adopted stamps nothing. + * + * Matched the way the connect path below matches — `ignoreCase` — and NOT through + * [DeviceRegistry.deviceForPeripheralId], whose `WHERE peripheralId = ?` is case-SENSITIVE in SQLite. + * A stored address differing only in case is the same strap to the guard below, so resolving it more + * strictly here would adopt on connect and then silently stamp nothing on disconnect — the same + * quiet staleness this whole change exists to remove. The Swift twin keeps its exact lookup because + * its connect path compares exactly too; each side matches ITS OWN adopt rule. (#1527) */ + private suspend fun touchLastSeenForStrap(address: String) { + val row = registry.all().firstOrNull { it.peripheralId.equals(address, ignoreCase = true) } ?: return + registry.touchLastSeen(row.id) + } + /** * The BLE engine connected to a WHOOP strap at [address] (null on disconnect). Persist that stable * identity onto the CURRENTLY ACTIVE device when it's a WHOOP and hasn't adopted one yet — so the @@ -230,8 +247,18 @@ class SourceCoordinator( // Track the live strap's address for the WHOOP->WHOOP adopt-in-place skip (#74). A null address is a // disconnect/never-connected republish: clear it so a later make-active can't wrongly match a stale // link, then fall through to the existing ignore. + val previouslyConnectedTo = connectedWhoopAddress connectedWhoopAddress = address - if (address == null) return + if (address == null) { + // Disconnect edge. The strap was in hand right up to this instant, so stamp it NOW rather than + // leaving the connect-time value: after a ten-hour overnight session that would have the card + // reading "Last seen 10 h ago" the moment the link dropped. Only when we were actually + // connected — a null republish with no prior link is not a sighting. (#1527) + if (previouslyConnectedTo != null) { + scope.launch { runCatching { touchLastSeenForStrap(previouslyConnectedTo) } } + } + return + } scope.launch { val activeId = registry.activeDeviceId() ?: return@launch val devices = registry.all() @@ -240,11 +267,14 @@ class SourceCoordinator( val existing = row.peripheralId when { - existing == null -> + existing == null -> { // First connect for this WHOOP row → adopt the strap's stable identity (its address). registry.setPeripheralId(activeId, address) + registry.touchLastSeen(activeId) + } existing.equals(address, ignoreCase = true) -> { - // Already adopted this exact strap → nothing to do. + // Already adopted this exact strap → only the sighting is new. (#1527) + registry.touchLastSeen(activeId) } else -> // A DIFFERENT strap connected under this WHOOP row. Never silently overwrite — that would diff --git a/android/app/src/main/java/com/noop/data/DeviceRegistry.kt b/android/app/src/main/java/com/noop/data/DeviceRegistry.kt index 200499f35f..22ec1f9cf2 100644 --- a/android/app/src/main/java/com/noop/data/DeviceRegistry.kt +++ b/android/app/src/main/java/com/noop/data/DeviceRegistry.kt @@ -121,6 +121,11 @@ class DeviceRegistry( } } + /** Stamp a device as seen right now — a real connect or disconnect, not every inbound packet, which + * would be a write per second for no more truth. Twin of Swift `DeviceRegistry.touchLastSeen`. (#1527) */ + suspend fun touchLastSeen(id: String, now: Long = System.currentTimeMillis() / 1000) = + dao.touchLastSeen(id, now) + /** Archive a device — keeps its row and samples (invariant I4). */ suspend fun archive(id: String) = dao.archiveDevice(id) diff --git a/android/app/src/main/java/com/noop/data/DeviceRegistryDao.kt b/android/app/src/main/java/com/noop/data/DeviceRegistryDao.kt index 02bbd6402e..7275f2839d 100644 --- a/android/app/src/main/java/com/noop/data/DeviceRegistryDao.kt +++ b/android/app/src/main/java/com/noop/data/DeviceRegistryDao.kt @@ -34,6 +34,15 @@ interface DeviceRegistryDao { @Query("UPDATE pairedDevice SET status = 'active', lastSeenAt = :now WHERE id = :id") suspend fun promote(id: String, now: Long) + /** Stamp a device as seen right now. Nothing in the BLE path wrote `lastSeenAt` before #1527 — it was + * set only when the row was created or promoted to active — so the Devices card reported + * time-since-ADDED, and a strap syncing daily could read "Last seen 45 d ago". + * + * Archived rows are excluded: "Removed - data kept" is a deliberate resting state and a stray connect + * must not quietly resurrect one into looking live. Twin of the Swift store's `touchLastSeen`. */ + @Query("UPDATE pairedDevice SET lastSeenAt = :now WHERE id = :id AND status != 'archived'") + suspend fun touchLastSeen(id: String, now: Long) + /** Archive a device (keeps the row + its samples — invariant I4). */ @Query("UPDATE pairedDevice SET status = 'archived' WHERE id = :id") suspend fun archiveDevice(id: String) diff --git a/android/app/src/test/java/com/noop/analytics/RegistryDayOwnerSourceTest.kt b/android/app/src/test/java/com/noop/analytics/RegistryDayOwnerSourceTest.kt index 3169fce5c8..dc0008b194 100644 --- a/android/app/src/test/java/com/noop/analytics/RegistryDayOwnerSourceTest.kt +++ b/android/app/src/test/java/com/noop/analytics/RegistryDayOwnerSourceTest.kt @@ -44,6 +44,12 @@ class RegistryDayOwnerSourceTest { devices[id]?.let { devices[id] = it.copy(model = model) } } override suspend fun renameDevice(id: String, nickname: String?) {} + override suspend fun touchLastSeen(id: String, now: Long) { + // #1527, mirrors the query's `AND status != 'archived'`. + devices[id]?.let { + if (it.status != DeviceStatus.archived.name) devices[id] = it.copy(lastSeenAt = now) + } + } override suspend fun setPeripheralId(id: String, peripheralId: String?) { devices[id]?.let { devices[id] = it.copy(peripheralId = peripheralId) } } diff --git a/android/app/src/test/java/com/noop/ble/SourceCoordinatorAdoptionTest.kt b/android/app/src/test/java/com/noop/ble/SourceCoordinatorAdoptionTest.kt index b84ed1eb39..d86a7dba31 100644 --- a/android/app/src/test/java/com/noop/ble/SourceCoordinatorAdoptionTest.kt +++ b/android/app/src/test/java/com/noop/ble/SourceCoordinatorAdoptionTest.kt @@ -60,6 +60,12 @@ class SourceCoordinatorAdoptionTest { override suspend fun setPeripheralId(id: String, peripheralId: String?) { devices[id]?.let { devices[id] = it.copy(peripheralId = peripheralId) } } + override suspend fun touchLastSeen(id: String, now: Long) { + // Reproduces the query's `AND status != 'archived'` guard. + devices[id]?.let { + if (it.status != DeviceStatus.archived.name) devices[id] = it.copy(lastSeenAt = now) + } + } override suspend fun deviceForPeripheralId(peripheralId: String): PairedDeviceRow? = devices.values.firstOrNull { it.peripheralId == peripheralId } override suspend fun setDayOwner(row: DayOwnershipRow) { owners[row.day] = row } @@ -384,4 +390,152 @@ class SourceCoordinatorAdoptionTest { assertEquals("a different WHOOP must drop the current link", 1, stops) assertEquals("a different WHOOP must reconnect", 1, starts) } + + // MARK: - "Last seen" is stamped by the link, not by the wizard (#1527) + + /** + * The regression: a strap that connects must move `lastSeenAt`. Nothing in the BLE path wrote the + * column before this, so the Devices card reported time-since-ADDED and a strap syncing every day + * read "Last seen 45 d ago". + */ + @Test + fun aConnectStampsTheActiveRowAsSeen() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = "AA:BB:CC:DD:EE:01") + } + coordinatorOver(dao).connectedPeripheralChanged("AA:BB:CC:DD:EE:01") + assertTrue("a connect must move lastSeenAt off its seeded value", + dao.devices["my-whoop"]!!.lastSeenAt > 100) + } + + /** A first connect adopts the identity AND records the sighting — the two are the same event. */ + @Test + fun aFirstConnectAdoptsTheIdentityAndStamps() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = null) + } + coordinatorOver(dao).connectedPeripheralChanged("AA:BB:CC:DD:EE:01") + assertEquals("AA:BB:CC:DD:EE:01", dao.devices["my-whoop"]!!.peripheralId) + assertTrue(dao.devices["my-whoop"]!!.lastSeenAt > 100) + } + + /** + * The disconnect edge stamps too. Without it the card would report the time of the CONNECT, so an + * overnight session would read "Last seen 10 h ago" the instant the link dropped. The connect stamp is + * rewound here so the assertion can only pass on the disconnect's own write. + */ + @Test + fun aDisconnectStampsTheRowAsSeenRatherThanLeavingTheConnectTime() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = "AA:BB:CC:DD:EE:01") + } + val coordinator = coordinatorOver(dao) + coordinator.connectedPeripheralChanged("AA:BB:CC:DD:EE:01") + dao.devices["my-whoop"] = dao.devices["my-whoop"]!!.copy(lastSeenAt = 100) // rewind the connect stamp + coordinator.connectedPeripheralChanged(null) + assertTrue("the disconnect edge must stamp the row itself", + dao.devices["my-whoop"]!!.lastSeenAt > 100) + } + + /** A null republish with no live link is not a sighting — nothing was ever connected to record. */ + @Test + fun aNullWithNoPriorConnectIsNotASighting() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = "AA:BB:CC:DD:EE:01") + } + coordinatorOver(dao).connectedPeripheralChanged(null) + assertEquals(100, dao.devices["my-whoop"]!!.lastSeenAt) + } + + /** + * The guard that matters on a multi-WHOOP install: a DIFFERENT strap connecting under this row must + * neither overwrite its identity nor claim to be a sighting OF IT. Same reasoning as the peripheralId + * guard above — that strap is not this row. + */ + @Test + fun aDifferentStrapDoesNotStampThisRow() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = "AA:BB:CC:DD:EE:01") + } + coordinatorOver(dao).connectedPeripheralChanged("AA:BB:CC:DD:EE:99") + assertEquals("AA:BB:CC:DD:EE:01", dao.devices["my-whoop"]!!.peripheralId) + assertEquals(100, dao.devices["my-whoop"]!!.lastSeenAt) + } + + /** + * The disconnect stamps the strap that was CONNECTED, not whatever is active by the time it drops. + * Make another WHOOP active while this one is live — a multi-WHOOP install can — and resolving "the + * active row" would record a sighting of a strap nobody connected. That is the same mis-mapping the + * peripheralId guard exists to prevent, so the disconnect resolves by adopted identity instead. + */ + @Test + fun theDisconnectStampsTheStrapThatWasConnectedNotWhateverIsActiveNow() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["whoop-a"] = whoopRow("whoop-a", peripheralId = "AA:BB:CC:DD:EE:01") + devices["whoop-b"] = whoopRow("whoop-b", peripheralId = "AA:BB:CC:DD:EE:02") + .copy(status = DeviceStatus.paired.name) + } + val coordinator = coordinatorOver(dao) + coordinator.connectedPeripheralChanged("AA:BB:CC:DD:EE:01") + + // whoop-b becomes active while whoop-a is still the live link; rewind both stamps so the + // assertions can only be satisfied by the disconnect's own write. + dao.devices["whoop-a"] = dao.devices["whoop-a"]!!.copy( + status = DeviceStatus.paired.name, lastSeenAt = 100) + dao.devices["whoop-b"] = dao.devices["whoop-b"]!!.copy( + status = DeviceStatus.active.name, lastSeenAt = 100) + + coordinator.connectedPeripheralChanged(null) + + assertTrue("the strap that was connected must be the one stamped", + dao.devices["whoop-a"]!!.lastSeenAt > 100) + assertEquals("a strap that was never connected must not be stamped", + 100, dao.devices["whoop-b"]!!.lastSeenAt) + } + + /** + * The disconnect must resolve identity the way the connect path does. That comparison is + * `ignoreCase` — a stored address differing only in case is the SAME strap to the adopt guard — so a + * stricter lookup here would adopt on connect and then quietly stamp nothing on disconnect. + */ + @Test + fun aDisconnectResolvesTheStrapCaseInsensitivelyLikeTheAdoptGuard() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = "aa:bb:cc:dd:ee:01") + } + val coordinator = coordinatorOver(dao) + coordinator.connectedPeripheralChanged("AA:BB:CC:DD:EE:01") // same strap, upper-cased + dao.devices["my-whoop"] = dao.devices["my-whoop"]!!.copy(lastSeenAt = 100) + coordinator.connectedPeripheralChanged(null) + assertTrue("a case-different address is the same strap to the adopt guard, so it must stamp", + dao.devices["my-whoop"]!!.lastSeenAt > 100) + } + + /** An address no row has adopted stamps nothing rather than guessing at the active row. */ + @Test + fun aDisconnectFromAnUnadoptedStrapStampsNothing() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["my-whoop"] = whoopRow("my-whoop", peripheralId = "AA:BB:CC:DD:EE:01") + } + val coordinator = coordinatorOver(dao) + coordinator.connectedPeripheralChanged("AA:BB:CC:DD:EE:77") // different strap, never adopted + dao.devices["my-whoop"] = dao.devices["my-whoop"]!!.copy(lastSeenAt = 100) + coordinator.connectedPeripheralChanged(null) + assertEquals(100, dao.devices["my-whoop"]!!.lastSeenAt) + } + + /** + * "Removed - data kept" is a deliberate resting state: a stray connect must not quietly resurrect an + * archived row into looking live. The real guard is the query's `AND status != 'archived'`; the fake + * reproduces it, so this pins the CONTRACT callers rely on rather than Room's SQL. + */ + @Test + fun anArchivedRowIsNotStamped() = runBlocking { + val dao = FakeRegistryDao().apply { + devices["old-whoop"] = whoopRow("old-whoop", peripheralId = "AA:BB:CC:DD:EE:02") + .copy(status = DeviceStatus.archived.name) + } + registryWith(dao).touchLastSeen("old-whoop") + assertEquals(100, dao.devices["old-whoop"]!!.lastSeenAt) + } } diff --git a/android/app/src/test/java/com/noop/data/DeviceRegistryTest.kt b/android/app/src/test/java/com/noop/data/DeviceRegistryTest.kt index 97fbb06a5b..fa9b475580 100644 --- a/android/app/src/test/java/com/noop/data/DeviceRegistryTest.kt +++ b/android/app/src/test/java/com/noop/data/DeviceRegistryTest.kt @@ -62,6 +62,12 @@ class DeviceRegistryTest { override suspend fun renameDevice(id: String, nickname: String?) { devices[id]?.let { devices[id] = it.copy(nickname = nickname) } } + override suspend fun touchLastSeen(id: String, now: Long) { + // #1527, mirrors the query's `AND status != 'archived'`. + devices[id]?.let { + if (it.status != DeviceStatus.archived.name) devices[id] = it.copy(lastSeenAt = now) + } + } override suspend fun setPeripheralId(id: String, peripheralId: String?) { devices[id]?.let { devices[id] = it.copy(peripheralId = peripheralId) }