product: rebuild as durable Sky Notify service - #1
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:9471ed161b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Compile | ||
| run: python -m compileall -q src tests | ||
| - name: Lint | ||
| run: ruff check src tests |
There was a problem hiding this comment.
In the newly added verify lint step, running the exact documented command ruff check src tests exits with status 1 because tests/test_main.py:9 triggers E712 (== True). Consequently, every CI run stops at this step before tests, auditing, or the image build; fix the retained violation or explicitly configure the intended Ruff rules before making this a required gate.
Useful? React with 👍 / 👎.
| raise HTTPException(status_code=400, detail="credentials are not allowed in webhook URLs") | ||
| @app.get("/healthz") |
There was a problem hiding this comment.
Once lint passes, the checked-in tests/test_main.py still requests /health and /api/v1/process, but this change replaces those routes with /healthz and the notification API. Both retained tests therefore receive 404 responses under the newly added pytest -q CI step, so the new service cannot pass its advertised pipeline unless those tests are updated or compatibility routes are retained.
Useful? React with 👍 / 👎.
| def claim_due(self, now: float) -> sqlite3.Row | None: | ||
| with self.connect() as db: | ||
| db.execute("BEGIN IMMEDIATE") | ||
| row = db.execute("SELECT * FROM notifications WHERE status='pending' AND next_attempt<=? ORDER BY created_at LIMIT 1", (now,)).fetchone() |
There was a problem hiding this comment.
When the process is killed after claim_due commits the sending state but before finish runs—for example during the webhook request—the record no longer matches this status='pending' query after restart. With no lease expiry or startup recovery for sending, that notification remains stuck forever and reaches neither retry nor dead-letter handling, defeating durability during routine restarts.
Useful? React with 👍 / 👎.
| return {"status": "idle"} | ||
| ok, error = await deliver(row) | ||
| store.finish(row["id"], ok, error, time.time()) | ||
| return {"status": "delivered" if ok else "retry_scheduled", "id": row["id"]} |
There was a problem hiding this comment.
When a provider failure occurs on the final allowed attempt, finish changes the record to dead_letter, but this response still unconditionally reports retry_scheduled for every failure. For example, a notification with maxAttempts=1 is already terminal after this call, so callers and schedulers receive an incorrect state and may wait for a retry that will never occur.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
Product #10 of the SKYCOIN4444 standalone-product master plan.
This release candidate replaces the placeholder notification router with a durable single-node notification service using FastAPI + SQLite/WAL. It adds idempotent submission, explicit pending/sending/delivered/dead-letter states, bounded retry budgets, HTTPS webhook delivery with exact hostname allowlisting and no redirects, deterministic log delivery for local/CI verification, optional bearer protection, bounded JSON payloads, health/readiness/metrics, modern Python 3.12 dependencies, lifecycle tests, dependency audit, and a non-root container.
Truthful boundary: this PR does not claim distributed exactly-once delivery, multi-region HA, email/SMS/push providers, tenant isolation, encryption at rest, or compliance certification. Merge only after the exact PR head passes compile, Ruff, pytest, pip-audit, Docker build, and non-root image verification.