fix: retry GCSClient.isdir() on transient 5xx errors

[mski-iksm]

Description

Attach @gcs_retry to GCSClient.isdir() so that the two raw execute() calls inside it (buckets().get() and objects().list()) inherit the same 5-attempt exponential backoff already used by _obj_exists, _do_put, and download. Both calls are idempotent GET/LIST, so retrying is safe.

Scope is limited to isdir. Side-effectful methods (remove, copy, etc.) are intentionally left alone — retrying those safely needs additional idempotency handling and belongs in a separate PR.

Motivation and Context

A single transient 5xx from objects.list can kill a long-running batch, because the Target.exists() → GCSClient.exists() → GCSClient.isdir() fallback path was calling execute() without any retry:

File "luigi/contrib/gcs.py", line 200, in exists
    return self.isdir(path)
File "luigi/contrib/gcs.py", line 217, in isdir
    resp = self.client.objects().list(bucket=bucket, prefix=obj, maxResults=20).execute()
googleapiclient.errors.HttpError: <HttpError 503 ... "We encountered an internal error. Please try again.">

gcs_retry already exists and is attached to the sibling methods; isdir was just missing the decorator.

Have you tested this? If so, how?

I have included unit tests. Four new mock-based cases were added to RetryTest, covering both the retry paths and the untouched happy paths:

• test_isdir_no_retry_on_success — 200 returns immediately (regression guard)
• test_isdir_returns_false_when_no_items — empty prefix returns False
• test_isdir_retries_on_5xx — 503 → 503 → 200 succeeds on the third attempt
• test_isdir_fails_after_retry_limit — five consecutive 503s re-raise HttpError

All 6 tests in RetryTest pass locally. The @pytest.mark.gcloud integration tests were not re-run (they need live GCS credentials), but isdir's behavior on 2xx responses is unchanged.

[dlstadther]

I've got some minor feedback on the organization and reuse of the tests. Rather than commenting on them, I'll push a couple commits to address my own feedback.

[dlstadther]

Pushed 5 follow-up test-only commits, no production code changes:

• e9d4b0ba test(gcs): mark RetryTest with pytest.mark.gcloud — the new class had no markers, so tox -e py310-gcloud (which filters on -m gcloud) silently deselected all 6 of its tests instead of running them.
• c1b1df3a test(gcs): hoist httplib2 import to module-level try/except — httplib2 is a google-auth dependency, so the per-test import httplib2 was redundant with the file's existing top-level import guard; also names google-auth in the skip message.
• e99a88e8 test(gcs): hoist repeated 404/503 HttpError fixtures to module scope — de-dupes the identical httplib2.Response/HttpError pairs each test was rebuilding.
• a40c94eb test(gcs): align RetryTest arrange steps with their assertions — replaces hardcoded retry counts (5) and item-name strings ("foo/bar") with values derived from gcs_retry's own retry.stop.max_attempt_number and from the test's own path, so they can't silently drift out of sync with what's actually being asserted.
• 205b0384 test(gcs): split isdir retry tests into their own class with shared setUp — splits the generic gcs_retry-decorator tests from the isdir-specific ones into GCSClientIsDirRetryTest, with a setUp for the shared mock client. Left a comment noting these tests conceptually belong on GCSClientTest, but that class does live-GCS integration setup via _GCSBaseTestCase and would need its own refactor to plain unit tests first.

Verified with:

tox run -e py310-gcloud -- test/contrib/gcs_test.py::GCSClientIsDirRetryTest

Result: 4 passed, 5 warnings in 21.92s

[dlstadther]

I was good with these changes. But i made some test adjustments to ensure they run with the gcloud marker, DRY some behavior, and reorganize (with some compromise) where tests live. No behaviors were changed.

I'm approving the PR, but since I made adjustment to your code, please confirm that I've indeed retained your intended test behaviors.

[mski-iksm]

Thanks for the cleanup, @dlstadther — I confirmed the test behaviors are preserved (the gcloud marker catch was a great one).

That said, re-reading the change I noticed a nested-retry issue I want to fix before this lands. isdir internally calls _obj_exists, which is already @gcs_retry-decorated. Putting @gcs_retry on isdir too means both layers retry on a persistent 5xx, giving a multiplicative 5 × 5 = 25-attempt budget with full exponential backoff — not what I intended.

The traceback that motivated this PR wasn't from _obj_exists (already retry-protected) but from the two raw execute() calls inside isdir: buckets().get() and objects().list(). So I've pushed a follow-up that drops @gcs_retry from isdir itself and instead extracts those two raw calls into small @gcs_retry-decorated helpers (_bucket_exists, _list_prefix). Each API call now has its own independent 5-attempt budget (15 max total) with no nesting. The existing tests only needed a one-line change to point at _list_prefix.retry instead of isdir.retry.

Would you mind taking another look?

[dlstadther]

Removed an unnecessary comment block. Otherwise, all good