fix(agents): handle rejections from .finally() on background tasks - #2519
Open
Rehansanjay wants to merge 3 commits into
Open
Rehansanjay wants to merge 3 commits into
Rehansanjay wants to merge 3 commits into
Conversation
promise.finally(cb) returns a new promise that rejects with the original error, or with anything cb throws. Nothing handled that derived promise at these sites, so a failing speech task, participant entrypoint, TTS request or worker task produced an unhandled promise rejection. Speech replies register their cleanup with Task.addDoneCallback, SpeechHandle runs each done callback in its own try/catch, and participant entrypoint, worker, process pool and avatar stream failures are logged. Fixes livekit#2508 Written with AI assistance (Claude Code); reviewed and verified before pushing.
🦋 Changeset detectedLatest commit: 49b4613 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
addDoneCallback queued a callback registered on an already-done handle with queueMicrotask and no guard, so a throwing callback became an uncaught exception. Both paths now run callbacks through runDoneCallback, which logs a failure the same way. Python schedules the late callback with call_soon, where the event loop logs the exception. Written with AI assistance (Claude Code); reviewed and verified before pushing.
Task.addDoneCallback queued a callback registered on an already-done task with queueMicrotask and no guard, so a throwing callback became an uncaught exception. It now shares runDoneCallback with the completion loop, which logs the failure. This is the same fix as the SpeechHandle one. Written with AI assistance (Claude Code); reviewed and verified before pushing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2508.
promise.finally(cb)returns a new promise that rejects with the original error, or with anythingcbthrows. At the sites below nothing handled that derived promise, so a failing speech task, participant entrypoint, TTS request or worker task produced an unhandled promise rejection. In a job process that only reaches thedebug-level handler inipc/job_proc_lazy_main.ts; the worker process installs no handler, so Node's default applies.Each site now handles it the way the equivalent Python code does, or the way the surrounding JS already does.
Changes Made
voice/agent_activity.ts(3 sites)task.addDoneCallback(() => this.onPipelineReplyDone(lease))instead oftask.result.finally(...).Taskruns done callbacks without exposing the task's rejection and logs a throwing callback. Python registers_on_pipeline_reply_donewithadd_done_callbackat the same three places.voice/speech_handle.tstry/catchand a failure is logged withwarn, so one throwing callback no longer skips the rest or rejects the derived promise. A callback added after the handle is done goes through the same guard instead of an unguardedqueueMicrotask. Python'sSpeechHandledoes the same.utils.ts(Task.addDoneCallback)try/catchas the completion loop, so a throwing late callback is logged instead of becoming an uncaught exception.job.tserror in participant entrypoint, as Python's_on_donedoes.worker.ts(availability, termination, job request),ipc/proc_pool.ts.finallynow ends in a.catchthat logs. A task is removed from#tasks/taskswhen it settles, so the shutdownallSettlednever observes its error; this log is where it surfaces.proc_pool.tsgains a logger for this.voice/avatar/datastream_io.tsstreamWriter.close()inflush()is logged withwarn; the writer is still cleared.tts/tts.ts(SynthesizeStream.pushText,ChunkedStreamconstructor)close(), andChunkedStreamemits its failure on the TTSerrorevent before rethrowing.Behaviour change to be aware of: an error from one of the worker or process-pool tasks is now logged instead of reaching Node as an unhandled rejection, which by default terminates the process.
Pre-Review Checklist
Testing
restaurant_agent.tsandrealtime_agent.tswork properly (for major changes)New regression tests, each of which fails on
main:speech_handle.test.ts: a throwing done callback no longer prevents the next callback and leaves no unhandled rejection; a throwing callback added after the handle is done no longer becomes an uncaught exception.utils.test.ts: a throwing done callback added after a task is done no longer becomes an uncaught exception.job.test.ts: a rejecting participant entrypoint leaves no unhandled rejection.tts.test.ts: aChunkedStreamwhoserun()fails reports through theerrorevent and leaves no unhandled rejection.The
agent_activity.tschange relies onTask.addDoneCallback, whichutils.test.tsalready covers. Theworker.ts,proc_pool.tsanddatastream_io.tssites have no new tests; they need a worker connection, a process pool or a room to exercise.Locally:
pnpm exec prettier --check,pnpm exec eslintandtsc --noEmitpass on the changed files, and a fullagents/srcrun shows no failures from this change. The failures that remain locally also fail onmain:amd_session_close.test.tsneeds a builtdist, and tworun_context.test.tsfiller tests are timing sensitive.utils.test.ts > Task > should handle nested tasks that complete successfullyfails intermittently, about one run in three, onmainas well as on this branch.Additional Notes
This covers the
.finally(...)part of #2508. Theno-floating-promisesrule discussed there would be a separate change.