Skip to content

fix(ipc): close uninitialized supervised process - #1774

Open
rosetta-livekit-bot[bot] wants to merge 14 commits into
mainfrom
civies-hopes-remedy
Open

fix(ipc): close uninitialized supervised process#1774
rosetta-livekit-bot[bot] wants to merge 14 commits into
mainfrom
civies-hopes-remedy

Conversation

@rosetta-livekit-bot

@rosetta-livekit-bot rosetta-livekit-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Ports livekit/agents#6051 to agents-js.\n\nWhen a supervised process is closed before initialization completes, reject the initialization future to unblock the supervisor task, kill the child process directly, and wait for join cleanup instead of sending a graceful shutdown the child cannot handle yet.\n\nTests:\n- pnpm test agents/src/ipc/supervised_proc.test.ts\n- pnpm build:agents\n\nNo tests were added, matching the porting instruction.


Ported from livekit/agents#6051

Original PR description

Summary

test_slow_initialization flakes on CI with a leaked ProcJobExecutor._supervise_task at teardown. The root cause is a real shutdown race in SupervisedProc, not a test issue.

When the owning task (e.g. ProcPool._proc_spawn_task during pool close) is cancelled while awaiting start(), the shielded _start() keeps running and creates the supervise task afterwards. The cleanup path then calls aclose(), which no-ops because the proc isn't marked started yet — leaking the supervise task and the child process. The orphaned child blocks forever waiting for InitializeRequest, and its non-daemon join thread can hang worker shutdown indefinitely.

Changes

  • start() keeps a handle on the shielded _start() task; aclose() waits for it before checking started, so an abandoned start can't race past the check.
  • aclose() kills a never-initialized process instead of attempting a graceful shutdown: the child reads InitializeRequest before servicing any other message, so a ShutdownRequest could never be acked.
  • kill() resolves a pending _initialize_fut so the supervise task (which waits on it before supervising) can observe the process exit.
  • Added test_aclose_after_cancelled_start, which deterministically reproduces the leak (fails the leaked-tasks check and hangs pytest exit without the fix).

nit: Also sets the agent session tests to speed = 1 and raises the default drain_delay — they run under virtual time, so the speed factor no longer buys wall-clock time.

toubatbrian and others added 14 commits May 28, 2026 14:43
…1525)

Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
Co-authored-by: u9g <jason.lernerman@livekit.io>
Agent.llmNode now returns ReadableStream<ChatChunk | string | FlushSentinel>,
but the agent_v2 hook overrides and AgentHookAdapter still declared the
narrower ChatChunk | string union, so passing super.llmNode as the fallback
failed to type-check. Widen the override return types and the adapter's
fallback/return signatures to include FlushSentinel.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Brian Yin <brian.yin@livekit.io>
Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
Co-authored-by: u9g <jason.lernerman@livekit.io>
Catch end-call close listener errors to avoid unhandled rejections during shutdown, and make public tool type guards return false for null inputs.
Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
…egment (#1760)

Co-authored-by: Cursor <cursoragent@cursor.com>
…#1698)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 409b1b6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@rosetta-livekit-bot
rosetta-livekit-bot Bot requested a review from longcw June 12, 2026 00:17

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines +232 to +238
if (!this.init.done) {
this.init.reject(new Error('process closed before initialization completed'));
this.proc?.kill();
await this.#join.await.then(() => {
this.clearTimers();
});
return;

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.

🚩 initialize() caller may hang when close() is called during initialization

The new close() path correctly rejects init and resolves #join, fixing the issue where close() itself would hang. However, if a caller (e.g. proc_pool.ts:108-109 in procWatchTask) is concurrently awaiting proc.initialize(), that call hangs forever on once(this.proc!, 'message') at supervised_proc.ts:217 because killing a child process emits 'exit'/'close' but NOT 'error', so events.once() never resolves or rejects. This is a pre-existing issue not introduced by this PR — in fact, the old code was worse because close() itself would also hang. The fix here is a meaningful improvement, but a complete solution would also need initialize() to detect the killed process (e.g., by racing once(proc, 'message') with once(proc, 'exit') or using an AbortSignal).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Base automatically changed from 1.5.0 to main July 2, 2026 13:48

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 4 new potential issues.

Open in Devin Review

Comment on lines +786 to 799
const oldToolNames = new Set(Object.keys(oldToolCtx.functionTools));
const oldToolsets = oldToolCtx.toolsets;
const newToolCtx = new ToolContext(tools);
const newToolsets = newToolCtx.toolsets;
const addedToolsets = newToolsets.filter((ts) => !oldToolsets.includes(ts));
const removedToolsets = oldToolsets.filter((ts) => !newToolsets.includes(ts));

