fix(artifacts): do not derive a save's version number from a failed listing

[svetanis]

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: #1418

2. Or, if no issue exists, describe the change:

Problem:

GcsArtifactService.listVersions returns an empty list when the listing raises StorageException.
saveArtifactAndReturnBlob derives the next version number from that result
(versions.isEmpty() ? 0 : max(versions) + 1) and writes with no precondition. At the point where
the version number is chosen, an empty list can mean either that the artifact has no versions or
that the listing did not complete — so a 503, a 429, a timeout or IAM propagation makes the save
compute version 0 and write over the object already stored at version 0, while returning success and
version 0 to the caller.

listArtifactKeys issues the same storageClient.list(bucketName, BlobListOption.prefix(...)) call
and surfaces a failure as VerifyException. listVersions is the one whose result selects the blob
name a save writes to.

Solution:

Separate the listing from the decision about what a failed listing means. The query moves into a
private readVersions that lets StorageException propagate, and each caller applies its own
policy: listVersions catches it and returns an empty list as before, the save path catches it and
throws. Four pieces:

#	Piece	What it is
1	readVersions — new private method	the old body of listVersions, moved verbatim; the stream pipeline is unchanged
2	listVersions — public, behavior unchanged	call readVersions, catch StorageException, return ImmutableList.of()
3	versionsBeforeSaving — new private method	call readVersions, catch StorageException, throw VerifyException("Failed to list artifact versions from GCS", e) — the same form listArtifactKeys already uses for this operation, naming what failed
4	saveArtifactAndReturnBlob — one line	was listVersions(...), now Single.fromCallable(() -> versionsBeforeSaving(...)); everything after it is untouched

private ImmutableList<Integer> versionsBeforeSaving(
    String appName, String userId, String sessionId, String filename) {
  try {
    return readVersions(appName, userId, sessionId, filename);
  } catch (StorageException e) {
    throw new VerifyException("Failed to list artifact versions from GCS", e);
  }
}

listVersions has three callers besides the save path — loadArtifact, deleteArtifact, and
ArtifactController in the dev module — so removing the catch outright would change
all of them as well. Leaving it in place keeps the diff to the one caller that overwrites stored
data: those three behave exactly as before, and listVersions_storageException_returnsEmptyList
passes untouched. That is a scoping choice, not a claim that the empty list is right for them; the
issue lists them as related and uncovered. If you would rather the catch went away and those callers
moved with it, say so and I will make that change instead.

File	Change
core/src/main/java/…/artifacts/GcsArtifactService.java	the four pieces above, +81/-18
core/src/test/java/…/artifacts/GcsArtifactServiceTest.java	2 tests

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

GcsArtifactServiceTest: Tests run: 26, Failures: 0, Errors: 0, Skipped: 0. Both new tests were
confirmed failing on unmodified main by reverting GcsArtifactService.java alone and re-running —
Tests run: 26, Failures: 2, exactly these two:

Test	Asserts	On main
save_listStorageException_propagates	the save raises VerifyException, with hasCauseThat().isInstanceOf(StorageException.class) so the underlying failure stays visible	❌ fails
save_listStorageException_doesNotWrite	verify(mockStorage, never()).create(...)	❌ fails

Both also verify(mockStorage).list(...), so neither can pass by failing earlier for an unrelated
reason.

Manual End-to-End (E2E) Tests:

• Run against a real GCS bucket, two arms. Both save PAYLOAD-A through a fully-permissioned
  client, then save PAYLOAD-B to the same filename — arm 1 through an identity denied
  storage.objects.list, arm 2 through a client that can list. The bucket is inspected afterwards
  with a privileged credential, since the restricted identity cannot list.

	before	after
save 1, healthy client	v0 = PAYLOAD-A	v0 = PAYLOAD-A
save 2, client under test	reports v0, success	raises; the cause names storage.objects.list
bucket after	1 object = PAYLOAD-B	1 object = PAYLOAD-A
loadArtifact(0)	PAYLOAD-B	PAYLOAD-A

The control arm is unchanged by the fix in both runs — versions [0, 1], both objects kept — so the
two arms differ only in which client performs the second save.

Checklist

• I have read the CONTRIBUTING.md document.
• My pull request contains a single commit.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

[hemasekhar-p]

Hi @svetanis, thank you for your contribution! We appreciate you taking the time to submit this pull request. I noticed that the branch is currently out of sync with the base branch. could you please resolve the merge conflicts so we can proceed with the review?