fix: persist state after initial scan and meaningful progress #19

Closed

heiko wants to merge 77 commits from fix/13-state-progress-save into master

heiko commented

2026-05-11 21:28:41 +02:00

Owner

Closes #13

Adds saveStates callback to runMailbox and calls it after updating state in the UIDValidity-changed branch, ensuring progress is persisted before entering IDLE.

Closes #13 Adds `saveStates` callback to `runMailbox` and calls it after updating state in the UIDValidity-changed branch, ensuring progress is persisted before entering IDLE.

heiko added 2 commits

2026-05-11 21:28:41 +02:00

test: require state save after initial scan 7dc60a2849

fix: save state after scan progress b4b048c70d

heiko commented

2026-05-11 21:44:03 +02:00

Author

Owner

Review findings (ai:codex)\n\nMedium: cmd/xr-invoiced/main.go:196\nsaveStates() now persists HighestModSeq even when SearchUIDs fails in the st.LastUID > 0 fallback path. In the migration/CONDSTORE bootstrap case where HighestModSeq == 0 and LastUID > 0, a transient UID search failure advances and saves HighestModSeq to the mailbox current value without scanning those messages. On the next run, the CONDSTORE branch starts from that saved modseq and can permanently skip messages that should have been scanned.\n\nMedium: cmd/xr-invoiced/main.go:237\nsaveStates() serializes file writes, but it does not synchronize access to the state being marshaled. With multiple mailboxes, one goroutine can marshal all states while another mutates its state fields around LastUID, HighestModSeq, or LastExit updates. The new mid-run saves make that race more likely during normal operation; a race test or state-locking design is missing.\n\nLow: cmd/xr-invoiced/state_progress_regression_test.go:26\nThe regression test string-searches main.go for saveStates/Save before the IDLE log marker. It would pass if the call were before state fields are updated, inside unrelated code, or otherwise not actually persisting the intended state. A behavioral test around runMailbox/state file contents would protect this regression more directly.\n\nVerification: the focused regression test passed. Full go test ./cmd/xr-invoiced ./internal/state could not complete in the sandbox because existing doctor tests require listen tcp 127.0.0.1:0; internal/state passed.

heiko reviewed

2026-05-11 21:54:10 +02:00

heiko left a comment

Review: fix/13-state-progress-save

The PR is correct and solves issue #13.

Observations

Minimal and targeted: Adds saveStates func() parameter to runMailbox, calls it once after the UIDValidity branch updates state. Ensures a crash before IDLE no longer loses initial scan progress.
Other scan branches do not save: CONDSTORE and UID-search branches also update st.LastUID/st.HighestModSeq without persisting. A crash during a long incremental scan loses that progress. Consider saving after those branches too.
Concurrency note (pre-existing): saveMu protects Save() but not the struct field mutations. One goroutine writing fields while another is inside json.MarshalIndent is technically a data race under -race.
Function signature: 8th bare func() parameter is getting unwieldy. Consider moving to the options struct in a follow-up.
Regression test: Source-scanning (grep for saveStates). Fragile on refactoring but acceptable as a guardrail.

No correctness issues found.

— reviewed by ai:claude

## Review: fix/13-state-progress-save **The PR is correct and solves issue #13.** ### Observations 1. **Minimal and targeted:** Adds `saveStates func()` parameter to `runMailbox`, calls it once after the UIDValidity branch updates state. Ensures a crash before IDLE no longer loses initial scan progress. 2. **Other scan branches do not save:** CONDSTORE and UID-search branches also update `st.LastUID`/`st.HighestModSeq` without persisting. A crash during a long incremental scan loses that progress. Consider saving after those branches too. 3. **Concurrency note (pre-existing):** `saveMu` protects `Save()` but not the struct field mutations. One goroutine writing fields while another is inside `json.MarshalIndent` is technically a data race under `-race`. 4. **Function signature:** 8th bare `func()` parameter is getting unwieldy. Consider moving to the `options` struct in a follow-up. 5. **Regression test:** Source-scanning (grep for `saveStates`). Fragile on refactoring but acceptable as a guardrail. No correctness issues found. — reviewed by ai:claude

heiko added 1 commit

2026-05-11 22:00:24 +02:00

fix: guard HighestModSeq advancement behind search success, save after all scan branches 65ac9a743b

In the UID-search fallback branch, HighestModSeq was advanced and state
was saved unconditionally — even when SearchUIDs failed. A transient
error could permanently skip messages on the next CONDSTORE-based run.

Move `st.HighestModSeq` update and `saveStates()` inside the success
(`else`) block so they only execute after a successful search.

heiko closed this pull request

2026-05-11 22:25:43 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

IUS/xr-invoiced!19

No description provided.

Rows
Columns