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

New issue

Closed

opened 2026-09-05 23:24:06 +00:00 by bcox · 1 comment

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.

bcox referenced this issue from a commit

2026-09-05 23:28:17 +00:00

A count this branch made false, replaced with one that cannot go stale

bcox referenced this issue from a commit

2026-09-10 05:18:09 +00:00

The truncation sweeps stop building a voice pool per byte

bcox commented

2026-09-10 22:33:27 +00:00

Author

Owner

Closed by b65fc2c ("The truncation sweeps stop building a voice pool per byte"), which took option 2: Init() now clears patch_.lfo (plugin/instrument.h:1608), so there is no leak left to count. The read path already had the matching reset (plugin/patch_format.h:7582, the "no chunk means no inherited state" prelude), so both sides are covered.

Item 1 collapsed into item 2. With the leak closed there is nothing for a rewritten comment to enumerate, so the comment above what is now Bank.EveryBlobIsTheSameBuiltAloneOrAfterTheWholeBank carries no registry indices, no "last patch to set", and no blob counts -- the class of claim this issue was filed about. The test itself widened from the macro knobs to byte equality on the whole serialized patch, iterating patches::Count(), which is the derive-don't-restate the issue asked for.

Re-verified here rather than taken from the commit message, since the standard in CLAUDE.md is that the injections that count are the ones re-run by something that did not write the code. Removing patch_.lfo = {}; from Init() and running //patches:patches_test --test_filter='Bank.EveryBlobIsTheSameBuiltAloneOrAfterTheWholeBank':

patches/patches_test.cc:586: Failure
Expected equality of these values:
  reused[b]  Which is: '\x3' (3)
  alone[b]   Which is: '\x1' (1)
filter-whistle differs at byte 14412 when it is built after helicopter

Restored, tree clean, test passes. That failure is also this issue's substantive correction reproduced independently: helicopter was never the last patch to set voice LFOs, and filter-whistle -- the very next entry -- is the first blob it contaminated.

One postscript for the #63 theme. This issue's own figures have already drifted in the five days since it was filed: it measured against a 31-patch registry, and the bank is 40 now (Bank.HasFortyPatches, patches/patches_test.cc:3300). "16 of 31" would be wrong today for the same reason "twenty of thirty" was wrong when it was written. That is the argument for having fixed the leak rather than recounted it -- and the surviving count is a test that fails loudly on the next addition, not prose that goes quietly stale.

Closed by `b65fc2c` ("The truncation sweeps stop building a voice pool per byte"), which took option 2: `Init()` now clears `patch_.lfo` (`plugin/instrument.h:1608`), so there is no leak left to count. The read path already had the matching reset (`plugin/patch_format.h:7582`, the "no chunk means no inherited state" prelude), so both sides are covered. Item 1 collapsed into item 2. With the leak closed there is nothing for a rewritten comment to enumerate, so the comment above what is now `Bank.EveryBlobIsTheSameBuiltAloneOrAfterTheWholeBank` carries no registry indices, no "last patch to set", and no blob counts -- the class of claim this issue was filed about. The test itself widened from the macro knobs to byte equality on the whole serialized patch, iterating `patches::Count()`, which is the derive-don't-restate the issue asked for. Re-verified here rather than taken from the commit message, since the standard in CLAUDE.md is that the injections that count are the ones re-run by something that did not write the code. Removing `patch_.lfo = {};` from `Init()` and running `//patches:patches_test --test_filter='Bank.EveryBlobIsTheSameBuiltAloneOrAfterTheWholeBank'`: patches/patches_test.cc:586: Failure Expected equality of these values: reused[b] Which is: '\x3' (3) alone[b] Which is: '\x1' (1) filter-whistle differs at byte 14412 when it is built after helicopter Restored, tree clean, test passes. That failure is also this issue's substantive correction reproduced independently: helicopter was never the last patch to set voice LFOs, and filter-whistle -- the very next entry -- is the first blob it contaminated. One postscript for the #63 theme. This issue's own figures have already drifted in the five days since it was filed: it measured against a 31-patch registry, and the bank is 40 now (`Bank.HasFortyPatches`, `patches/patches_test.cc:3300`). "16 of 31" would be wrong today for the same reason "twenty of thirty" was wrong when it was written. That is the argument for having fixed the leak rather than recounted it -- and the surviving count is a test that fails loudly on the next addition, not prose that goes quietly stale.

bcox closed this issue

2026-09-10 22:33:31 +00:00