fix(notifications): make pipeline.flush() drain in-flight dispatches#41
Merged
Conversation
…spatches Third latent bug (issue #25): flush() was an empty no-op, so the fire-and-forget telegram/push/channel dispatches were silently dropped on shutdown — a notification could be lost the moment the process exits. - Track each already-`.catch()`-chained fire-and-forget promise in an inFlight Set via a void `track()` helper (.finally removes it). The 5 sites: telegram + push + channels fan-out in notifyPermissionPending; channels fan-out in notifySessionIdle and notifySessionError. The two awaited telegram calls in idle/error are NOT fire-and-forget, untouched. - flush() now awaits the in-flight set with a BOUNDED timeout (FLUSH_TIMEOUT_MS = 5_000) via Promise.race; on timeout it audits `notifications.flush_timeout` with the pending count (observable per AGENTS.md, never silent), always clearTimeout (no leaked handle), fast-path returns immediately when nothing is in flight, never throws. Hot path unchanged: track() is void, no new await added to the notify functions; notifyPermissionPending returns the same boolean at the same point. Test seam: optional `createNotificationService(deps, { flushTimeoutMs })` — production caller (server/index.ts) unchanged, defaults to 5s. The #28 no-op characterization test was replaced with 4 tests that genuinely fail on main's no-op flush (verified discriminating). bun test 627/0, tsc clean. Independent fresh-context review: APPROVE, no blocking issues (hot path + flush bound + non-tautological tests all verified). Refs #25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the 3rd and final latent bug from the #25 comment (surfaced by #28's characterization tests).
pipeline.flush()was an empty no-op. The genuinely fire-and-forget dispatches (telegram/push/channelsfan-out) were never tracked, so a notification in flight when the process shut down was silently dropped — exactly the AGENTS.md §"No silent failures" anti-pattern.Fix
.catch()-chained fire-and-forget promise in aninFlightSet via avoid track()helper (.finallyremoves it). 5 sites: telegram + push + channels innotifyPermissionPending; channels fan-out innotifySessionIdle/notifySessionError. The twoawait-ed telegram calls in idle/error are not fire-and-forget → untouched.flush()now awaits the in-flight set with a bounded timeout (FLUSH_TIMEOUT_MS = 5_000) viaPromise.race. On timeout it auditsnotifications.flush_timeoutwith the pending count (observable, never silent), alwaysclearTimeouts (no leaked handle), fast-paths when nothing is in flight, and never throws/hangs beyond the bound.Guarantees (fresh-review verified)
track()isvoid; no newawaitadded to any notify function;notifyPermissionPendingreturns the samebooleanat the same point → the permission hook's blocking decision timing is identical tomain..catch()(never-rejecting) promises are tracked.server/index.tsstillawait notifications.flush()in the same position; now the bound is meaningful.createNotificationService(deps, { flushTimeoutMs })— production caller unchanged, defaults to 5s.Testing
main's no-op flush (fresh review verified them discriminating, not tautological); timeout test uses an injected 50ms bound (no real 5s wait, deterministic).bun test627/0 ·bunx tsc --noEmitclean · scope =pipeline.ts+pipeline.test.tsonlyIndependent off
main. No overlap with #33 wave-2 PRs or #40.Refs #25 — completes all 3 latent items (with #40). #25's remaining lint/arch items were shipped earlier (#27/#30-32).