Skip to content

fix: quarantine poison GitHub notifications - #200

Merged
ecarreras merged 3 commits into
mainfrom
issue-189-quarantine
Sep 23, 2026
Merged

ecarreras merged 3 commits into
mainfrom
issue-189-quarantine

Conversation

@pilipilisbot

@pilipilisbot pilipilisbot commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • add a durable quarantined_notifications table for GitHub ingestion failures, including UID, message id, headers, error, metadata, and a bounded body excerpt
  • quarantine permanent enqueue/parsing/policy failures from the IMAP reader so last_uid can advance after the failure is safely recorded
  • keep SQLite enqueue/storage failures retryable by preserving the high-water mark until a later poll succeeds
  • surface unresolved quarantines through monitor/dashboard metrics and monitor alerts

Fixes #189.

Tests

  • .venv/bin/pytest -q tests/test_reader.py
  • .venv/bin/pytest -q

Risk

  • Availability-oriented behavior for poison notifications: an ingestion exception is quarantined once recorded, so operators get an alert instead of the reader repeatedly blocking newer GitHub mail.
  • Transient SQLite/storage errors still preserve at-least-once ingestion by aborting the poll before last_uid advances.

Co-authored-by: ecarreras <294235+ecarreras@users.noreply.github.com>
Co-authored-by: ecarreras <294235+ecarreras@users.noreply.github.com>
@ecarreras
ecarreras requested a review from giscebot September 22, 2026 15:43

@giscebot giscebot 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.

The durable quarantine schema, monitor/dashboard alerting, existing-database upgrade coverage, and malformed-message continuation path are coherent, and all three GitHub checks pass. However, the reader currently quarantines every enqueue() exception on the first occurrence and then advances last_uid. This also catches transient infrastructure failures such as sqlite3.OperationalError (lock/I/O): if the immediately following quarantine write succeeds, the original notification is skipped without ever creating a job. That weakens the existing at-least-once ingestion invariant and does not match #189’s “fails repeatedly or non-transient” boundary.

Please keep transient enqueue/storage failures retryable (do not advance the high-water mark), and quarantine only errors known to be permanent, or add an explicit bounded retry/attempt mechanism before quarantine. Add a regression test where enqueue() fails transiently and verify last_uid is not advanced; the next poll must retry the notification.

Keep SQLite enqueue/storage failures retryable by letting them abort the poll before the high-water mark advances. Poison notification parsing failures still enter quarantine.

Co-authored-by: giscebot <286264155+giscebot@users.noreply.github.com>
@pilipilisbot

pilipilisbot commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator Author

Addressed the requested change in b129f29.

  • sqlite3.Error from enqueue() now aborts the IMAP poll instead of being quarantined, so last_uid is not advanced and the same notification is retried on the next poll.
  • Poison/non-storage ingestion failures still go through quarantine_notification() once safely recorded.
  • Added regression coverage for a transient sqlite3.OperationalError("database is locked"): first poll raises and leaves last_uid=0, second poll retries and enqueues without quarantine.

Validation:

  • .venv/bin/pytest -q tests/test_reader.py -> 4 passed
  • .venv/bin/pytest -q -> 357 passed, 2 warnings
  • GitHub checks -> dashboard, pytest (3.11), pytest (3.12) all passed

@ecarreras
ecarreras requested a review from giscebot September 23, 2026 06:01
@ecarreras

Copy link
Copy Markdown
Member

@giscebot revisa de nou la pull-request

@giscebot giscebot 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.

Revisió completada sobre b129f29.

El canvi sol·licitat queda resolt: els errors de persistència SQLite (sqlite3.Error) ja no entren a quarantena, sinó que interrompen el poll abans d'actualitzar last_uid. El nou test de regressió demostra el comportament complet: primer intent amb database is locked, high-water mark intacte, segon poll reintentat i notificació encolada sense quarantena.

També he revisat el flux complet de quarantena, la migració compatible de l'esquema per bases existents, la deduplicació per message_id, i l'exposició al monitor/dashboard. No hi veig regressions ni riscos bloquejants.

Validació:

  • pytest -q tests/test_reader.py tests/test_queue.py tests/test_monitor.py — 53 passed
  • git diff --check origin/main...HEAD — correcte
  • GitHub CI — dashboard, Python 3.11 i Python 3.12 verds

Nota d'entorn: la suite completa local no ha pogut ni recollir-se perquè l'entorn global no té fastapi; aquesta cobertura completa sí que està validada pels checks de la PR.

@ecarreras
ecarreras merged commit e373582 into main Sep 23, 2026
3 checks passed
@ecarreras
ecarreras deleted the issue-189-quarantine branch September 23, 2026 07:12
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.

Quarantine poison GitHub notifications during IMAP ingestion

3 participants