Skip to content

Fix source manager lifetime race - #1772

Merged
summeroff merged 3 commits into
stagingfrom
fix-source-manager-lifetime-race
Sep 10, 2026
Merged

Fix source manager lifetime race#1772
summeroff merged 3 commits into
stagingfrom
fix-source-manager-lifetime-race

Conversation

@aleksandr-voitenko

@aleksandr-voitenko aleksandr-voitenko commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Description

Fix a source lifetime race when reading OBS source properties concurrently with the source’s final release.

Motivation

GetProperties could retrieve a raw obs_source_t * shortly before OBS destroyed its reference control block. Attempting to acquire a strong reference through that stale pointer could crash the backend, as observed in the concurrent browser-source test on macOS.

Context

Locking the source manager around the raw-pointer lookup was insufficient because the manager did not retain an OBS reference. The source could begin destruction before the manager received its destroy callback.

The manager now retains a weak OBS reference for each registered source. GetProperties promotes that weak reference to a strong reference while holding the manager lock. If destruction has already started, it returns InvalidReference.

Implementation details

  • Retain weak OBS references alongside registered source IDs.
  • Remove weak references during free() and clear().
  • Avoid duplicate registrations for the same source.
  • Use weak-reference promotion in GetProperties.
  • Add a deterministic regression test covering both race outcomes:
    • Final release wins and lookup returns InvalidReference.
    • Lookup wins and keeps the source alive until the operation completes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@aleksandr-voitenko
aleksandr-voitenko force-pushed the fix-source-manager-lifetime-race branch from 4935d67 to 822ed2b Compare September 9, 2026 21:51
@aleksandr-voitenko aleksandr-voitenko changed the title [WIP] Fix source manager lifetime race Fix source manager lifetime race Sep 9, 2026
@summeroff
summeroff requested a balanced review from Copilot September 10, 2026 00:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Private scenes can retain weak references indefinitely because their allocation path lacks destroy-signal registration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a race between source-property lookup and final source release.

Changes:

  • Retains weak source references and promotes them safely.
  • Prevents duplicate source registrations.
  • Adds deterministic lifecycle regression tests.
File summaries
File Description
obs-studio-server/source/osn-source.hpp Adds weak-reference lifecycle management.
obs-studio-server/source/osn-source.cpp Uses safe promotion in GetProperties.
obs-studio-server/tests/test-osn-source.cpp Tests both race outcomes.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread obs-studio-server/source/osn-source.hpp

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The lifetime fix is coherent, cleanup paths are covered, and deterministic tests validate both race outcomes.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@summeroff
summeroff merged commit f59217e into staging Sep 10, 2026
21 checks passed
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.

3 participants