Skip to content

fix(sync): keep scheduled sources on cadence under load - #956

Open
salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:beeper-blocking-operation
Open

salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:beeper-blocking-operation

Conversation

@salmonumbrella

Copy link
Copy Markdown
Contributor

What changed

  • Scheduled syncs no longer pack attachments inline. When a sync writes new loose blobs, a new attachment-pack job packs them every 6 hours. The daily attachment-maintenance job still packs and repacks.
  • Due analytics cache rebuilds run in a background refresher. It holds no operation gate, and requests that arrive during a build collapse into one follow-up. The post-sync check reads the committed marker without the builder lock, so it never waits behind a running build.
  • A full build that overlaps a sync publishes its snapshot instead of failing. The publication is marked so the next build is full, so rows committed during the export are still picked up.
  • A restart within min_rebuild_interval serves the existing publication and schedules the rebuild for when the interval ends. A failed automatic startup build keeps serving a usable publication, including under engine = "duckdb".
  • The scheduler coalesces a tick that fires while its job is still running into one follow-up run, and logs it. A job that has held the gate for a minute while others are queued is asked to stop at its next safe point; Beeper honors that today. /api/v1/scheduler/status reports queued, pending and started_at, and startup logs show the real next run instead of 0001-01-01.
  • Scheduled Beeper jobs:
    • stop after 3 minutes, or earlier when asked to yield, at a chat or history-page boundary, and resume from their cursors;
    • rotate the starting account;
    • sync chats with new messages before backfills, and backfills before quiet chats that only need the daily tail probe;
    • carry the tail probe's progress across runs.
  • Beeper page fetches retry twice. A run with fetch errors left completes with an error count instead of failing, so healthy chats keep their progress and the failed-run counters don't force a full cache rebuild.
  • An account whose Beeper message IDs were reassigned is marked. Scheduled runs skip it with one warning instead of failing every time, and a manual sync-beeper --account <id> verifies again and clears the mark.
  • Planner statistics refresh at most every 6 hours after syncs. A new daily sqlite-maintenance job runs PRAGMA optimize and a retried wal_checkpoint(TRUNCATE). Gmail full syncs and calendar syncs use a passive checkpoint.
  • /api/v1/stats and /api/v1/cli/accounts wait about 2 seconds for fresh counts, then serve the previous counts with stale: true and as_of. Vector stats get their own 3-second deadline and set vector_stats_unavailable instead of failing. Account counts come from one grouped query.

The operation gate stays exclusive. Starvation is fixed by shortening and preempting holds, not by letting jobs write concurrently. Incremental pack discovery lives in kit/packstore and isn't part of this change.

Why

On a large archive every scheduled job runs through one slot. Packing after each sync, inline cache rebuilds, lock waits and unbounded Beeper backfills each held it for minutes to hours, so a 5-minute schedule turned into a few syncs a day. Restarts and overlapping builds added repeated 16-minute builds that then failed.

Usage

No new configuration. min_rebuild_interval now also applies at daemon startup. The new daemon jobs are attachment-pack (41 */6 * * *) and sqlite-maintenance (29 4 * * *).

Closes #955

@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (736fd4c)

Verdict: Changes require fixes for 2 findings.

High

  • internal/beeper/importer.go:950: The scheduled Beeper stop flag is checked only before chats and after backfill pages, so incrementalChat, reconcileChat, probeChatTail, and chat enumeration can continue through many pages without observing the 3-minute budget or cooperative preemption. A large chat may hold the operation gate and starve other scheduled jobs.

    Fix: Poll the stop/preemption signal at every message-page boundary and during enumeration, propagate Stopped through syncChat, checkpoint the cursor, and return normally at the next safe boundary.

Medium

  • cmd/msgvault/cmd/serve.go:1111: Startup throttling treats a publication marked FullRebuildRequired as an ordinary usable cache. A restart within min_rebuild_interval can defer mandatory repair and serve a known partial snapshot, leaving missing related rows visible for the whole interval.

    Fix: Carry an explicit partial-publication or mandatory-repair flag in staleness and bypass the minimum-interval throttle for it, including the startup path.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 10m54s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 736fd4c to 5ea68f1 Compare September 26, 2026 03:55
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (5ea68f1)

