fix: fuzz test invocation with referring protobufs

[simonresch]

When constructing a mutator for a protobuf message the mutator is cached based on the protobuf descriptor. This led to issues for fuzz tests that take two protobuf messages where one message is referring to the other.

While building the mutator for the top level message nested messages are constructed with the type DynamicMessage which lead to 'argument type mismatch' exceptions when invoking the fuzz test method. This is fixed by including the message class in the mutator cache key which ensures that a generated protobuf message class does not re-use the corresponding DynamicMessage mutator.

Note, that this causes some recursion breaking to happen one layer deeper (see test assertion changes).

[Copilot]

Pull Request Overview

This PR fixes a bug where fuzz tests with multiple protobuf message parameters fail with "argument type mismatch" errors when one message type is a submessage of another. The fix adds the message class to the mutator cache key, preventing incorrect reuse of DynamicMessage mutators for concrete message types.

Changes

• Modified the CacheKey class to include the message class alongside the descriptor and anySource
• Added a regression test to verify fuzz test invocation with referring protobufs
• Re-enabled a previously commented-out fuzz test that was failing due to this bug

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
BuilderMutatorFactory.java	Updated CacheKey to include message class in cache key, fixing mutator reuse for nested protobuf messages
ArgumentsMutatorTest.java	Added regression test testProtoWithSubmessage() to verify the fix
ArgumentsMutatorFuzzTest.java	Re-enabled fuzz_MutuallyReferringProtobufs test that was previously failing

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[oetr]

Nice fix!

I think, we could prevent these sort of bugs entirely, if we changed the HashMap internedMutators from global to per fuzz test argument. Like this, cycles in recursive protos will still be broken like before, but the fuzz test arguments will not influence each other's cached internedMutators.

[simonresch]

I think, we could prevent these sort of bugs entirely, if we changed the HashMap internedMutators from global to per fuzz test argument. Like this, cycles in recursive protos will still be broken like before, but the fuzz test arguments will not influence each other's cached internedMutators.

I was thinking in this direction as well, however I could not find a clean entry point to separate the BuilderMutatorFactory instances by argument. We could adjust the argument/product mutator factory to work on an individual mutator factory per argument, but some mutators could benefit from sharing state across different arguments (maybe CachedConstructorMutatorFactory?).

WDYT, is this worth investigating further? Do you expect any downsides from breaking the recursion one layer deeper?

[oetr]

I think, we could prevent these sort of bugs entirely, if we changed the HashMap internedMutators from global to per fuzz test argument. Like this, cycles in recursive protos will still be broken like before, but the fuzz test arguments will not influence each other's cached internedMutators.

I was thinking in this direction as well, however I could not find a clean entry point to separate the BuilderMutatorFactory instances by argument. We could adjust the argument/product mutator factory to work on an individual mutator factory per argument, but some mutators could benefit from sharing state across different arguments (maybe CachedConstructorMutatorFactory?).

WDYT, is this worth investigating further? Do you expect any downsides from breaking the recursion one layer deeper?

I don't see any downsides, it's a great solution with minimal changes that can and should be merged already!

I am a bit scared of temporary protobuf enum mutators (or others) ending up in internedMutators, and still getting matched when constructing mutators for subsequent args when they shouldn't. Now that you added messageClass to the cache key this shouldn't happen anymore. Right? But I can't tell for sure.

Sharing state between mutators can be done by wiring it though tryCreate, like in the draft PR for the dictionary provider.
The recursion-breaking mutator cache, and all other global data that need to be shared between several mutators, or reset per-argument, can be stored in the new class MutatorRuntime instead of being hidden in some mutator factory.
It's also a nice place to put type-based TORCs 😉