-
Notifications
You must be signed in to change notification settings - Fork 3.8k
simulation: participant-sync race, fake-job metadata fallback, no audio recording #6069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9420e2c
654da62
db3db53
99c9e3f
195d743
138a8b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -461,14 +461,11 @@ def simulation_context(self) -> SimulationContext | None: | |
| metadata = dispatch_json | ||
| break | ||
| if not metadata: | ||
| # older servers sent the dispatch in the job metadata; | ||
| # fake_job_context places it there too | ||
| metadata = self._info.job.metadata | ||
| if not metadata: | ||
| # The simulator participant is only visible once the room is | ||
| # connected; a miss before then (AgentSession.start consults | ||
| # _text_only pre-connect) must not be cached. | ||
| self._simulation_resolved = self._room.isconnected() | ||
| # The simulator joins before the agent, so a miss is only final | ||
| # once the room is connected and a remote participant is visible. | ||
| self._simulation_resolved = ( | ||
| self._room.isconnected() and len(self._room.remote_participants) > 0 | ||
| ) | ||
| return None | ||
|
Comment on lines
463
to
469
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π© Removal of job-metadata fallback breaks pre-connect simulation detection for all _text_only callers in start() The removal of the Was this helpful? React with π or π to provide feedback.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback was already inert for real jobs: the server stopped populating job metadata with the dispatch when it moved to participant attributes (production runs detect via attributes with empty metadata). Pre-connect _text_only resolution therefore did not work through metadata before this PR either; connect-first entrypoints are unaffected. |
||
|
|
||
| self._simulation_resolved = True | ||
|
|
@@ -597,8 +594,10 @@ async def connect( | |
| await self._room.connect(self._info.url, self._info.token, options=room_options) | ||
| self._on_connect() | ||
|
|
||
| if self.simulation_context() is not None: | ||
| self._room.on("participant_disconnected", self._on_simulator_disconnected) | ||
| # Always registered: the callback ignores participants without the | ||
| # simulator attribute, and gating on simulation_context() here would | ||
| # race the participant-list sync. | ||
| self._room.on("participant_disconnected", self._on_simulator_disconnected) | ||
|
|
||
| for p in self._room.remote_participants.values(): | ||
| self._participant_available(p) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π© job_metadata parameter remains in fake_job_context but is no longer consumed for simulation dispatch The (Refers to line 31) Was this helpful? React with π or π to provide feedback.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional: no deployed server sends the dispatch via job metadata, so the simulation use of job_metadata was already dead in practice. The parameter stays for generic ctx.job.metadata consumers; in-process simulation tests can set the simulation context on the yielded JobContext directly. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -667,6 +667,8 @@ async def start( | |
| record = job_ctx.job.enable_recording if job_ctx else False | ||
|
|
||
| self._recording_options = _resolve_recording_options(record) # type: ignore[arg-type] | ||
| if self._text_only: | ||
| self._recording_options["audio"] = False | ||
|
Comment on lines
+670
to
+671
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π© New _text_only check at line 670 is ineffective for real (non-fake) simulation jobs The new This is consistent with the existing Was this helpful? React with π or π to provide feedback.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the thread below: effective in the connect-first flow every simulation entrypoint uses; the auto-connect ordering limitation predates this PR.
Comment on lines
+670
to
+671
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π‘ New text-simulation audio recording disable is ineffective because room is not yet connected The newly added code at lines 670-671 attempts to disable audio recording for text simulations by checking The same issue affects the pre-existing Prompt for agentsWas this helpful? React with π or π to provide feedback.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partially right, but the impactful flow works: every simulation entrypoint awaits ctx.connect() before session.start() (e.g. examples/frontdesk), so at line 670 the room is connected and participants are synced β with this PR's no-cache guard the check is effective there. The auto-connect flow (start() scheduling connect at line 813) indeed decides before connect; that limitation predates this PR and is unchanged by it. Worth noting the metadata fallback never helped here in practice: the server stopped sending the dispatch via job metadata when it moved to participant attributes, and last night's production runs confirm detection succeeded via attributes with empty job metadata. |
||
|
|
||
| is_primary = True | ||
| if job_ctx: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.