Skip to content

fix(session): fail when multiple persisted sessions lack sessionId - #407

Closed
Mochxd wants to merge 3 commits into
appium:mainfrom
Mochxd:fix/ambiguous-session-rehydration
Closed

fix(session): fail when multiple persisted sessions lack sessionId#407
Mochxd wants to merge 3 commits into
appium:mainfrom
Mochxd:fix/ambiguous-session-rehydration

Conversation

@Mochxd

@Mochxd Mochxd commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • resolveDriver() no longer picks the first persisted session when several exist and no sessionId is passed
  • returns Multiple persisted sessions: id1, id2. Pass sessionId. (sorted ids)
  • single persisted session still auto-rehydrates; explicit sessionId unchanged

@KazuCocoa

KazuCocoa commented Jun 15, 2026

Copy link
Copy Markdown
Member

Could you give me more concrete example with appium-mcp logs?

https://github.com/appium/appium-mcp/blob/a44107e144d6c8221515689913d52e6f492b63a6/src/tools/tool-response.ts#L130C1-L175C1

I assume your reported case is persisted below. So, it might have multiple stored sessions.

  const candidates = sessionId
    ? persisted.filter((p) => p.sessionId === sessionId)
    : persisted;

Then, if the first candidate is valid, it will be returned. While not, the next one will be tried out (by end of the candidates).
If the session id was invalid, await (client as Client).getTimeouts(); raises an exception and it goes to the next. If await (client as Client).getTimeouts(); didn't raise an exception, then we should fix the line to raise exception or handle error case properly instead of this PR change.

  for (const entry of candidates) {
    try {
      const client = await attachToRemoteSession({
        remoteServerUrl: entry.remoteServerUrl,
        sessionId: entry.sessionId,
        capabilities: entry.capabilities,
      });
      // attachToSession does not verify liveness on the remote server. Issue
      // a cheap call to confirm the session is still valid before adopting it.
      try {
        await (client as Client).getTimeouts();
      } catch (verifyErr) {
        log.warn(
          `Persisted session ${entry.sessionId} failed liveness check (${
            (verifyErr as Error).message
          }); pruning.`
        );
        await removePersistedSession(entry.sessionId);
        continue;
      }
      const seedCaps: SessionCapabilities = { ...(entry.capabilities ?? {}) };
      if (entry.platform) {
        seedCaps.platformName = entry.platform;
      }
      if (entry.automationName) {
        seedCaps['appium:automationName'] = entry.automationName;
      }
      if (entry.deviceName) {
        seedCaps['appium:deviceName'] = entry.deviceName;
      }
      await setSession(
        client,
        entry.sessionId,
        seedCaps,
        entry.ownership,
        entry.remoteServerUrl
      );
      log.info(
        `Rehydrated attached session ${entry.sessionId} from persisted store.`
      );
      return { sessionId: entry.sessionId };
    } catch (err) {

@Mochxd

Mochxd commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

@KazuCocoa the case I'm targeting is when multiple persisted sessions are still alive, not the prune-and-continue path.

Example:

  1. action=attach to remote session android-abc (Android)
  2. Later action=attach to ios-def (iOS) so we have both persisted on disk
  3. MCP process restarts (stdio host recycle)
  4. Next tool call with no sessionId --> old code walks candidates in readdir order; first one that passes getTimeouts() wins
    If both are live, we silently rehydrate whichever comes first --> wrong device/platform for the user's flow.

Logs (old behavior):
[INFO] Rehydrated attached session android-abc from persisted store --> when the agent expected the iOS session
this PR fails fast instead:
multiple persisted sessions: android-abc, ios-def. Pass sessionId, I agree the loop already handles dead sessions via getTimeouts() + prune, this change is only for the ambiguous case where more than one candidate is valid and the caller didn't specify which one

@KazuCocoa

Copy link
Copy Markdown
Member

I see. Actually that condition occurs.

One missing case is that even if the persistent dir had multiple files, only one session is "alive". Then, this logic might not work expectedly.

This lets me think the rehydrateAttachedSession should require a session id (or active session id) instead of handling this kind of error case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants