[mutianf]

Summary

• _get_status packs unpacked protobuf error details: google.api_core.exceptions.from_grpc_error unpacks google.protobuf.any_pb2.Any messages from grpc-status-details-bin into concrete google.rpc.error_details_pb2 objects (ErrorInfo, RetryInfo, DebugInfo, etc.) in exc.details. Previously _get_status passed details=exc.details straight to status_pb2.Status(...), raising TypeError: Parameter to initialize message field must be dict or instance of same class: expected <class 'google.protobuf.any_pb2.Any'> got ... whenever a MutateRows RPC failed with rich gRPC error details. _get_status now normalizes each item: any_pb2.Any is kept as-is, a raw google.protobuf.message.Message is packed into an Any, and a proto-plus wrapper is packed via its ._pb. Anything it cannot pack is skipped rather than failing the whole conversion, and exc.message defaults to "" since protobuf string fields reject None.
• _get_status renders exceptions safely in the UNKNOWN fallback: GoogleAPICallError.__str__ reads exc.details, which itself raises when the exception was constructed with a malformed details value — so the fallback path could raise while building the error it was meant to report. The new _exc_message helper falls back to the raw exc.message, then to the type name.
• _get_error_type validates status codes: core_exceptions.from_grpc_status(call_code, "") maps 0 (grpc.StatusCode.OK) and unmapped integers to the base class GoogleAPICallError, which made retries.if_exception_type match — and therefore retry — every GoogleAPICallError subclass. _get_error_type now raises ValueError for booleans and for any code that falls back to base GoogleAPICallError (including 0 / grpc.StatusCode.OK).
• MutationsBatcherAsync._wait_for_batch_results catches unexpected task exceptions: await task moved inside the try: block so an unexpected task exception is wrapped in FailedMutationEntryError rather than aborting _flush_internal before _add_exceptions — which previously caused batcher.close() to silently drop failed mutations. This is async-only; the generated sync code is unchanged, since CrossSync resolves the branch to the pre-existing task.result() call.
• Legacy MutationsBatcher unwraps RetryExceptionGroup: when surfacing FailedMutationEntryError.__cause__, a RetryExceptionGroup cause is replaced by its terminal exception so users see the actual final error instead of the retry-chain wrapper.

Test plan

• Added unit tests in tests/unit/data/test__helpers.py for _get_status packing unpacked protobuf details, _get_status surviving malformed details, and _get_error_type rejecting invalid status codes (0, grpc.StatusCode.OK, booleans, and unmapped codes).
• Added unit tests in tests/unit/data/_async/test_mutations_batcher.py (and regenerated tests/unit/data/_sync_autogen/test_mutations_batcher.py) for the batch-completion callback being invoked with UNKNOWN statuses on an unexpected error, and for _wait_for_batch_results wrapping unexpected task exceptions.
• tests/unit passes (5509 passed, 8 skipped), including test_sync_up_to_date.py.
• The legacy batcher.py RetryExceptionGroup unwrap is not yet covered by a test in tests/unit/v2_client/test_batcher.py.

[gemini-code-assist]

Code Review

This pull request optimizes the mutations batcher by deferring status list creation until a user callback is present, and wraps the async task awaiting in a try-catch block to handle unexpected exceptions. It also updates error helper functions to correctly pack unpacked error details into protobuf Any messages and validate gRPC error status codes, accompanied by comprehensive unit tests. The review feedback suggests a valuable simplification in _wait_for_batch_results to directly assign the awaited task result in async mode instead of calling task.result() redundantly.

[mutianf]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several robustness improvements to the Bigtable mutations batcher and helper utilities. Key changes include extracting the terminal exception from a RetryExceptionGroup in the batcher, refactoring user batch completion callback execution to properly handle synchronous and asynchronous contexts, moving task awaiting inside try-except blocks to catch unexpected task exceptions, packing unpacked error details into status_pb2.Status objects, and validating gRPC error status codes in _get_error_type. Corresponding unit tests have been added to verify these behaviors. Feedback on the changes suggests defaulting exc.message to an empty string when constructing status_pb2.Status to prevent potential TypeError exceptions if the message is None.

[daniel-sanche]

Previously, this exception was only being logged as a warning. Now it actually shows up as a failed request. And IIRC, this method intends to return errors as a list, so this exception would remove that signal.

Is that intentional?

[mutianf]

Good point, reverted this change. Maybe we should do this check and construction time?

[daniel-sanche]

I think this is passed after construction. Currently, it's only used as an internal attribute though I believe

[daniel-sanche]

LGTM

[daniel-sanche]

nit: a comment here could be useful.

On a skim, this may look like a standard isinstance check, but this is specifically checking for the base class, because of a problem we ran into

[daniel-sanche]

I think this is passed after construction. Currently, it's only used as an internal attribute though I believe

[mutianf]

Rebased onto upstream/main (now d54a6e0) to clear the failing GitHub Actions Scan check. The zizmor known-vulnerable-actions audit was flagging dorny/paths-filter@v4.0.1 in the workflows at this PR's old base; main has since bumped it to v4.0.3.

No code changes — the diff against the base is identical apart from hunk offsets. New head: ece90f38263.