I think this is here:

This is the code that splits the circuit into a unitary ("predicate/matching") prefix and a non-unitary suffix, so that the result of the unitary prefix can be cached and reused for each repetition, so that only the non-unitary part has to be run subsequently.

It looks like the code places the empty classical controlled circuit in the prefix, since it has no qubits, so it'll get executed before the measurement (which goes in the suffix). I can't remember offhand why that's two separate if blocks. Seems like

            if not predicate(op) or not qs.isdisjoint(blocked_qubits):
                general_part.append(op)
                blocked_qubits |= qs
            else:
                matching_part.append(op)

would be more succinct and fix the error?

However, to be thorough, the function should be revised to take measurement keys into account just like qubits: once an op with a measurement key M has been allocated to the suffix ("general_part"), any subsequent op with the same measurement key M should also go into the suffix. So just set up a "blocked_measurement_keys" set and check those too e.g. if not predicate(op) or not qs.isdisjoint(blocked_qubits) or not keys.isdisjoint(blocked_keys). The normal simulator uses is_unitary as predicate, but other simulators use other predicates, so the same bug might happen in other simulators unless the measurement keys are explicitly taken into account.

I see, that makes sense.

I was going to say an even easier fix would be to ensure anything that touches a measurement key should go in the suffix. But it turns out that's actually not always the case: in principle, a classical simulator should put everything in the prefix. The only circuits it can handle are deterministic anyway, so samplers should simulate the circuit once and just return the same result N times rather than re-running the simulation. (Currently it's not implemented that way; one would have to override _can_be_in_run_prefix on ClassicalSimulator to return True always. Granted, nobody is running classical simulations with large repeat counts, but still, it's an easy improvement).

So, long story short, this should be fixed the "right" way by explicitly taking measurement keys into account, as described above.

This is the code that splits the circuit into a unitary ("predicate/matching") prefix and a non-unitary suffix, so that the result of the unitary prefix can be cached and reused for each repetition, so that only the non-unitary part has to be run subsequently.

cirq-sync: @daxfohl do you know what was the reason for splitting the circuit into prefix and suffix? and is it still relevant?

The reason is in that quote. So for example, if you run a simulation with 100 iterations of a unitary circuit with terminal measurements, then this splitting up allows it to run the unitary prefix once, cache the waveform at that point just prior to measurement, and then run the 100 iterations starting from that waveform and just doing the measurement steps on it, so subsequent iterations get to skip the whole simulation and perform like a sampler. Without this, each iteration would rerun the whole circuit from scratch. Similarly, if there is a channel or classical logic near the end of the circuit, then same thing, the unitary prefix result gets cached and the repetitions can all start over from the first non-unitary spot.

Density matrix simulator overrides the predicate, so mixtures and channels can be included in the prefix but measurements classical controls still cannot.

Is it still relevant?

Should be. But one could argue that in the first case, the user should just use a sampler instead of a simulator. And other cases, IDK how often it occurs in practice that most of the circuit is unitary until the very end. Removing this optimization would adversely affect those cases, but maybe they're infrequent enough not to be worthwhile.