feat: Replace Meteor LiveQueryHandle flow with AbortSignals - #1812
Draft
Julusian wants to merge 13 commits into
Draft
feat: Replace Meteor LiveQueryHandle flow with AbortSignals#1812Julusian wants to merge 13 commits into
Julusian wants to merge 13 commits into
Conversation
Adds meteor/server/lib/observerLifetime.ts: createChildAbort and processLifetimeSignal as permanent primitives, plus temporary interop shims (stopOnAbort, attachPendingHandles, handleFromSignalSetup) for migrating LiveQueryHandle-based observers to AbortSignal lifetimes. Refactors driveSubscriptionFromCursor and the observe shims in asyncCollection/changeStreamCursor onto them. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
observe/observeChanges (and the cursor's observeAsync/observeChangesAsync) now accept a `signal` in the options and return a readiness promise instead of a stop handle. The signal is the observer's lifetime: aborting it stops the observer, aborting during setup resolves quietly leaving nothing running, and a rejection means setup failed with nothing left running. The handle-returning forms remain, deprecated, until callers are migrated. driveSubscriptionFromCursor now uses the signal form directly, so the DDP cursor contract is signal-based. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
InMemoryMongoCollection.observe/observeChanges/onChange accept an AbortSignal defining the observer's lifetime, alongside the deprecated handle-returning forms. WithSignal/hasSignal move to corelib's mongo.ts so both the in-memory and async collection layers share one definition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the LiveQueryHandle/stop() plumbing across the backend observer stack with AbortSignal-scoped lifetimes, fed from the per-subscription signal that DdpPublicationContext already owns. - observerLifetime: createChildAbort (AbortScope, composed via AbortSignal.any so dropped scopes are collectable) and runOnAbort as permanent primitives; stopOnAbort/attachPendingHandles/ handleFromSignalSetup as temporary interop. - CustomPublish exposes `signal`, retiring the single-slot onStop that silently overwrote a second registration. - optimizedObserverBase owns a per-worker controller and passes its signal into setupObservers, so a throw partway through setup releases everything already started. - ReactiveMongoObserverGroup, observerChain, RundownsObserver and the content-observer classes drop their handle arrays and dispose() methods in favour of caller-supplied signals; restartable scopes use per-generation child scopes. - Process-lifetime observers take processLifetimeSignal. This closes the sequential-setup leaks in pieceContentStatusUI, bucketContentObserver and StudioObserver, where a throw between observer creations orphaned the earlier ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the AbortSignal migration by deleting the handle-based API now that nothing uses it: - LiveQueryHandle/LiveQueryHandleSync, waitForAllObserversReady, the interop shims (stopOnAbort, attachPendingHandles, handleFromSignalSetup), ReactiveMongoObserverGroup and SetupObserversResult are gone. - observe/observeChanges now take a required `signal` in their options and return Promise<void>; there is no longer a way to start an observer without stating its lifetime. - PublicationContext.onStop, CustomPublish.onStop and DdpPublicationContext's stopCallbacks are replaced by the signal. Removing SetupObserversResult surfaced eight publications (studio, studioUI, showStyleUI, triggeredActionsUI, peripheralDeviceForDevice, externalEventSubscriptions and the two packageManager ones) that were still returning handle arrays which nothing consumed; those observers are now bound to the publication signal. executeFunction.ts likewise had a window where its handle was still null while setup was in flight, so an early cleanup() leaked the observer; aborting a controller works whenever it happens. Adds observerLeak.test.ts, which asserts that a setupObservers throwing after starting two observers leaves none running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the module-level processLifetimeSignal singleton with an AbortController created in main.ts and threaded into each startup function: setupSystemStatusObservers, startMediaObjectDurationMonitor, startStudioMappingsHashObserver, startRundownVersionHashObservers, startBlueprintConfigPresetObservers, startExternalMessageQueueStatusMonitor and startDeviceTriggersObserver (plus ObserveChangesForHash/ObserveChangesHelper underneath). Aborting it now actually releases those observers, so a graceful shutdown has something to call, and tests scope their own observers to a controller they abort in afterEach rather than sharing a singleton that can never be reset between suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two related tidy-ups, both removing machinery that existed to work
around callees that half-cleaned up after themselves.
ObserveMultiplexer#addSubscriber threw its startup error while the
subscriber was still registered, relying on the caller aborting to tear
it down ("who will abort, tearing us down"). It now deregisters before
throwing, so rejecting means nothing was left running. That makes
startObserveOnSignal redundant - its child scope existed only to have
something to abort on failure - so observe/observeChanges pass the
caller's signal straight through, saving an AbortScope and an
AbortSignal.any composite per observer. The contract is now stated on
the collection interface and covered by a test.
The deviceTriggers observers took change handlers that returned a
cleanup function, which each class stored and replayed on abort. Of the
three, one returned nothing, one returned a literal no-op, and only
clearTriggers was real. StudioObserver now takes a named handler object
including an explicit onRundownContentGone, registered once per
generation with runOnAbort, and the classes lose their #cleanup fields.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
createReactiveContentCache now takes the signal and self-cancels,
returning just the cache rather than a { cache, cancel } pair that every
caller had to wire up with runOnAbort.
Both deviceTriggers observers collapse to a single constructor line: the
reaction signature already matched their ChangedHandler, and the
`if (signal.aborted) return` guard they each carried is now enforced by
the cache itself, which stops delivering reactions once aborted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds createDebounce(fn, wait, signal) and gives PromiseDebounce a signal in its constructor, so debounces end with their owner instead of every call site pairing a `_.debounce` with a runOnAbort cancel and an `if (signal.aborted) return` guard in the body. Both now ignore calls made after the signal aborts and discard a pending invocation at that point. createDebounce is implemented on plain setTimeout rather than wrapping _.debounce: underscore captures Date.now at module load, so its debounce never fires under jest's fake timers, which is why the observers relying on it could only be tested through waitUntil polling loops. The new one is directly testable. Converts the debounces that have a lifetime - the two deviceTriggers reactive caches, mountedTriggers' publish buffer, StudioObserver's two, and both RundownsObservers. StudioObserver.stop() is now just an abort. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The six _.debounce class fields in observers that own an AbortSignal now use createDebounce. They previously guarded their bodies with `if (this.#signal.aborted) return` but never cancelled, so a pending timer outlived the observer. These classes assign #signal in the constructor body, so the debounces had to move there too - a field initializer cannot read it. Their bodies are unchanged apart from dropping the aborted guard, which createDebounce now enforces. bucketContentObserver no longer needs #signal at all. Also propagates the abort reason: PromiseDebounce rejects waiters with signal.reason rather than a bare 'Cancelled', and StudioObserver ends its observer chain with `trigger` instead of `call` - the chain discards the return value, so `call`'s promise was unhandled and would surface as an unhandled rejection when the debounce was cancelled. The only remaining _.debounce is the module-level one in api/blueprints/development.ts, which has no lifetime to bind to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes the last of the migration scaffolding now that nothing needs it. WithSignal existed so the signal-taking overload could be spelled `Options & WithSignal` alongside the deprecated plain-`Options` one, and hasSignal was the runtime discriminator between them. With the deprecated overloads gone, hasSignal had no call sites and the intersection was noise, so `signal` now lives on the interfaces themselves: a new ObserveOptions carries it, ObserveChangesOptions extends that, and FindObserveChangesOptions inherits it. FindOptions deliberately does not - a plain find has no lifetime. All 25 `& WithSignal` intersections collapse to the plain options type. corelib's MongoLiveQueryHandle and the last three deprecated overloads (InMemoryMongoCollection.onChange/observe/observeChanges) are deleted, so an AbortSignal is required there too. webui re-exports meteor-lib's separate copy of the type, not this one, so corelib had no consumers outside its own tests; meteor-lib's stays as it is frontend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The engine test's waits now say what they were still waiting for, which is what identified the residual intermittent failure as a stale-snapshot revert (published val goes 1 -> 2 -> 1 -> 2 for a single update) rather than slow delivery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Julusian
added
Contribution from SuperFly.tv
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
About the Contributor
This pull request is posted on behalf of Superfly
Type of Contribution
This is a: Code improvement
Current Behavior
Meteor had a convention of returning
stopcallbacks for mongo observers and other bits of reactivity that want to be stopped.We were semi-constrained to this previously, but now that we no longer use meteor, we are free to easily change this.
In the past these stop callbacks have given us some bugs, as it is very easy to setup a bunch of observers, and not handle them correctly. In particular I remember a bug where there were a bunch setup inside a Promise.all, and if one of them threw then the others would be leaked.
New Behavior
A modern and js standard approach to this is to use AbortSignals instead of returning callbacks.
This means that the observe calls are bound to a signal instead of returning their stop function, and we can abort the signal and let that bubble through to all the observers setup on it.
In particular, for the ddp publications, when a subscription is started it now creates an AbortSignal, which will be aborted either when the client disconnects, or unsubscribes. This signal gets passed down through the call stack, sometimes wrapped in a new AbortSignal, to allow a local point for stopping and restarting, right through the mongo logic.
The signal is set as required in the collection methods, to enforce it is provided and ensure that cleanup is thought about.
Similarly, there has been at least one bug where a debounce in some custom publication logic has fired and restarted a stopped publication. The debounce used in these now also gets bound to this signal, and even if it did fire, the signal would be aborted and the new observer would fail to initialise because of this.
This is a POC that needs some further testing
Testing
Affected areas
Time Frame
Other Information
Status