Conversation
systemTTSEngine.Speak ran espeak-ng via exec.Command with no deadline. A hung TTS child blocked the single TTS queue forever. Use CommandContext with a 30s timeout so a hung Speak returns instead of parking the queue. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 5, 2026, 3:58 PM ET / 19:58 UTC. ClawSweeper reviewWhat this changesThe PR gives each system text-to-speech command a 30-second deadline and adds a subprocess timeout regression test. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 4 items remain The fix remains necessary on current main, but the previously reported long-speech cutoff remains unresolved at the unchanged head. The supplied runtime evidence supports timeout recovery, not compatibility with healthy long utterances. Priority: P1 Review scores
Verification
How this fits togetherClawgo receives final chat responses from the gateway and sends their text through a serial speech queue. The system speech engine invokes a local executable, whose completion lets the queue advance. flowchart LR
A[Gateway chat response] --> B[Final response text]
B --> C[Serial speech queue]
C --> D[Local speech executable]
D --> E{Completes within deadline?}
E -->|Yes| F[Next queued response]
E -->|No| G[Kill process and log error]
G --> F
Decision needed
Why: A universal wall-clock cutoff changes established playback behavior, and choosing a replacement policy for arbitrary custom commands requires maintainer intent. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve complete playback by default, making a strict cutoff opt-in unless a speech-aware deadline is validated for supported speech rates and custom commands. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the failure path: a healthy speech command lasting over 30 seconds is killed despite unrestricted input text. This review did not execute a reproduction. Is this the best way to solve the issue? No. Context-based cancellation is appropriate, but a universal 30-second budget confuses slow or long speech with a hung process. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against c6e46796a1c8. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles) |
What Problem This Solves
Default
clawgo run(-chat-subscribe true,-tts-engine system) speaks chatfinaltext through a single TTS queue. That queue callssystemTTSEngine.Speak, which ranespeak-ng(or-tts-system-command) withexec.CommandandRun(). There was no context and no process deadline.If the TTS binary hangs, the queue goroutine stays inside
Run()forever. Later chat speech sits in the buffer-16 channel, then is dropped when the channel is full. One stuckespeak-ng(or a stand-in such assleep) silences the node untilclawgois killed.This is separate from #7, which stops a leaked queue on reconnect. Here the current queue is alive and blocked on one child.
Evidence
Live
go runof the oldexec.Commandpath versusCommandContextwith a 200ms deadline. Child issleep 2, the same argv shape Speak uses (commandplus the spoken text as the last argument):Same two items on a serial queue (the TTS loop shape). Without a deadline, item 2 cannot start until
sleep 2finishes. With a 200ms deadline, item 1 is killed and item 2 runs:On this branch,
systemTTSEngine.Speakuses thatCommandContextpath. A hung child (sleep 2, 200ms deadline) now returns an error in 0.20s instead of succeeding after 2s.Real behavior proof
fix/tts-speak-deadlineat/tmp/clawgo-F005.go run /tmp/clawgo-f005-speak-demo.goandgo run /tmp/clawgo-f005-queue-demo.go. Then invoked productionsystemTTSEngine.Speakwith commandsleep, text2, and a 200ms deadline.go runhelpers above. Unboundedsleep 2returned nil after 2.008s. Bounded Speak killed the child at 201ms (signal: killed). The serial queue then started the next item at 203ms total instead of waiting the full 2s.exec.Commandpath still waits for the child to exit on its own.espeak-nghang, utterances longer than 30s, and grandchild processes that outlive the killed TTS parent.Summary
Call chain: chat
final->ChatSubscriber.speak->TTSQueue.Speak->TTSQueue.loop->systemTTSEngine.Speak->exec.Command(...).Run().Fix:
exec.CommandContextwith a 30s deadline (defaultTTSSpeakTimeout). The queue already logstts error: %vwhen Speak fails.Introduced in
f601408(2026-01-04, 241 days). Still present after thec6e4679rewrite in #8.Related work:
modules/stt/brabble.goalready starts the STT child withexec.CommandContext.exec.CommandContextkills the process when the context is done.