// Resolve added factory toolsets before re-flattening, so their tools are included in the
// advertised set (newToolNames is computed below, after resolution).
await this.setupToolsetList(addedToolsets);
newToolCtx.updateTools(newToolCtx.tools);
const newToolNames = new Set(Object.keys(newToolCtx.functionTools));
const toolsAdded = [...newToolNames].filter((name) => !oldToolNames.has(name));
const toolsRemoved = [...oldToolNames].filter((name) => !newToolNames.has(name));

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.

🟡 Provider tool additions and removals are silently ignored in update notifications, despite being tracked at startup

Provider tool IDs are omitted from the added/removed diff when tools are updated at runtime (updateTools at agents/src/voice/agent_activity.ts:786-799), even though they are included in the initial config notification at startup (agents/src/voice/agent_activity.ts:497-500), so the session history and chat context never record mid-session provider-tool changes.

Impact: The LLM's chat history shows provider tools appearing at startup but never being added or removed later, which could confuse context-aware models or break audit trails.

Mechanism: initial setup includes provider tool IDs but updateTools only diffs function tools

At startup (agents/src/voice/agent_activity.ts:497-500):

const initialTools = [
  ...Object.keys(this.agent._toolCtx.functionTools),
  ...this.agent._toolCtx.providerTools.map((t) => t.id),
];

But in updateTools (agents/src/voice/agent_activity.ts:786-799):

const oldToolNames = new Set(Object.keys(oldToolCtx.functionTools));
...
const newToolNames = new Set(Object.keys(newToolCtx.functionTools));

Only function tool names are compared. Provider tools from oldToolCtx.providerTools and newToolCtx.providerTools are never diffed, so adding or removing a ProviderTool via updateTools produces no AgentConfigUpdate entry in the chat context or session history.

Prompt for agents
In agent_activity.ts updateTools(), the toolsAdded/toolsRemoved diff only considers function tool names (Object.keys of functionTools). But the initial setup at lines 497-500 includes provider tool IDs via providerTools.map(t => t.id). To be consistent, updateTools should also diff provider tool IDs. Compute old and new provider tool ID sets alongside the function tool name sets, and include any added/removed provider tool IDs in the AgentConfigUpdate.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread agents/src/index.ts
Comment on lines +23 to 27
export * from './llm/index.js';
export * as llm from './llm/index.js';
export * from './log.js';
export * from './metrics/index.js';
export * as metrics from './metrics/index.js';

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.

🚩 Flattening llm/voice/metrics exports to the package root may cause future name collisions

The new agents/src/index.ts:23-24 does both export * from './llm/index.js' and export * as llm from './llm/index.js' (similarly for voice at lines 39-40 and metrics at lines 26-27). This means every named export from those modules is now available at the package root AND under the namespace. Today there are no collisions between llm, voice, and metrics exports, but any future addition of a symbol with the same name in two of these modules would cause a TypeScript error at the re-export site. The old approach (namespace-only exports) was collision-proof by design.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +4220 to +4225
updateTools: (tools) => {
ts._setTools(tools);
void this.onToolsetToolsChanged().catch((error) =>
this.logger.error({ error }, 'error re-advertising toolset tools'),
);
},

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.

🚩 Dynamic toolset push uses fire-and-forget async, which can interleave with concurrent pushes

When a toolset pushes new tools at runtime via the updateTools callback wired in setupToolsetList (agents/src/voice/agent_activity.ts:4220-4225), onToolsetToolsChanged is called fire-and-forget (void this.onToolsetToolsChanged().catch(...)). If two toolsets push tools concurrently, two onToolsetToolsChangedupdateTools calls can interleave, potentially producing incorrect toolsAdded/toolsRemoved diffs in the AgentConfigUpdate (the second call reads oldToolCtx before the first call has written newToolCtx). The final tool state converges correctly because each call re-flattens from the source toolsets, but transient config-update entries in the chat history could be wrong.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +653 to +654
const serverVad = this.#opts.serverVad;
const commitStrategy = serverVad === undefined || serverVad === null ? 'manual' : 'vad';

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.

🚩 ElevenLabs STT default commit strategy changed from 'vad' to 'manual'

The #connectWs method in plugins/elevenlabs/src/stt.ts:653-654 changed the default commit strategy. Previously, when serverVad was undefined (not explicitly set by the user), the condition this.#opts.serverVad === null was false, so commitStrategy defaulted to 'vad'. Now the condition serverVad === undefined || serverVad === null catches both cases, defaulting to 'manual'. This is a behavioral change for users who don't explicitly set serverVad: their ElevenLabs STT streams will now use manual commit instead of server-side VAD. The tests were updated to match, and a new END_OF_SPEECH emission was added for the server-VAD path at line 799-803.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

2 participants