fix(wiretap): safely retry unresolved cache entries - #190
Merged
steipete merged 5 commits intoSep 4, 2026
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
clawsweeper
Bot
added
merge-risk: 🚨 message-delivery
Contributor
|
Codex review: blocked before merge. Reviewed September 4, 2026, 7:57 AM ET / 11:57 UTC. ClawSweeper reviewWhat this changesThis PR makes Discord Desktop Wiretap retry cache messages whose channel metadata was initially unavailable while preserving newer or deleted archived messages, deduplicating events, and checkpointing completed full-cache files. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 1 item remains The prior full-cache checkpoint finding is resolved on the current head, with focused regression coverage and credible real CLI/SQLite proof. The PR remains necessary and is ready for normal merge gating. Priority: P1 Review scores
Verification
How this fits togetherWiretap imports Discord Desktop cache artifacts into Discrawl’s local SQLite archive. It turns cache payloads into message records and events, while per-file checkpoints determine which artifacts are retried later. flowchart LR
A[Desktop cache files] --> B[Wiretap importer]
B --> C[Channel metadata lookup]
C --> D[Archive write decisions]
D --> E[SQLite messages and events]
C --> F[Per-file checkpoints]
F --> G[Later cache scans]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the focused importer and store safeguards after the exact-head test workflow completes successfully, retaining the source-specific full-cache checkpoint behavior and regression coverage. Do we have a high-confidence way to reproduce the issue? Yes. The PR provides a clear source-backed path and recorded current-main versus candidate CLI/SQLite traces for the unresolved-metadata and repeat-scan cases. Is this the best way to solve the issue? Yes. Per-source retry checkpoints plus transactional preservation of newer revisions and tombstones is a narrow solution to the reported importer behavior. AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against b262f754180d. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (15 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
レビュー指摘 [P1] Preserve event history during the v2 migration への対応。 ## 問題 `wiretapFileIndexScope` を v1 から v2 へ変えると、`loadScanState` が読むキーが 変わるため v1 の checkpoint が全て消えたように見え、取り込み済みキャッシュ ファイルまで再走査される。解析済みメッセージは `AppendEvent: true` を立て、 store はそのミューテーションごとに無条件でイベントを挿入するため、message は upsert されて件数が変わらない一方 `message_events` だけが重複追記される。 既存アーカイブのイベント履歴がアップグレードで複製される。 ## 対応 イベント追記を抑止するのではなく、索引を引き継ぐ形にした。v2 の索引が未作成の ときだけ v1 を読み、**imported の checkpoint だけを** v2 へ移す (`migrateFileIndexFromV1`)。 - 取り込み済みファイルは checkpoint が残るので再走査されない → イベント複製なし - v1 は解決できなかった項目も imported として記録していたため区別できないが、 本PRの `checkpointScannedCandidates` が今後は skipped で記録するので、 以降の未解決項目は正しく再試行される - v1 に無いファイルは通常どおり走査される ## 回帰テスト `TestImportMigratesV1FileIndexWithoutReplayingEvents` を追加。v1 相当の取り込みを 行い、索引を v1 キーへ移して v2 を空にした「アップグレード直前」を作り、再走査後に `FilesScanned == 0` / `message_events` の件数が不変であることを固定する。 ミューテーション検証: 移行を無効化して `state.previous` を空マップへ戻すと `FilesScanned` が 0 ではなく 1 になり本テストが落ちることを確認済み。 検証: go build ./... 成功、gofmt 差分なし、go test ./... = 11 パッケージ全 ok。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@clawsweeper re-review [P1] Preserve event history during the v2 migration を修正し、シード付きアップグレード回帰テストと実挙動トレースを追加しました（PR本文の
head: |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Recheck ambiguous legacy checkpoints once and keep unresolved payloads retryable across default, context, and full-cache input paths. Preserve the existing first-import pruning boundary during upgrades. Protect newer edited messages and tombstones inside the message transaction, including FTS, attachments and mentions, and deduplicate identical cache events. Keep newer edits and equal-revision metadata updates supported. Co-authored-by: Nicolas Hidemaru Ogoshi / 大越ニコラス秀丸 <40846197+Nicolas0315@users.noreply.github.com>
steipete
changed the title
clawsweeper
Bot
added
the
merge-risk: 🚨 session-state
clawsweeper
Bot
added
merge-risk: 🚨 availability
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unresolved Desktop cache files were recorded as imported, so unchanged files stayed skipped after channel metadata arrived. The original v2 migration copied those ambiguous checkpoints. Retrying a mixed batch also rewrote its already-resolved messages, allowing old cache content to replace a newer archived edit and its search, attachment, and mention data.
This completes the existing retry fix with one recheck of v1/v2 checkpoints, while retaining the legacy index so an upgrade does not repeat first-import pruning. Unresolved payloads remain retryable in default cache, JSON/context, and full-cache modes. Message writes preserve newer edited revisions and tombstones inside the same transaction as related rows; identical Wiretap events are not appended twice. Equal-revision metadata refreshes and genuinely newer cache edits remain supported.
Validation includes the complete importer/store suites, their race run, SQLC v1.31.1 regeneration, docs build, and a 30-command proof using stock built CLIs, synthetic Desktop cache files, and real SQLite. A small fixture helper uses the production Store API to seed a newer archived message and its related rows.
okThe regression tests first reproduced missing legacy rows, stale-message replacement, and the context/full-cache retry gaps. They also cover retaining legacy archive rows during migration and preserving the first events when a previously partial batch finally resolves.
Current main is integrated without rewriting either contributor commit. The first CI run caught two test-fixture lint issues (unchecked JSON encoding and constant formatting). Both are fixed; full local lint now passes. All exact-head checks passed for the final full-cache correction at
c84c341dbe97a05e897f5bc717265237465ac7cb: CI, Docker, CodeQL, and secret scanning. A subsequent review found that one unresolved full-cache payload invalidated every scanned-file checkpoint. The correction tracks each message’s source files, so only unresolved sources remain retryable and completed files keep their checkpoints.Thanks @Nicolas0315 (Nicolas Hidemaru Ogoshi / 大越ニコラス秀丸) for the original retry work and investigation.
Curated CLI and archive evidence
Rebuilt and ran the stock CLI at
c84c341dbe97a05e897f5bc717265237465ac7cb. Commands used an isolated synthetic config and cache directory. This excerpt selects relevant JSON fields from actual command output; fixture paths and timestamps are omitted. All 30 CLI invocations exited 0.Command in the legacy-v1 sequence:
discrawl --config fixture.toml --json wiretap --path fixture-cache. The first main import archives the known message; the second encounters an unresolved message; metadata is added before the third. Candidate then upgrades that same archive and repeats:[ { "binary": "main", "exit": 0, "files_scanned": 1, "files_unchanged": 0, "messages": 1, "skipped_messages": 0 }, { "binary": "main", "exit": 0, "files_scanned": 1, "files_unchanged": 1, "messages": 0, "skipped_messages": 1 }, { "binary": "main", "exit": 0, "files_scanned": 1, "files_unchanged": 2, "messages": 0, "skipped_messages": 0 }, { "binary": "candidate", "exit": 0, "files_scanned": 3, "files_unchanged": 0, "messages": 2, "skipped_messages": 0 }, { "binary": "candidate", "exit": 0, "files_scanned": 0, "files_unchanged": 3, "messages": 0, "skipped_messages": 0 } ]Actual SQLite snapshots (
select id,content,edited_at,deleted_at from messages order by id) after metadata arrives:{ "legacy_v1_before": [ [ "333333333333333346", "Sapphire already archived", null, null ] ], "legacy_v1_after": [ [ "333333333333333346", "Sapphire already archived", null, null ], [ "333333333333333347", "Sapphire newly recovered", null, null ] ] }The original event remains byte-for-byte equal; event count changes 1 → 2 and stays 2 on repeat.
discrawl --config fixture.toml --json search '"newly recovered"'returns one row with contentSapphire newly recovered. The v2 upgrade produces the same result.For the mixed-batch case, the production Store helper seeds a newer edited message plus one attachment and one mention. The old PR CLI replaces it with
old cached messageand removes both related rows. Candidate retainsSphinx authoritative newer message,new.txt, and the mention; actualsearch Sphinxreturns that retained message. A later cache edit with a newer timestamp is accepted. After production Store deletion and another import, the archived deletion timestamp and FTS state remain unchanged. All final SQLite integrity checks returnok.Full-cache checkpoint correction
A final discussion review identified over-broad full-cache retry: with one unresolved payload, completed files were rescanned and their messages rewritten on each invocation. The correction preserves source-file provenance while still reconciling channel metadata globally. Every file containing an unresolved message stays retryable, including duplicate copies; other completed fingerprints remain imported.
An additional 16 actual stock-CLI invocations used
discrawl --config fixture.toml --json wiretap --full-cache --path fixture-cachewith synthetic files and SQLite. Selected observed output on the second unchanged import:files_scanned/messagesfiles_scanned/messagesAll SQLite integrity checks returned
ok. The new mixed-file regression failed before the correction and passes afterward. The original 30-command upgrade/replay proof also passes again, as dogo test ./..., focused race coverage, full static analysis and the docs build.