patches_test.cc: the VLFO leak comment names the wrong patch and undercounts the blobs it reaches #70

New issue

Open

opened 2026-09-05 23:24:06 +00:00 by bcox · 0 comments

bcox commented

2026-09-05 23:24:06 +00:00

Owner

patches/patches_test.cc, in the comment block above
Bank.EveryBlobsMacroKnobsAreItsOwn, records a known and deliberately-unfixed
data issue: Init() does not reset patch().lfo, so //patches:install's
reused Instrument leaks one patch's voice-level LFO shapes into every blob
serialized after it.

The finding is real and still real. Its specifics are wrong, and the way
they are wrong hides how far the leak now spreads.

The comment says:

patches/helicopter.h (registry index 9) is the last patch to set them, so
all twenty blobs after it carry helicopter's LFO shapes. Nineteen bytes
inside VLFO's fifty-six, in twenty of thirty blobs.

Measured against the registry as it stands (31 patches):

helicopter is at index 11, not 9.
helicopter is not the last patch to set voice LFOs. Seven patches set
them: kargyraa (8), rattlesnake (10), helicopter (11),
filter-whistle (12), juno-pad (16), sem-pad (17), and
arp2600-burble (21). The last is arp2600-burble.
So the leak is not one patch's shapes across one tail. It is segmented:
each blob carries whichever LFO-setting patch most recently preceded it.
wind (9) carries kargyraa's; 13-15 carry filter-whistle's; 18-20 carry
sem-pad's; 22-30 -- nine blobs -- carry arp2600-burble's.
That is roughly 16 of 31 blobs carrying LFO shapes belonging to some
other patch, against the "twenty of thirty" the comment states.

What is still true, and worth keeping when this is rewritten: only
patches/rattlesnake.h and patches/helicopter.h route kSharedLfo1 or
kSharedLfo2 at all, so for every contaminated blob this remains dead data
that the editor would nonetheless show. The leak is still inaudible. It is the
scale and the shape of it that the comment now understates.

The comment's closing trade-off is also stale: it says closing the leak "moves
twenty shipped blobs where the macro fix moved three." On the numbers above it
would move about sixteen, and they would move to different values than the
comment implies, since the contaminating patch differs per segment.

Two things to do, and the second is the point:

Rewrite the comment against a measurement rather than against these
inferred counts. I derived the segmentation from the registry order and a
grep for patch().lfo; the byte-level claim ("nineteen bytes inside VLFO's
fifty-six") should be re-measured, not carried forward.
Consider whether the leak should just be closed now. It was left open
because it moved twenty blobs; it is not obvious that a leak which has
silently grown a segmented shape is more safely left than fixed. Closing it
is Init() resetting patch().lfo, and the test above is already the
instrument that would catch a regression.

This is a fifth instance of the hand-maintained-count problem in #63 -- the
registry index, the "last patch", and both blob counts all drifted together
because nothing derives them. Anything rewritten here should derive from
patches::Count() and the registry rather than restate them.

Found while fixing an adjacent stale count on branch fm-followups, which
added a 31st patch and shifted every index in that comment by one. The
substantive error -- helicopter not being last -- predates that branch.

`patches/patches_test.cc`, in the comment block above `Bank.EveryBlobsMacroKnobsAreItsOwn`, records a known and deliberately-unfixed data issue: `Init()` does not reset `patch().lfo`, so `//patches:install`'s reused `Instrument` leaks one patch's voice-level LFO shapes into every blob serialized after it. The finding is real and still real. Its **specifics are wrong**, and the way they are wrong hides how far the leak now spreads. The comment says: > `patches/helicopter.h` (registry index 9) is the last patch to set them, so > all twenty blobs after it carry helicopter's LFO shapes. Nineteen bytes > inside VLFO's fifty-six, in twenty of thirty blobs. Measured against the registry as it stands (31 patches): - `helicopter` is at index **11**, not 9. - `helicopter` is **not** the last patch to set voice LFOs. Seven patches set them: `kargyraa` (8), `rattlesnake` (10), `helicopter` (11), `filter-whistle` (12), `juno-pad` (16), `sem-pad` (17), and `arp2600-burble` (21). The last is `arp2600-burble`. - So the leak is not one patch's shapes across one tail. It is **segmented**: each blob carries whichever LFO-setting patch most recently preceded it. `wind` (9) carries kargyraa's; 13-15 carry filter-whistle's; 18-20 carry sem-pad's; 22-30 -- nine blobs -- carry arp2600-burble's. - That is roughly **16 of 31** blobs carrying LFO shapes belonging to some other patch, against the "twenty of thirty" the comment states. **What is still true**, and worth keeping when this is rewritten: only `patches/rattlesnake.h` and `patches/helicopter.h` route `kSharedLfo1` or `kSharedLfo2` at all, so for every contaminated blob this remains dead data that the editor would nonetheless show. The leak is still inaudible. It is the scale and the shape of it that the comment now understates. The comment's closing trade-off is also stale: it says closing the leak "moves twenty shipped blobs where the macro fix moved three." On the numbers above it would move about sixteen, and they would move to *different* values than the comment implies, since the contaminating patch differs per segment. Two things to do, and the second is the point: 1. Rewrite the comment against a measurement rather than against these inferred counts. I derived the segmentation from the registry order and a grep for `patch().lfo`; the byte-level claim ("nineteen bytes inside VLFO's fifty-six") should be re-measured, not carried forward. 2. Consider whether the leak should just be closed now. It was left open because it moved twenty blobs; it is not obvious that a leak which has silently grown a segmented shape is more safely left than fixed. Closing it is `Init()` resetting `patch().lfo`, and the test above is already the instrument that would catch a regression. This is a fifth instance of the hand-maintained-count problem in #63 -- the registry index, the "last patch", and both blob counts all drifted together because nothing derives them. Anything rewritten here should derive from `patches::Count()` and the registry rather than restate them. Found while fixing an adjacent stale count on branch `fm-followups`, which added a 31st patch and shifted every index in that comment by one. The substantive error -- helicopter not being last -- predates that branch.