Remove the duplicate GCS signing cache from orch.py

[christophervoelpel]

Summary

orch.py and util/gcs_wrapper.py previously contained two copies of the same GCS IAM signing-credential caching logic. PR #195 fixed only the gcs_wrapper.py copy, leaving the hot path in orch.py (upload URLs, single GETs, and batch signing) running the stale copy without thread-safety locks and with dead refresh-on-expiry logic.

This change:

1. Deletes the duplicate _storage_client, _signing_credentials, and _get_signing_credentials from orch.py, delegating to the single cached signing context in util/gcs_wrapper.py.
2. Renames the shared accessor to gcs_wrapper.get_signing_context() and updates its callers; the old private accessor is removed.
3. Fixes the local-ADC defect in util/gcs_wrapper.py so that non-service-account ADC credentials (from gcloud auth application-default login) raise an immediate, actionable RuntimeError at the point of detection explaining that URL signing requires a service account identity, and providing the exact impersonation or service account key instructions needed for local setup.
4. Retains the explicit _CACHED_SIGNING_CREDENTIALS is None check in the caching guard.
5. Reads the service-account email after refreshing credentials, so Cloud Run signing uses the resolved identity rather than the initial default placeholder.
6. Adds concurrency lock-path tests and actionable local-ADC error tests in test/test_gcs_wrapper.py, and verifies orch delegation with no second cache in test/test_frontdoor_data.py.

Deployment note

Deploy a new image after merging to activate the shared signing-context and refreshed-identity fixes. This PR update does not deploy them.

TAG=agy
CONV=ec862d59-a9a7-4a7c-9eae-b59095e8cd08

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [mariamnyoussef]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [sadcat]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [nfrontini]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [alzmcr]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [chris-feldman]

[christophervoelpel]

PR #198 — P1 — signing identity captured before refresh

Reviewed head: 6ec31b0bb9ee9a93033cde2a36f867b1719900fa.

Please read service_account_email after refreshing the credentials in util/gcs_wrapper.py:66–88.

With the real pinned google.auth.compute_engine.Credentials() class, the email starts as default. Refresh resolves the runtime service-account email, but this PR has already copied default into sa_email, which is then passed to both iam.Signer and IDTokenCredentials. The exact base uses the resolved email; the head uses default. This breaks the normal Cloud Run signing path for mediated media URLs.

Reproduced independently against exact base/head implementations, using real Google credentials and mocked metadata transport—no live cloud call. Existing tests install the final email on the mock before refresh, so they miss this lifecycle.

Smallest fix: refresh, then read/validate the email; keep the current cache/lock. Add a regression with a real credential instance whose email changes during refresh, checking that the signer and signing credential use the resolved identity. Then rerun GCS/front-door tests and CI. No auth/cache redesign needed.

[christophervoelpel]

Review: merge after small fixes

Multi-agent review (reviewer → independent critique agent re-verifying each claim against the code). One first-pass finding was overturned by the critique agent and is recorded below so it doesn't resurface.

The orch.py side is exactly right. −41/+10, every call site converted, _SIGNED_GET_TTL / _SIGNED_PUT_TTL, key shape and per-call expiration all preserved. It also silently fixes a dead check: orch's old copy never set expiry, so its expired test never fired and the credentials were never refreshed. Verified no stale references remain — _get_storage_client, _get_signing_credentials and _get_cached_signing_context have zero occurrences branch-wide, and stacked PR #203 touches neither symbol (it only shares orch.py / test_frontdoor_data.py textually; its hunks start ~line 1175 while these end ~1061, so no conflict).

Worth fixing before merge

1. Two naive datetime.now() expiries — test/test_gcs_wrapper.py:157 (and the pre-existing line 117 it copies):

   mock_cred.expiry = datetime.datetime.now() + datetime.timedelta(hours=1)

   Credentials.expired compares against google.auth._helpers.utcnow(), which is naive UTC. On any machine west of UTC these creds read as already expired, so refresh.call_count becomes 10 instead of 1 and the test fails. Fine on a UTC CI box, a latent trap for contributors. Use datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) (or utcnow()) on both lines.

2. Delete the three hasattr asserts at test/test_frontdoor_data.py:2080-2082:

   assert not hasattr(orch, '_storage_client')

   That pins the absence of private names — a pure change-detector. The called_with_creds == [sentinel_creds] assertion in the same test already proves the delegation this PR is about.

