From 0647ec0d22aabca258138e6990499f2921428273 Mon Sep 17 00:00:00 2001 From: Robert Navarro Date: Thu, 20 Aug 2026 08:35:03 -0700 Subject: [PATCH 1/4] fix(sync): don't trust history_complete when no messages are stored --- internal/store/query.go | 10 + internal/store/sqlc/queries.sql | 3 + internal/store/storedb/queries.sql.go | 11 + internal/syncer/channel_catalog_test.go | 14 + internal/syncer/history_verification_test.go | 281 +++++++++++++++++++ internal/syncer/message_sync.go | 91 +++++- internal/syncer/message_sync_helpers_test.go | 4 +- internal/syncer/records.go | 8 + 8 files changed, 418 insertions(+), 4 deletions(-) create mode 100644 internal/syncer/history_verification_test.go diff --git a/internal/store/query.go b/internal/store/query.go index 94013dea..6092cee3 100644 --- a/internal/store/query.go +++ b/internal/store/query.go @@ -59,6 +59,16 @@ func (s *Store) GetSyncState(ctx context.Context, scope string) (string, error) return cursor, nil } +// ChannelHasMessages reports whether any message rows are stored for the +// channel. It is an O(1) existence probe against idx_messages_channel_id and is +// safe to call once per channel per sync. +func (s *Store) ChannelHasMessages(ctx context.Context, channelID string) (bool, error) { + if channelID == "" { + return false, nil + } + return s.q.ChannelHasMessages(ctx, channelID) +} + func (s *Store) ChannelMessageBounds(ctx context.Context, channelID string) (string, string, error) { row, err := s.q.ChannelMessageBounds(ctx, channelID) if err != nil { diff --git a/internal/store/sqlc/queries.sql b/internal/store/sqlc/queries.sql index 0dec26ab..186b74cd 100644 --- a/internal/store/sqlc/queries.sql +++ b/internal/store/sqlc/queries.sql @@ -14,6 +14,9 @@ on conflict(scope) do update set delete from sync_state where scope = ?; +-- name: ChannelHasMessages :one +select exists(select 1 from messages where channel_id = ? limit 1) as has_messages; + -- name: ChannelMessageBounds :one select cast(coalesce(min(id), '') as text) as oldest_id, cast(coalesce(max(id), '') as text) as newest_id diff --git a/internal/store/storedb/queries.sql.go b/internal/store/storedb/queries.sql.go index c9b18f6d..bb537868 100644 --- a/internal/store/storedb/queries.sql.go +++ b/internal/store/storedb/queries.sql.go @@ -54,6 +54,17 @@ func (q *Queries) CatalogIntegrity(ctx context.Context) (CatalogIntegrityRow, er return i, err } +const channelHasMessages = `-- name: ChannelHasMessages :one +select exists(select 1 from messages where channel_id = ? limit 1) as has_messages +` + +func (q *Queries) ChannelHasMessages(ctx context.Context, channelID string) (bool, error) { + row := q.db.QueryRowContext(ctx, channelHasMessages, channelID) + var has_messages bool + err := row.Scan(&has_messages) + return has_messages, err +} + const channelMessageBounds = `-- name: ChannelMessageBounds :one select cast(coalesce(min(id), '') as text) as oldest_id, cast(coalesce(max(id), '') as text) as newest_id diff --git a/internal/syncer/channel_catalog_test.go b/internal/syncer/channel_catalog_test.go index 8d672003..8167a415 100644 --- a/internal/syncer/channel_catalog_test.go +++ b/internal/syncer/channel_catalog_test.go @@ -508,6 +508,13 @@ func TestSyncSkipsUnchangedThreadsWhenHistoryComplete(t *testing.T) { })) require.NoError(t, s.SetSyncState(ctx, channelLatestScope("t1"), "200")) require.NoError(t, s.SetSyncState(ctx, channelHistoryCompleteScope("t1"), "1")) + // A genuinely complete channel has the messages to show for it. Without a + // stored row the channel is re-crawled to recover the missing history. + require.NoError(t, s.UpsertMessage(ctx, store.MessageRecord{ + ID: "200", GuildID: "g1", ChannelID: "t1", ChannelName: "bug-report", + AuthorID: "u1", AuthorName: "User", CreatedAt: time.Now().UTC().Format(time.RFC3339Nano), + Content: "hello", NormalizedContent: "hello", RawJSON: `{}`, + })) client := &fakeClient{ guilds: []*discordgo.UserGuild{{ID: "g1", Name: "Guild"}}, @@ -556,6 +563,13 @@ func TestSyncSkipsUnchangedTextChannelsWhenHistoryComplete(t *testing.T) { })) require.NoError(t, s.SetSyncState(ctx, channelLatestScope("c1"), "200")) require.NoError(t, s.SetSyncState(ctx, channelHistoryCompleteScope("c1"), "1")) + // A genuinely complete channel has the messages to show for it. Without a + // stored row the channel is re-crawled to recover the missing history. + require.NoError(t, s.UpsertMessage(ctx, store.MessageRecord{ + ID: "200", GuildID: "g1", ChannelID: "c1", ChannelName: "general", + AuthorID: "u1", AuthorName: "User", CreatedAt: time.Now().UTC().Format(time.RFC3339Nano), + Content: "hello", NormalizedContent: "hello", RawJSON: `{}`, + })) client := &fakeClient{ guilds: []*discordgo.UserGuild{{ID: "g1", Name: "Guild"}}, diff --git a/internal/syncer/history_verification_test.go b/internal/syncer/history_verification_test.go new file mode 100644 index 00000000..2830c088 --- /dev/null +++ b/internal/syncer/history_verification_test.go @@ -0,0 +1,281 @@ +package syncer + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/bwmarrin/discordgo" + "github.com/stretchr/testify/require" + + "github.com/openclaw/discrawl/internal/store" +) + +func TestNeedsHistoryVerification(t *testing.T) { + t.Parallel() + + channel := &discordgo.Channel{ID: "c1", LastMessageID: "200"} + silent := &discordgo.Channel{ID: "c1"} + + require.False(t, needsHistoryVerification(nil, channelSyncState{BackfillComplete: true})) + // Not marked complete: the normal backfill path already covers it. + require.False(t, needsHistoryVerification(channel, channelSyncState{})) + // Marked complete and messages are actually stored: trustworthy. + require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true, HasMessages: true})) + // Marked complete, nothing stored, Discord reports content: verify. + require.True(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true})) + // Already verified empty once: do not crawl again. + require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true, VerifiedEmpty: true})) + // Discord reports no content either: nothing to recover. + require.False(t, needsHistoryVerification(silent, channelSyncState{BackfillComplete: true})) + + // shouldSkipChannelSync must defer to it. + require.False(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300"})) + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", HasMessages: true})) + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", VerifiedEmpty: true})) + // A channel Discord reports as empty still skips on an empty cursor. + require.True(t, shouldSkipChannelSync(silent, channelSyncState{BackfillComplete: true})) +} + +func verificationFixture(t *testing.T, messages []*discordgo.Message) (context.Context, *store.Store, *fakeClient, *Syncer, *discordgo.Channel) { + t.Helper() + + ctx := context.Background() + s, err := store.Open(ctx, filepath.Join(t.TempDir(), "discrawl.db")) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + require.NoError(t, s.UpsertChannel(ctx, store.ChannelRecord{ID: "c1", GuildID: "g1", Kind: "text", Name: "general", RawJSON: `{}`})) + + // The stranded shape seen in production: history_complete plus a latest + // cursor at or ahead of the channel head, but no message rows at all. + require.NoError(t, s.SetSyncState(ctx, channelHistoryCompleteScope("c1"), "1")) + require.NoError(t, s.SetSyncState(ctx, channelLatestScope("c1"), "300")) + + client := &fakeClient{messages: map[string][]*discordgo.Message{"c1": messages}} + channel := &discordgo.Channel{ID: "c1", GuildID: "g1", Name: "general", Type: discordgo.ChannelTypeGuildText, LastMessageID: "300"} + return ctx, s, client, New(client, s, nil), channel +} + +func storedMessage(id string) *discordgo.Message { + return &discordgo.Message{ + ID: id, + ChannelID: "c1", + GuildID: "g1", + Author: &discordgo.User{ID: "u1", Username: "user"}, + Content: "hello", + Timestamp: time.Date(2026, 5, 8, 12, 0, 0, 0, time.UTC), + } +} + +func TestHistoryVerificationRecoversStrandedChannel(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300"), storedMessage("100")}) + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Equal(t, 2, count) + require.Positive(t, client.messageCalls["c1"]) + + has, err := s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.True(t, has) + + // A recovered channel is not marked verified empty. + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker) + + // Second run sees stored messages and skips again. + before := client.messageCalls["c1"] + count, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Zero(t, count) + require.Equal(t, before, client.messageCalls["c1"]) +} + +func TestHistoryVerificationStopsAfterGenuinelyEmptyChannel(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixture(t, nil) + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Zero(t, count) + firstRun := client.messageCalls["c1"] + require.Positive(t, firstRun) + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", marker) + + // Every later run must be a no-op: no re-fetch loop. + for range 3 { + count, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Zero(t, count) + } + require.Equal(t, firstRun, client.messageCalls["c1"]) +} + +func TestHistoryVerificationSkipsChannelWithMessages(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) + require.NoError(t, s.UpsertMessage(ctx, store.MessageRecord{ + ID: "300", GuildID: "g1", ChannelID: "c1", ChannelName: "general", + AuthorID: "u1", AuthorName: "user", CreatedAt: time.Now().UTC().Format(time.RFC3339Nano), + Content: "hello", NormalizedContent: "hello", RawJSON: `{}`, + })) + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Zero(t, count) + require.Zero(t, client.messageCalls["c1"]) +} + +func TestHistoryVerificationIgnoresChannelWithoutLastMessage(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, _ := verificationFixture(t, nil) + require.NoError(t, s.DeleteSyncState(ctx, channelLatestScope("c1"))) + silent := &discordgo.Channel{ID: "c1", GuildID: "g1", Name: "general", Type: discordgo.ChannelTypeGuildText} + + count, err := svc.syncChannelMessages(ctx, "g1", silent, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Zero(t, count) + require.Zero(t, client.messageCalls["c1"]) + + // Nothing was crawled, so nothing may claim to have been verified. + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker) +} + +func TestHistoryVerificationFullRunRechecksVerifiedEmpty(t *testing.T) { + t.Parallel() + + ctx, _, client, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) + require.NoError(t, svc.store.SetSyncState(ctx, channelVerifiedEmptyScope("c1"), "1")) + + // A routine run trusts the marker. + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Zero(t, count) + require.Zero(t, client.messageCalls["c1"]) + + // An explicit full run re-checks it and recovers the message. + count, err = svc.syncChannelMessages(ctx, "g1", channel, true, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Equal(t, 1, count) + require.Positive(t, client.messageCalls["c1"]) +} + +func TestVerifiedEmptyNotWrittenForWindowedSync(t *testing.T) { + t.Parallel() + + // Every message predates the since window, so filterMessagesSince drops + // them all before they are persisted. That is not evidence of emptiness. + ctx, s, _, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) + since := time.Date(2027, 1, 1, 0, 0, 0, 0, time.UTC) + + _, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, since, false, nil) + require.NoError(t, err) + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker, "a since-windowed crawl must not mark a channel verified empty") + + require.NoError(t, (*Syncer)(nil).recordVerifiedEmptyChannel(ctx, "c1", time.Time{})) + require.NoError(t, svc.recordVerifiedEmptyChannel(ctx, "", time.Time{})) +} + +func TestChannelHasMessagesProbe(t *testing.T) { + t.Parallel() + + ctx := context.Background() + s, err := store.Open(ctx, filepath.Join(t.TempDir(), "discrawl.db")) + require.NoError(t, err) + defer func() { _ = s.Close() }() + + has, err := s.ChannelHasMessages(ctx, "") + require.NoError(t, err) + require.False(t, has) + + has, err = s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.False(t, has) + + require.NoError(t, s.UpsertChannel(ctx, store.ChannelRecord{ID: "c1", GuildID: "g1", Kind: "text", Name: "general", RawJSON: `{}`})) + require.NoError(t, s.UpsertMessage(ctx, store.MessageRecord{ + ID: "100", GuildID: "g1", ChannelID: "c1", ChannelName: "general", + AuthorID: "u1", AuthorName: "user", CreatedAt: time.Now().UTC().Format(time.RFC3339Nano), + Content: "hello", NormalizedContent: "hello", RawJSON: `{}`, + })) + + has, err = s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.True(t, has) +} + +// LatestOnly is the default for routine syncs, so stranded channels reach the +// verification path through it. Both of its branches issue the same first +// request as a full crawl (before=""), so a zero-result page is the same +// evidence of emptiness in either mode. +func TestHistoryVerificationUnderLatestOnly(t *testing.T) { + t.Parallel() + + ctx, s, _, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300"), storedMessage("100")}) + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 2, count) + + has, err := s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.True(t, has) + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker, "a channel that yielded messages must not be marked verified empty") +} + +func TestHistoryVerificationUnderLatestOnlyMarksEmptyChannel(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixture(t, nil) + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Zero(t, count) + firstRun := client.messageCalls["c1"] + require.Positive(t, firstRun) + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", marker) + + // And it does not loop on later runs. + _, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, firstRun, client.messageCalls["c1"]) +} + +func TestHistoryVerificationUnderLatestOnlyForThread(t *testing.T) { + t.Parallel() + + ctx, s, _, svc, _ := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) + thread := &discordgo.Channel{ + ID: "c1", GuildID: "g1", ParentID: "f1", Name: "thread", + Type: discordgo.ChannelTypeGuildPublicThread, LastMessageID: "300", + } + + count, err := svc.syncChannelMessages(ctx, "g1", thread, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 1, count) + + has, err := s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.True(t, has) +} diff --git a/internal/syncer/message_sync.go b/internal/syncer/message_sync.go index 84e7784f..47998fb1 100644 --- a/internal/syncer/message_sync.go +++ b/internal/syncer/message_sync.go @@ -224,6 +224,45 @@ func (s *Syncer) syncChannelMessages(ctx context.Context, guildID string, channe if err != nil { return 0, err } + if full { + // An explicit full run re-checks a channel previously verified empty. + state.VerifiedEmpty = false + } + verifying := needsHistoryVerification(channel, state) + if verifying { + // The stored cursors describe a history that is not actually present, + // so discard them and crawl the channel from scratch. Dropping them is + // safe: AdvanceChannelLatestMessageID only ever moves the stored + // pointer forward, so it cannot be rewound from here. + state = channelSyncState{} + } + count, err := s.syncChannelHistory(ctx, channel, state, full, embeddings, since, latestOnly, progress) + if err != nil || !verifying { + return count, err + } + return count, s.recordVerifiedEmptyChannel(ctx, channel.ID, since) +} + +// recordVerifiedEmptyChannel marks a channel whose verification crawl completed +// without storing anything, so needsHistoryVerification stops re-crawling it on +// every run. It is deliberately not written when the crawl was windowed by +// since: filterMessagesSince can drop every fetched message before it is +// persisted, which is not evidence that the channel is empty. +func (s *Syncer) recordVerifiedEmptyChannel(ctx context.Context, channelID string, since time.Time) error { + if s == nil || s.store == nil || channelID == "" || !since.IsZero() { + return nil + } + hasMessages, err := s.store.ChannelHasMessages(ctx, channelID) + if err != nil { + return err + } + if hasMessages { + return nil + } + return s.store.SetSyncState(ctx, channelVerifiedEmptyScope(channelID), "1") +} + +func (s *Syncer) syncChannelHistory(ctx context.Context, channel *discordgo.Channel, state channelSyncState, full bool, embeddings bool, since time.Time, latestOnly bool, progress *messageSyncProgress) (int, error) { if full { if err := s.seedChannelSyncState(ctx, channel.ID, &state); err != nil { return 0, err @@ -255,12 +294,39 @@ type channelSyncState struct { StoredLatest string BackfillCursor string BackfillComplete bool + // HasMessages reports whether any message rows exist locally for the + // channel. history_complete alone is not evidence that the history was + // actually stored: a channel can carry the marker with zero local rows. + HasMessages bool + // VerifiedEmpty reports that a previous verification pass re-fetched the + // channel from scratch and still found nothing, so it must not be + // re-fetched every run. + VerifiedEmpty bool +} + +// needsHistoryVerification reports whether a channel marked history_complete +// must be re-fetched because nothing is stored locally while Discord still +// reports the channel holds content. Without this, a channel whose messages are +// missing is skipped forever, including under --full. +func needsHistoryVerification(channel *discordgo.Channel, state channelSyncState) bool { + if channel == nil || !state.BackfillComplete { + return false + } + if state.HasMessages || state.VerifiedEmpty { + return false + } + // Discord reporting no last message is consistent with an empty channel, + // so there is nothing to recover. + return channel.LastMessageID != "" } func shouldSkipChannelSync(channel *discordgo.Channel, state channelSyncState) bool { if !state.BackfillComplete || channel == nil { return false } + if needsHistoryVerification(channel, state) { + return false + } if channel.LastMessageID == "" { return state.Latest == "" } @@ -290,12 +356,33 @@ func (s *Syncer) loadChannelSyncState(ctx context.Context, channelID string) (ch if err != nil { return channelSyncState{}, err } - return channelSyncState{ + state := channelSyncState{ Latest: latest, StoredLatest: latest, BackfillCursor: backfillCursor, BackfillComplete: backfillComplete != "", - }, nil + } + if !state.BackfillComplete { + // Only a completed channel can be wrongly trusted, so the probes below + // are unnecessary work for every other channel. + return state, nil + } + hasMessages, err := s.store.ChannelHasMessages(ctx, channelID) + if err != nil { + return channelSyncState{}, err + } + state.HasMessages = hasMessages + if hasMessages { + return state, nil + } + // Rare path: complete but empty. Only these channels pay for the extra + // lookup, so it costs nothing across a normal fleet-wide sync. + verifiedEmpty, err := s.store.GetSyncState(ctx, channelVerifiedEmptyScope(channelID)) + if err != nil { + return channelSyncState{}, err + } + state.VerifiedEmpty = verifiedEmpty != "" + return state, nil } func (s *Syncer) seedChannelSyncState(ctx context.Context, channelID string, state *channelSyncState) error { diff --git a/internal/syncer/message_sync_helpers_test.go b/internal/syncer/message_sync_helpers_test.go index a02f6f5b..5c0d9e08 100644 --- a/internal/syncer/message_sync_helpers_test.go +++ b/internal/syncer/message_sync_helpers_test.go @@ -57,7 +57,7 @@ func TestChannelSyncStateHelpers(t *testing.T) { require.False(t, shouldSkipChannelSync(nil, channelSyncState{BackfillComplete: true})) require.True(t, shouldSkipChannelSync(&discordgo.Channel{ID: "c1"}, channelSyncState{BackfillComplete: true, Latest: ""})) require.False(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: ""})) - require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300"})) + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", HasMessages: true})) require.False(t, shouldSkipLatestOnlyChannelSync(nil, channelSyncState{Latest: "300"})) require.False(t, shouldSkipLatestOnlyChannelSync(channel, channelSyncState{})) require.True(t, shouldSkipLatestOnlyChannelSync(channel, channelSyncState{Latest: "300"})) @@ -111,7 +111,7 @@ func TestChannelSyncStateStoreHelpers(t *testing.T) { require.NoError(t, s.SetSyncState(ctx, channelHistoryCompleteScope("c1"), "1")) loaded, err := svc.loadChannelSyncState(ctx, "c1") require.NoError(t, err) - require.Equal(t, channelSyncState{Latest: "200", StoredLatest: "200", BackfillCursor: "100", BackfillComplete: true}, loaded) + require.Equal(t, channelSyncState{Latest: "200", StoredLatest: "200", BackfillCursor: "100", BackfillComplete: true, HasMessages: true}, loaded) } func TestMessageChannelSyncBranches(t *testing.T) { diff --git a/internal/syncer/records.go b/internal/syncer/records.go index 50182ed8..2a83d42f 100644 --- a/internal/syncer/records.go +++ b/internal/syncer/records.go @@ -212,6 +212,14 @@ func channelHistoryCompleteScope(channelID string) string { return "channel:" + channelID + ":history_complete" } +// channelVerifiedEmptyScope records that a channel marked history_complete was +// re-fetched from scratch and genuinely yielded no messages. It stops the +// history verification in needsHistoryVerification from re-fetching such a +// channel on every run. +func channelVerifiedEmptyScope(channelID string) string { + return "channel:" + channelID + ":verified_empty" +} + func channelMessageUnavailableScope(channelID string) string { return "channel:" + channelID + ":unavailable" } From 365cb32c94edfb25304088cfb909dd70737f1969 Mon Sep 17 00:00:00 2001 From: Robert Navarro Date: Thu, 20 Aug 2026 10:47:07 -0700 Subject: [PATCH 2/4] fix(sync): always crawl full history when verifying a channel 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. --- internal/syncer/history_verification_test.go | 227 +++++++++++++++++-- internal/syncer/message_sync.go | 98 ++++++-- internal/syncer/message_sync_helpers_test.go | 8 +- 3 files changed, 283 insertions(+), 50 deletions(-) diff --git a/internal/syncer/history_verification_test.go b/internal/syncer/history_verification_test.go index 2830c088..f1afa918 100644 --- a/internal/syncer/history_verification_test.go +++ b/internal/syncer/history_verification_test.go @@ -2,7 +2,9 @@ package syncer import ( "context" + "errors" "path/filepath" + "strconv" "testing" "time" @@ -17,29 +19,42 @@ func TestNeedsHistoryVerification(t *testing.T) { channel := &discordgo.Channel{ID: "c1", LastMessageID: "200"} silent := &discordgo.Channel{ID: "c1"} + windowed := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - require.False(t, needsHistoryVerification(nil, channelSyncState{BackfillComplete: true})) + require.False(t, needsHistoryVerification(nil, channelSyncState{BackfillComplete: true}, time.Time{})) // Not marked complete: the normal backfill path already covers it. - require.False(t, needsHistoryVerification(channel, channelSyncState{})) + require.False(t, needsHistoryVerification(channel, channelSyncState{}, time.Time{})) // Marked complete and messages are actually stored: trustworthy. - require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true, HasMessages: true})) + require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true, HasMessages: true}, time.Time{})) // Marked complete, nothing stored, Discord reports content: verify. - require.True(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true})) + require.True(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true}, time.Time{})) // Already verified empty once: do not crawl again. - require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true, VerifiedEmpty: true})) + require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true, VerifiedEmpty: true}, time.Time{})) // Discord reports no content either: nothing to recover. - require.False(t, needsHistoryVerification(silent, channelSyncState{BackfillComplete: true})) + require.False(t, needsHistoryVerification(silent, channelSyncState{BackfillComplete: true}, time.Time{})) + // A windowed run cannot complete a recovery, so it does not start one. + require.False(t, needsHistoryVerification(channel, channelSyncState{BackfillComplete: true}, windowed)) // shouldSkipChannelSync must defer to it. - require.False(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300"})) - require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", HasMessages: true})) - require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", VerifiedEmpty: true})) + require.False(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300"}, time.Time{})) + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", HasMessages: true}, time.Time{})) + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", VerifiedEmpty: true}, time.Time{})) // A channel Discord reports as empty still skips on an empty cursor. - require.True(t, shouldSkipChannelSync(silent, channelSyncState{BackfillComplete: true})) + require.True(t, shouldSkipChannelSync(silent, channelSyncState{BackfillComplete: true}, time.Time{})) + // Under a window the stranded channel takes its unchanged pre-verification + // path, which for a cursor at the channel head is a skip. + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300"}, windowed)) } func verificationFixture(t *testing.T, messages []*discordgo.Message) (context.Context, *store.Store, *fakeClient, *Syncer, *discordgo.Channel) { t.Helper() + return verificationFixtureAt(t, messages, "300") +} + +// verificationFixtureAt builds the stranded shape around an explicit channel +// head so a fixture can carry more than one page of history. +func verificationFixtureAt(t *testing.T, messages []*discordgo.Message, head string) (context.Context, *store.Store, *fakeClient, *Syncer, *discordgo.Channel) { + t.Helper() ctx := context.Background() s, err := store.Open(ctx, filepath.Join(t.TempDir(), "discrawl.db")) @@ -50,10 +65,10 @@ func verificationFixture(t *testing.T, messages []*discordgo.Message) (context.C // The stranded shape seen in production: history_complete plus a latest // cursor at or ahead of the channel head, but no message rows at all. require.NoError(t, s.SetSyncState(ctx, channelHistoryCompleteScope("c1"), "1")) - require.NoError(t, s.SetSyncState(ctx, channelLatestScope("c1"), "300")) + require.NoError(t, s.SetSyncState(ctx, channelLatestScope("c1"), head)) client := &fakeClient{messages: map[string][]*discordgo.Message{"c1": messages}} - channel := &discordgo.Channel{ID: "c1", GuildID: "g1", Name: "general", Type: discordgo.ChannelTypeGuildText, LastMessageID: "300"} + channel := &discordgo.Channel{ID: "c1", GuildID: "g1", Name: "general", Type: discordgo.ChannelTypeGuildText, LastMessageID: head} return ctx, s, client, New(client, s, nil), channel } @@ -68,6 +83,17 @@ func storedMessage(id string) *discordgo.Message { } } +// storedMessages returns n messages newest first. The ids are equal-width +// decimals so the fake client's string comparisons order them the way Discord +// orders snowflakes, and the store accepts them as canonical. +func storedMessages(n int) []*discordgo.Message { + out := make([]*discordgo.Message, 0, n) + for i := n - 1; i >= 0; i-- { + out = append(out, storedMessage(strconv.Itoa(1000+i))) + } + return out +} + func TestHistoryVerificationRecoversStrandedChannel(t *testing.T) { t.Parallel() @@ -175,20 +201,31 @@ func TestHistoryVerificationFullRunRechecksVerifiedEmpty(t *testing.T) { func TestVerifiedEmptyNotWrittenForWindowedSync(t *testing.T) { t.Parallel() - // Every message predates the since window, so filterMessagesSince drops - // them all before they are persisted. That is not evidence of emptiness. - ctx, s, _, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) + // A windowed run must not verify at all: it can only reach back to the + // window, and completing there would lock the channel into a fraction of + // its history. It takes its unchanged pre-verification path instead. + ctx, s, client, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) since := time.Date(2027, 1, 1, 0, 0, 0, 0, time.UTC) - _, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, since, false, nil) + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, since, false, nil) require.NoError(t, err) + require.Zero(t, count) + require.Zero(t, client.messageCalls["c1"], "a windowed run must not start a verification crawl") marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) require.NoError(t, err) require.Empty(t, marker, "a since-windowed crawl must not mark a channel verified empty") - require.NoError(t, (*Syncer)(nil).recordVerifiedEmptyChannel(ctx, "c1", time.Time{})) - require.NoError(t, svc.recordVerifiedEmptyChannel(ctx, "", time.Time{})) + // The untouched history_complete marker is the direct evidence that no + // verification began: verifyChannelHistory clears it before crawling. + complete, err := s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", complete) + + require.NoError(t, (*Syncer)(nil).recordVerifiedEmptyChannel(ctx, "c1")) + require.NoError(t, svc.recordVerifiedEmptyChannel(ctx, "")) + require.NoError(t, (*Syncer)(nil).restoreHistoryCompleteAfterFailedVerification(ctx, "c1")) + require.NoError(t, svc.restoreHistoryCompleteAfterFailedVerification(ctx, "")) } func TestChannelHasMessagesProbe(t *testing.T) { @@ -220,9 +257,9 @@ func TestChannelHasMessagesProbe(t *testing.T) { } // LatestOnly is the default for routine syncs, so stranded channels reach the -// verification path through it. Both of its branches issue the same first -// request as a full crawl (before=""), so a zero-result page is the same -// evidence of emptiness in either mode. +// verification path through it. Verification ignores latest-only and crawls the +// whole channel; see TestHistoryVerificationLatestOnlyCrawlsWholeChannel for +// the multi-page case that separates the two. func TestHistoryVerificationUnderLatestOnly(t *testing.T) { t.Parallel() @@ -265,17 +302,157 @@ func TestHistoryVerificationUnderLatestOnlyMarksEmptyChannel(t *testing.T) { func TestHistoryVerificationUnderLatestOnlyForThread(t *testing.T) { t.Parallel() - ctx, s, _, svc, _ := verificationFixture(t, []*discordgo.Message{storedMessage("300")}) + // Threads already took the full-crawl branch before this change, and they + // still do: more than one page of history comes back whole. + ctx, s, _, svc, _ := verificationFixtureAt(t, storedMessages(250), "1249") thread := &discordgo.Channel{ ID: "c1", GuildID: "g1", ParentID: "f1", Name: "thread", - Type: discordgo.ChannelTypeGuildPublicThread, LastMessageID: "300", + Type: discordgo.ChannelTypeGuildPublicThread, LastMessageID: "1249", } count, err := svc.syncChannelMessages(ctx, "g1", thread, false, false, time.Time{}, true, nil) require.NoError(t, err) - require.Equal(t, 1, count) + require.Equal(t, 250, count) + + oldest, newest, err := s.ChannelMessageBounds(ctx, "c1") + require.NoError(t, err) + require.Equal(t, "1000", oldest) + require.Equal(t, "1249", newest) +} + +// A non-thread channel in latest-only mode is the shape the one-page bug hit: +// the zeroed cursor sent it down syncLatestChannelHistory, which stored the +// newest page and stopped, and history_complete then made every later run skip +// it. A verification must crawl the channel to its first message. +func TestHistoryVerificationLatestOnlyCrawlsWholeChannel(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixtureAt(t, storedMessages(250), "1249") + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 250, count, "a verification must not stop after one page") + + // Reaching the oldest message is the proof the crawl ran to the start: the + // one-page path leaves the oldest stored id at the 151st message. + oldest, newest, err := s.ChannelMessageBounds(ctx, "c1") + require.NoError(t, err) + require.Equal(t, "1000", oldest) + require.Equal(t, "1249", newest) + + // The crawl reached the start, so it restored history_complete itself. + complete, err := s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", complete) + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker) + + // The next run skips a channel that is now genuinely complete, and the + // history it locks in is the whole history. + calls := client.messageCalls["c1"] + count, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Zero(t, count) + require.Equal(t, calls, client.messageCalls["c1"]) + + oldest, _, err = s.ChannelMessageBounds(ctx, "c1") + require.NoError(t, err) + require.Equal(t, "1000", oldest) +} + +// A windowed run leaves a stranded channel exactly as it found it, including +// under latest-only, so nothing about it changes until an unwindowed run. +func TestHistoryVerificationSkippedForWindowedLatestOnlySync(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixtureAt(t, storedMessages(250), "1249") + since := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, since, true, nil) + require.NoError(t, err) + require.Zero(t, count) + require.Zero(t, client.messageCalls["c1"]) has, err := s.ChannelHasMessages(ctx, "c1") require.NoError(t, err) - require.True(t, has) + require.False(t, has) + + complete, err := s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", complete) + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker) + + // The unwindowed run that follows still repairs it. + count, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 250, count) +} + +// A verification that fails before storing anything leaves the channel in the +// state that triggered it, so the next run tries again. +func TestHistoryVerificationRestoresMarkerWhenNothingWasStored(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixtureAt(t, storedMessages(250), "1249") + client.messageErrors = map[string]error{"c1": errors.New("discord unavailable")} + + _, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.Error(t, err) + + complete, err := s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", complete, "a crawl that stored nothing must leave the channel verifiable") + + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker, "a failed crawl is not evidence of emptiness") + + delete(client.messageErrors, "c1") + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 250, count) +} + +// A verification that fails after storing part of the history must not leave +// history_complete over those partial rows: the rows make HasMessages true, so +// the marker would make every later run skip the channel for good. +func TestHistoryVerificationLeavesMarkerClearedAfterPartialCrawl(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixtureAt(t, storedMessages(250), "1249") + client.beforeErrors = map[string]map[string]error{"c1": {"1150": errors.New("discord unavailable")}} + + _, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.Error(t, err) + + oldest, _, err := s.ChannelMessageBounds(ctx, "c1") + require.NoError(t, err) + require.Equal(t, "1150", oldest, "the first page should have been stored") + + complete, err := s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Empty(t, complete, "a partial history must not be marked complete") + + cursor, err := s.GetSyncState(ctx, channelBackfillScope("c1")) + require.NoError(t, err) + require.Equal(t, "1150", cursor) + + // The channel is a resumable backfill, so a full run finishes it. + delete(client.beforeErrors, "c1") + count, err := svc.syncChannelMessages(ctx, "g1", channel, true, false, time.Time{}, false, nil) + require.NoError(t, err) + require.Equal(t, 150, count) + + oldest, _, err = s.ChannelMessageBounds(ctx, "c1") + require.NoError(t, err) + require.Equal(t, "1000", oldest) + + complete, err = s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", complete) } diff --git a/internal/syncer/message_sync.go b/internal/syncer/message_sync.go index 47998fb1..03f21d2a 100644 --- a/internal/syncer/message_sync.go +++ b/internal/syncer/message_sync.go @@ -2,6 +2,7 @@ package syncer import ( "context" + "errors" "fmt" "sync" "time" @@ -228,28 +229,74 @@ func (s *Syncer) syncChannelMessages(ctx context.Context, guildID string, channe // An explicit full run re-checks a channel previously verified empty. state.VerifiedEmpty = false } - verifying := needsHistoryVerification(channel, state) - if verifying { - // The stored cursors describe a history that is not actually present, - // so discard them and crawl the channel from scratch. Dropping them is - // safe: AdvanceChannelLatestMessageID only ever moves the stored - // pointer forward, so it cannot be rewound from here. - state = channelSyncState{} + if needsHistoryVerification(channel, state, since) { + return s.verifyChannelHistory(ctx, channel, embeddings, progress) } - count, err := s.syncChannelHistory(ctx, channel, state, full, embeddings, since, latestOnly, progress) - if err != nil || !verifying { - return count, err + return s.syncChannelHistory(ctx, channel, state, full, embeddings, since, latestOnly, progress) +} + +// verifyChannelHistory re-crawls a channel that carries history_complete while +// holding no message rows, and records the outcome. It always crawls the whole +// channel: routing a verification through the latest-only or incremental paths +// stores a single page (or a since window) and then lets history_complete lock +// that partial history in, because the next run sees stored rows and skips the +// channel for good. Forcing the full path here matches what syncChannelHistory +// already does for an incomplete thread. +func (s *Syncer) verifyChannelHistory(ctx context.Context, channel *discordgo.Channel, embeddings bool, progress *messageSyncProgress) (int, error) { + // The stored cursors describe a history that is not actually present, so + // crawl from scratch with an empty state. Dropping the in-memory copy is + // safe: AdvanceChannelLatestMessageID only ever moves the stored pointer + // forward, so it cannot be rewound from here. + // + // history_complete is cleared for the duration of the crawl. Leaving it in + // place strands the channel if the crawl fails partway: the rows it did + // store make HasMessages true, so the next run neither verifies nor skips + // its way back into a backfill. A crawl that reaches the start of the + // channel sets the marker again itself (syncBackfillPages), and + // verification only ever runs unwindowed, so success always restores it. A + // process killed mid-crawl leaves the marker off, which is the same + // resumable state as any other interrupted backfill. + if err := s.store.DeleteSyncState(ctx, channelHistoryCompleteScope(channel.ID)); err != nil { + return 0, err + } + count, err := s.syncFullChannelHistory(ctx, channel, channelSyncState{}, embeddings, time.Time{}, progress) + if err != nil { + return count, errors.Join(err, s.restoreHistoryCompleteAfterFailedVerification(ctx, channel.ID)) } - return count, s.recordVerifiedEmptyChannel(ctx, channel.ID, since) + return count, s.recordVerifiedEmptyChannel(ctx, channel.ID) +} + +// restoreHistoryCompleteAfterFailedVerification puts history_complete back when +// a verification crawl failed without storing anything. The channel is then in +// exactly the state that triggered verification, so the next run looks at it +// again instead of treating an untouched channel as incomplete. When the failed +// crawl did store rows the marker stays cleared: re-marking a partial history +// complete is the lock-in this path exists to prevent, and the channel is left +// as a resumable backfill that a full run continues from its cursor. +func (s *Syncer) restoreHistoryCompleteAfterFailedVerification(ctx context.Context, channelID string) error { + if s == nil || s.store == nil || channelID == "" { + return nil + } + // The store is the authority here, not the returned message count: a page + // can be committed by persistMessagePage and still report zero if a later + // step in the same call fails. + hasMessages, err := s.store.ChannelHasMessages(ctx, channelID) + if err != nil { + return err + } + if hasMessages { + return nil + } + return s.store.SetSyncState(ctx, channelHistoryCompleteScope(channelID), "1") } // recordVerifiedEmptyChannel marks a channel whose verification crawl completed // without storing anything, so needsHistoryVerification stops re-crawling it on -// every run. It is deliberately not written when the crawl was windowed by -// since: filterMessagesSince can drop every fetched message before it is -// persisted, which is not evidence that the channel is empty. -func (s *Syncer) recordVerifiedEmptyChannel(ctx context.Context, channelID string, since time.Time) error { - if s == nil || s.store == nil || channelID == "" || !since.IsZero() { +// every run. It needs no since guard of its own: needsHistoryVerification +// declines to verify a windowed sync at all, so a channel can never be recorded +// empty because filterMessagesSince dropped everything before it was persisted. +func (s *Syncer) recordVerifiedEmptyChannel(ctx context.Context, channelID string) error { + if s == nil || s.store == nil || channelID == "" { return nil } hasMessages, err := s.store.ChannelHasMessages(ctx, channelID) @@ -267,12 +314,12 @@ func (s *Syncer) syncChannelHistory(ctx context.Context, channel *discordgo.Chan if err := s.seedChannelSyncState(ctx, channel.ID, &state); err != nil { return 0, err } - if shouldSkipChannelSync(channel, state) { + if shouldSkipChannelSync(channel, state, since) { return 0, nil } return s.syncFullChannelHistory(ctx, channel, state, embeddings, since, progress) } - if shouldSkipChannelSync(channel, state) { + if shouldSkipChannelSync(channel, state, since) { return 0, nil } if latestOnly { @@ -308,23 +355,32 @@ type channelSyncState struct { // must be re-fetched because nothing is stored locally while Discord still // reports the channel holds content. Without this, a channel whose messages are // missing is skipped forever, including under --full. -func needsHistoryVerification(channel *discordgo.Channel, state channelSyncState) bool { +func needsHistoryVerification(channel *discordgo.Channel, state channelSyncState, since time.Time) bool { if channel == nil || !state.BackfillComplete { return false } if state.HasMessages || state.VerifiedEmpty { return false } + // A windowed run cannot complete a recovery: it can only fetch back to the + // window, and finishing there would mark a fraction of the history + // complete. Leave the channel on its normal path instead. Routine syncs + // carry no --since, so a stranded channel is still repaired by the next + // unwindowed run, and declining here costs nothing beyond that delay while + // keeping a deliberately narrow sync from turning into a full crawl. + if !since.IsZero() { + return false + } // Discord reporting no last message is consistent with an empty channel, // so there is nothing to recover. return channel.LastMessageID != "" } -func shouldSkipChannelSync(channel *discordgo.Channel, state channelSyncState) bool { +func shouldSkipChannelSync(channel *discordgo.Channel, state channelSyncState, since time.Time) bool { if !state.BackfillComplete || channel == nil { return false } - if needsHistoryVerification(channel, state) { + if needsHistoryVerification(channel, state, since) { return false } if channel.LastMessageID == "" { diff --git a/internal/syncer/message_sync_helpers_test.go b/internal/syncer/message_sync_helpers_test.go index 5c0d9e08..c8a20b38 100644 --- a/internal/syncer/message_sync_helpers_test.go +++ b/internal/syncer/message_sync_helpers_test.go @@ -54,10 +54,10 @@ func TestChannelSyncStateHelpers(t *testing.T) { t.Parallel() channel := &discordgo.Channel{ID: "c1", LastMessageID: "200"} - require.False(t, shouldSkipChannelSync(nil, channelSyncState{BackfillComplete: true})) - require.True(t, shouldSkipChannelSync(&discordgo.Channel{ID: "c1"}, channelSyncState{BackfillComplete: true, Latest: ""})) - require.False(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: ""})) - require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", HasMessages: true})) + require.False(t, shouldSkipChannelSync(nil, channelSyncState{BackfillComplete: true}, time.Time{})) + require.True(t, shouldSkipChannelSync(&discordgo.Channel{ID: "c1"}, channelSyncState{BackfillComplete: true, Latest: ""}, time.Time{})) + require.False(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: ""}, time.Time{})) + require.True(t, shouldSkipChannelSync(channel, channelSyncState{BackfillComplete: true, Latest: "300", HasMessages: true}, time.Time{})) require.False(t, shouldSkipLatestOnlyChannelSync(nil, channelSyncState{Latest: "300"})) require.False(t, shouldSkipLatestOnlyChannelSync(channel, channelSyncState{})) require.True(t, shouldSkipLatestOnlyChannelSync(channel, channelSyncState{Latest: "300"})) From f123ed470eba95cc67a862e0e0cf3137e8017e80 Mon Sep 17 00:00:00 2001 From: Robert Navarro Date: Thu, 20 Aug 2026 10:51:25 -0700 Subject: [PATCH 3/4] fix(sync): keep the verification marker restore off the crawl context 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. --- internal/syncer/history_verification_test.go | 20 ++++++++++++++++++++ internal/syncer/message_sync.go | 6 +++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/internal/syncer/history_verification_test.go b/internal/syncer/history_verification_test.go index f1afa918..d26ad269 100644 --- a/internal/syncer/history_verification_test.go +++ b/internal/syncer/history_verification_test.go @@ -456,3 +456,23 @@ func TestHistoryVerificationLeavesMarkerClearedAfterPartialCrawl(t *testing.T) { require.NoError(t, err) require.Equal(t, "1", complete) } + +// The per-channel deadline is the likeliest way a full verification crawl +// fails, so the marker restore must survive a cancelled context. +func TestHistoryVerificationRestoresMarkerAfterContextDeadline(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixtureAt(t, storedMessages(250), "1249") + blocked := make(chan struct{}) + defer close(blocked) + client.messageBlocks = map[string]chan struct{}{"c1": blocked} + + crawlCtx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + defer cancel() + _, err := svc.syncChannelMessages(crawlCtx, "g1", channel, false, false, time.Time{}, true, nil) + require.ErrorIs(t, err, context.DeadlineExceeded) + + complete, err := s.GetSyncState(ctx, channelHistoryCompleteScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", complete, "a crawl killed by its deadline must leave the channel verifiable") +} diff --git a/internal/syncer/message_sync.go b/internal/syncer/message_sync.go index 03f21d2a..d91db0d8 100644 --- a/internal/syncer/message_sync.go +++ b/internal/syncer/message_sync.go @@ -261,7 +261,11 @@ func (s *Syncer) verifyChannelHistory(ctx context.Context, channel *discordgo.Ch } count, err := s.syncFullChannelHistory(ctx, channel, channelSyncState{}, embeddings, time.Time{}, progress) if err != nil { - return count, errors.Join(err, s.restoreHistoryCompleteAfterFailedVerification(ctx, channel.ID)) + // The crawl now spans every page rather than one, so the per-channel + // deadline in messageChannelContext is a likely way for it to fail. + // The restore has to outlive that: on a cancelled context both of its + // queries fail, and the marker this function removed stays removed. + return count, errors.Join(err, s.restoreHistoryCompleteAfterFailedVerification(context.WithoutCancel(ctx), channel.ID)) } return count, s.recordVerifiedEmptyChannel(ctx, channel.ID) } From 93ea12271c17f9886818a33cd3d7ecd9325fa694 Mon Sep 17 00:00:00 2001 From: Robert Navarro Date: Thu, 20 Aug 2026 12:06:22 -0700 Subject: [PATCH 4/4] fix(sync): retire verified_empty once a channel holds messages Nothing deleted channel::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. --- internal/syncer/history_verification_test.go | 54 ++++++++++++++++++++ internal/syncer/message_sync.go | 35 +++++++++---- 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/internal/syncer/history_verification_test.go b/internal/syncer/history_verification_test.go index d26ad269..9dab8329 100644 --- a/internal/syncer/history_verification_test.go +++ b/internal/syncer/history_verification_test.go @@ -143,6 +143,12 @@ func TestHistoryVerificationStopsAfterGenuinelyEmptyChannel(t *testing.T) { require.Zero(t, count) } require.Equal(t, firstRun, client.messageCalls["c1"]) + + // The channel is still empty, so nothing may retire the marker that stops + // the re-fetch loop. + marker, err = s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Equal(t, "1", marker) } func TestHistoryVerificationSkipsChannelWithMessages(t *testing.T) { @@ -196,6 +202,11 @@ func TestHistoryVerificationFullRunRechecksVerifiedEmpty(t *testing.T) { require.NoError(t, err) require.Equal(t, 1, count) require.Positive(t, client.messageCalls["c1"]) + + // The crawl disproved the marker, so it is retired in the same run. + marker, err := svc.store.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker) } func TestVerifiedEmptyNotWrittenForWindowedSync(t *testing.T) { @@ -476,3 +487,46 @@ func TestHistoryVerificationRestoresMarkerAfterContextDeadline(t *testing.T) { require.NoError(t, err) require.Equal(t, "1", complete, "a crawl killed by its deadline must leave the channel verifiable") } + +// verified_empty must describe the channel as it is now, not as it was. A +// channel that gains messages after being marked keeps no marker, so losing +// those messages later still triggers a verification. +func TestVerifiedEmptyMarkerClearedOnceMessagesArrive(t *testing.T) { + t.Parallel() + + ctx, s, client, svc, channel := verificationFixture(t, []*discordgo.Message{storedMessage("400")}) + require.NoError(t, s.SetSyncState(ctx, channelVerifiedEmptyScope("c1"), "1")) + channel.LastMessageID = "400" + + // An ordinary incremental run stores the new message. It never consults + // verified_empty and never calls recordVerifiedEmptyChannel, which is why + // clearing the marker there alone would leave this channel stale. + count, err := svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 1, count) + has, err := s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.True(t, has) + + // Whatever that run left behind, the next load reconciles the marker + // against the rows now on disk. + _, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + marker, err := s.GetSyncState(ctx, channelVerifiedEmptyScope("c1")) + require.NoError(t, err) + require.Empty(t, marker, "a channel holding messages must not stay marked verified empty") + + // Losing the rows again puts the channel back into the stranded shape, and + // with no stale marker in the way it is verified rather than skipped. + require.NoError(t, s.DeleteGuildData(ctx, "g1")) + require.NoError(t, s.UpsertChannel(ctx, store.ChannelRecord{ID: "c1", GuildID: "g1", Kind: "text", Name: "general", RawJSON: `{}`})) + calls := client.messageCalls["c1"] + + count, err = svc.syncChannelMessages(ctx, "g1", channel, false, false, time.Time{}, true, nil) + require.NoError(t, err) + require.Equal(t, 1, count) + require.Greater(t, client.messageCalls["c1"], calls, "the channel must be re-crawled, not skipped") + has, err = s.ChannelHasMessages(ctx, "c1") + require.NoError(t, err) + require.True(t, has) +} diff --git a/internal/syncer/message_sync.go b/internal/syncer/message_sync.go index d91db0d8..0e3c1045 100644 --- a/internal/syncer/message_sync.go +++ b/internal/syncer/message_sync.go @@ -225,6 +225,19 @@ func (s *Syncer) syncChannelMessages(ctx context.Context, guildID string, channe if err != nil { return 0, err } + if state.HasMessages && state.VerifiedEmpty { + // The channel holds rows, so the marker describes a state that has + // since gone away. Clearing it here, before anything is decided from + // it, is what keeps it honest on every path: rows also arrive from the + // gateway tail and from ordinary incremental syncs, neither of which + // passes through verification, and a marker left over from before them + // would suppress the verification that recovers those rows if they are + // lost again. + if err := s.store.DeleteSyncState(ctx, channelVerifiedEmptyScope(channel.ID)); err != nil { + return 0, err + } + state.VerifiedEmpty = false + } if full { // An explicit full run re-checks a channel previously verified empty. state.VerifiedEmpty = false @@ -294,9 +307,12 @@ func (s *Syncer) restoreHistoryCompleteAfterFailedVerification(ctx context.Conte return s.store.SetSyncState(ctx, channelHistoryCompleteScope(channelID), "1") } -// recordVerifiedEmptyChannel marks a channel whose verification crawl completed -// without storing anything, so needsHistoryVerification stops re-crawling it on -// every run. It needs no since guard of its own: needsHistoryVerification +// recordVerifiedEmptyChannel records the outcome of a verification crawl. A +// crawl that stored nothing marks the channel, so needsHistoryVerification +// stops re-crawling it on every run; a crawl that recovered rows clears any +// marker instead, because a channel that holds messages is not empty and a +// marker saying otherwise would suppress a later recovery. It needs no since +// guard of its own: needsHistoryVerification // declines to verify a windowed sync at all, so a channel can never be recorded // empty because filterMessagesSince dropped everything before it was persisted. func (s *Syncer) recordVerifiedEmptyChannel(ctx context.Context, channelID string) error { @@ -308,7 +324,9 @@ func (s *Syncer) recordVerifiedEmptyChannel(ctx context.Context, channelID strin return err } if hasMessages { - return nil + // The crawl disproved the marker, so retire it in the same run rather + // than leaving a stale one for the next load to reconcile. + return s.store.DeleteSyncState(ctx, channelVerifiedEmptyScope(channelID)) } return s.store.SetSyncState(ctx, channelVerifiedEmptyScope(channelID), "1") } @@ -432,11 +450,10 @@ func (s *Syncer) loadChannelSyncState(ctx context.Context, channelID string) (ch return channelSyncState{}, err } state.HasMessages = hasMessages - if hasMessages { - return state, nil - } - // Rare path: complete but empty. Only these channels pay for the extra - // lookup, so it costs nothing across a normal fleet-wide sync. + // The marker is read even when rows are stored, so a stale one can be + // spotted and cleared. It is a point read on the sync_state primary key, + // paid only by channels already marked complete, and it is the single place + // the marker is read, so no caller can act on one this load did not see. verifiedEmpty, err := s.store.GetSyncState(ctx, channelVerifiedEmptyScope(channelID)) if err != nil { return channelSyncState{}, err