Verdict: Changes require fixes for 3 findings.

Medium

  • cmd/msgvault/cmd/serve.go:307-309: The cache refresher is only shut down on the normal daemon shutdown path; if a later startup step fails, a delayed timer can still launch cache work after daemon/store cleanup.

    Fix: Register an idempotent refresher shutdown defer immediately after creation, before store cleanup, and cancel delayed timers on every exit path.

  • cmd/msgvault/cmd/sync_beeper.go:270-279: The Beeper three-minute budget is only cooperative: imports use the uncapped parent context, and a stuck page can exceed the advertised budget through HTTP timeouts, retries, and list-page retries.

    Fix: Propagate a deadline or cancellation covering the scheduled budget into each import/page request, stop retries when it expires, and map expiration to a resumable stopped result.

  • internal/store/store.go:602-612: analysis_limit is set only on connections[0], while ANALYZE sqlite_schema runs on every reserved connection, allowing unrestricted analysis on the remaining connections.

    Fix: Set analysis_limit on every connection before ANALYZE, or perform one bounded analysis and only reload schema state on the other connections.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 12m0s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 5ea68f1 to 71233e5 Compare September 26, 2026 04:38
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (71233e5)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/scheduler/scheduler.go:855: The scheduler only sets a cooperative preemption flag, but only Beeper consumes it. Other long-running scheduled sources never observe the flag and can continue holding the operation gate indefinitely while queued jobs wait.

    Fix: Propagate preemption handling to every resumable scheduled source, or cancel with a recognized yield cause and ensure each source checkpoints and resumes safely.

  • internal/beeper/importer.go:367: The Beeper three-minute budget only bounds provider requests. After a stop, RecomputeConversationStats, checkpointing, and CompleteSync still run on the uncapped parent context, so a large archive can keep the scheduled job past the advertised limit.

    Fix: Bound or defer the expensive post-stop local phases while preserving resumable sync finalization, so the scheduled job's total duration is actually budgeted.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 11m12s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 71233e5 to 34ec0b4 Compare September 26, 2026 09:51
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (34ec0b4)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/beeper/importer.go:802: When a tail-probe page fetch fails after retries, probeChatTail returns success without incrementing FetchErrors. The caller may record TailProbed and advance LastTailScan, omitting the documented partial-sync error and delaying affected messages until the next tail scan.

    Fix: Record the failed probe as a fetch error, preserve the scan as due, and only mark the chat probed or advance LastTailScan after a successful probe.

  • internal/store/sqlite_maintenance.go:48: RunDailyMaintenance calls CheckpointWAL without propagating its context, so SQLite runs the checkpoint with context.Background and a 30-second busy timeout. Scheduler preemption or shutdown cannot interrupt checkpoint attempts and backoffs while the operation gate is held.

    Fix: Add a context-aware WAL checkpoint path, check cancellation before each retry, and ensure the PRAGMA itself can be interrupted or uses a bounded cancellation-aware timeout.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 10m50s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch 2 times, most recently from 33db9c5 to 5c130a8 Compare September 26, 2026 10:38
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (5c130a8)

Verdict: Changes require fixes for 4 findings.

Medium

  • internal/api/snapshot_cache.go:94: A completed snapshot clears entry.flight before publishing the flight result and closing done, allowing a concurrent request to start duplicate work and treat the newly published value as stale.

    Fix: Publish the flight result and close its completion signal before making the entry available for a new flight, under the same mutex.

  • cmd/msgvault/cmd/cache_refresher.go:159: cachePublicationThrottled calls InspectCacheReadiness on every scheduled sync when min_rebuild_interval is enabled, causing archive-scale I/O while holding the scheduler operation gate.

    Fix: Use a cheap committed-marker/stat check or memoized fingerprint for throttling, leaving full readiness inspection to the background builder.

  • internal/store/dialect_sqlite.go:2097: The inline PASSIVE WAL checkpoint obtains a pooled SQLite connection with context.Background, so it can wait indefinitely for a free connection and ignore cancellation or scheduler preemption.

    Fix: Accept a context and bounded timeout for the passive checkpoint, or remove the inline checkpoint and rely on daily maintenance.

  • cmd/msgvault/cmd/serve.go:1184: MandatoryRepair/FullRebuildRequired publications are still treated as stale-but-servable, allowing known incomplete caches to be served after repair failure or partial publication.

    Fix: Exclude MandatoryRepair publications from servesStale and either fail engine=duckdb or use live SQL while scheduling a mandatory full repair.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 12m19s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 5c130a8 to 91e6b43 Compare September 26, 2026 11:27
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (91e6b43)

