Copy, don't retain, message data carried across calls - #218
Merged
Conversation
`BinnedAggregateTransformer` and `DiffTransformer` kept `message.data` (or a view of it) in their state and read it back on the next call. Message data is not the transformer's to keep: over a cross-process link it is a view into a shared-memory slot the publisher recycles, so the retained samples silently become whatever overwrote them -- valid arrays of the right shape and dtype, wrong numbers, no exception. Copy at each point of retention with `xp_copy`: - binned_aggregate: the `carry is None` branch when no bin completes, and the leftover tail after the last completed bin (a view of `message.data` whenever there was no prior carry; copying also stops a couple of carried samples from pinning a whole chunk). - diff: the trailing sample kept for the next message's first diff, and the reset-state slice, which was not exposed but left state aliasing the message. Nothing in the suite could catch this: in-process links pass messages by reference with no serialization, so aliased and owned arrays behave identically. `tests/helpers/recycled_shm.py` adds a harness that marshals through ezmsg's own `Marshal` into a single slot -- the publisher's ring compressed so reuse is deterministic on the next message -- and asserts that a transformer's output is identical whether its input is owned or recycled. `tests/unit/test_buffer_recycling.py` applies it to both transformers, and includes a deliberately-retaining canary transformer so the harness cannot go blind. Fixes #214
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.
Fixes #214.
BinnedAggregateTransformerandDiffTransformerkeptmessage.data(or a view of it) in their state and read it back on the next call. Message data is not the transformer's to keep: over a cross-process link it is a view into a shared-memory slot the publisher recycles (PEP 574 out-of-band buffers,msg_id % num_buffersring), so the retained samples silently become whatever overwrote them — valid arrays of the right shape and dtype, wrong numbers, no exception.Only cross-process links are affected; in-process publishers pass the object by reference with no serialization, which is part of why this survived testing.
Fix
Copy at each point of retention, via the existing backend-portable
util.array.xp_copy:binned_aggregate.py— thecarry is Nonebranch when no bin completes (thexp.concatbranch already allocates), and the leftover tail after the last completed bin, which was a view ofmessage.datawhenever there was no prior carry. Copying the tail also stops a couple of carried samples from pinning a whole chunk alive.diff.py— the trailing sample kept for the next message's first diff, and the_reset_stateslice, which was not exposed (overwritten within the same call) but left state aliasing the message.This matches the convention already used elsewhere in the package:
Sampler,Resample,ResampleConcatandWindowall buffer throughHybridBufferwithupdate_strategy="immediate"precisely so nothing is retained by reference.Tests
Nothing in the suite could catch this, so the fix comes with a harness for the whole bug class.
tests/helpers/recycled_shm.pymarshals each message through ezmsg's ownMarshalinto a single slot — the publisher's 32-slot ring compressed so reuse is deterministic on the very next message rather than 32 later.assert_survives_buffer_recycling(make_proc, messages)runs the same inputs through two fresh transformers, one on owned arrays and one on recycled ones, and requires bit-identical outputs: same arithmetic on the same numbers, so any difference at all means state was read back from recycled memory.tests/unit/test_buffer_recycling.pyapplies it to:diff, bothscale_by_fssettings, plus one test pinning the actual value of the cross-message boundary diffbinned_aggregate: no-bin-completed, leftover-tail, fractional and sample-locked grids at an off-nominal 1013 Hz, and stacked MIN/MAXplus two tests of the harness itself — a deliberately-retaining canary transformer that must trip the assertion, and a check that the slot really aliases and really gets overwritten — so it cannot go blind and let the others pass for the wrong reason.
All 8 target tests fail against the unfixed source and pass with it. Full unit suite: 4049 passed, 6 skipped.
One caveat worth knowing: whether corruption of a small carry is observable depends on the data (a two-sample corruption can average away under MEAN, or fall inside the bin's range under MIN/MAX). The multi-op test uses a seed where it is observable — 6 of 8 seeds tried detect it. The leftover-tail test covers the same code path without that sensitivity.
🤖 Generated with Claude Code