[abhishekacharya200]

Fixes #18764

upload_chunks_concurrently takes a timeout argument and documents it as the time to wait for the server response, but it never passed it on. The XML multipart requests it makes (initiate, each part upload, finalize, and cancel when something fails) all ran with the (61, 60) default of the media layer, so a caller could not make a stuck upload fail sooner or give a slow connection more time.

This passes timeout to XMLMPUContainer.initiate, finalize and cancel, and adds it to the _upload_part worker so every XMLMPUPart.upload gets it as well. Thread and process workers behave the same, since the timeout is a plain number or tuple.

One small change for callers who don't pass a timeout: these requests now use the documented default of 60 seconds (_DEFAULT_TIMEOUT, the value every Blob upload and download already sends) instead of (61, 60). Only the connect timeout moves, by one second.

The cleanup cancel gets the same timeout as the other requests. If the cancel itself fails, for example with a very short timeout, the failure is logged as a warning with the upload id and the original upload error is still the one raised. test_upload_chunks_concurrently_keeps_original_error_when_cancel_fails covers that path.

With the script from the issue, which records the timeout each request gets:

main                            this branch
initiate     timeout=(61, 60)   initiate     timeout=5
upload part  timeout=(61, 60)   upload part  timeout=5
upload part  timeout=(61, 60)   upload part  timeout=5
finalize     timeout=(61, 60)   finalize     timeout=5

Added test_upload_chunks_concurrently_passes_timeout and test_upload_chunks_concurrently_passes_timeout_to_cancel, which fail on main and pass with this change. The existing tests that checked these calls now also check that the default timeout is sent, and test__upload_part checks that the worker hands its timeout to the part upload. The full tests/unit suite passes locally (2068 passed), and the noxfile's ruff==0.14.14 import and format checks and flake8 report nothing.

• Issue opened before writing the code (storage: upload_chunks_concurrently ignores its timeout argument #18764)
• Tests and linter pass
• Code coverage does not decrease
• No docs changes needed

[gemini-code-assist]

Code Review

This pull request propagates the timeout parameter to various container and part operations within upload_chunks_concurrently (such as initiate, finalize, cancel, and _upload_part) and adds corresponding unit tests. The review feedback recommends wrapping the container.cancel call inside the exception handler in a nested try...except block to prevent cancellation failures from masking the original exception.

[gemini-code-assist]

If container.cancel raises an exception (for example, due to a timeout or network issue during the cleanup attempt), it will mask the original exception that triggered the except block. To ensure the original error is preserved and propagated to the caller for proper debugging, wrap the container.cancel call in a nested try...except block. Avoid silently ignoring the exception with a broad except Exception: pass block; instead, log the exception using logger.warning to aid in debugging and prevent masking underlying issues.

    except Exception:
        try:
            container.cancel(blob._get_transport(client), timeout=timeout)
        except Exception as e:
            logger.warning("Failed to cancel container: %s", e)
        raise

References

1. Avoid broad except Exception: blocks that silently return None. Instead, log the exception (e.g., using logger.warning) to aid in debugging and prevent masking underlying issues.

[abhishekacharya200]

Good point. This was already the case before this change, but passing the caller's timeout to cancel makes it a little more likely. I kept the PR focused on the timeout, and I'm happy to wrap the cancel call so the original error is always the one raised, or to leave cancel on the default timeout, whichever the maintainers prefer.

[shradhakatyal]

Thanks for checking! Let's keep passing timeout=timeout to container.cancel (so fail-fast or extended caller timeouts are respected consistently across all requests) and wrap container.cancel in a try...except Exception block so a cleanup failure never masks the original upload exception.

Two small notes when adding that:

1. transfer_manager.py doesn't define a logger yet — please use _logger = logging.getLogger(name) (matching blob.py) and include upload_id in the _logger.warning(...) message.

2. Since nox -s cover enforces 100% branch coverage, please also add a unit test where container.cancel raises during cleanup to verify the original exception is preserved and the warning is logged.

[abhishekacharya200]

Thanks @shradhakatyal, done in 1520e8b. cancel keeps the caller's timeout and is wrapped so a failed cleanup never masks the upload error. The warning includes the upload id, and a new test covers a failing cancel. Ready for another look.

[shradhakatyal]

/gcbrun (ccc12b7)