Skip to content

fix: bound system TTS Speak with a process deadline - #10

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/tts-speak-deadline
Open

SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/tts-speak-deadline

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

Default clawgo run (-chat-subscribe true, -tts-engine system) speaks chat final text through a single TTS queue. That queue calls systemTTSEngine.Speak, which ran espeak-ng (or -tts-system-command) with exec.Command and Run(). 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 stuck espeak-ng (or a stand-in such as sleep) silences the node until clawgo is 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 run of the old exec.Command path versus CommandContext with a 200ms deadline. Child is sleep 2, the same argv shape Speak uses (command plus the spoken text as the last argument):

$ go run /tmp/clawgo-f005-speak-demo.go
unbounded sleep 2: err=<nil> elapsed=2.008s
bounded 200ms sleep 2: err=signal: killed elapsed=201ms

Same two items on a serial queue (the TTS loop shape). Without a deadline, item 2 cannot start until sleep 2 finishes. With a 200ms deadline, item 1 is killed and item 2 runs:

$ go run /tmp/clawgo-f005-queue-demo.go
unbounded item=1 text="2" err=<nil> item_elapsed=2.007s total=2.007s
unbounded item=2 text="0" err=<nil> item_elapsed=2ms total=2.01s
bounded200ms item=1 text="2" err=signal: killed item_elapsed=201ms total=201ms
bounded200ms item=2 text="0" err=<nil> item_elapsed=2ms total=203ms

On this branch, systemTTSEngine.Speak uses that CommandContext path. A hung child (sleep 2, 200ms deadline) now returns an error in 0.20s instead of succeeding after 2s.

Real behavior proof

  • Behavior or issue addressed: A hung system TTS child no longer blocks the single Speak queue forever.
  • Real environment tested: macOS 26.6.2, Darwin 25.6.0 arm64, Go 1.27.0, branch fix/tts-speak-deadline at /tmp/clawgo-F005.
  • Exact steps or command run after this patch: Ran go run /tmp/clawgo-f005-speak-demo.go and go run /tmp/clawgo-f005-queue-demo.go. Then invoked production systemTTSEngine.Speak with command sleep, text 2, and a 200ms deadline.
  • Evidence after fix: terminal output from the live go run helpers above. Unbounded sleep 2 returned 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.
  • Observed result after fix: Speak returns when the deadline fires. The queue can move to the next utterance. The old exec.Command path still waits for the child to exit on its own.
  • What was not tested: a live gateway chat stream, a real espeak-ng hang, 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.CommandContext with a 30s deadline (defaultTTSSpeakTimeout). The queue already logs tts error: %v when Speak fails.

Introduced in f601408 (2026-01-04, 241 days). Still present after the c6e4679 rewrite in #8.

Related work:

  • In-repo sibling: modules/stt/brabble.go already starts the STT child with exec.CommandContext.
  • #5 reconnect backoff cancel, #6 waitForPair/waitForHello context, #7 TTS queue leak on reconnect. This PR is only the Speak child deadline.
  • Go exec.CommandContext kills the process when the context is done.
  • openclaw#125530 classifies TTS transport timeouts (same class, different layer).

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>
@clawsweeper

clawsweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 2, 2026
@clawsweeper

clawsweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 5, 2026, 3:58 PM ET / 19:58 UTC.

ClawSweeper review

What this changes

The PR gives each system text-to-speech command a 30-second deadline and adds a subprocess timeout regression test.

Regression provenance

Possible 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
Reviewed head: e8df603a82e911d19f87eede0c8ab2b155875e1a
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused implementation and useful real subprocess evidence are offset by the unresolved playback regression.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured macOS evidence reports production Speak terminating a real sleep child at approximately 201ms and demonstrates subsequent serial work proceeding; this supports the direct-child timeout behavior, while long-speech compatibility remains a separate unresolved finding.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured macOS evidence reports production Speak terminating a real sleep child at approximately 201ms and demonstrates subsequent serial work proceeding; this supports the direct-child timeout behavior, while long-speech compatibility remains a separate unresolved finding.
Evidence reviewed 7 items Current main still lacks a speech deadline: The fetched default branch runs the speech executable with exec.Command and waits synchronously; the queue has one consumer and drops incoming text when its 16-item buffer fills. No existing speech-timeout option was found. Local tags were empty, so no shipped-fix claim is established.
Introduced cutoff affects unrestricted speech: The pinned introduced diff assigns a 30-second timeout to every constructed system engine. Final response text reaches Speak without splitting or a length limit; the existing CLI exposes speech rate and a custom executable but no deadline override.
Existing playback contract: README documents speaking chat responses through espeak-ng and configuring its words-per-minute rate. The implementation defaults to 180 words per minute, making speech longer than approximately 90 words liable to exceed the proposed deadline.
Findings 1 actionable finding [P1] Preserve healthy long speech instead of a fixed cutoff
Security None None.

How this fits together

Clawgo 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
Loading

Decision needed

Question Recommendation
Should system TTS preserve existing playback by default, or adopt an automatic speech-aware deadline? Preserve default playback: Keep existing playback semantics and make strict process deadlines an explicit operator choice.

Why: A universal wall-clock cutoff changes established playback behavior, and choosing a replacement policy for arbitrary custom commands requires maintainer intent.

