Add .NET Sdk

[sp-jcberleur]

What changes are proposed in this pull request?

Initial Databricks Zerobus Ingest SDK for .NET to ingest data from .NET applications

How is this tested?

dotnet test

[teodordelibasic-db]

Thanks a lot for contributing and sorry for the delayed review, we've been quite overwhelmed with product stabilization these past few weeks. I'll prioritize now so that we can get it in, a few other customers also want to use it. I am nowhere near a C#/.NET expert so I am reviewing mostly with LLM help.

A few more general comments below as well:

---

We should integrate the .NET CI into general repo CI. Currently PRs touching only dotnet/** pass CI without any checks and Rust FFI changes don't trigger cross-SDK .NET tests. In .github/workflows/push.yml we should add:

1. dotnet output in the changes job.
2. dotnet path filter (dotnet/** + .github/workflows/ci-dotnet.yml)
3. dotnet job calling ci-dotnet.yml
4. dotnet in the gate job's needs array + result check loop
5. cross-sdk-dotnet job (mirroring cross-sdk-go) triggered when rust/** changes

---

A couple of missing misc files in the dotnet/ directory that we have in other SDKs:

1. CONTRIBUTING.md - guidelines how to setup a devloop for making changes and contributing them for this SDK.
2. NEXT_CHANGELOG.md - used by the release process for unreleased changes
3. NOTICE - license/attribution file

---

As far as I understand, this is a sync-only API, LLM suggests we could benefit from an API returning Task object for methods CreateStream, WaitForOffset, Flush, and Close. ASP.NET Core users calling current API on request threads will block thread pool threads.

Not a blocker obviously, just want to cross-check with you if it makes sense to add it in the future.

[RichardCupples]

Any progress, PR has been parked for 2 months now?

[RichardCupples]

Hi, I understood this PR was to be prioritized, could it please be merged soon?

[at-besa]

Also waiting for this some time now 👍🏽

[teodordelibasic-db]

File: rust/ffi/src/lib.rs:1092

The ingest and flush paths keep the retryable flag by building the result with CResult::error(err), which reads err.is_retryable() (lib.rs:692). But create_stream (lib.rs:1092), create_stream_with_headers_provider (lib.rs:1160) and recreate_stream (lib.rs:1206) turn the error into a plain String first and then call write_error_result(result, &err, false), which hardcodes the flag to false.

So a .NET caller always sees ZerobusException.IsRetryable as false for a failed create or recreate, even when Rust considered the error retryable. Create and recreate are exactly where a caller decides whether to retry, so this looks like a real bug rather than just cosmetic.

Could we keep the typed error all the way to the result and use CResult::error(err) for create, create with headers provider, and recreate, the same way ingest does? A test that injects a retryable failure on create and checks that IsRetryable is true would cover it.

[teodordelibasic-db]

File: rust/ffi/src/lib.rs:573

Only zerobus_arrow_stream_is_closed exists (lib.rs:573). There is no is_closed for the proto or JSON stream, and the .NET wrapper does not expose one. The Rust stream and the Go SDK both have is_closed(), and it is the natural way to decide when to call RecreateStream. Without it, .NET callers can only guess from caught exceptions. We could add it (a small FFI addition).

[teodordelibasic-db]

File: rust/ffi/src/lib.rs:833

CallbackHeadersProvider::get_headers sets an in_use flag and returns the error "Concurrent headers provider callback detected - Go callback must be thread-safe" if it sees a second call at the same time (lib.rs:835). This is existing shared FFI code, but the recreate path added in this PR makes it easier to hit: recreate_stream calls Arc::clone(&stream.headers_provider) (lib.rs:578), so the old and new streams share one provider during recovery, and if both call it at once a user could get this error through no fault of their own. Two small suggestions: document on IHeadersProvider that implementations must be thread-safe, and change the user visible error text so it does not mention Go.

[teodordelibasic-db]

File: rust/ffi/src/lib.rs:1226

None of the FFI entry points wrap their body in std::panic::catch_unwind (zero occurrences in the file). A Rust panic that unwinds across the extern "C" boundary is undefined behavior and in practice takes down the whole .NET process. This is existing shared FFI code and also affects Go, so it may be out of scope for this PR, but since .NET now calls into it, it would be worth a guard on the entry points (or a tracking issue) so a panic becomes an error result instead of a crash.

[teodordelibasic-db]

README.md:98 says the stream is thread-safe, and the XML docs (ZerobusStream.cs:12) say you can call IngestRecord from many threads at once. There is no test that actually ingests from several threads on one stream, and because Dispose can race with an in-flight call, the claim is not currently safe.

Whatever the final design is, it would be good to make the docs match it. For example, we could say that concurrent ingest is safe but ingest at the same time as Dispose or Close is not, or make it fully safe and keep the promise. It would be better not to ship the claim as it stands.

[sp-jcberleur]

I think the reader/writer lock in a697429 fixes this

[teodordelibasic-db]

Every operation is a blocking call over an async Rust operation running on a shared Tokio runtime (RUNTIME.block_on). WaitForOffset, Flush, Close, stream creation, and ingest under backpressure all hold the calling .NET thread for the whole network wait, and there is no CancellationToken. In ASP.NET Core or worker services this ties up thread pool threads and can starve them under load.

Matching the Go SDK's blocking style is reasonable for a first version, but it would be good to make it a clear choice rather than a surprise. At a minimum, we could say in the README that these are blocking calls and should not run on request handling threads. As a follow up, we could add async versions (CreateStreamAsync, IngestRecordAsync returning ValueTask<long>, WaitForOffsetAsync, FlushAsync, CloseAsync), IAsyncDisposable, and CancellationToken support. Real async needs the FFI to signal completion through a callback; wrapping in Task.Run only moves the blocking, it does not remove it. If that is out of scope for now, it would help to say so.

[sp-jcberleur]

added in 514c7f9, c52ed4f & 38f7c0d

It did require ffi changes, if you want I can remove them from this PR and do a follow up

[teodordelibasic-db]

The tests run sequentially (the Parallelizable attribute only parallelizes independent fixtures). It would be good to add before merge:

• Many threads ingesting on one stream while another thread disposes, closes, or recreates it, to exercise the use-after-free path.
• A close or flush failure, then GetUnackedRecords still returns the records, then RecreateStream still works.
• Assertions that IsRetryable is true for a retryable create or recreate failure.
• A headers provider test that runs the real C# callback allocation path on Windows, Linux and macOS.

The current unit tests (ZerobusSdkTests, NativeInteropTests) are fairly thin, mostly null argument checks.

[sp-jcberleur]

added more tests in e47ad12

[teodordelibasic-db]

RecreateStream(stream) returns a new stream and also marks the stream passed in as disposed, freeing its native pointer (stream.Recreate, ZerobusStream.cs:299). Passing an object in and having it become unusable is a little surprising in C#, and it does not work well with using var stream = .... We could either return the new stream without changing the argument, or make the behavior clear in the method's XML docs and the README so callers know not to use the old handle afterward.

[sp-jcberleur]

improved docs in e185a24

[teodordelibasic-db]

Returning object[], where each element is either a string or a byte[], forces boxing and type checks at the call site and is hard to change once the API is public. We could use a typed shape instead, for example a readonly record struct UnackedRecord(byte[] Data, bool IsJson) with an AsJson() helper, or return ReadOnlyMemory<byte>[] and let the caller decode. Worth doing before this is public.

[teodordelibasic-db]

ConvertConfig calls the native GetDefaultConfig() when options is null (NativeInterop.cs:405), but when options is set it fills the unset fields from the hardcoded C# defaults (StreamConfigurationOptions.Default, NativeInterop.cs:407). The record's default values (StreamConfigurationOptions.cs) copy the Rust zerobus_get_default_config values (lib.rs:1651). If Rust changes a default, the C# record would quietly fall out of sync. We could seed the record from GetDefaultConfig() once, or add a test that asserts the two sets of defaults are equal so any drift is caught.

[sp-jcberleur]

fixed in 3dd542d

[teodordelibasic-db]

In NativeCallback the header array is allocated (HeadersProviderBridge.cs:44) and then each key and value string is allocated in the loop. If one of those allocations throws (for example out of memory in AllocUtf8String), the catch block (HeadersProviderBridge.cs:63) sets Headers to zero and only allocates an error message. The array and strings already allocated are leaked, because Rust can only free what is returned in CHeaders. In an auth callback that may run again and again on transient failures, this adds up. We could track the pointers allocated and free them in the catch or a finally before returning the error.

[sp-jcberleur]

fixed in f32ff50

[teodordelibasic-db]

A caller can make a stream with RecordType.Json and still call the byte[] (proto) overload, or make a proto stream with no descriptor, and only find out when Rust rejects it later. Since one ZerobusStream exposes every ingest overload, examples cannot catch this at compile time. Not a blocker, but before publishing we could add typed factories such as CreateJsonStream and CreateProtoStream(descriptor) that return stream types exposing only the matching overloads. That would also remove the overlap between TableProperties.DescriptorProto and RecordType. If that is too much, we could at least document that JSON streams must set RecordType.Json and use the string overloads.

[sp-jcberleur]

added in fbd1c96, f75a54b

[teodordelibasic-db]

LGTM to merge, we should just rebase, if you think there are any follow ups feel free to create issues so we can track.

We also have to see how we will be able to merge this, CI is broken for forks for the last couple of months, we are fine with granting you write access but can't seem to find the GitHub user for some reason.

[teodordelibasic-db]

LGTM to merge, we should just rebase, if you think there are any follow ups feel free to create issues so we can track.

We also have to see how we will be able to merge this, CI is broken for forks for the last couple of months, we are fine with granting you write access but can't seem to find the GitHub user for some reason.

Okay I can't grant you write access because you are not a member of the databricks GitHub org, so we will probably have to go with me pushing your branch to main and opening a PR so that CI can run normally. Are you fine with that? If yes, we can proceed once conflicts are resolved.

[sp-jcberleur]

LGTM to merge, we should just rebase, if you think there are any follow ups feel free to create issues so we can track.
We also have to see how we will be able to merge this, CI is broken for forks for the last couple of months, we are fine with granting you write access but can't seem to find the GitHub user for some reason.

Okay I can't grant you write access because you are not a member of the databricks GitHub org, so we will probably have to go with me pushing your branch to main and opening a PR so that CI can run normally. Are you fine with that? If yes, we can proceed once conflicts are resolved.

yes that's fine, conflicts are fixed