MEDIUM: EqualityChain shares an unsynchronized mutable cache — race/lost-update hazard under AsyncScheduler

[docxology]

Summary

EqualityChain (built for every variable of degree>1, variables/random.jl) shares two BitVectors (cacheleft/cacheright) and per-node mutable Message caches among all outbound mappings. materialize! (src/nodes/equality.jl:130-152) and ChainInvalidationCallback (equality.jl:169-182) read/write those structures with no lock. Under the documented ScheduleOnStreamPostprocessor(AsyncScheduler()) (postprocessors/scheduled.jl), concurrent outbound computations for different indices can tear/overwrite the cache and produce outbound messages from stale or partially-written prefix products — a silent-wrong-message hazard.

Impact / scope

• Default single-threaded scheduler is safe; this is latent.
• Opt-in async scheduling + high-degree variables ⇒ potential lost updates → silent wrong messages.

Suggested fix

Hold a per-chain ReentrantLock across materialize! and the invalidation callback (or use atomic cache bits and re-validate iscached before trusting a read). This is a design/trade-off change, so a PR description (not a committed branch) is drafted in prs/pr-17-equalitychain-lock.md for maintainer discussion.

[bvdmitri]

Maintainer triage: the premise of this report is incorrect — the described data race cannot occur — and the suggested fix would cost real performance to defend against it. Leaving open as a note rather than closing outright, since the underlying "is the cache reasoning airtight?" question is a fair one to keep on record.

AsyncScheduler is not multithreaded

The report's impact rests on "under threaded scheduling ... concurrent outbound computations for different indices can tear/overwrite the cache" and calls it "undefined behavior". But AsyncScheduler is a Rocket.jl scheduler built on coroutines, not threads. In Rocket/src/schedulers/async.jl every concurrency primitive is @async:

src/schedulers/async.jl:57:        @async begin
src/schedulers/async.jl:105:    channeling_task = @async begin
src/schedulers/async.jl:114:    subscription_task = @async begin

There is no Threads.@spawn anywhere in the file, and AsyncSchedulerInstance synchronizes through a Channel. @async tasks are cooperatively scheduled onto a single thread, so two of them never execute simultaneously and no shared-memory tearing is possible. "Unsynchronized mutation of shared mappings vectors is undefined behavior" simply does not apply to this execution model.

Cooperative interleaving is also not a hazard here

The remaining theoretical concern would be a task yielding mid-update and another observing a half-written cache. That requires a yield point inside the critical section. materialize! (src/nodes/equality.jl:130-152) contains only getrecent, prod, setcache! and index arithmetic — no wait, no channel operation, no @async, nothing that yields. Same for ChainInvalidationCallback (equality.jl:169-182), which is two fill_bitarray! calls. Both are effectively atomic with respect to the task scheduler, so a reader can never see a partially-written prefix product.

Why we would not add the lock even if it were free

EqualityChain is constructed for every variable of degree > 1 (src/variables/random.jl), and materialize! is on the hottest path in the whole library — it runs once per outbound message per iteration. Wrapping it and the invalidation callback in a ReentrantLock would add uncontended lock/unlock traffic to every message computation in every model, plus block inlining of a small function that currently inlines. That is a measurable cost on all users, paid to prevent a class of bug that the execution model rules out.

If ReactiveMP ever grows genuinely thread-parallel scheduling (Threads.@spawn-based), this becomes a real design question and the cache would need revisiting — along with a fair amount of other shared mutable state. Until then there is nothing to fix.

Related: #637 makes the same "thread-unsafe shared buffers" claim about src/approximations/importance.jl and is wrong for the same reason. That issue does contain separate, genuine defects (the n_eff computation and a missing isinf guard), which are being fixed — see the comment there.

[bvdmitri]

Related annotation-race work has been merged — #639 fixed the ProductInputArgumentsRecord in-place mutation (#622) and #640 fixed the deferred-missing annotation crash (#623). This issue is distinct and remains open: the unsynchronized cacheleft/cacheright cache in EqualityChain under a threaded scheduler is not touched by those PRs and needs a dedicated locking/design change (no PR yet).