3. get_signing_context is newly public but has no return annotation and no Returns: / Raises: sections, unlike get_signed_url right below it. Worth 3 lines: -> tuple[storage.Client, compute_engine.IDTokenCredentials].

4. Fix the ADC error message's doc pointer. util/gcs_wrapper.py:67-77 tells the user to "See DEVELOPING.md ('The local loop') for details", but that section (DEVELOPING.md:113) says only "run gcloud auth application-default login once first" — i.e. it instructs the user to do the exact thing that triggers the error. There is no --impersonate-service-account or GOOGLE_APPLICATION_CREDENTIALS guidance there. Either inline the command the error already suggests, or add three lines to that DEVELOPING.md section.

Overturned: there is no local-ADC regression

The first-pass review flagged the new RuntimeError as breaking /api/uploadUrl under local ADC, and proposed adding a get_storage_client() accessor for the two call sites (orch.py:969, :1032) that only want a client. That premise is false.

upload_url_handler signs a GET URL unconditionally at orch.py:1018 — on both the exists and not-exists branches:

'url': _signed_url(blob, 'GET', _SIGNED_GET_TTL)

Pre-PR, _get_signing_credentials() read source_credentials.service_account_email, which user ADC credentials do not have → AttributeError → 500. So /api/uploadUrl never worked under local ADC. The new RuntimeError is a strict improvement: it turns an AttributeError into an answer. No new accessor needed.

Minor note for completeness: get_signing_context() does build signing credentials even when the caller wants only a client, so the first /api/uploadUrl after cold start pays one extra signBlob-path setup. It's cached module-wide, so ~0 after warmup — not worth a second accessor.

Explicitly not recommended

• Don't add get_storage_client() — see above.
• Don't split the ADC RuntimeError into a follow-up PR. It is scope creep relative to "remove the duplicate cache", but it's 11 lines that convert a confusing AttributeError into an actionable message. Fix the doc pointer and keep it.
• Don't delete test_signing_context_built_once_across_concurrent_callers (~38 lines). It's a weak test — with the GIL it would very likely pass without the lock — but it is the only executable record of why _SIGNING_CONTEXT_LOCK exists. Fix its expiry bug (finding 1) and move on.
• Don't reflow lines for the 80-col rule. Several new lines exceed pyproject.toml's line-length = 80, but util/gcs_wrapper.py:56 is already 88 chars in the same function and nothing in CI enforces it. Don't make this PR the first to pay.
• Consider trimming the four substring assertions at test_gcs_wrapper.py:212-215 that pin exact error copy including the string 'DEVELOPING.md' — pytest.raises(RuntimeError) plus one substring is enough.

PR description

The body says _get_cached_signing_context is "aliased for backwards compatibility" — no such alias exists in the diff. The code is right (no caller needs it, verified across the tree); it's the description that needs the edit.

[christophervoelpel]

Consolidated Review

Verdict: Fix one blocker, then merge. This reconciles the 19:29 and 19:50 comments against head 6ec31b0; every item was re-verified by tracing callers in util/gcs_wrapper.py and orch.py and running the tests and probes under Evidence. The 19:50 comment's 'merge after small fixes' verdict did not consider the item below, visible in a side-by-side read of the diff.

Do before merge

1. util/gcs_wrapper.py:66-79 (get_signing_context): read sa_email after cred.refresh(auth_request), not before. On google.auth.compute_engine.Credentials, the email is the placeholder default until refresh resolves it; the head function captures default and passes it to both iam.Signer and IDTokenCredentials, neither of which re-resolves it. orch.py routes every signed URL through this function with no fallback, breaking uploads, GETs and batch signing on Cloud Run. Acceptance: a regression test using a credential whose service_account_email changes between construction and refresh, asserting the signer and IDTokenCredentials receive the resolved email; today's mocks pass because they pre-install the email before refresh.

Optional, does not block

