Fix race between adapter connection publication and session attachment#2049
Fix race between adapter connection publication and session attachment#2049aperez wants to merge 2 commits into
Conversation
`Connection.__init__` publishes a new connection to `_connections` under `_lock`, then releases the lock before classifying it. In that window another thread can attach the connection to a session (via an `attach` request or the launch flow), which sets `conn.server` and the session's `pid`. When the constructor resumes, `sessions.get(self.pid)` returns that just-attached session, and because it is not a process replacement the old code logged "is not expecting replacement" and closed the channel, dropping a session that was already live. This shows up with subprocess debugging and in-place process replacement. Fix it by re-checking under `_lock` whether the connection was already attached (`self.server is not None`) before treating it as an unexpected replacement. Closing the channel under `_lock` is deadlock-free: `JsonMessageChannel.close()` only takes the channel's own lock and the disconnect handler runs on the parser thread. Add a regression test that overlaps publication and attachment.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree company="Meta" |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| connection_thread.start() | ||
| try: | ||
| assert attachment_started.wait(5) | ||
| assert classification_waiting.wait(5) |
There was a problem hiding this comment.
This test feels a bit over-coupled to the implementation. The ObservedLock + classification_waiting scaffolding asserts that the fix re-acquires _lock specifically during classification — a correct-but-different fix (e.g. snapshotting self.server a different way) would fail here even though the behavior is fine. The real value is in the final behavioral checks (connections[0].server is attached_session.server and not channel.closed). Would it make sense to do that instead?
There was a problem hiding this comment.
Thanks for the feedback!
I rewrote the test to be behavioral: it now drives the real attach path (wait_for_connection → attach_to_session) and asserts only outcomes, connections[0].server is session.server and not channel.closed. I dropped the ObservedLock/classification_waiting scaffolding entirely. The test still fails against the pre-fix code (channel gets closed).
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
heejaechang
left a comment
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
3 participants
Connection.__init__publishes a new connection to_connectionsunder_lock, then releases the lock before classifying it. In that window another thread can attach the connection to a session (via anattachrequest or the launch flow), which setsconn.serverand the session'spid. When the constructor resumes,sessions.get(self.pid)returns that just-attached session, and because it is not a process replacement the old code logged "is not expecting replacement" and closed the channel, dropping a session that was already live. This shows up with subprocess debugging and in-place process replacement.Fix it by re-checking under
_lockwhether the connection was already attached (self.server is not None) before treating it as an unexpected replacement. Closing the channel under_lockis deadlock-free:JsonMessageChannel.close()only takes the channel's own lock and the disconnect handler runs on the parser thread.Add a regression test that overlaps publication and attachment.