Skip to content

fix: prevent a stale teardown from affecting a newer session generation - #1231

Merged
mykola-mokhnach merged 15 commits into
appium:masterfrom
droidrun:timo/fix-stale-teardown-generation
Aug 30, 2026
Merged

fix: prevent a stale teardown from affecting a newer session generation#1231
mykola-mokhnach merged 15 commits into
appium:masterfrom
droidrun:timo/fix-stale-teardown-generation

Conversation

@Timo972

@Timo972 Timo972 commented Aug 26, 2026

Copy link
Copy Markdown

+killActiveSessionAndWaitForTeardown waits at most FB_KILL_WAIT_TIMEOUT_SEC (35s) for an in-progress teardown, then proceeds regardless. A worst-case teardown is already close to that budget:

  • up to 20s in stopScreenRecordingWithUUID: (STOP_SCREEN_RECORDING_TIMEOUT_SEC)
  • up to 5s in -fb_isTestedApplicationSameAsSystemAppWithTimeout:
  • up to 5s in -fb_terminateTestedApplicationWithTimeout:

plus the notification post and -disableAlertsMonitor. So ~30s of bounded work against a 35s wait — an overrunning teardown can still be mid-flight when POST /session gives up waiting and starts a replacement session.

The remaining teardown steps then mutate process-wide state on behalf of a session that is already gone:

  • [application terminate] — the replacement session usually runs the same bundle identifier, so "terminate the old app" terminates the new session's app. The existing isAllowedToTerminate guard only covers the terminate step overrunning its own 5s timeout, not the overall wait having expired.
  • the screen-recording cleanup — the promise is read out of FBScreenRecordingContainer.sharedInstance at teardown time, so by then it can already be the replacement session's. Both the stopScreenRecordingWithUUID: and the reset can therefore hit the new session's recording rather than the old one.

This adds a session generation counter, bumped in +killActiveSessionAndWaitForTeardown — i.e. at the moment a caller takes ownership of the device, before it launches anything, since the launch window is itself long enough for a stale teardown to do damage. -kill captures the generation it started with, and each step that mutates shared state re-checks it via +isSessionGenerationCurrent:. The terminate check happens inside the main-queue block rather than before dispatching, since that block is exactly where the unbounded delay occurs; the recording cleanup is skipped as a whole, covering the stop as well as the reset.

Because the wait is bounded, teardowns can genuinely overlap, so -kill's in-progress marker is a count rather than a flag — otherwise the first teardown to finish would wake waiters while another was still running.

_activeSession, the teardown count and the generation are all guarded by a single lock (+teardownCondition), and -kill clears the active session and registers its teardown in one critical section. Without that, a concurrent session creation could observe neither an active session nor a registered teardown, skip its wait, and start launching before the old teardown had registered itself. The lock is only ever held for those short transitions — never across the teardown work, the main-queue dispatch, or the bounded wait.

🤖 Generated with Claude Code

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 26, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Timo972

Timo972 commented Aug 26, 2026

Copy link
Copy Markdown
Author

Follow-up commit: _isTeardownInProgress is now a count rather than a boolean.

Since the wait in +killActiveSessionAndWaitForTeardown is bounded, two teardowns can genuinely overlap — a replacement session can be created while an earlier teardown is still finishing, and then be torn down itself. Sharing one flag meant the first teardown to finish cleared it and woke waiters while the second was still running; the next session creation could then bump the generation and cause that still-running teardown to skip terminating its app and resetting the recording container. So the generation guard in this PR closed the dangerous direction (killing the new session's app) but left the opposite one (the old app never being cleaned up at all).

Waiters now block until the count reaches zero, i.e. until every in-flight teardown has finished.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Timo972

Timo972 commented Aug 26, 2026

Copy link
Copy Markdown
Author

Follow-up commit: the generation is now claimed in +killActiveSessionAndWaitForTeardown instead of in +markSessionActive:.

The guard was being claimed too late to protect the thing it was meant to protect. +handleCreateSession: calls +killActiveSessionAndWaitForTeardown before -prepareApplicationForSessionWithBundleID:, but +markSessionActive: only runs after the application has been launched. So when the bounded wait expired with a teardown still in flight, that teardown still saw its own generation as current for the whole launch window — and could terminate the process that had just been launched, which normally shares the old session's bundle identifier.

Claiming it at the moment a caller takes ownership of the device closes that window. A teardown that the call actually ran to completion is already finished, so invalidating it there is a no-op.

Comment thread WebDriverAgentLib/Routing/FBSession.m Outdated
Comment thread WebDriverAgentLib/Routing/FBSession.m Outdated
Comment thread WebDriverAgentLib/Routing/FBSession.m
… lock

Clearing _activeSession and registering its teardown happened under two
different locks, so a concurrent session creation could observe no active
session and no teardown in progress and skip its wait. All three statics now
share +teardownCondition, and -kill does both in one critical section.
The promise was read from the process-wide container at teardown time, so a
stale -kill could stop a newer session's recording even though the reset was
generation-guarded. Both steps now run only while the generation is current.
Every comment added or touched here is now at most three lines.
Comment thread WebDriverAgentLib/Routing/FBSession.m Outdated
Comment thread WebDriverAgentLib/Routing/FBSession.m Outdated
Timo972 and others added 3 commits August 29, 2026 07:27
The stale-teardown check was only a check: a replacement could bump the
generation between it and the read of the process-wide container, so the
teardown could stop the replacement's recording. Read the promise inside the
critical section that validates the generation instead, and make the reset
conditional on the container still holding that exact promise - the stop can
take up to 20 seconds, long enough for a replacement to store its own.
isSessionGenerationCurrent: released the lock before -terminate ran, so a
replacement could claim the next generation and start launching in between,
leaving the stale teardown to terminate the replacement's process - they share
a bundle ID, so capturing an identity does not help here. Turn the check into a
claim taken under the same lock as the generation bump and held across
-terminate, and make the bump wait (bounded by the termination timeout) for any
claim to be released.
Comment thread WebDriverAgentLib/Routing/FBSession.m Outdated
Comment thread WebDriverAgentLib/Routing/FBSession.m Outdated
Comment thread WebDriverAgentLib/Routing/FBSession.m
Timo972 and others added 5 commits August 29, 2026 09:30
The identity check and the reset were separate steps, so a replacement session
could store its own promise in between and have it cleared by the stale
teardown. Move the comparison into the container, behind its own lock.
The bounded wait fell through and bumped the generation anyway, so a session
could start launching while the previous one's committed -terminate was still
running. Raise FBSessionCreationException instead of handing out a generation.
+activeSession, +killActiveSessionAndWaitForTeardown and +sessionWithIdentifier:
touched the global without the lock, and the last one read it twice, so it could
validate one session's identifier and return its replacement.
@mykola-mokhnach
mykola-mokhnach merged commit 83642a1 into appium:master Aug 30, 2026
53 of 57 checks passed
github-actions Bot pushed a commit that referenced this pull request Aug 30, 2026
## [16.11.4](v16.11.3...v16.11.4) (2026-08-30)

### Bug Fixes

* prevent a stale teardown from affecting a newer session generation ([#1231](#1231)) ([83642a1](83642a1))
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 16.11.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants