Skip to content

fix(desktop): harden conversation reconciliation#7210

Open
waffensam wants to merge 2 commits into
BasedHardware:mainfrom
waffensam:codex/desktop-conversation-reconciliation
Open

fix(desktop): harden conversation reconciliation#7210
waffensam wants to merge 2 commits into
BasedHardware:mainfrom
waffensam:codex/desktop-conversation-reconciliation

Conversation

@waffensam
Copy link
Copy Markdown
Contributor

Summary

Addresses #6418.
Related to #6952.

Desktop reconciliation could still regress a local transcription session after the backend had already associated it with a conversation. This hardens the client-side path by:

  • centralizing desktop conversation matching around source + started_at tolerance
  • only binding memory_created events to the session captured before rotation
  • rejecting conflicting backend conversation IDs once a session already has a backend identity
  • excluding backend-linked sessions from retry/recovery queues so retry does not mark them failed with No matching desktop conversation found on backend

Files touched

  • desktop/Desktop/Sources/AppState.swift
  • desktop/Desktop/Sources/TranscriptionRetryService.swift
  • desktop/Desktop/Sources/Rewind/Core/TranscriptionModels.swift
  • desktop/Desktop/Sources/Rewind/Core/TranscriptionStorage.swift
  • desktop/Desktop/Tests/StopReconciliationTests.swift
  • desktop/Desktop/Tests/TranscriptionSessionRecordTests.swift

Test plan

  • xcrun swift test --package-path Desktop --filter StopReconciliationTests
  • xcrun swift test --package-path Desktop --filter TranscriptionSessionRecordTests
  • git diff --check

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR hardens desktop conversation reconciliation by extracting match logic into a shared DesktopConversationMatchPolicy enum used by AppState, TranscriptionRetryService, and tests, and by adding guards throughout TranscriptionStorage that prevent backend-linked sessions from re-entering retry queues or being overwritten by conflicting completion events.

  • Centralized matching: matchesDesktopConversation and memoryEventMatchesFinishedSession replace three separate inline blocks; the startedAtTolerance constant is now a single source of truth.
  • Session protection: hasSyncedBackendIdentity and canAcceptCompletion guards in TranscriptionStorage block finishSession, markSessionFailed, incrementRetryCount, updateSessionStatus, and all four retry-queue queries from touching already-linked sessions.
  • Narrowed memory_created binding: The event is now only bound to finishedSessionId (captured before rotation), preventing live-session contamination; a started_at delta check provides an additional rejection layer.

Confidence Score: 4/5

The reconciliation hardening is solid and well-tested; the only concerns are logging inconsistencies that could mislead future debugging but do not affect correctness.

The core session-guard and queue-filter logic is correct and covered by new tests. Two logging issues exist: finishSession's success message still fires on the skipped path because the return exits only the db.write closure, and the Waiting for API reconciliation message in AppState fires even when targetSessionId is nil. Neither affects data or session state.

TranscriptionStorage.swift (finishSession log) and AppState.swift (else-branch diagnostic log) are the two spots worth a second look before merging.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/AppState.swift Centralizes matching logic via DesktopConversationMatchPolicy; narrows memory_created binding to finishedSessionId only; adds misleading log when no finished session exists
desktop/Desktop/Sources/Rewind/Core/TranscriptionStorage.swift Adds hasSyncedBackendIdentity guards to finishSession, markSessionFailed, incrementRetryCount, and updateSessionStatus; filters retry queues; finishSession success log still fires on the skipped path
desktop/Desktop/Sources/Rewind/Core/TranscriptionModels.swift Adds hasSyncedBackendIdentity and canAcceptCompletion; logic is correct and well-guarded
desktop/Desktop/Sources/TranscriptionRetryService.swift Replaces inline matching logic with DesktopConversationMatchPolicy; functionally equivalent refactor
desktop/Desktop/Tests/StopReconciliationTests.swift Updates tests to call policy methods directly; adds memory_created event matching tests with good boundary coverage
desktop/Desktop/Tests/TranscriptionSessionRecordTests.swift New test file covering hasSyncedBackendIdentity and canAcceptCompletion; all five cases tested

Comments Outside Diff (2)

  1. desktop/Desktop/Sources/Rewind/Core/TranscriptionStorage.swift, line 82-93 (link)

    P2 The return inside the db.write closure exits the closure but not finishSession itself, so the success log on line 93 still fires even when the session is skipped. A debugging session could show "Finished session X" in the log while the record was never actually updated, making it hard to diagnose reconciliation behaviour.

  2. desktop/Desktop/Sources/AppState.swift, line 18-50 (link)

    P2 memoryEventMatchesFinishedSession treats a missing source field as a permissive pass-through, while matchesDesktopConversation requires source == .desktop strictly. A non-desktop event whose started_at falls within the 10-second window and whose payload omits source could incorrectly bind to the finished desktop session. The asymmetry is tested, but an inline comment explaining the intentional difference would help reviewers avoid tightening the check inadvertently.

Reviews (1): Last reviewed commit: "fix(desktop): harden conversation reconc..." | Re-trigger Greptile

Comment on lines +2519 to 2532
} else {
didBindLocalSession = false
if memoryId != "?" {
if let startTime = targetStartTime,
let memoryStartedAt = DesktopConversationMatchPolicy.parseMemoryEventDate(
memory?["started_at"] ?? memory?["startedAt"]) {
let delta = abs(memoryStartedAt.timeIntervalSince(startTime))
if delta >= DesktopConversationMatchPolicy.startedAtTolerance {
log("Transcription: Ignoring memory_created event; started_at delta \(String(format: "%.1f", delta))s exceeds session match tolerance")
}
}
log("Transcription: Waiting for API reconciliation before binding memory_created \(memoryId) to a local session")
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 When targetSessionId is nil (no finished session was captured before this event), the "Waiting for API reconciliation" message still logs unconditionally. There is no pending session to reconcile, so the message implies a state that doesn't exist and will confuse log-based debugging.

Suggested change
} else {
didBindLocalSession = false
if memoryId != "?" {
if let startTime = targetStartTime,
let memoryStartedAt = DesktopConversationMatchPolicy.parseMemoryEventDate(
memory?["started_at"] ?? memory?["startedAt"]) {
let delta = abs(memoryStartedAt.timeIntervalSince(startTime))
if delta >= DesktopConversationMatchPolicy.startedAtTolerance {
log("Transcription: Ignoring memory_created event; started_at delta \(String(format: "%.1f", delta))s exceeds session match tolerance")
}
}
log("Transcription: Waiting for API reconciliation before binding memory_created \(memoryId) to a local session")
}
}
} else {
didBindLocalSession = false
if memoryId != "?" {
if let startTime = targetStartTime,
let memoryStartedAt = DesktopConversationMatchPolicy.parseMemoryEventDate(
memory?["started_at"] ?? memory?["startedAt"]) {
let delta = abs(memoryStartedAt.timeIntervalSince(startTime))
if delta >= DesktopConversationMatchPolicy.startedAtTolerance {
log("Transcription: Ignoring memory_created event; started_at delta \(String(format: "%.1f", delta))s exceeds session match tolerance")
} else {
log("Transcription: Waiting for API reconciliation before binding memory_created \(memoryId) to a local session")
}
} else if targetSessionId == nil {
log("Transcription: No pending finished session; memory_created \(memoryId) will be picked up by API reconciliation")
} else {
log("Transcription: Waiting for API reconciliation before binding memory_created \(memoryId) to a local session")
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant