feat(ndk): expose init return code

[hannojg]

Return int from SentryNdk.init reflecting sentry_init result.
This can then be used by sentry-java/android to double check if the Ndk integration could be initialized successfully (which currently isn't happening, we just call the native method and assume it went successful).

Upstream PR in getsentry/sentry-java for android ndk:

• feat(ndk): handle native init failure code sentry-java#4842

[supervacuus]

@markushi, I think we discussed this some time ago, but I can't remember whether return code or Exception was the choice back then.

[hannojg]

Just as info, I made it so that an exception will be thrown by the ndk integration in getsentry/sentry-java if the return code is not 0 for success (as there it seems to be the place where we throw exception if init fails)

[hannojg]

But yeah, interestingly i think these ENSURE_OR_FAIL calls will currently go as silent errors (as it doesn't throw or log or return an error code)

sentry-native/ndk/lib/src/main/jni/sentry.c

Lines 299 to 302 in 9895a5c

	ENSURE_OR_FAIL(outbox_path);
	transport = sentry_transport_new(send_envelope);
	ENSURE_OR_FAIL(transport);

With this change its also sub-optimal, since the developer might turn on debug logging, check their logs but doesn't get any valuable information from it.
I am actually wondering why we are doing that checking here, given that sentry_core.c already seems to do the same checking + provide proper error logging:

sentry-native/src/sentry_core.c

Lines 180 to 184 in 9895a5c

	if (sentry__path_create_dir_all(options->database_path)) {
	SENTRY_WARN("failed to create database directory or there is no write "
	"access to this directory");
	goto fail;
	}

?

// Edit: okay i don't know if sentry__path_create_dir_all would crash with an invalid string, so potentially we'd want to either throw or also log warnings here?

[supervacuus]

I think that was the main reason exceptions were raised back then: it allowed clients to differentiate between a native sentry_init failure and an error from the JNI layer, where the JNI layer could also provide more detailed feedback on which stage failed. What I cannot remember: whether there was a reason we didn't raise exceptions.

[markushi]

@sentry review

[sentry]

The public init() method should document the return value semantics for callers. Add a @return javadoc comment explaining that 0 indicates success and non-zero indicates failure, similar to the documentation added to the private native method.
_{Severity: MEDIUM}

💡 Suggested Fix

Suggested change

	public static int init(@NotNull final NdkOptions options) {
	loadNativeLibraries();
	initSentryNative(options);
	return initSentryNative(options);
	}
	/**
	* Initializes sentry-native and returns 0 on success, non-zero on failure.
	*
	* @param options the NdkOptions
	* @return 0 on success, non-zero on failure
	*/
	public static int init(@NotNull final NdkOptions options) {

_{Did we get this right? 👍 / 👎 to inform future reviews.}

[sentry]

The error handling returns -1 for general initialization failures but returns the actual sentry_init() result on success. This creates inconsistent error code semantics. Consider either: (1) documenting the different error code meanings (what specific non-zero values from sentry_init() mean vs -1 for setup failures), or (2) normalizing the return value to always return -1 for any failure and only 0 for success. This will make error handling more predictable for Java callers.
_{Severity: MEDIUM}

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: ndk/lib/src/main/jni/sentry.c#L358-L369

Potential issue: The error handling returns -1 for general initialization failures but
returns the actual sentry_init() result on success. This creates inconsistent error code
semantics. Consider either: (1) documenting the different error code meanings (what
specific non-zero values from sentry_init() mean vs -1 for setup failures), or (2)
normalizing the return value to always return -1 for any failure and only 0 for success.
This will make error handling more predictable for Java callers.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

[markushi]

@hannojg thanks for opening this up! This is definitely an area of improvement and I think using a return code is the way to go right now. It would also allow some flexibility, e.g. returning some positive integer > 1 when init succeeds but feature XY from options could not be enabled.
In the long run we should improve this even further, as we're e.g. not checking for Java Exceptions within the C code (ExceptionOccurred() as outlined in https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#java_exceptions)

[markushi]

Thanks again you for contributing, much appreciated! I left a comment on how far the return code should be propagated up. If something is unclear, just let me know.

[markushi]

I think we should keep using void here and throw an exception within init if the error code is non-0. This keeps API compatibility and ties the error code handling closer to the actual C code.

We also don't need any extra handling within sentry-java then, as it's covered by this try-catch here.

Suggested change

	public static int init(@NotNull final NdkOptions options) {
	loadNativeLibraries();
	initSentryNative(options);
	return initSentryNative(options);
	}
	public static void init(@NotNull final NdkOptions options) {
	loadNativeLibraries();
	final int returnCode = initSentryNative(options);
	if (returnCode != 0) {
	throw new IllegalStateException("Failed to initialize sentry-native, return code: " + returnCode);
	}
	}

[supervacuus]

I would even go one step further here and check whether the return code was 1 (in which case there is some native-internal issue which could be resolved via logs and there is no need to render a return code) and having an enum range in the negative return code range, that results in an explanatory expection message for each particular return code.

Even if we don't go that far, we should at least differentiate between > 0 and < 0 returnCode exceptions, to clarify where the error happened (in the JNI layer or native proper). Since the primary failure mode in the JNI layer is about allocation, most exceptions from there would not be actionable to client code. However, having a basic differentiation between the source of the exception, would still be useful.

[supervacuus]

Just my 2c here:

• I am fine if we don't throw from the JNI layer, however
• if we create a new protocol, then we should make it clear what things mean, especially the negative range currently does not provide any insight into the cause (we could use an enum to represent the failure states from the JNI layer)
• there is not a lot of overlap between the checks done in the JNI layer and the one in sentry_init, since all the checks here are classic client side checks, since for many option interfaces a NULL can be a valid value, meaning only the client knows whether that is an error (for instance, setting a transport to NULL is a valid configuration from the POV of the native SDK)