fix(sync): verify stored messages before trusting history_complete - #184
fix(sync): verify stored messages before trusting history_complete#184rnavarro wants to merge 4 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 session-state
|
Codex review: needs changes before merge. Reviewed August 27, 2026, 1:46 AM ET / 05:46 UTC. ClawSweeper reviewWhat this changesThe PR verifies persisted Discord history-complete state against SQLite message rows and fully re-crawls channels whose local history is missing. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessKeep open: the recovery is needed because current main still trusts completion state without checking stored rows, but failed-crawl restoration can wait indefinitely after the channel deadline. Priority: P1 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherDisCrawl syncs Discord channel history into SQLite and uses stored cursors and completion markers to decide whether a channel can be skipped. This change compares that stored state with actual message rows before sending a channel to either the normal sync path or a full recovery crawl. flowchart LR
A[Discord channel metadata] --> C[Sync decision]
B[SQLite sync markers] --> C
D[SQLite message rows] --> C
C -->|complete and rows exist| E[Skip unchanged channel]
C -->|marker without rows| F[Full history recovery]
F --> D
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Preserve the recovery design, but give its post-failure SQLite restoration a short independent deadline and prove that a blocked store returns control promptly while retaining the intended marker state. Do we have a high-confidence way to reproduce the issue? Yes: current main directly represents the reported marker-plus-cursor state and skips it without inspecting message rows; the branch's focused tests model that state. I did not execute the test suite because this is a read-only review. Is this the best way to solve the issue? No: the full-recovery approach addresses the reported state, but its detached failure cleanup needs an independent timeout before it is safe to merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 5ab5d11351b9. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
A non-thread channel marked history_complete with no stored rows was left with a single page of history in latest-only mode. Verification zeroed the in-memory cursor, so syncChannelHistory took the syncLatestChannelHistory branch, stored one page, and the untouched history_complete marker made every later run skip the channel. Route verification through syncFullChannelHistory directly so it always crawls the whole channel, and clear history_complete for the duration of the crawl so a crawl that fails partway leaves a resumable backfill rather than a partial history marked complete. The marker is restored when the crawl failed without storing anything, so the channel stays verifiable. Skip verification entirely when since is set: a windowed crawl can only reach back to the window, and completing there would lock the channel into a fraction of its history. needsHistoryVerification now holds that rule, so recordVerifiedEmptyChannel no longer needs its own since guard.
A verification crawl now covers every page instead of one, so the per-channel deadline in messageChannelContext is a realistic way for it to fail. On a cancelled context both queries in the restore fail and the history_complete marker the crawl removed stays removed, which leaves the channel with a latest cursor, no rows and no marker: latest-only runs skip it and only a full run plus one more run unstick it. Run the restore on context.WithoutCancel so it outlives the crawl.
|
The P1 is real. Fixed, with the before/after you asked for. The findingVerification zeroed the in-memory state but passed Verification now calls It also clears Before/afterTwo builds of the same tree differing only by this change (both carry a one-line auth patch so a user token authenticates; upstream main cannot otherwise run against my archive). Scratch copy of a real guild. A text channel, not a thread. Both runs start from a byte-identical database file and use the same command with no flags, which is the latest-only path. That was a full crawl, not one page: a page is 100 messages, The channel held 995 rows before I stranded it. Four ids from June to August are no longer served by Discord, and nothing new appeared, so the count carries that drift; the Two notes on the review itself: my original description said 24 channels, it is 27 today (4 text, 23 threads), both just what the query returned on the day. And the Live Verification step failed in |
clawsweeper
Bot
added
proof: sufficient
Nothing deleted channel:<id>:verified_empty. A channel marked empty that later gained messages kept the marker, so if those rows were lost again loadChannelSyncState reloaded VerifiedEmpty, needsHistoryVerification returned false, and the channel was skipped permanently. That is the state this branch exists to prevent. Reconcile the marker at load, in syncChannelMessages, whenever rows are present. That is the only point the marker is read, so no caller can act on one that was not checked against the rows first, and it covers every writer including the gateway tail and the ordinary incremental path, neither of which passes through verification. loadChannelSyncState now reads the marker for every completed channel rather than only for empty ones, which is one extra point read on the sync_state primary key. recordVerifiedEmptyChannel also deletes the marker when its crawl found rows, so a --full recheck that recovers a channel retires it in the same run rather than leaving a stale one behind.
|
A channel marked I now clear it in |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
1 participant
What I saw
I keep a personal archive of 13 guilds (about 1.1M messages, 3.9 GB SQLite) synced hourly. While tracking down forum threads that were present in
channelsbut empty inmessages, I found 24 channels carrying achannel:<id>:history_completemarker with zero stored message rows. Those channels are skipped on every run and stay skipped, including under--full. The only way I could get them to fetch was deleting theirsync_staterows by hand.Mechanism
shouldSkipChannelSyncdecides to skip from two inputs, both read out ofsync_state:state.BackfillComplete, set by thehistory_completemarkerchannel.LastMessageIDNeither input looks at
messages. So a channel that has the marker but not the rows satisfies the skip condition, and satisfies it again on every later run.--fullreaches the same check throughsyncChannelMessages, so it does not recover the channel either.What this changes
Detection:
channelSyncStategainsHasMessages, populated by a newChannelHasMessagesquery.needsHistoryVerificationreturns true when a channel is marked complete, holds no local rows, and has a non-emptyLastMessageID(Discord still reports content).AdvanceChannelLatestMessageIDonly moves the stored pointer forward, so a verification pass cannot rewind it.Two guards stop that from re-crawling forever:
channel:<id>:verified_emptyand stops re-checking.--fullclears that marker, so an explicit full run always re-checks.verified_emptyis never written when--sinceis set.filterMessagesSincecan drop every fetched message before it is persisted, so an empty result inside a window says nothing about whether the channel is empty.Cost
The probe is
select exists(select 1 from messages where channel_id = ? limit 1), which uses the existingchannel_idindex. On my archiveexplain query planreportsSEARCH messages USING COVERING INDEX idx_messages_channel_id.loadChannelSyncStatereturns early for channels that are not marked complete. The extra lookup therefore runs only for channels already carryinghistory_complete, and the second only for those that turn out to hold nothing.Changes to existing tests
Four existing tests build their fixture out of exactly the state this change stops trusting: a
history_completemarker plus a cursor, with no message rows. They fail against the fix for that reason alone, not because of what they assert.I added the missing message row to each of those four fixtures, so the setup now describes a channel that genuinely finished backfilling. Every assertion is unchanged.
What I could not determine
I do not have a trace of how these 24 channels reached that state. I can confirm the state exists in a real archive, and that nothing in the current code path recovers from it. A crawl that records the marker before its rows land, an interrupted run, or a restore that loses message rows would each produce it. This change is defensive rather than a fix for an identified write path.
Testing
go test ./...passes.internal/syncer/history_verification_test.gocovers six cases: marked-complete-with-messages still skips, marked-complete-without-messages re-fetches, an emptyLastMessageIDbehaves sanely, a zero-result verification does not re-fetch on the next run,--fullre-checks a channel markedverified_empty, and--sincedoes not write that marker.