fix(artifacts): honor the "user:" namespace in InMemoryArtifactService

[shoemoney]

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):

None found.

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

Problem:

ADK has a "user:" filename-prefix convention: a filename starting with user: is scoped to the user across all their sessions, not just the session it was saved from.

GcsArtifactService implements this. fileHasUserNamespace(filename) (GcsArtifactService.java:65) checks the prefix, and getBlobPrefix (GcsArtifactService.java:78-84) branches on it to store at appName/userId/user/filename/version, deliberately leaving the sessionId segment out. This is exercised directly in GcsArtifactServiceTest (save_userNamespace_savesCorrectly, load_userNamespace_loadsCorrectly).

InMemoryArtifactService had no equivalent branch. Its getArtifactsMap(appName, userId, sessionId) always keyed by appName -> userId -> sessionId -> filename, and every public method (saveArtifact, loadArtifact, listArtifactKeys, deleteArtifact, listVersions) went through it. So if code called CallbackContext.saveArtifact("user:config.json", part) in one session, then loadArtifact("user:config.json") in a different session for the same app/user, it got Maybe.empty(). No exception, just silently missing data.

This matters because InMemoryArtifactService is the default backend used in local dev, tests, and the quickstarts. An app can be written and tested entirely against InMemoryArtifactService, rely on the documented cross-session behavior of user:-prefixed filenames, and only discover it never worked once it's pointed at GcsArtifactService in prod, or never discover it at all if it stays on InMemoryArtifactService.

Solution:

Mirror the GCS branch. getArtifactsMap now also takes the filename, using the same fileHasUserNamespace check and naming as GcsArtifactService, and routes user-namespaced filenames to a shared per-user bucket (USER_NAMESPACE_SESSION_KEY) instead of the caller's sessionId. saveArtifact, loadArtifact, deleteArtifact, and listVersions all pass the filename through unchanged. listArtifactKeys doesn't have a filename to branch on, so it now merges the session-scoped bucket with the shared user-namespace bucket, which is the same thing GcsArtifactService#listArtifactKeys does (it merges a session-prefix listing with a user-prefix listing).

No behavior changes for non-user:-prefixed filenames. GcsArtifactService is untouched.

Testing Plan

Unit Tests:

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

Added to InMemoryArtifactServiceTest, mirroring the shape of the GcsArtifactServiceTest user-namespace cases:

• save_userNamespace_visibleAcrossSessions - save from session A, load from session B, same value comes back.
• load_nonNamespacedFilename_notVisibleAcrossSessions - regression guard. A non-prefixed filename saved in session A must still be invisible from session B. This is what proves the fix scopes only the user: case instead of making every artifact global.
• listArtifactKeys_userNamespace_visibleAcrossSessions
• listVersions_userNamespace_visibleAcrossSessions
• deleteArtifact_userNamespace_removesAcrossSessions

I ran InMemoryArtifactServiceTest against the unpatched source first to confirm these actually catch the bug:

Tests run: 8, Failures: 4, Errors: 0, Skipped: 0
InMemoryArtifactServiceTest.save_userNamespace_visibleAcrossSessions - FAILED
InMemoryArtifactServiceTest.listArtifactKeys_userNamespace_visibleAcrossSessions - FAILED
InMemoryArtifactServiceTest.listVersions_userNamespace_visibleAcrossSessions - FAILED
InMemoryArtifactServiceTest.deleteArtifact_userNamespace_removesAcrossSessions - FAILED

load_nonNamespacedFilename_notVisibleAcrossSessions and the three pre-existing tests passed in both states, as expected.

With the fix applied:

mvn -pl core -am test -Dtest=InMemoryArtifactServiceTest
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0

Also ran GcsArtifactServiceTest to confirm no regression there, since it's untouched:

mvn -pl core -am test -Dtest=GcsArtifactServiceTest
Tests run: 24, Failures: 0, Errors: 0, Skipped: 0

Manual End-to-End (E2E) Tests:

Not applicable, this is contained to InMemoryArtifactService's in-process map and is fully covered by the unit tests above.

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.

Additional context

I checked the other two open PRs that touch artifacts, #1378 and #1412. Both are about Runner/plugin-level input-blob offload and don't touch InMemoryArtifactService or the user-namespace behavior, so there's no overlap with this change.

[hemasekhar-p]

Hi @shoemoney, thank you for taking the time to submit your PR. It is currently under review by our team and we will keep you updated if any additional information is required. Thank you.

[sherryfox]

Python ends list_artifact_keys with return sorted(filenames). ImmutableList.copyOf over a HashSet gives arbitrary order instead. Additionally, Java's own GcsArtifactService already sorts (:186), so the two backends currently disagree.

How about ListArtifactsResponse.builder().filenames(ImmutableList.sortedCopyOf(filenames)).build()); ?

[sherryfox]

This test passes when the list would be full of ghosts so it might be worth adding one more, proving a failed load leaves nothing behind

@Test
public void listArtifactKeys_failedUserNamespaceLoad_doesNotCreatePhantomKey() {

Optional<Part> missing = 
  asOptional(service.loadArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME));

assertThat(missing).isEmpty();

ListArtifactsResponse response = 
  service.listArtifactKeys(APP_NAME, USER_ID, SESSION_B).blockingGet();

assertThat(response.filenames()).isEmpty();

}

and possibly similarly for listVersions?

[sherryfox]

In a read path computeIfAbsent writes an empty entry on a miss, and listArtifactKeys reports keySet(). So a failed load invents a file. For reference, Python's read is pure (self.artifacts.get(path)).

This is a pre-existing issue — except this PR changes its reach. On main the ghost stays in the caller's session; now user: files share one bucket, so every session of that user sees it:

loadArtifact(..., sessionA, "user:missing.json") -> empty
listArtifactKeys(..., sessionB) -> ["user:missing.json"] (main: [])

What do you think of going back to a pure read, ImmutableList.of() is a shared singleton, so it allocates nothing

List<Part> versions = 
  getArtifactsMap(appName, userId, sessionId, filename)
    .getOrDefault(filename, ImmutableList.of());

[sherryfox]

I checked and this one leaks across sessions as well. Python's list_versions is also a pure read. Same fix could apply:

int size =
  getArtifactsMap(appName, userId, sessionId, filename)
    .getOrDefault(filename, ImmutableList.of())
    .size();