Advance lastHitAt on a deduped delivery

[grimicorn-agent]

What changed and why

updateSourceStats was the only writer of sources.lastHitAt, and it only runs on a fresh, non-deduped ingest. Both dedup paths in server/api/hooks/[slug].post.ts returned a 202 without touching stats:

1. Pre-insert idempotency — a provider (Stripe/GitHub) redelivers an already-ingested delivery id; the handler returns the existing record early.
2. Unique-index race loser — two identical deliveries race, onConflictDoNothing lets only one insert land, and the loser resolves to the winner's record.

So a source under a provider redelivery storm showed a frozen/absent "last hit" in the UI even though it was actively receiving traffic.

Both dedup returns now call a new best-effort touchSourceLastHit(sourceId) that advances lastHitAt without incrementing recordCount — that column counts stored records, and the record already counted on its first ingest. Re-running the full side effects (recordCount bump + "received" event) on every retry would double-count.

Implementation decisions

• Separate touchSourceLastHit rather than reusing updateSourceStats: the two writes differ (stats bumps recordCount; the dedup touch must not), so they are distinct functions.
• Best-effort with a whole-body try/catch: a failed timestamp touch is a stale label, not a lost delivery, so it is swallowed and logged. The try wraps the entire body (not just the query promise) so a synchronous throw from getDb()/the builder can't surface as a 500 — a 500 here would make the provider redeliver an already-stored delivery, looping the touch into repeated reprocessing.
• Scoped to accepted-but-deduped deliveries only. Rejected deliveries (401/400/429/409) deliberately do not count as a hit; tests pin this so a future refactor can't hoist the touch above the signature check and let an unsigned caller bump another user's lastHitAt.
• Not folded into the throttle's per-request UPDATE: reviewer raised advancing lastHitAt on every request via recordWebhookHit. Declined — it would mark rejected (400/403) deliveries as hits and couple rate-limiting with activity display, both beyond this issue's scope.

Tests

Added to tests/server/api/hooks/slug.post.test.ts:

• Both dedup paths advance lastHitAt (exactly one update, { lastHitAt } payload only, scoped to this source) and never bump recordCount.
• The fresh-ingest path still bumps recordCount (positive counterpart, so dropping the increment can't go unnoticed).
• Best-effort: a rejecting update and a synchronously-throwing update on both dedup branches still return 202 and log.
• 401/429/409 rejected deliveries do not touch lastHitAt.

Where it's viewable

The Sources list UI (/sources) — each source card's "last hit" / last-activity timestamp now advances on deduped provider redeliveries.

Closes #231

Follow-up suggestions

• Debounce the dedup lastHitAt touch — gate the new best-effort touch behind a per-source staleness interval so a provider redelivery storm collapses to one write per interval instead of one row-locking UPDATE per duplicate (suggested: P3, effort: S, evidence: server/api/hooks/[slug].post.ts touchSourceLastHit)

[grimicorn-agent]

Independent code review trail

Reviewer ran on Opus against git diff origin/main...HEAD, looping until findings settled.

Round 1

• Positive recordCount assertion missing (dedup tests only asserted "no bump", nothing pinned that fresh ingest does bump) → fixed: added an exact { lastHitAt, recordCount } assertion to the fresh-ingest test.
• Extra touchSourceLastHitBestEffort wrapper + divergent convention → fixed: collapsed into a single self-swallowing touchSourceLastHit.
• Redundant rationale comments across source + call sites → fixed: trimmed call-site restatements.
• lastHitAt clock race (concurrent writers) → skipped: cosmetic (millisecond label), pre-existing in updateSourceStats (new Date()); fixing properly means changing that function too (out of scope) and mixing now()/new Date() would create two conventions for one column.
• Fold touch into throttle's per-request UPDATE → skipped: would advance lastHitAt on rejected 400/403 deliveries and couple rate-limiting with activity display; out of scope.

Round 2

• .catch() on the query builder didn't cover a synchronous throw from getDb()/the builder → real bug → fixed: whole-body try/catch; added a sync-throw test.
• Update not scoped to this source in tests; not.toHaveBeenCalledWith(recordCount) weaker than it reads → fixed: assert updateWhere carries SOURCE_UUID, assert toHaveBeenCalledTimes(1) + exact { lastHitAt }, dropped the redundant negative.
• Race-loser branch failing-touch untested → fixed: added a sibling best-effort test on that branch.
• Pin exact sql fragment shape for the increment → skipped: expect.anything() catches the realistic regression (deletion); pinning the tagged-template shape couples the test to the drizzle mock's private representation.

Round 3

• Scope claim broader than the change → fixed: narrowed the comment (covers accepted-but-deduped paths; rejected deliveries deliberately don't count).
• Inaccurate test comment ("getDb() blowing up") → fixed: reworded to "the update builder throwing synchronously".
• Inline throw stub vs helper convention → fixed: added stubThrowingUpdate to tests/server/helpers.ts.

Round 4 (settle)

• Scope-note invariant (401/429/409 don't count as a hit) unenforced → fixed: added expect(updateMock).not.toHaveBeenCalled() to the 401, 429, and 409 tests — this also guards a defense-in-depth property (a refactor hoisting the touch above the signature check would fail).
• Write-contention / debounce the touch behind a staleness gate → skipped: bounded by the 30/min throttle, lastHitAt renders in coarse buckets, and adding a staleness-gating constant + predicate is scope the issue didn't ask for.
• Two best-effort conventions coexist → held: internal-swallow avoids duplicating the .catch across two call sites; reviewer guidance conflicted across rounds (Round 1 asked to remove the wrapper), so keeping the single-swallow-point design as the deliberate choice.

Local gate before push: npm run lint:ci clean, full vitest suite 1751 passing.

[grimicorn-agent]

Superseded by main — recommend closing

While resolving this PR's DIRTY conflict against main, I found that this change has been fully superseded by the webhook-idempotency rework already merged to main (commit `e867368` "Make webhook side effects idempotent (source stats + ok event)"), which introduced `claimStatsBump` / `applyStatsBump` / `touchLastHitAt` and `writeIngestSideEffects`.

On main, both dedup paths in server/api/hooks/[slug].post.ts now call `writeIngestSideEffects` → `applyStatsBump`. For a deduped delivery the record's `countedAt` is already set, so `claimStatsBump` returns false and `touchLastHitAt` runs — advancing `lastHitAt` without bumping `recordCount`. That is exactly the behavior this PR set out to add for #231, and main's version additionally heals recordCount under-counts and dedups the "received" ok-event.

Evidence: resolving both conflicted files to main's side (git checkout --theirs) leaves server/api/hooks/[slug].post.ts and tests/server/api/hooks/slug.post.test.ts byte-identical to origin/main. The only residual delta of this branch vs main is one now-unused test helper (stubThrowingUpdate in tests/server/helpers.ts) whose sole consumers were this PR's now-obsolete tests.

Keeping this branch's touchSourceLastHit calls instead would discard main's recordCount-healing and ok-event dedup — so the only conflict resolution that discards no work is taking main's implementation, which makes this PR an empty diff.

Recommendation: close this PR as superseded. Issue #231 is still open but its requested behavior is already delivered on main — it can likely be closed too (verify against main's applyStatsBump/touchLastHitAt path). Marking blocked and assigning for that decision rather than pushing an empty merge.