angelandClaude Sonnet 5
1fd75ea675
Dispatching an event to the listener previously happened from an un-awaited goroutine, with only a log line on failure - once processed_at was set, a dropped or failed notification was permanently and silently lost, with no way to tell it had happened. Replaces the processed/processed_successfully booleans with a notified_at/processed_at pair (migration 000031): the dispatcher sets notified_at and hands the event to MockEventListener.Notify, which now also receives an ack callback the listener calls whenever it's truly done, synchronously or arbitrarily later. Anything still "notified" but unacked past notifyRetryAfter (a tunable field, not a stored per-row timestamp) gets notified again on the dispatcher's normal poll/reactive loop - no new retry mechanism needed. ack is idempotent, since a late ack from an earlier attempt and one from a retry can both eventually fire for the same event. Dispatch is deferred until after the transaction that recorded notified_at has actually committed, so ack's independent write can't race a still-open transaction it implicitly depends on being visible. The real SSE listener (server/sse/db_event_publisher.go) acks inline, since its work is synchronous. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XEDaCB7C2NEBgyvqEtZuxY
41 lines
5.1 KiB
Markdown
41 lines
5.1 KiB
Markdown
# Work Summary — 2026-08-05 20:17
|
|
|
|
## Task
|
|
Insight #2 from the `domains/amazon` design review: fire-and-forget listener notifications meant a dropped/failed SSE push was silently permanent, with the underlying event already marked `processed`. Fixed per a design worked out collaboratively with the user (not unilaterally chosen - this one had real tradeoffs).
|
|
|
|
## Design (agreed with user before implementing)
|
|
Non-blocking dispatch is a hard constraint - events come from external systems whose APIs may change unexpectedly, so the dispatcher must never wait on the listener's real work. The agreed shape:
|
|
|
|
- Three states, driven by two nullable timestamps rather than a stored `notify_again_at`: `unprocessed` (`notified_at IS NULL`) → `notified` (`notified_at` set, `processed_at` still null) → `processed` (`processed_at` set).
|
|
- The dispatcher sets `notified_at` and calls the listener; it never sets `processed_at` itself.
|
|
- The listener signals completion via a callback passed into `Notify` itself: `Notify(ctx, e, ack func(context.Context) error) error`. `ack` can be called synchronously (fast listeners, like the real SSE one) or arbitrarily later from elsewhere (slow/async listeners) - exactly once is the contract, and extra calls are safe (see below).
|
|
- If an event sits in `notified` past a retry threshold without being acked, the dispatcher's normal loop re-notifies it - reusing the existing poll/reactive machinery, no new loop. The threshold (`notifyRetryAfter`) is a Go-side field with a sensible default, not a stored per-row timestamp, so the cadence can change without touching any row (explicit user preference over storing `notify_again_at`).
|
|
|
|
## Changes
|
|
- `database_migrations/000031_amazon_event_notify_ack.{up,down}.sql`: drops `processed`/`processed_successfully` booleans on `mock.shop_amazon_events`, adds nullable `notified_at`/`processed_at` timestamps, updates the partial index accordingly. Applied to both the test DB and the real dev DB.
|
|
- `domains/amazon/mock.go`:
|
|
- `MockEventListener.Notify` gained the `ack` parameter.
|
|
- `Mocks` gained a `notifyRetryAfter` field (default 30s) and `WithNotifyRetryAfter(d)` builder for tuning/tests.
|
|
- `processUnprocessedEvents`'s query now selects events that are either brand new or notified-but-stale (`processed_at IS NULL AND (notified_at IS NULL OR notified_at < retry_after)`).
|
|
- Restructured so the async dispatch to the listener happens **after** the transaction that recorded `notified_at` has committed, not from inside it - `ack`'s own independent write can never race a still-open transaction it implicitly depends on being visible.
|
|
- `ack` is idempotent (`... AND processed_at IS NULL` in its UPDATE), since a late ack from an earlier notification and a fresh one from a retry can both eventually fire for the same event.
|
|
- `server/sse/db_event_publisher.go`: the real listener now acks inline right after a successful SSE publish - its work is synchronous, so there's no reason to defer completion.
|
|
- `domains/amazon/mock_test.go`: updated `notifySpy` for the new signature (records ack callbacks, supports disabling auto-ack to simulate a listener that doesn't finish), updated `isProcessed`/added `isNotified` to check the new columns, and added `TestProcessUnprocessedEvents_RetriesUnackedNotification` - the core new-behavior test: a non-acking listener gets re-notified after the retry window (not before), and once *any* recorded ack for that event fires (including a stale one from an earlier attempt, not just the latest), the event settles into `processed` and stops being retried.
|
|
|
|
## A test bug caught and fixed along the way (not production code)
|
|
First run: `TestProcessUnprocessedEvents_ProcessesAllEventsAcrossBatches` failed (8/150 events still unprocessed) and the new retry test hung/timed out. Both were bugs in my own `notifySpy`, not the production code:
|
|
- It signaled "notified" *before* calling the auto-ack, so a test could observe "all N notified" before all N acks had actually landed in the DB.
|
|
- `waitForCount` drained a fixed number of channel signals per call instead of tracking a cumulative total, so calling it twice (once for 1, once for 2) double-counted and hung waiting for a signal that would never come.
|
|
|
|
Fixed by replacing the channel with a cumulative `completedCount` incremented only after any auto-ack attempt returns, and made `waitForCount` poll that count (safe to call repeatedly with increasing thresholds).
|
|
|
|
## Verification
|
|
- `go build ./...` / `go vet ./...` clean.
|
|
- `go test ./domains/amazon/... -v -race`: all 5 tests pass (4 existing + 1 new).
|
|
- 10x repeated runs (`-count=1 -race`) with no flakes, ~1.3-1.5s each.
|
|
- `make test` and `make test-against-dev-db`: full suite green both ways; confirmed zero leftover rows in the dev DB afterward.
|
|
- Migration applied cleanly to both the test DB and the real dev DB; verified `\d mock.shop_amazon_events` matches the intended shape on both.
|
|
|
|
## Follow-ups / not done here
|
|
- Insights #3 (hardcoded 1-minute poll interval) and #4 (a LISTEN-connection error is fatal to the *entire application*, most consequential) remain open - continuing one at a time per the user's request.
|