Skip to content

fix: scope reprocess/retries to specific batch#304

Open
julio4 wants to merge 1 commit into
JerryZLiu:mainfrom
julio4:feat/retry-correctness
Open

fix: scope reprocess/retries to specific batch#304
julio4 wants to merge 1 commit into
JerryZLiu:mainfrom
julio4:feat/retry-correctness

Conversation

@julio4

@julio4 julio4 commented Jul 6, 2026

Copy link
Copy Markdown

Retrying/reprocessing a batch reused the live card-generation path, which regenerates cards over a 45-minute sliding window and replaces that whole window under the reprocessed batch's id.

For a live, forward-moving batch that's fine.
For a retry of an old batch it isn't: it sweeps in neighbouring batches' cards (including their "Processing failed" cards), re-tags them to the retried batch, and rewrites them; so stale error cards get duplicated on every retry, compounding into large numbers of identical "Processing failed" cards.

This scopes reprocessing to the batch's own range (so it can't carry forward or duplicate neighbours), while still merging the regenerated card with the immediately-preceding real card so retried runs collapse into merged cards, like live does.

Changes

  • processBatch gains an isReprocess flag (default false = current live behavior).
  • When reprocessing:
    • The card-generation window is scoped to the batch's own range (windowStartTime = batchStartDate) instead of the 45-min lookback, and the write only touches that range; mirroring the failure path, which is already batch-scoped.
    • Merge context is just the immediately-preceding real card (via fetchLastTimelineCard(endingBefore:), skipping "Processing failed" cards). generateActivityCards applies the same gap/duration/LLM merge rules as live; if it merges, the write extends to absorb that one neighbour, otherwise the untouched neighbour is dropped from the write.
  • AnalysisManager.reprocessBatch / reprocessSpecificBatches pass isReprocess: true. Live processing and reprocessDay keep the default (reprocessDay already rebuilds a day from empty, so its merge is correct).

Risk / scope

  • isReprocess defaults to false, so live processing is unchanged; only the reprocess entry points change.

Reprocessing reused the live 45-min sliding window, which rewrote and re-tagged
neighbouring cards under the retried batch, duplicating stale "Processing
failed" cards on every retry. Scope reprocess writes to the batch's own range
(new isReprocess flag) and merge the regenerated card with the immediately-
preceding real card so retried runs still collapse like live.

@praxstack praxstack left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

thanks for digging into #285, this nails the right root cause: the 45-min live window sweeping neighbouring batches' cards (including stale error cards) into every retry write is exactly what amplified the duplicates. scoping the reprocess write to the batch range is the right shape of fix, and leaving the live path and reprocessDay untouched keeps the blast radius small.

built and ran the suite on this branch locally (Xcode 26.6): build succeeds, all 6 unit tests pass, matching main exactly.

one must-fix before merge though: the write-scoping block assumes the model echoes the anchor card unmodified at cards[0], and the card prompt actively tells it to do otherwise. it can silently erase part of the retried batch or reintroduce the exact duplication class this PR fixes. concrete traces and a suggested fix inline.

smaller things, take or leave:

  • the PR description says "skipping 'Processing failed' cards", but the flatMap on fetchLastTimelineCard nils the anchor out when the preceding card is failed instead of skipping past it to the previous real card. behaviour is safe (no anchor, batch-scoped write), just worth aligning the description or the code.
  • fetchLastTimelineCard(endingBefore:) has no day constraint, so the first retried batch of a day can anchor-merge into yesterday's last card. probably wants a same-day guard.

this complements #306, which handles the recap-side token growth (cause 2) of the same issue.

Comment on lines +802 to +810
let didMerge =
!existingActivityCards.isEmpty && cards.count == existingActivityCards.count
if didMerge, let anchor = reprocessMergeAnchor {
writeFromTime = Date(timeIntervalSince1970: TimeInterval(anchor.startTs))
cardsToWrite = cards
} else {
writeFromTime = batchStartDate
cardsToWrite = Array(cards.dropFirst(existingActivityCards.count))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is the one data-loss risk in the PR: didMerge only fires when the model returns exactly 1 card (the anchor path always yields existingActivityCards.count == 1), and every other output shape falls into dropFirst(1), which assumes cards[0] is an unmodified anchor echo. the prompt guarantees neither:

  • GeminiDirectProvider+ActivityCards.swift tells the model previous cards are "a draft you're revising and extending, not locked history", asks "Does your first new card continue the last previous card's work? ... merge them in your output", and says "DEFAULT TO MERGING"
  • validateTimeCoverage is order-insensitive and only checks time coverage, so both traces below pass validation (and the local/backup providers don't run it at all)

two concrete traces:

  1. merge then split (prompt-instructed, common): anchor 10:00-10:15 "coding", retried batch 10:15-10:30 is 7 more min of coding then youtube. model obeys the continuity rules and returns [10:00-10:22 coding (merged), 10:22-10:30 youtube]. count is 2, so didMerge is false, dropFirst(1) drops the merged card, and the write starts at 10:15, so the 10:15-10:22 segment is silently erased. permanent timeline gap.
  2. reorder: model returns [newCard, anchorEcho]. dropFirst(1) drops the real batch card and inserts the anchor echo as a new row while the original anchor row survives outside the delete range, giving a duplicate anchor plus a lost batch card, the same duplication class this PR is fixing.

suggest matching the anchor by time instead of by position/count:

Suggested change
let didMerge =
!existingActivityCards.isEmpty && cards.count == existingActivityCards.count
if didMerge, let anchor = reprocessMergeAnchor {
writeFromTime = Date(timeIntervalSince1970: TimeInterval(anchor.startTs))
cardsToWrite = cards
} else {
writeFromTime = batchStartDate
cardsToWrite = Array(cards.dropFirst(existingActivityCards.count))
}
if let anchor = reprocessMergeAnchor {
// Drop an exact anchor echo wherever it appears (the model may reorder).
var kept = cards
if let echoIndex = kept.firstIndex(where: {
$0.startTime == anchor.startTimestamp && $0.endTime == anchor.endTimestamp
}) {
kept.remove(at: echoIndex)
}
// A card still starting at the anchor's start means the model merged the
// anchor into it: pull the write range back so the stored anchor row is
// replaced instead of left to duplicate.
let didMerge = kept.contains { $0.startTime == anchor.startTimestamp }
writeFromTime =
didMerge
? Date(timeIntervalSince1970: TimeInterval(anchor.startTs))
: batchStartDate
cardsToWrite = kept
} else {
writeFromTime = batchStartDate
cardsToWrite = cards
}

string compare works because the anchor's "h:mm a" timestamps are handed to the model verbatim in the prompt; if you'd rather be strict, parse both sides with the same h:mm a / en_US_POSIX formatter before comparing. worst remaining case (model rewrites the anchor's start time) degrades to an overlapping card rather than a silent gap.

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.

2 participants