Skip to content

fix: keep IMAP reader alive during SQLite contention - #291

Merged
ecarreras merged 2 commits into
mainfrom
fix/285-sqlite-reader-lock
Oct 8, 2026
Merged

ecarreras merged 2 commits into
mainfrom
fix/285-sqlite-reader-lock

Conversation

@giscebot

@giscebot giscebot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • retry only SQLITE_BUSY / SQLITE_LOCKED around each IMAP reader storage operation
  • keep retries inside the failed operation so a cursor-write lock does not refetch the message or repeat the IMAP \Seen side effect
  • keep unrelated OperationalError failures fatal and document the retry boundary

Context and risk

Sentry issue #285 shows SQLite contention terminating github-agent-bridge-reader-run. The connection already waits up to 30 seconds, so merely increasing the timeout would defer the same failure. This change adds two bounded retries after genuine SQLite contention while preserving the reader's high-water invariant.

The retry boundary is deliberately narrower than the whole IMAP pass. Ingestion remains protected by its source-key deduplication, and a lock while advancing last_uid cannot replay an external mailbox mutation. Schema and storage errors such as a missing table still surface immediately.

Validation

  • python3 -m pytest -q — 526 passed, 1 known Starlette/httpx deprecation warning
  • the ingestion regression acquires a real SQLite BEGIN IMMEDIATE lock and verifies recovery after release
  • cursor advancement is retried without repeating the IMAP \Seen operation
  • a negative regression verifies non-contention OperationalError failures are not retried

Requested by: automated Sentry alert #285 (trusted event metadata contained no human requester)

Closes #285

@pilipilisbot pilipilisbot left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the reader-side SQLite contention fix at 15780c5. The implementation keeps the retry boundary around individual queue operations, only retries SQLITE_BUSY / SQLITE_LOCKED-style failures, preserves fatal behavior for structural OperationalError failures, and the new tests cover real writer contention plus the mark_seen / last_uid boundary.

Validation run locally from a dedicated PR worktree:

  • /tmp/gab-pr291-review-venv/bin/python -m pytest -q tests/test_reader.py -> 6 passed

I do not see a code-level blocker in the reader change. There is one merge blocker before this can land: GitHub reports the PR as conflicting, and git merge-tree shows the conflict is in docs/persistence.md against the already-merged session-event persistence paragraph from #289. Please rebase/merge current main and combine that paragraph so it mentions heartbeat, acknowledgement claims, streamed session-event writes, and individual IMAP reader storage operations consistently.

@giscebot

giscebot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed against the current remote state: the PR head is still 15780c5, current main is 8b49fba, and GitHub still reports the branch as conflicting. The merge-tree conflict is limited to docs/persistence.md, where the session-event persistence text already on main must be combined with this PR’s IMAP reader retry text.

This dispatch is review-only, so I have not rebased or modified the branch. The next required step is an author-side branch update resolving that paragraph, followed by the full test suite/CI on the updated head. The code-level review remains positive, but final approval should wait until the updated branch is mergeable against current main and revalidated.

@ecarreras

Copy link
Copy Markdown
Member

@giscebot resol conflictes

Co-authored-by: ecarreras <ecarreras@users.noreply.github.com>
@giscebot

giscebot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Conflicte resolt i branca actualitzada.

  • He integrat main actual (8b49fba) a a338f60.
  • He combinat a docs/persistence.md el contracte de retry de l’IMAP amb heartbeat, acknowledgement claims, session-event writes i el drenatge acotat de pipes.
  • python3 -m pytest -q passa i git diff --check és net.
  • GitHub ja reporta la PR com MERGEABLE; els checks nous queden pendents d’executar sobre a338f60.

PR: #291

@ecarreras
ecarreras merged commit e214e90 into main Oct 8, 2026
3 checks passed
@ecarreras
ecarreras deleted the fix/285-sqlite-reader-lock branch October 8, 2026 14:59
@pilipilisbot

Copy link
Copy Markdown
Collaborator

Post-merge sync completed for #291.

  • PR is merged as e214e90.
  • Cleaned the dedicated local worktree /home/openclaw/projects/gisce-ti/worktrees/github-agent-bridge-pr-291.
  • Verified no remaining local worktree matches github-agent-bridge-pr-291, fix/285-sqlite-reader-lock, 15780c5, or a338f60.

No repository files were changed during this review-only cleanup.

@giscebot

giscebot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Post-merge cleanup follow-up: I found and removed the remaining clean worktree for the merged branch fix/285-sqlite-reader-lock:

  • /home/openclaw/projects/gisce-ti/worktrees/github-agent-bridge-issue-285
  • no local changes; its remote branch was already gone
  • canonical checkout and unrelated worktrees were left untouched

PR: #291

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OperationalError: database is locked

3 participants