Batch 3 Bugfix: Fix NullReferenceException in ByteStreamReader.WriteToFile default args#252
Draft
MaxHeimbrock wants to merge 1 commit intomainfrom
Draft
Batch 3 Bugfix: Fix NullReferenceException in ByteStreamReader.WriteToFile default args#252MaxHeimbrock wants to merge 1 commit intomainfrom
MaxHeimbrock wants to merge 1 commit intomainfrom
Conversation
ByteStreamReader.WriteToFile(string directory = null, string nameOverride = null)
threw ArgumentNullException from the generated protobuf setter when either
argument was null (which is the documented default). The two affected
assignments are the same pattern the rest of the codebase uses with a null
guard (see StreamTextOptions.ToProto, StreamByteOptions.ToProto, etc.);
the WriteToFile path simply missed them.
writeToFileReq.Directory = directory; // ArgumentNullException if null
writeToFileReq.NameOverride = nameOverride; // ArgumentNullException if null
Adds null guards matching the existing codebase convention.
Validation: locally ran a PlayMode E2E regression test that called
reader.WriteToFile(outDir) from a byte-stream handler. Before this fix, the
call threw synchronously. After, the call returns a WriteToFileInstruction
and the handler proceeds normally. (The instruction's final completion is
the Rust side's job and is orthogonal to this C# null-safety fix.)
A persistent regression test is not added in this PR because any test that
creates a ByteStreamReader and releases it through normal scope exit trips
CLT-2773 (FfiHandle's SafeHandle finalizer calls FfiDropHandle off the
Tokio runtime, aborting the Unity process on teardown). NUnit reports the
test as passing before the abort, but the script's exit code captures the
abort. That's pre-existing, tracked separately, and out of scope here.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MaxHeimbrock
changed the title
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
1 participant
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.
Summary
`ByteStreamReader.WriteToFile(string directory = null, string nameOverride = null)` threw `ArgumentNullException` from the generated protobuf setter when either argument was null — which is exactly the documented default:
```
System.ArgumentNullException: Value cannot be null. Parameter name: value
at Google.Protobuf.ProtoPreconditions.CheckNotNull[T] (...)
at LiveKit.Proto.ByteStreamReaderWriteToFileRequest.set_NameOverride (...)
at LiveKit.ByteStreamReader.WriteToFile (...) in DataStream.cs:397
```
Same pattern the rest of the codebase already uses a null guard for (see `StreamTextOptions.ToProto`, `StreamByteOptions.ToProto` which each have a `// TODO: these fields are optional, but the generated proto is not allowing null values` comment and guard accordingly). The `WriteToFile` path just missed them.
The fix
```diff
```
Validation
Ran a PlayMode E2E regression test locally (`reader.WriteToFile(outDir)` inside a byte-stream handler). Before the fix, the call threw synchronously. After, the call returns a `WriteToFileInstruction` and the handler proceeds normally.
Why no regression test in this PR
Any PlayMode test that creates a `ByteStreamReader` and releases it through normal scope exit trips CLT-2773: the `FfiHandle` `SafeHandle` finalizer calls `FfiDropHandle` off the Tokio runtime, aborting the Unity process on teardown. NUnit reports the test as passing before the abort, but the script's exit code captures the abort and reports "iteration failed." That's pre-existing, tracked separately, and explicitly out of scope for the current test-coverage workstream. Once CLT-2773 is resolved, a regression test can be added as a small follow-up.
Related
There are two other similar null-safety issues in this file that I noticed while in here — `ByteStreamWriter.Close(string reason = null)` and `TextStreamWriter.Close(string reason = null)` both pass `reason` directly to their proto setters and would throw on `Close()` without args. Leaving those for a follow-up to keep this PR focused.
🤖 Generated with Claude Code