Fix KafkaError error strings raising/garbling on non-UTF-8 locales (#448)

[Sparsh (iSparshP)]

Fixes #448.

Problem

KafkaError.str() decodes the underlying C error string with PyUnicode_FromString(), which assumes strict UTF-8. When librdkafka or the OS returns an error string in the system locale encoding rather than UTF-8 — e.g. on a non-English Windows install — this raises UnicodeDecodeError or produces garbled output, as reported in #448.

Fix

Add a small helper cfl_PyUnistr_FromStringSafe() that decodes via PyUnicode_DecodeUTF8(s, len, "replace"). Invalid bytes degrade to U+FFFD (�) instead of raising, so error reporting never blows up on a locale-encoded string. Both the custom-message and rd_kafka_err2str() paths in KafkaError_str() now use it.

This is intentionally the minimal, non-raising fix: it guarantees a usable string in all locales rather than attempting full locale-encoding detection.

Test

Added test_kafka_error_non_ascii_str covering a non-ASCII (Cyrillic) error message. Full tests/test_error.py passes locally (7 passed) against a fresh build.

[confluent-cla-assistant]

🎉 All Contributor License Agreements have been signed. Ready to merge.
✅ iSparshP
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

[Pranav Shah (prashah-confluent)]

Hi Sparsh (@iSparshP),

Thanks for digging into this.

Nice diagnosis — the strict-vs-replace distinction is exactly the right root cause.

One gap: KafkaError_reduce() decodes the same self->str bytes through the strict
path, so it still raises. See src/confluent_kafka/src/confluent_kafka.c:139.

__reduce__ is what pickle calls, and pickling happens implicitly whenever a
KafkaError crosses a process boundary or is copy.deepcopy()d — serialization
support was added deliberately in #2153. I verified this empirically against the
2.15.0 wheel by simulating the #448 condition (injecting a 0xE5 byte — CP1252 'å',
invalid UTF-8 — into a live error's self->str via ctypes):

• repr(err) / print(err) → work, render v�rden (already "replace" via
  PyUnicode_FromFormat)
• err.str() → UnicodeDecodeError (the KafkaError error strings are unicode on Windows when language is non-english #448 crash this PR fixes)
• pickle.dumps(err) / copy.deepcopy(err) → SystemError: <built-in method __reduce__ ...> returned a result with an error set, with the
  UnicodeDecodeError as __cause__ (the unchecked NULL sends the function down
  the no-reason branch with an exception still pending, which CPython flags as a
  SystemError)
• a KafkaError returned from a ProcessPoolExecutor worker → the parent's
  future.result() raises that same SystemError (the result is pickled in the
  child on its way back), so the caller never receives the error object at all

Same one-line substitution as lines 99/101:

        if (self->str) {
                reason = cfl_PyUnistr_FromStringSafe(self->str);
        } else {
                reason = NULL;
        }

This makes the decode non-raising, which also moots the unchecked-result issue for
the encoding case. Without it, #448 isn't fully closed — the same bytes still crash,
just from the pickling path instead of the display path.

[Pranav Shah (prashah-confluent)]

Thanks Sparsh (@iSparshP)

Have few comments which could ensure that all the places are covered and the coding style guidelines are honoured.

[Sparsh (iSparshP)]

Thanks for the thorough review Pranav Shah (@prashah-confluent), all three addressed in the amended commit:

1. __reduce__ gap : good catch, that was the real hole. KafkaError_reduce() now decodes self->str through cfl_PyUnistr_FromStringSafe() too, so pickling / copy.deepcopy() (and anything crossing a process boundary, e.g. a ProcessPoolExecutor result) no longer raises on the same bytes. Verified locally: pickle.loads(pickle.dumps(err)) and copy.deepcopy(err) both round-trip a non-ASCII message correctly, and routing the reduce path through the non-raising helper resolves the SystemError from the unchecked-NULL branch for the encoding case.

2. clang-format nit: removed the space in cfl_PyUnistr_FromStringSafe(const char *s) to match SpaceBeforeParens: ControlStatements and the neighboring cfl_PyUnistr_AsUTF8 definitions.

3. Test scope: agreed the public constructor cannot inject invalid bytes, so this only exercises the valid-UTF-8 non-ASCII path. Reworded the commit from "regression test" to "non-ASCII round-trip test" and added a comment on the test spelling out that the invalid-byte case (reachable only from C or a genuinely non-UTF-8 locale) is not covered by CI. The invalid-byte crash itself was confirmed via your ctypes 0xE5 injection against the 2.15.0 wheel; both str() and now the reduce/pickle path route those bytes through the "replace" decode instead of raising.

tests/test_error.py passes (7 passed) against a fresh build. Also signing the CLA.

[Sparsh (iSparshP)]

Heads up on the CLA: the signing link from @confluent-cla-assistant (https://ironcladapp.com/public-launch/66bcfa630dec9c9aeb2fc0da) just renders an "Error Occurred" box and never loads the agreement.

I ruled out my setup, reproduced it in a clean Incognito window with all extensions disabled, and on a second browser, so it looks like an Ironclad-side / workflow issue rather than anything local. Re-running the check via an empty commit didn't help since the link is a fixed workflow ID.

Could a maintainer regenerate the CLA workflow or point me to an alternate way to sign? Happy to push an empty commit to re-run the check as soon as it's working.

[Pranav Shah (prashah-confluent)]

/sem-approve

[Pranav Shah (prashah-confluent)]

Thanks Sparsh (@iSparshP) for the fixes, we will be taking care of the cla-status-check issue and will merge the PR after one final review.

[Pranav Shah (prashah-confluent)]

Sparsh (@iSparshP) , the team has fixed the cla link. Can you give it a try? If required do an empty commit as well?

[Kaushik Raina (k-raina)]

Thanks for PR! On comment

[Kaushik Raina (k-raina)]

Can we add test case for pickle/reduce()?