Before merge

  • Preserve healthy long speech instead of a fixed cutoff (P1) - Every system engine now receives this 30-second limit, but final chat text is neither capped nor split. At the default 180 words per minute, ordinary responses longer than roughly 90 words can exceed it; slower configured rates reach it sooner. CommandContext then kills healthy playback, and users cannot override the timeout for custom commands. Preserve complete playback through an approved speech-aware budget or an explicit opt-in deadline. This previously reported blocker remains unchanged.
  • Resolve merge risk (P1) - Existing installations would begin cutting healthy speech off after 30 seconds, including custom commands; fresh-install and upgrade compatibility for long utterances remain unproven.
  • Complete next step (P2) - Approve a playback-compatible timeout policy, replace the unconditional cutoff, and verify long speech on fresh and upgraded setups alongside timeout recovery.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Preserve healthy long speech instead of a fixed cutoff — cmd/clawgo/main.go:911
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +11/-2 (net +9); tests +22 The focused production growth is justified by bounding subprocess execution, while the new test covers only the timeout case.

Merge-risk options

Maintainer options:

  1. Set a compatible timeout policy (recommended)
    Approve a policy that preserves healthy playback, then revise the cutoff and demonstrate both timeout recovery and long-utterance compatibility.

Technical review

Best 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:

  • [P1] Preserve healthy long speech instead of a fixed cutoff — cmd/clawgo/main.go:911
    Every system engine now receives this 30-second limit, but final chat text is neither capped nor split. At the default 180 words per minute, ordinary responses longer than roughly 90 words can exceed it; slower configured rates reach it sooner. CommandContext then kills healthy playback, and users cannot override the timeout for custom commands. Preserve complete playback through an approved speech-aware budget or an explicit opt-in deadline. This previously reported blocker remains unchanged.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against c6e46796a1c8.

Labels

Label justifications:

  • P1: The proposed default would truncate otherwise healthy spoken responses in an existing core node workflow.
  • merge-risk: 🚨 compatibility: Every existing system-TTS setup inherits a new, non-configurable 30-second playback limit.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The captured macOS evidence reports production Speak terminating a real sleep child at approximately 201ms and demonstrates subsequent serial work proceeding; this supports the direct-child timeout behavior, while long-speech compatibility remains a separate unresolved finding.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured macOS evidence reports production Speak terminating a real sleep child at approximately 201ms and demonstrates subsequent serial work proceeding; this supports the direct-child timeout behavior, while long-speech compatibility remains a separate unresolved finding.

Evidence

What I checked:

  • Current main still lacks a speech deadline: The fetched default branch runs the speech executable with exec.Command and waits synchronously; the queue has one consumer and drops incoming text when its 16-item buffer fills. No existing speech-timeout option was found. Local tags were empty, so no shipped-fix claim is established. (cmd/clawgo/main.go:941, c6e46796a1c8)
  • Introduced cutoff affects unrestricted speech: The pinned introduced diff assigns a 30-second timeout to every constructed system engine. Final response text reaches Speak without splitting or a length limit; the existing CLI exposes speech rate and a custom executable but no deadline override. (cmd/clawgo/main.go:948, e8df603a82e9)
  • Existing playback contract: README documents speaking chat responses through espeak-ng and configuring its words-per-minute rate. The implementation defaults to 180 words per minute, making speech longer than approximately 90 words liable to exceed the proposed deadline. (README.md:68, e8df603a82e9)
  • Captured real subprocess evidence: The supplied PR body, captured under sourceRevision 86805ca94a02e97bd297c6fdf75009a4e0c56d81bd2be9b6bfaac1c14b378ac6, reports invoking production systemTTSEngine.Speak on macOS with a real sleep child and a 200ms deadline. Its terminal evidence records termination around 201ms and a serial-queue demonstration advancing at 203ms. It explicitly excludes long utterances and surviving grandchildren. These commands were inspected as evidence, not executed. (e8df603a82e9)
  • Prior blocker remains applicable: The completed prior review at fix: bound system TTS Speak with a process deadline #10 (comment) requested preservation of healthy long speech and coverage beyond 30 seconds. The prior reviewed SHA equals the current head, and the comparison is empty; no supplied human disposition resolves those requests. (cmd/clawgo/main.go:911, e8df603a82e9)
  • Routing history and inspection limits: Available main-branch logs repeatedly name Mariano Belinky for cmd/clawgo/main.go work; Peter Steinberger authored the recent dependency refresh and README work. Historical blame and some commit-diff inspection failed because required objects were unavailable, so exact source-line introduction is not established and routing candidates remain unverified. (cmd/clawgo/main.go, c6e46796a1c8)

Likely related people:

  • Mariano Belinky: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve the maintainer timeout-policy choice and preserve healthy long speech.
  • Demonstrate fresh and upgraded setups completing long utterances, alongside bounded hung-child recovery.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-02T17:06:59.544Z sha e8df603 :: found issues before merge. :: [P1] Preserve healthy long speech instead of a fixed cutoff
  • reviewed 2026-09-03T15:03:21.901Z sha e8df603 :: blocked before merge. :: [P1] Preserve healthy long speech instead of a fixed cutoff

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed P2 Normal priority bug or improvement with limited blast radius. labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant