Repository navigation
T7: notify() - #43
T7: notify()#43
Conversation
Adds client.notify(title, body?, severity?, fields?, urgent?), the mobile Bugsee.notify relay to the app's messaging integrations. The collector route POST /v2/notifications already accepts browser calls (public CORS, app token in the body, no session, no app-type check), so no backend change is needed. - core: createNotifyRelay (in-memory queue, 50 / 256 KB batches, notifyFlushDelay window, urgent skip-ahead, 2 s to 60 s backoff, per-item failed retry), BugseeApi.notify, notifySender client seam; flush()/stop() deliver - protocol/core: notifyFlushDelay option (com.bugsee.option.config.notify-flush-delay, default 0) - browser, node: wire the sender over the existing transport - docs: T7 findings in mobile-api-alignment.md. Not persisted, unlike mobile. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
T7 notify() — deep review
Reviewed d68efad / f7d6e8f against the T7 plan (docs/design/mobile-api-alignment.md), the Android-shaped relay contract, BugseeApi.notify, browser/node composition roots, and the existing client lifecycle (stop/flush/launch/KILL_SDK). Five failure modes below were reproduced with a throwaway vitest harness (not committed).
Findings
- P1
stop(timeout)/flush(timeout)do not halt the notify relay.drainPendingraces the caller’s deadline againstnotifyRelay.flush()but never cancels it;suspend()only clears timers. An in-flight send that later fails callsscheduleRetry()after Stopped. Reproduced: hung send +stop(50)+ release asretry→ a new POST 2s later on a stopped client. - P2
urgentskips backoff, not just the flush window. WhileretryTimeris set,scheduleFlushcorrectly holds deferred items;sendUrgentPOSTs immediately. Reproduced: first batchretry, thenurgent→ second POST before the 2s wait. A 429 + chattyurgentpath hammers the limiter. - P2 Batch size is measured before
deliveryis stamped. A batch that takeBatch treated as an exact 256 KB fit is larger on the wire. Reproduced: two ~128 KB items →JSON.stringify(batch) > 256 KiB. A collector400 InvalidBatchis classifiedrejectedand the whole batch is dropped. - P2 Failed
urgentitems are prepended thentrim()shifts from the front. At the 1000 cap that discards the urgent message and keeps 1000 deferred ones. Reproduced. - P2 Same-client
launch()after a failedstop()drain never resumes parked notifications.suspend()cancels retry;launch()does notflush/scheduleFlush. Reproduced: parked item stays unsent for 120s after relaunch unless anothernotify()/flush()/stop()happens.
KILL_SDK (collector 99099) on /v2/notifications is also invisible to enterKillState — { ok: false } is just rejected. That is secondary to the five above (HTTP 403 must not kill; only 99099 should).
Overall risk: Medium
Merge recommendation: Request changes
The public notify() path, snapshot-at-call, never-throw host boundary, and browser CORS/UA handling are in good shape. Shutdown/backoff/size/overflow need to match the comments this PR already wrote.
Most important to fix
- Give the relay a suspended/stopped flag:
drainmust exit aftersuspend(),scheduleRetry/sendUrgentmust no-op, andstop(timeout)must not leave a live sender. - Hold
urgentwhileretryTimeris set (skip the window, not the backoff). - Budget
deliveryinjsonBytes/takeBatch(or stamp before measuring). - On overflow, drop newest, not the retried/urgent front; kick the relay on
launch().
Sent by Cursor Automation: Bugsee code review
| const drainPending = (timeout?: number): Promise<boolean> => { | ||
| const drained = Promise.allSettled([...pendingReports]).then( | ||
| () => uploadPipeline?.flush(timeout) ?? true, | ||
| ); | ||
| const drained = Promise.allSettled([...pendingReports]).then(async () => { | ||
| // Notifications go out alongside the report uploads; either one still pending makes this false. | ||
| const [uploaded, notified] = await Promise.all([ | ||
| uploadPipeline?.flush(timeout) ?? true, | ||
| notifyRelay?.flush() ?? true, |
There was a problem hiding this comment.
P1 — stop(timeout) / flush(timeout) do not cancel notify delivery
notifyRelay.flush() is unbounded. The Promise.race with sleep(timeout) only resolves the caller; it does not abort drain(), and this wait starts after pendingReports settle, so a slow in-flight report can spend the whole budget before a notification is even attempted.
Then stop()’s finally calls suspend() (timers only). If the in-flight send later returns retry, drain → scheduleRetry() arms a new 2s timer on a client whose status is already Stopped.
Reproduced: notifySender gated, notify('parked'), stop(50) resolves false, release the send as { outcome: 'retry' }, advance 2s → another POST.
Impact: A long-running Node/browser host that stop()s the SDK (feature flag, logout, tests) keeps hitting POST /v2/notifications. stop(timeout) also cannot bound process exit while that HTTP is alive.
Fix: Pass the deadline into the relay (or abort via a generation/suspended flag). After suspend(): stop the drain loop, ignore scheduleRetry / sendUrgent, leave items queued. uploadPipeline.flush(timeout) already receives the budget; notify should too.
| suspend(): void { | ||
| clearFlushTimer(); | ||
| clearRetryTimer(); | ||
| }, |
There was a problem hiding this comment.
P1 (same defect) — suspend() is timer-only
The client comment at stop() says a stopped SDK must not keep a timer or a request alive. This method clears flushTimer/retryTimer and nothing else:
runningdrain()keeps loopingdispatch(takeBatch())after Stopped.dispatch→!done→scheduleRetry()re-arms backoff aftersuspend().sendUrgentis ungated, so a caller holding the same client can still fire POSTs if they racedlaunch()while the old drain is finishing.
Add a suspended (or generation) flag checked at the top of drain, scheduleRetry, scheduleFlush, and sendUrgent. flush() / enqueue after a later launch() should clear it and resume.
| const sendUrgent = (item: NotifyItem): void => { | ||
| const attempt = dispatch([item], true).then((done) => { | ||
| if (!done) { | ||
| scheduleRetry(); | ||
| } | ||
| }); | ||
| urgentSends = Promise.allSettled([urgentSends, attempt]); | ||
| }; |
There was a problem hiding this comment.
P2 — urgent bypasses retry backoff
scheduleFlush refuses to send while retryTimer !== null (“hammer a failing endpoint and reorder the queue”). sendUrgent does not look at that timer.
Reproduced: enqueue a normal item, first send returns retry (backoff 2s), enqueue { urgent: true } → second POST on the same tick.
The T7 write-up says urgent skips the flush window (notifyFlushDelay), not the 2s→60s backoff. Combined with 429 QuotaExceeded, every notify(..., true) while backing off is another unthrottled request.
Fix: If retryTimer is set (or a drain is in flight with a retryable failure), queue the urgent item at the front and let the existing backoff own the next send. Still stamp delivery: 'immediate' on the first attempt; the existing “retried urgent is deferred” test already covers the retry.
| const stamped = batch.map( | ||
| (item): NotifyItem => ({ | ||
| ...item, | ||
| delivery: | ||
| skipAhead || (item.urgent !== true && flushDelayMs <= 0) ? 'immediate' : 'deferred', | ||
| }), | ||
| ); |
There was a problem hiding this comment.
P2 — delivery is stamped after the 256 KB budget is computed
enqueue / takeBatch call jsonBytes(item) on the unstamped object. dispatch then adds delivery: 'immediate' | 'deferred' (~22–23 bytes per item).
Reproduced (same construction as notify-relay.test.ts “treats exactly 256 KB as a full batch that still fits”): two items sized to 128 KiB unstamped → Buffer.byteLength(JSON.stringify(stampedBatch)) > 256 KiB.
T7 findings and the appserver contract cap a batch at 256 KB. classifyNotify maps other 4xx (including 400 InvalidBatch) to rejected, so an over-budget batch is dropped, not retried.
Fix: Stamp delivery before measuring, or subtract a reserved per-item stamp from MAX_BATCH_BYTES / MAX_ITEM_BYTES. The test that currently documents pre-stamp sizing should assert the wire size instead.
| const wanted = result.retryIds; | ||
| const back = wanted === undefined ? batch : batch.filter((item) => wanted.includes(item.id)); | ||
| queue = [...back, ...queue]; | ||
| trim(); |
There was a problem hiding this comment.
P2 — failed urgent / in-flight items are dropped at the 1000 cap
On retry, items are prepended (queue = [...back, ...queue]) and trim() does queue.shift() — drop from the front. Urgent sends never sit in the queue, so a failure at MAX_PENDING prepends 1 and immediately shifts that item off.
Reproduced: 1000 deferred items, urgent send gated then { outcome: 'retry' } → pending() === 1000 and onError reports “dropped the oldest”. The skip-ahead message is gone; the deferred backlog remains.
The same prepend+shift drops a failed drain batch when the queue is refilled during the in-flight request (take 50 from 1000, 50 new enqueues, fail → trim eats the 50 that should be retried).
Fix: Overflow should drop newest (queue.length = MAX_PENDING after retry prepend), or trim before concatenating so retried/urgent ids cannot be the ones shifted. Urgent-at-cap should displace a deferred tail item, not itself.
| // A notification still waiting (offline) keeps its place for a re-launch but stops retrying: a | ||
| // stopped SDK must not keep a timer, and so a request, alive. | ||
| notifyRelay?.suspend(); |
There was a problem hiding this comment.
P2 — parked notifications are not resumed on relaunch
This comment is the contract: keep the queue across stop() so a later launch() on the same client can deliver them. suspend() cancels the backoff and launch() never kicks the relay (flush / scheduleFlush). Public Bugsee.relaunch() builds a new client after stop(), so it does not help either — but the same-client path this comment describes is exercised by client-notify.test.ts and client.test.ts (“can relaunch after stop”).
Reproduced: notify('parked'), sender always retry, await stop(), launch(), advance 120s → no further POST. The item sits until some later notify()/flush()/stop().
Fix: On launch(), if notifyRelay.pending() > 0, scheduleFlush() or void notifyRelay.flush(). Pair with the P1 suspended flag so launch() is what clears it.
…tem; document node .stopped timing - PR #41/#43 reviews 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): Relay halts on stop and honours the retry backoff for urgent sends (5bf4791); a failed urgent at the queue cap is kept (a1caa99, f1982c5). The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve. 🤖 Generated with Claude Code |
1 participant
Mobile parity:
Bugsee.notify(title, body?, severity?, fields?, urgent?).Findings (appserver source):
POST /v2/notifications, app token in the body, public CORS, no session needed, so it works from a browser unchanged. Implemented a core relay (batching vianotifyFlushDelay, urgent skip-ahead, retry onfailed, flush onflush()/stop()); browser and node wired.Review focus / known gaps
notify()is a no-op there.🤖 Generated with Claude Code