• test/test_gcs_wrapper.py:117 and :157: datetime.datetime.now() for mock expiry is naive local time versus naive UTC, failing off-UTC machines. Use datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None). CI runs under TZ=UTC, so this is test-hygiene only.
• test/test_frontdoor_data.py:2078-2080: three hasattr asserts are change-detectors; called_with_creds already proves the delegation.
• util/gcs_wrapper.py:51-52: add a return annotation and Returns:/Raises: section to get_signing_context, matching get_signed_url.
• The RuntimeError message points to DEVELOPING.md's "local loop" section, which lacks impersonation guidance; it already inlines the real command, a minor doc-pointer fix.
• The PR body's "aliased for backwards compatibility" claim for _get_cached_signing_context does not match the diff; fix the description.
• Trim the four exact-string assertions at test_gcs_wrapper.py:212-215 to one substring check plus pytest.raises(RuntimeError).

Rejected or superseded, do not re-litigate

• The local-ADC regression claim (raised as a first-pass finding and overturned inside the 19:50 comment itself) stays overturned: upload_url_handler signs a GET URL unconditionally, and before this PR that path already failed under user ADC credentials with an unhandled AttributeError, a bare 500. The RuntimeError is a strict improvement and no get_storage_client() accessor is needed.
• Do not delete test_signing_context_built_once_across_concurrent_callers; it is the only record of why _SIGNING_CONTEXT_LOCK exists. Fix its expiry bug instead.
• Do not reflow lines for the 80-column rule; util/gcs_wrapper.py:56 was already over that length pre-PR, unenforced by CI.
• The hunk overlap with stacked PR Treat an absent storyboard key as no change #203 is textual only, disjoint ranges, no functional conflict.

Evidence

• git diff --stat origin/main..HEAD -- orch.py: -41/+10, matches the claimed conversion.
• grep -rn for the three deleted symbol names: zero hits.
• Independent probe with the real compute_engine.Credentials class and a faked metadata transport, run twice: head yields default for the signer and IDTokenCredentials; base yields the resolved email.
• Live check of the identity the head passes to the signer: POST https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/default:signBlob returns HTTP 400 INVALID_ARGUMENT, "Invalid form of account ID default. Should be [Gaia ID |Email |Unique ID |] of the account". The same call with a real service-account email returns 403 for a caller without signBlob, so the API accepts the name form and rejects only default. A signer built with default cannot sign.
• TZ=UTC python -m pytest -q test/test_gcs_wrapper.py test/test_frontdoor_data.py -k 'signing or signed_url or adc': 4 passed.
• TZ=America/Los_Angeles and TZ=Pacific/Kiritimati reruns: failures both directions, confirming the naive-datetime trap.
• Not run: live deployment of this head to Cloud Run, full backend or UI suite reruns.

Way forward

• Suggested order for the batch: Use an explicit MIME allow-list and support AVIF #196, Clear video source on resolve failure and keep cached blobs #205, Validate video duration limits and audio parameter types #204, Exclude archived candidates from the rendered video #199 and Clamp clip duration to the footage remaining after skip_time #200 first (independent, ready or one small fix each), then Preserve deployed env values on redeploy and accept quoted config #197 and Remove the duplicate GCS signing cache from orch.py #198 once their fixes land, then the stacked children Validate model-returned product and image ids #201 (on Use an explicit MIME allow-list and support AVIF #196), Treat an absent storyboard key as no change #203 (on Remove the duplicate GCS signing cache from orch.py #198) and Guard setup inputs on the real project load error and hydration state #202 (on Exclude archived candidates from the rendered video #199), each rebased onto main after its parent squash-merges and retargeted to main so CI runs.
• Fold the small fixes into this PR rather than a follow-up; they touch the same function and tests.
• Confirm the new regression test fails on current code before the fix lands.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [alzmcr,
  chris-feldman]

[christophervoelpel]

Follow-up on Consolidated Review

I addressed the required identity-refresh fix and the timezone-sensitive test fixtures from the Consolidated Review on head 01861539a357.

• get_signing_context() now reads the service-account email after credential refresh, so Cloud Run signing uses the resolved identity rather than the initial default placeholder.
• Updated the expiry fixtures to use UTC-derived timestamps, keeping the concurrency and delegation coverage intact.

Verification: all 10 GCS-wrapper tests passed under UTC, Los Angeles, and Kiritimati; the signing/signed-URL/ADC selection across GCS and frontdoor tests passed with 9 tests. The independent Compute Engine credential probe confirmed both signer consumers receive the refreshed email. No live deployment or IAM signing call was performed.

Current-head GitHub CI is complete: Python 3.11/3.12/3.13, UI build/lint/tests, deploy checks and security scans passed. Conditional zizmor jobs were skipped.