Verdict: Changes require fixes for 2 findings.

Medium

  • cmd/msgvault/cmd/cache_refresher.go:165: cachePublicationThrottled treats a marker plus dataset directories as usable without verifying DatasetFingerprint or committed Parquet shards, so scheduled refreshes can throttle an unusable cache within min_rebuild_interval.

    Fix: Validate publication readiness, including marker completeness and shard presence or fingerprint, before applying the throttle; bypass throttling when readiness cannot be established.

  • internal/beeper/importer.go:217: The advertised three-minute scheduled Beeper budget is applied only after source setup, allowing stale re-derivation and identity replay to run with an uncapped context and delay other sources.

    Fix: Apply the scheduled deadline and cooperative-stop checks to re-derivation and identity replay, or move and bound those operations so scheduled imports can yield and resume safely.


Reviewers: codex, codex (security) | Synthesis: codex, 5s | Total: 10m18s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch 2 times, most recently from c57bdae to 5804c20 Compare September 26, 2026 11:50
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (5804c20)

Verdict: Changes require fixes for 2 findings.

High

  • internal/beeper/syncstate.go:116: Newer checkpoints cannot clear TailProbeCursor or TailScanStarted because Merge copies them only when non-empty. If a probe or scan completes and the run then fails, stale values from the previous successful state survive, causing later runs to resume from an obsolete cursor or skip chats as already scanned.

    Fix: Treat these checkpoint fields as authoritative and preserve explicit clears, or add presence metadata so newer empty values overwrite the baseline.

Medium

  • cmd/msgvault/cmd/cache_refresher.go:198: cachePublicationThrottled checks only that one parquet file exists in each required dataset and that the stored fingerprint is non-empty; it does not verify the current shard set against the fingerprint. A missing or corrupted shard can therefore be treated as a recent usable publication and defer recovery for the throttle interval.

    Fix: Perform an authoritative cache-readiness or fingerprint validation before applying throttling, and bypass the throttle whenever the committed cache has drifted or is unusable.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 8m51s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 5804c20 to 80bb2a4 Compare September 26, 2026 12:23
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (80bb2a4)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/scheduler/scheduler.go:310-320: RemoveAccount deletes the cron registration but leaves pending[email] set, so an active scheduled run can start a queued follow-up after the account is removed.

    Fix: Clear pending[email] during removal and/or have finishAccountRun verify that the account remains registered before launching a follow-up run.


Reviewers: codex, codex (security) | Synthesis: codex, 5s | Total: 9m20s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 80bb2a4 to 9310551 Compare September 26, 2026 12:35
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (9310551)

Verdict: No findings at or above medium severity.


Reviewers: codex, codex (security) | Synthesis: codex, 4s | Total: 11m16s

@wesm wesm self-assigned this Sep 26, 2026
@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 9310551 to 98bf612 Compare September 26, 2026 14:48
@wesm

wesm commented Sep 26, 2026

Copy link
Copy Markdown
Member

Addressed the confirmed problems in four follow-up commits:

  • Interrupted syncs now queue another turn behind waiting jobs. Forced cancellation is limited to sources that save resumable progress; maintenance keeps its own time budget.
  • Beeper rotates past stopped accounts and remembers finished chats and repair progress. Stopped imports update conversation counts and previews. Permanent request failures are not retried, and completed checks for older chat history are not repeated.
  • Removed the new permanent account-skip flag. Problems with changed Beeper message IDs now appear as errors, and a repaired account can resume automatically.
  • Cache rebuilds respect the configured interval even after an overlapping sync. Restarting continues to serve a readable snapshot if its refresh fails. Waiting for another cache build now stops on shutdown.
  • Packing queues bounded follow-up passes until the backlog is drained, and checks for leftover blobs after restart. Background statistics queries retain the initiating request ID.

