Add parallel IMAP fetch and --full option#9
Conversation
Split new UIDs into batches and fetch concurrently with one IMAP connection per thread, auto-scaled to CPU core count. Processing (parse/save) remains sequential on the main thread. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Re-fetch all messages ignoring previously fetched history. Useful for re-processing reports after parser changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use \x1b[K (erase to end of line) after \r to prevent leftover characters from previous longer output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use spawned counter as array index for contexts/threads to prevent gaps when Thread.spawn fails. Track assigned UIDs separately for accurate progress reporting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move parallel IMAP fetch, result processing, and TLS-RPT content type detection from main.zig and lib.zig into a shared fetch.zig module to eliminate code duplication. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a parallel IMAP fetching implementation (multi-threaded with multiple IMAP connections), introduces a --full option to re-fetch all messages regardless of prior history, and improves the terminal progress display cleanup.
Changes:
- Add new
src/fetch.zigmodule implementing parallel IMAP fetch + result processing helpers. - Wire parallel fetch +
--fullflag into the CLI fetch path and update progress rendering. - Update C-ABI fetch path to reuse the new fetch module.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/root.zig | Exposes the new fetch module from the library root. |
| src/main.zig | Adds --full flag handling and switches CLI fetch to the new parallel fetch implementation with progress display tweaks. |
| src/lib.zig | Refactors exported reports_fetch to use the new parallel fetch + shared processing logic. |
| src/fetch.zig | New module: worker thread orchestration, result collection, parsing/saving reports, and cleanup utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var new_uids: std.ArrayList(u32) = .empty; | ||
| for (uids) |uid| { | ||
| if (fetched_set.contains(uid)) continue; | ||
| const raw = client.fetchMessage(uid) catch continue; | ||
| defer allocator.free(raw); | ||
|
|
||
| const attachments = reports.mime.extractAttachments(allocator, raw) catch continue; | ||
| defer { | ||
| for (attachments) |att| { | ||
| allocator.free(att.filename); | ||
| allocator.free(att.content_type); | ||
| allocator.free(att.data); | ||
| } | ||
| allocator.free(attachments); | ||
| } | ||
|
|
||
| var saved = false; | ||
| for (attachments) |att| { | ||
| const decompressed = reports.mime.decompress(allocator, att.data, att.filename) catch continue; | ||
| defer allocator.free(decompressed); | ||
|
|
||
| if (reports.mtasts.parseJson(allocator, decompressed)) |report| { | ||
| defer report.deinit(allocator); | ||
| st.saveTlsReport(&report) catch continue; | ||
| saved = true; | ||
| } else |_| { | ||
| const report = reports.dmarc.parseXml(allocator, decompressed) catch continue; | ||
| defer report.deinit(allocator); | ||
| st.saveDmarcReport(&report) catch continue; | ||
| saved = true; | ||
| } | ||
| } | ||
| if (saved) { | ||
| st.markUidFetched(uid); | ||
| fetched_set.put(uid, {}) catch {}; | ||
| } | ||
| if (!fetched_set.contains(uid)) new_uids.append(allocator, uid) catch {}; | ||
| } |
There was a problem hiding this comment.
Ignoring allocation errors from new_uids.append(...) can silently skip UIDs and miss reports. Prefer handling the error (e.g., stop processing this account / return a failure code) rather than catch {} so the caller can react appropriately.
| st.markUidFetched(r.uid); | ||
| fetched_set.put(r.uid, {}) catch {}; |
There was a problem hiding this comment.
When --full is used, UIDs that are already present in fetched_set will be processed again, and markUidFetched will append duplicates to the .fetched_uids file. Over time this can grow without bound and slow down loadFetchedUids. Consider only calling markUidFetched when the UID was not already in the set (e.g., check contains before writing, or use getOrPut and only write on first insert).
| st.markUidFetched(r.uid); | |
| fetched_set.put(r.uid, {}) catch {}; | |
| if (!fetched_set.contains(r.uid)) { | |
| st.markUidFetched(r.uid); | |
| fetched_set.put(r.uid, {}) catch {}; | |
| } |
| ctx.mailbox, | ||
| ctx.tls, | ||
| ); | ||
| client.connect() catch return; |
There was a problem hiding this comment.
worker returns immediately if client.connect() fails, leaving the assigned results entries uninitialized and (when used with the CLI progress loop) preventing progress from ever reaching the total. This can cause the main thread to wait forever and silently skip batches. Consider filling results for all assigned UIDs (with .uid = uid, .data = null) and incrementing progress even on connect failure, or propagating the failure so the caller can abort and clean up.
| client.connect() catch return; | |
| client.connect() catch { | |
| for (ctx.uids, 0..) |uid, i| { | |
| ctx.results[i] = .{ | |
| .uid = uid, | |
| .data = null, | |
| }; | |
| if (ctx.progress) |p| { | |
| _ = p.fetchAdd(1, .monotonic); | |
| } | |
| } | |
| return; | |
| }; |
| .results = results[offset..][0..this_batch], | ||
| .progress = progress, | ||
| }; | ||
| job.threads[job.spawned] = std.Thread.spawn(.{}, worker, .{&job.contexts[job.spawned]}) catch continue; |
There was a problem hiding this comment.
std.Thread.spawn(...) catch continue; drops the entire batch on spawn failure (offset is still advanced) and also means progress may never reach uids.len, which can deadlock the CLI progress loop. Handle spawn failure by either (a) running worker synchronously for that batch, (b) returning null/error after freeing results, or (c) retrying with fewer workers while ensuring every UID is assigned to exactly one worker.
| job.threads[job.spawned] = std.Thread.spawn(.{}, worker, .{&job.contexts[job.spawned]}) catch continue; | |
| job.threads[job.spawned] = std.Thread.spawn(.{}, worker, .{&job.contexts[job.spawned]}) catch { | |
| worker(&job.contexts[job.spawned]); | |
| offset += this_batch; | |
| continue; | |
| }; |
| var progress = std.atomic.Value(usize).init(0); | ||
| var job = reports.fetch.startFetch(allocator, acct, new_uid_slice, &progress) orelse return .{ .dmarc = 0, .tls = 0 }; | ||
| defer reports.fetch.freeResults(allocator, job.results); |
There was a problem hiding this comment.
startFetch executes allocations and imap.Client operations on multiple threads using the passed-in allocator. In the CLI this is a GeneralPurposeAllocator, which is not thread-safe by default, so this can introduce data races/memory corruption. Use a thread-safe allocator for the worker threads (e.g., wrap with std.heap.ThreadSafeAllocator and pass that allocator consistently to startFetch and freeResults, or allocate per-worker with std.heap.c_allocator).
| var progress = std.atomic.Value(usize).init(0); | |
| var job = reports.fetch.startFetch(allocator, acct, new_uid_slice, &progress) orelse return .{ .dmarc = 0, .tls = 0 }; | |
| defer reports.fetch.freeResults(allocator, job.results); | |
| var thread_safe_allocator = std.heap.ThreadSafeAllocator{ .child_allocator = allocator }; | |
| const fetch_allocator = thread_safe_allocator.allocator(); | |
| var progress = std.atomic.Value(usize).init(0); | |
| var job = reports.fetch.startFetch(fetch_allocator, acct, new_uid_slice, &progress) orelse return .{ .dmarc = 0, .tls = 0 }; | |
| defer reports.fetch.freeResults(fetch_allocator, job.results); |
| for (uids) |uid| { | ||
| if (!fetched_set.contains(uid)) new_count += 1; | ||
| if (full or !fetched_set.contains(uid)) new_uids.append(allocator, uid) catch {}; | ||
| } |
There was a problem hiding this comment.
Ignoring allocation errors from new_uids.append(...) can silently skip UIDs (and thus skip fetching/saving reports) under memory pressure. Instead of catch {}, handle the error explicitly (e.g., abort the fetch for this account with a message / return early) so failures are visible and behavior is deterministic.
- Fill results and advance progress on connect failure so the progress loop does not hang - Fall back to synchronous execution when thread spawn fails instead of silently skipping the batch - Skip markUidFetched for UIDs already in fetched_set to prevent duplicate entries when using --full Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test worker connect failure path (results filled, progress advanced), startFetch covering all UIDs, null data skipping in processResults, duplicate UID prevention with fetched_set, and freeResults memory cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
--fullflag to re-fetch all messages ignoring previously fetched history\x1b[K) after\rBenchmark (224 messages)
~3.5x faster
Test plan
reports fetch --fullre-fetches all messagesreports fetchincremental fetch works as before🤖 Generated with Claude Code