Skip to content

Conversation

@gregory-at-cvs
Copy link

@gregory-at-cvs gregory-at-cvs commented Nov 22, 2025

PR #1: Session Manager Concurrency Fix

Description

The changes fix a thread-safety issue in SessionManager where concurrent calls to getSessionId()
could create duplicate sessions during session timeout/expiration.

Problem

The existing code had a documented TODO acknowledging this race condition:

// TODO FIXME: This is not threadsafe -- if two threads call getSessionId()
// at the same time while timed out, two new sessions are created

Solution:

  1. Changed session from mutable variable to AtomicReference
  2. Implemented compare-and-set for atomic session updates
  3. Only one thread successfully creates new session; others use the newly created one
  4. Extracted observer notification into separate method for clarity
  5. Added concurrency tests

Related PRs

This is part 1 of 3 in a session management enhancement:

This PR: Thread-safety fix (foundation)
PR 2: Session infrastructure (depends on this)
PR 3: Session telemetry integration (depends on PR 2)
Review order: This PR should be reviewed and merged first.

Type of Change

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature
[ ] Breaking change
[x] Documentation update

Checklist

[x] Code follows project style guidelines (spotless applied)
[x] Self-review completed
[x] Added tests that prove the fix is effective
[x] New and existing tests pass locally
[x] Documentation updated where applicable

Additional Context

This uses the atomic compare-and-set pattern commonly used for thread-safe operations in Java/Kotlin.

Part of a larger effort: We're contributing session management enhancements to bring Android SDK to parity with iOS (spans/logs) and beyond (metrics). This PR is the foundation - fixing a known thread-safety issue before building on it.

Related information: https://opentelemetry.io/docs/specs/semconv/general/session/

Related PRs

This is part 1 of 3 in a comprehensive session management enhancement:

Review order: This PR should be reviewed and merged first, as PRs 2 and 3

@gregory-at-cvs gregory-at-cvs requested a review from a team as a code owner November 22, 2025 03:15
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 22, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: gregory-at-cvs / name: Gregory Rasmussen (88fe715, da961df)

@gregory-at-cvs gregory-at-cvs force-pushed the fix/session-manager-concurrency branch from 04d8069 to 061ec54 Compare November 22, 2025 03:29
@codecov
Copy link

codecov bot commented Nov 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.59%. Comparing base (7e5a63e) to head (da961df).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
- Coverage   63.65%   63.59%   -0.07%     
==========================================
  Files         159      159              
  Lines        3134     3137       +3     
  Branches      326      326              
==========================================
  Hits         1995     1995              
- Misses       1042     1044       +2     
- Partials       97       98       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gregory-at-cvs gregory-at-cvs force-pushed the fix/session-manager-concurrency branch from 924c6e8 to da961df Compare November 22, 2025 05:09
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.

1 participant