Kept the author's new test for stopping during a conversation-count update. Refreshing Beeper's reference messages still waits for a completed cycle; every run checks saved or archived messages first.

The full test suite (make test), targeted race tests, go vet, CI lint, and documentation checks passed. The fixes remain in this PR as separate commits; I did not split it into new PRs.

@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (c2a1e3f)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/store/archive_marker.go:10: The new archive-marker API has no production call sites, so scheduled syncs still retry accounts after Beeper message-ID reassignment and manual recovery never clears markers as documented.

    Fix: Wire anchor-mismatch handling into the marker API: persist a source-specific marker, skip marked accounts during scheduled sync with a warning, and clear the marker only after successful manual verification.

  • internal/beeper/repair.go:104-122: Repair advances its in-memory checkpoint before repairing each row, records row errors without persisting a safe checkpoint, and unconditionally resets sync_config after the loop. A failed repair therefore rescans the archive from the beginning on the next run.

    Fix: Persist checkpoints only after successful row processing, preserve the last safe checkpoint on errors or cancellation, and clear the repair checkpoint only after the entire repair completes successfully.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 8m17s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from c2a1e3f to 98f7878 Compare September 26, 2026 16:51
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (98f7878)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/api/snapshot_cache.go:83-91: When the request context is canceled during a stats snapshot refresh, snapshotCache.get returns the previous stale snapshot with no error instead of propagating cancellation.

    Fix: Handle reqCtx.Done() separately and return reqCtx.Err() before falling back to the stale snapshot.


Reviewers: codex, codex (security) | Synthesis: codex, 5s | Total: 12m8s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 98f7878 to aae6e3d Compare September 26, 2026 17:10
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (aae6e3d)

Verdict: Changes require fixes for 2 findings.

Medium

  • cmd/msgvault/cmd/build_cache.go:2496: A scheduled cache subprocess can return success after skipping because a newer publication is still inside the rebuild interval. rebuildCacheNow treats the nil result as complete and does not schedule RequestAfter, so the stale cache may remain until another sync or restart.

    Fix: Propagate the child’s skipped/throttled outcome, or recheck staleness after the subprocess and schedule a delayed retry when work remains.

  • internal/beeper/importer.go:422: The deferred conversation-statistics update can set sum.Stopped after message processing, but the caller still marks the chat Visited. A later budget-bound run skips that chat, so a canceled statistics update is not retried and message counts/previews can remain stale.

    Fix: Only mark the chat visited when the statistics phase completed, or return a separate completion flag and retain the chat as pending when stats were deferred.


Reviewers: codex, codex (security) | Synthesis: codex, 12s | Total: 16m56s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from aae6e3d to 6a2d5ca Compare September 26, 2026 18:34
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (6a2d5ca)

Verdict: Changes require fixes for 3 findings.

High

  • cmd/msgvault/cmd/sync_slack.go:227: The preemptible Slack job always restarts its workspace loop at index zero, so repeated preemption can starve later registered workspaces.

    Fix: Persist or rotate the next workspace when interrupted and resume from it on the queued follow-up.

Medium

  • internal/beeper/repair.go:100: Scheduled Beeper repair work is not reliably bounded because archive scans, derived-text writes, and checkpoint writes use non-context-aware store methods.

    Fix: Thread the repair context through these operations or add interruptible checkpoints at the budget boundary.

  • internal/beeper/importer.go:668: syncChat always performs a cancellation-detached 15-second conversation-stat recomputation, delaying queued scheduler work after Beeper cancellation.

    Fix: Use the detached finalize context only for cooperative budget stops; for scheduler cancellation, skip or bound the update with the canceled context and let the next run repair it.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 14m18s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 6a2d5ca to c70935b Compare September 26, 2026 18:48
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (c70935b)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/scheduler/scheduler.go:520: Every account sync is marked preemptible at internal/scheduler/scheduler.go:520, but runScheduledSync also dispatches IMAP. runScheduledIMAPSync forces NoResume=true and performs a full pass, so cancellation under sustained gate contention discards progress and queued follow-up runs restart from the beginning; repeated contention can prevent an IMAP source from completing.

    Fix: Make preemption source-aware and disable it for non-resumable IMAP runs, or add durable progress checkpoints so a preempted IMAP pass can resume safely.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 17m39s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from c70935b to 46ceba3 Compare September 26, 2026 19:19
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (46ceba3)

Verdict: Changes require fixes for 3 findings.

High

  • internal/beeper/syncstate.go:129: SyncState.Merge ORs Done flags, so reopening a completed chat and being interrupted can leave Done=true and cause the next run to skip required backfill, permanently missing older messages.

    Fix: Treat the newer checkpoint's Done value as authoritative, or add generation metadata so an explicit false can reset prior completion.

Medium

  • cmd/msgvault/cmd/attachment_maintenance.go:254: runPendingPack clears packPending and returns nil after a bounded pack even when runAutomaticPack marks packPending again because the byte budget was exhausted, so the scheduler does not enqueue the promised follow-up pass.

    Fix: After a successful pass, return scheduler.ErrReschedule when packPending remains set.

  • internal/scheduler/scheduler.go:537 and internal/scheduler/scheduler.go:806: Scheduled account and generic jobs only treat cooperative preemption as a yield when the callback returns nil. A resumable import can checkpoint progress and return PartialSyncError, but the scheduler records a failure without queueing the required follow-up.

    Fix: Handle jobctx.PreemptionRequested independently of callback error, queue the follow-up, and preserve the callback error separately for status and logging.


Reviewers: codex, codex (security) | Synthesis: codex, 10s | Total: 14m31s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 46ceba3 to 61351f2 Compare September 26, 2026 19:49
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (61351f2)

Verdict: Changes require fixes for 4 findings.

High

  • internal/beeper/importer.go:496: A partial Beeper import completes the sync and then returns PartialSyncError, but the deferred error handler calls FailSync afterward, overwriting the completed run as failed.

    Fix: Track whether CompleteSyncContext succeeded and skip deferred FailSync for completed partial runs.

Medium

  • internal/beeper/importer.go:669: syncChat always uses finalizeContext, imposing a 15-second deadline even when StopAt is unset, so slow manual imports can fail despite having no documented time budget.

    Fix: Use the original context for manual imports and apply the bounded finalization context only to scheduled or explicitly stopped runs.

  • cmd/msgvault/cmd/serve.go:3515: Scheduler preemption sets jobctx.PreemptionRequested, but scheduledSyncYielded only checks the cancellation cause, allowing multi-source account runs to continue past cooperative boundaries.

    Fix: Check jobctx.PreemptionRequested at source boundaries and treat cooperative preemption as a non-error yield.

  • internal/api/snapshot_cache.go:73: When no previous statistics snapshot exists, snapshotCache.get ignores its wait duration and waits until computation or request cancellation, exceeding the documented roughly two-second bound.

    Fix: Apply the wait timer to the no-previous-value path and return an explicit unavailable or timeout response when no snapshot is ready.


Reviewers: codex, codex (security) | Synthesis: codex, 8s | Total: 10m17s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 61351f2 to 98e68d2 Compare September 26, 2026 20:18
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (98e68d2)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/serve.go:1052,1115,1186: Cache readiness still calls cacheNeedsBuild, which uses context.Background(), in the SQL-query path and both startup checks. If another process holds the cache builder lock, canceled requests or daemon shutdown can remain blocked until that build finishes.

    Fix: Use cacheNeedsBuildContext(ctx, ...) at all affected call sites and propagate cancellation before rebuilding or opening the analytics engine.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 18m38s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 98e68d2 to a79441a Compare September 26, 2026 21:19
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (a79441a)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/serve.go:805-808: cacheRefresherShutdown is set unconditionally even when Shutdown times out, causing deferred cleanup to skip its retry and allowing background cache work to outlive runServe teardown.

    Fix: Only mark the refresher stopped after a successful wait, retry cleanup with an independent context before shared resources close, and make cache-build mutex waiting cancellation-aware.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 13m25s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from a79441a to cea0dc4 Compare September 26, 2026 21:32
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (cea0dc4)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/serve.go:3506: When a scheduled source notices PreemptionRequested before the scheduler cancels its context, runScheduledSync returns ErrYieldedToWaiter unconditionally. The scheduler treats this as a yield, but callbackErrorAfterYield suppresses yield errors only when cancellation is caused by ErrYieldedToWaiter, so the sentinel can be stored as LastError while discarding the actual source error.

    Fix: Treat cooperative preemption as an expected no-error yield when the source completed successfully, while preserving any real source error and still queuing the follow-up; alternatively ignore the synthetic sentinel for PreemptionRequested without dropping other errors.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 16m36s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from cea0dc4 to aff3efe Compare September 26, 2026 22:06
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (aff3efe)

Verdict: No findings at or above medium severity.


Reviewers: codex, codex (security) | Synthesis: codex, 4s | Total: 17m57s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from aff3efe to 7dba29b Compare September 26, 2026 22:35
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (7dba29b)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/beeper/importer.go:406: A chat marked Visited by an interrupted discovery cycle is skipped before tail-scan logic runs, so a due tail scan can be recorded complete without probing it and delay newly backfilled history.

    Fix: Apply the Visited check only to non-tail runs; during tail scans, retain the independent TailProbed check for resumption.


Reviewers: codex, codex (security) | Synthesis: codex, 5s | Total: 16m53s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 7dba29b to 253cba0 Compare September 26, 2026 23:42
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (253cba0)

Verdict: Changes require fixes for 3 findings.

Medium

  • cmd/msgvault/cmd/serve.go:3517-3519,3560-3564: When runScheduledSync yields during Gmail fallback or multi-source dispatch, it returns before the common cache rebuild, so committed imports can leave analytics snapshots stale indefinitely.

    Fix: Refresh the cache on every attempted source before returning from a yielded run, ideally through a single deferred path using context.WithoutCancel.

  • cmd/msgvault/cmd/serve.go:3533-3565,3687-3692: runScheduledSync always restarts its source slice at index zero after preemption, so a large first Gmail, Teams, or Discord source can repeatedly preempt and starve later sources sharing the identifier.

    Fix: Persist a per-identifier round-robin source cursor, or only preempt between source boundaries and resume at the next source.

  • cmd/msgvault/cmd/sync_slack.go:227-246: The preemptible Slack job iterates workspaces from the beginning and breaks on cancellation, so repeated preemption while processing the first workspace can prevent later workspaces from running.

    Fix: Persist a Slack workspace rotation cursor across scheduled runs, or defer preemption until a workspace boundary.


Reviewers: codex, codex (security) | Synthesis: codex, 8s | Total: 14m36s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from 253cba0 to dcb1471 Compare September 26, 2026 23:55
@roborev-ci

roborev-ci Bot commented Sep 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (dcb1471)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/cache_staleness.go:127: After acquiring the cache build lock, cacheNeedsBuildContext calls cacheNeedsBuildLocked without passing the context. Store opening and SQLite queries are therefore non-context-aware, so canceled API requests or daemon shutdown may remain blocked in the staleness scan while holding the lock.

    Fix: Thread the context through cacheNeedsBuildLocked, use context-aware database opening and query methods, and check cancellation between individual staleness queries.


Reviewers: codex, codex (security) | Synthesis: codex, 6s | Total: 10m44s

@salmonumbrella
salmonumbrella force-pushed the beeper-blocking-operation branch from dcb1471 to 7dc6451 Compare September 27, 2026 00:50
@roborev-ci

roborev-ci Bot commented Sep 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (7dc6451)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/serve.go:3599: When a scheduled account has an earlier source failure and a later source is canceled to yield to a waiter, scheduledSyncYieldResult discards the accumulated errors and returns only ErrYieldedToWaiter. The scheduler treats the run as benign, so the source failure is missing from LastError and logs.

    Fix: Preserve non-yield errors while separately marking the run as yielded; suppress only the scheduler cancellation cause, not the entire aggregated error.


Reviewers: codex, codex (security) | Synthesis: codex, 5s | Total: 17m3s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Scheduled sources fall far behind their cron cadence on large archives

2 participants