fix(chat): keep restored legacy prompts out of the live tail (#1260)
Co-authored-by: m57 <41645758+Hy4ri@users.noreply.github.com>
This commit is contained in:
1 parent
35e6ebc8d5
commit
4d1720ad4d
5 files changed
+295
-13
No files matched your search
@@ -101,6 +101,12 @@ data class ChatMessage(
|
||||
val messageProvenance: MessageProvenance = MessageProvenance.UNKNOWN,
|
||||
/** Read from history, not observed live in this view. Never persisted as delivery state. */
|
||||
val isHistoricalCache: Boolean = false,
|
||||
/**
|
||||
* Legacy USER restored without a canonical identity or a current send receipt.
|
||||
* Placement only: keep it outside the live tail without asserting delivery.
|
||||
* Transient; UNKNOWN provenance and the persisted row remain unchanged.
|
||||
*/
|
||||
val isRestoredUnconfirmed: Boolean = false,
|
||||
)
|
||||
|
||||
/** Cached REST rows already carry their canonical identity in the persisted primary key. */
|
||||
|
||||
@@ -335,6 +335,7 @@ internal fun dedupeCachedMessages(
|
||||
restId = it.canonicalRestId,
|
||||
completionId = message.completionId ?: it.completionId,
|
||||
displayKind = message.displayKind ?: it.displayKind,
|
||||
isRestoredUnconfirmed = false,
|
||||
)
|
||||
} ?: message
|
||||
}
|
||||
@@ -403,6 +404,9 @@ internal fun mergeCachedTranscriptPage(
|
||||
restId = match.canonicalRestId ?: message.canonicalRestId,
|
||||
completionId = match.completionId ?: message.completionId,
|
||||
displayKind = match.displayKind ?: message.displayKind,
|
||||
isRestoredUnconfirmed =
|
||||
match.isRestoredUnconfirmed && message.isRestoredUnconfirmed &&
|
||||
match.canonicalRestId == null && message.canonicalRestId == null,
|
||||
)
|
||||
}.toMap()
|
||||
val resolvedOrders =
|
||||
@@ -437,6 +441,7 @@ internal fun mergeTranscriptWithLive(
|
||||
restId = (message.canonicalRestId ?: match.canonicalRestId).takeUnless { it == match.id },
|
||||
displayKind = message.displayKind ?: match.displayKind,
|
||||
isHistoricalCache = false,
|
||||
isRestoredUnconfirmed = false,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -464,6 +469,7 @@ internal fun mergeTranscriptWithLive(
|
||||
tokenCount = message.tokenCount ?: match.tokenCount,
|
||||
completionId = message.completionId ?: match.completionId,
|
||||
isHistoricalCache = false,
|
||||
isRestoredUnconfirmed = false,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -482,6 +488,7 @@ internal fun mergeTranscriptWithLive(
|
||||
content = mergedContent,
|
||||
completionId = message.completionId ?: match?.completionId,
|
||||
isHistoricalCache = false,
|
||||
isRestoredUnconfirmed = false,
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -533,9 +540,10 @@ private fun List<ChatMessage>.inTranscriptOrder(
|
||||
?: latestCanonical
|
||||
}
|
||||
if (hasPendingPredecessor) pendingOrderByLocal[message.id] = pendingLocalOrder ?: Long.MAX_VALUE
|
||||
} else if (message.isHistoricalCache) {
|
||||
} else if (message.isHistoricalCache || message.isRestoredUnconfirmed) {
|
||||
// Room groups UUID-only rows after all confirmed rows; that predecessor is
|
||||
// not a chronological anchor. Keep unresolved history before the server window.
|
||||
// not a chronological anchor. Restored legacy rows stay before the server
|
||||
// window too, without promoting their uncertain delivery to confirmed history.
|
||||
localAnchors[message.id] = beforeCanonical
|
||||
} else {
|
||||
hasPendingPredecessor = true
|
||||
|
||||
@@ -3334,6 +3334,14 @@ class ChatViewModel(
|
||||
role = MessageRole.USER,
|
||||
content = displayContent,
|
||||
tokenCount = TokenEstimator.estimate(displayContent).takeIf { it > 0 },
|
||||
// Follow-up to #1253: stripped /queue text is an unconfirmed prompt,
|
||||
// not a permanently-local command or an ambiguous legacy cache row.
|
||||
messageProvenance =
|
||||
if (result is SlashResult.QueuePrompt && displayContent != command) {
|
||||
MessageProvenance.LOCAL_PENDING
|
||||
} else {
|
||||
MessageProvenance.UNKNOWN
|
||||
},
|
||||
)
|
||||
val sessionId = _uiState.value.currentSessionId
|
||||
|
||||
@@ -4417,15 +4425,24 @@ class ChatViewModel(
|
||||
// Receipt bubbles can be absent after rejection/reconnect. Offer their identities
|
||||
// to the canonical matcher without displaying unconfirmed messages again.
|
||||
val visibleIds = snapshot.messages.map { it.id }.toSet()
|
||||
val currentSendScope = sendScope()
|
||||
val receiptRows =
|
||||
sendStore
|
||||
.all()
|
||||
.filter { it.scope == currentSendScope && it.sessionId == snapshot.currentSessionId }
|
||||
val receiptBackedIds =
|
||||
receiptRows
|
||||
.filter {
|
||||
it.state in
|
||||
setOf(PendingSendState.SENDING, PendingSendState.ACCEPTED, PendingSendState.UNKNOWN)
|
||||
}.mapTo(mutableSetOf()) { it.id }
|
||||
val receiptCandidates =
|
||||
if (cached) {
|
||||
emptyList()
|
||||
} else {
|
||||
sendStore
|
||||
.all()
|
||||
receiptRows
|
||||
.filter {
|
||||
it.scope == sendScope() && it.sessionId == snapshot.currentSessionId &&
|
||||
it.id !in visibleIds &&
|
||||
it.id !in visibleIds &&
|
||||
it.state in
|
||||
setOf(PendingSendState.SENDING, PendingSendState.ACCEPTED, PendingSendState.UNKNOWN)
|
||||
}.map {
|
||||
@@ -4444,8 +4461,8 @@ class ChatViewModel(
|
||||
val mapped = mapPage(snapshot.messages)
|
||||
val currentById = snapshot.messages.associateBy { it.id }
|
||||
// Durable provenance distinguishes new unsent prompts after process death.
|
||||
// Legacy UUID-only USER rows remain conservatively unconfirmed: UNKNOWN is
|
||||
// not evidence that the server delivered them.
|
||||
// Legacy UUID-only USER rows remain unconfirmed, but restored placement is
|
||||
// independent of delivery: UNKNOWN must not pin old prompts after fresh replies.
|
||||
val page =
|
||||
if (cached) {
|
||||
mapped.map { message ->
|
||||
@@ -4459,6 +4476,24 @@ class ChatViewModel(
|
||||
message.role == MessageRole.USER -> false
|
||||
else -> true
|
||||
},
|
||||
isRestoredUnconfirmed =
|
||||
when {
|
||||
message.role != MessageRole.USER ||
|
||||
message.canonicalRestId != null ||
|
||||
message.isPermanentlyLocal() ||
|
||||
message.messageProvenance == MessageProvenance.LOCAL_PENDING ||
|
||||
message.id in receiptBackedIds -> {
|
||||
false
|
||||
}
|
||||
|
||||
currentById[message.id] != null -> {
|
||||
currentById.getValue(message.id).isRestoredUnconfirmed
|
||||
}
|
||||
|
||||
else -> {
|
||||
true
|
||||
}
|
||||
},
|
||||
)
|
||||
}
|
||||
} else {
|
||||
|
||||
@@ -130,6 +130,38 @@ class ChatCachedHistoryTest {
|
||||
assertEquals(listOf(start.id, historicalUser.id, canonical.id), merged.map { it.id })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun restoredRepeatedUsersKeepBothOccurrencesUntilEachCanonicalPageConfirmsOne() {
|
||||
val first =
|
||||
ChatMessage(
|
||||
id = "restored-first",
|
||||
role = MessageRole.USER,
|
||||
content = "continue",
|
||||
localOrder = 1,
|
||||
isRestoredUnconfirmed = true,
|
||||
)
|
||||
val second = first.copy(id = "restored-second", localOrder = 2)
|
||||
val latest = ChatMessage(id = "rest-s-100", role = MessageRole.ASSISTANT, content = "Latest answer")
|
||||
val restored = mergeCachedTranscriptPage(listOf(first, second), listOf(latest))
|
||||
assertEquals(listOf(first.id, second.id, latest.id), restored.map { it.id })
|
||||
assertTrue(restored.take(2).all { it.canonicalRestId == null })
|
||||
|
||||
val older = ChatMessage(id = "rest-s-20", role = MessageRole.USER, content = "continue")
|
||||
val once = mergeTranscriptWithLive(listOf(older), restored, chronological = false)
|
||||
assertEquals(2, once.count { it.role == MessageRole.USER })
|
||||
assertEquals(1, once.count { it.canonicalRestId == older.id })
|
||||
val confirmed = once.single { it.canonicalRestId == older.id }
|
||||
assertFalse(confirmed.isRestoredUnconfirmed)
|
||||
val remaining = once.single { it.role == MessageRole.USER && it.canonicalRestId == null }
|
||||
assertTrue(remaining.isRestoredUnconfirmed)
|
||||
assertEquals(MessageProvenance.UNKNOWN, remaining.messageProvenance)
|
||||
|
||||
val twice = mergeTranscriptWithLive(listOf(older.copy(id = "rest-s-21")), once, chronological = false)
|
||||
assertEquals(listOf(first.id, second.id, latest.id), twice.map { it.id })
|
||||
assertEquals(listOf("rest-s-20", "rest-s-21"), twice.take(2).map { it.canonicalRestId })
|
||||
assertTrue(twice.none { it.isRestoredUnconfirmed })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun historicalRowsPrecedeCanonicalWindowWithoutChangingLiveTailOrAttachments() {
|
||||
val attachment = Attachment("content://test/file", "file.txt", "text/plain")
|
||||
|
||||
@@ -7564,6 +7564,204 @@ class ChatViewModelTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun paging_restoredLegacyUserBlockBeforeRestDoesNotFollowNewestReply() =
|
||||
runTest { verifyRestoredLegacyUserPlacement(cacheFirst = true) }
|
||||
|
||||
@Test
|
||||
fun paging_restoredLegacyUserBlockAfterRestDoesNotFollowNewestReply() =
|
||||
runTest { verifyRestoredLegacyUserPlacement(cacheFirst = false) }
|
||||
|
||||
private suspend fun TestScope.verifyRestoredLegacyUserPlacement(cacheFirst: Boolean) {
|
||||
val restored =
|
||||
(1..3).map { index ->
|
||||
ChatMessage(
|
||||
id = "restored-legacy-$index",
|
||||
role = MessageRole.USER,
|
||||
content = "Earlier prompt $index",
|
||||
// Placement must not infer delivery or chronology from wall-clock age.
|
||||
timestamp = Long.MAX_VALUE - index,
|
||||
)
|
||||
}
|
||||
val pending =
|
||||
ChatMessage(
|
||||
id = "real-pending",
|
||||
role = MessageRole.USER,
|
||||
content = "Unsent current prompt",
|
||||
timestamp = 1L,
|
||||
messageProvenance = MessageProvenance.LOCAL_PENDING,
|
||||
)
|
||||
(restored + pending).forEach { fakeRepo.persistMessage(it, "session-456") }
|
||||
val cacheRead = CompletableDeferred<Unit>()
|
||||
if (!cacheFirst) fakeRepo.dao.beforeRead = { cacheRead.await() }
|
||||
val response = CompletableDeferred<retrofit2.Response<SessionMessagesResponse>>()
|
||||
val (viewModel, _) = createViewModelWithSession()
|
||||
val api = ApiClient.hermesApi
|
||||
coEvery {
|
||||
api.getSessionMessages("session-456", any(), any(), any(), any())
|
||||
} coAnswers { response.await() }
|
||||
|
||||
viewModel.switchSession("session-456")
|
||||
runCurrent()
|
||||
response.complete(pagingResponse(100..102))
|
||||
runCurrent()
|
||||
cacheRead.complete(Unit)
|
||||
advanceUntilIdle()
|
||||
|
||||
val expected = restored.map { it.id } + (100..102).map { "rest-session-456-$it" } + pending.id
|
||||
assertEquals(
|
||||
"Restored legacy prompts must not form a stale block after the newest reply",
|
||||
expected,
|
||||
viewModel.uiState.value.messages
|
||||
.map { it.id },
|
||||
)
|
||||
restored.forEach { original ->
|
||||
val retained =
|
||||
viewModel.uiState.value.messages
|
||||
.single { it.id == original.id }
|
||||
assertEquals(original.content, retained.content)
|
||||
assertEquals(original.timestamp, retained.timestamp)
|
||||
assertEquals(MessageProvenance.UNKNOWN, retained.messageProvenance)
|
||||
assertNull("Moving a row must not invent delivery confirmation", retained.canonicalRestId)
|
||||
assertFalse("Restored placement is not proven historical delivery", retained.isHistoricalCache)
|
||||
}
|
||||
assertTrue(fakeRepo.dao.idsForSession("session-456").containsAll(restored.map { it.id } + pending.id))
|
||||
assertEquals(0, fakeRepo.dao.fullSessionReads)
|
||||
coVerify(exactly = 1) {
|
||||
api.getSessionMessages("session-456", any(), any(), any(), any())
|
||||
}
|
||||
verify(exactly = 0) { HermesWsClient.sendMessage(any(), any(), any(), any()) }
|
||||
|
||||
viewModel.syncCurrentSession()
|
||||
advanceUntilIdle()
|
||||
assertEquals(
|
||||
expected,
|
||||
viewModel.uiState.value.messages
|
||||
.map { it.id },
|
||||
)
|
||||
verify(exactly = 0) { HermesWsClient.sendMessage(any(), any(), any(), any()) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun paging_queuePromptsPersistPendingProvenanceAfterRestart() = runTest { verifyQueuedPromptRestoration("/queue") }
|
||||
|
||||
@Test
|
||||
fun paging_queueAliasPromptsPersistPendingProvenanceAfterRestart() = runTest { verifyQueuedPromptRestoration("/q") }
|
||||
|
||||
private suspend fun TestScope.verifyQueuedPromptRestoration(command: String) {
|
||||
val (viewModel, sessionId) = createViewModelWithSession()
|
||||
assertTrue(viewModel.sendMessage("$command pending queued prompt"))
|
||||
advanceUntilIdle()
|
||||
val queued =
|
||||
viewModel.uiState.value.messages
|
||||
.single { it.role == MessageRole.USER }
|
||||
assertEquals("pending queued prompt", queued.content)
|
||||
assertEquals(
|
||||
"Queued prompts must retain durable pending provenance",
|
||||
MessageProvenance.LOCAL_PENDING.name,
|
||||
fakeRepo.dao.getMessage(queued.id)?.messageProvenance,
|
||||
)
|
||||
|
||||
val api = ApiClient.hermesApi
|
||||
coEvery {
|
||||
api.getSessionMessages(sessionId, any(), any(), any(), any())
|
||||
} returns pagingResponse(100..102)
|
||||
val recreated = createViewModel()
|
||||
advanceUntilIdle()
|
||||
recreated.switchSession(sessionId)
|
||||
advanceUntilIdle()
|
||||
|
||||
val restored = recreated.uiState.value.messages
|
||||
assertEquals(queued.id, restored.last().id)
|
||||
assertTrue(restored.indexOfFirst { it.id == "rest-$sessionId-102" } in 0 until restored.lastIndex)
|
||||
assertEquals(MessageProvenance.LOCAL_PENDING, restored.last().messageProvenance)
|
||||
assertNull(restored.last().canonicalRestId)
|
||||
assertFalse(restored.last().isHistoricalCache)
|
||||
verify(exactly = 1) { HermesWsClient.sendMessage(any(), any(), any(), any()) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun paging_coldRestartReceiptBackedUnknownUserRemainsLiveTail() =
|
||||
runTest {
|
||||
val receiptBacked =
|
||||
ChatMessage(
|
||||
id = "receipt-backed-unknown-user",
|
||||
role = MessageRole.USER,
|
||||
content = "possibly accepted before restart",
|
||||
messageProvenance = MessageProvenance.UNKNOWN,
|
||||
)
|
||||
val rejected =
|
||||
ChatMessage(
|
||||
id = "receipt-backed-rejected-user",
|
||||
role = MessageRole.USER,
|
||||
content = "rejected before restart",
|
||||
messageProvenance = MessageProvenance.UNKNOWN,
|
||||
)
|
||||
fakeRepo.persistMessage(receiptBacked, "session-456")
|
||||
fakeRepo.persistMessage(rejected, "session-456")
|
||||
val store = ChatSendStore()
|
||||
val scope =
|
||||
listOf(
|
||||
AuthManager.getBaseUrl(),
|
||||
AuthManager.getSelectedProfileId() ?: AuthManager.DEFAULT_PROFILE_ID,
|
||||
AuthManager.activeProfileId.value ?: AuthManager.DEFAULT_PROFILE_ID,
|
||||
).joinToString("\u001f")
|
||||
store.put(
|
||||
PendingSend(
|
||||
id = receiptBacked.id,
|
||||
scope = scope,
|
||||
sessionId = "session-456",
|
||||
text = receiptBacked.content,
|
||||
mode = BusySendMode.QUEUE,
|
||||
state = PendingSendState.UNKNOWN,
|
||||
),
|
||||
)
|
||||
store.put(
|
||||
PendingSend(
|
||||
id = rejected.id,
|
||||
scope = scope,
|
||||
sessionId = "session-456",
|
||||
text = rejected.content,
|
||||
mode = BusySendMode.QUEUE,
|
||||
state = PendingSendState.REJECTED,
|
||||
),
|
||||
)
|
||||
val cacheRead = CompletableDeferred<Unit>()
|
||||
fakeRepo.dao.beforeRead = { cacheRead.await() }
|
||||
val (viewModel, _) = createViewModelWithSession(sendStore = store)
|
||||
coEvery {
|
||||
ApiClient.hermesApi.getSessionMessages("session-456", any(), any(), any(), any())
|
||||
} returns pagingResponse(100..102)
|
||||
|
||||
viewModel.switchSession("session-456")
|
||||
runCurrent()
|
||||
cacheRead.complete(Unit)
|
||||
advanceUntilIdle()
|
||||
|
||||
assertEquals(
|
||||
listOf(rejected.id) + (100..102).map { "rest-session-456-$it" } + receiptBacked.id,
|
||||
viewModel.uiState.value.messages
|
||||
.map { it.id },
|
||||
)
|
||||
val rejectedRetained =
|
||||
viewModel.uiState.value.messages
|
||||
.single { it.id == rejected.id }
|
||||
assertTrue(rejectedRetained.isRestoredUnconfirmed)
|
||||
assertEquals(MessageProvenance.UNKNOWN, rejectedRetained.messageProvenance)
|
||||
val retained =
|
||||
viewModel.uiState.value.messages
|
||||
.single { it.id == receiptBacked.id }
|
||||
assertFalse(retained.isHistoricalCache)
|
||||
assertFalse(retained.isRestoredUnconfirmed)
|
||||
assertEquals(MessageProvenance.UNKNOWN, retained.messageProvenance)
|
||||
assertNull(retained.canonicalRestId)
|
||||
assertEquals(PendingSendState.UNKNOWN, store.all().single { it.id == receiptBacked.id }.state)
|
||||
assertEquals(PendingSendState.REJECTED, store.all().single { it.id == rejected.id }.state)
|
||||
verify(exactly = 0) {
|
||||
HermesWsClient.sendMessage(any(), receiptBacked.content, any(), any())
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun paging_coldRestartLegacyUnknownUserRemainsConservativelyUnconfirmed() =
|
||||
runTest {
|
||||
@@ -7585,15 +7783,18 @@ class ChatViewModelTest {
|
||||
cacheRead.complete(Unit)
|
||||
advanceUntilIdle()
|
||||
assertEquals(
|
||||
(100..102).map { "rest-session-456-$it" } + ambiguous.id,
|
||||
listOf(ambiguous.id) + (100..102).map { "rest-session-456-$it" },
|
||||
viewModel.uiState.value.messages
|
||||
.map { it.id },
|
||||
)
|
||||
assertFalse(
|
||||
val retained =
|
||||
viewModel.uiState.value.messages
|
||||
.last()
|
||||
.isHistoricalCache,
|
||||
)
|
||||
.single { it.id == ambiguous.id }
|
||||
assertFalse(retained.isHistoricalCache)
|
||||
assertTrue(retained.isRestoredUnconfirmed)
|
||||
assertEquals(MessageProvenance.UNKNOWN, retained.messageProvenance)
|
||||
assertNull(retained.canonicalRestId)
|
||||
assertTrue(fakeRepo.dao.idsForSession("session-456").contains(ambiguous.id))
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Reference in new issue
Block a user