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

fix: bound system TTS Speak with a process deadline#10
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 <[email protected]>
@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 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 3, 2026, 11:03 AM ET / 15:03 UTC.

ClawSweeper review

What this changes

The PR runs each system TTS command under a 30-second process deadline and adds a regression test for a hung child process.

Merge readiness

Blocked before merge - 4 items remain

Keep this PR open: the deadline prevents a hung child from blocking the TTS queue, but its unconditional 30-second cutoff now terminates valid long chat responses and slower configured system-TTS commands.

Priority: P1
Reviewed head: e8df603a82e911d19f87eede0c8ab2b155875e1a
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused hang recovery has credible real-process evidence, but the unresolved fixed-cutoff compatibility defect blocks a higher readiness rating.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The changed production owner is the system TTS engine's local child-process call; the PR body supplies a terminal trace where its 200 ms deadline kills a real sleep child and lets the serial speech flow continue. That is sufficient proof of the intended hung-child recovery, though it does not address the separate long-speech compatibility finding.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is the system TTS engine's local child-process call; the PR body supplies a terminal trace where its 200 ms deadline kills a real sleep child and lets the serial speech flow continue. That is sufficient proof of the intended hung-child recovery, though it does not address the separate long-speech compatibility finding.
Evidence reviewed 6 items Verified introduced behavior: The verified PR delta replaces the default branch's unbounded process invocation with a fixed 30-second context deadline; current main is the PR base c6e4679, so this fix is not already implemented there.
Long-response path has no bound: Final chat content is forwarded directly to the TTS queue, and accumulated delta text is held in an unrestricted strings.Builder; no current maximum speech duration or text-length policy protects valid long responses from the new fixed cutoff.
Configured command has no timeout control: The CLI exposes voice, rate, and custom system-command settings, but no timeout setting; the new deadline therefore applies equally to the default engine and user-configured commands.
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 text from the gateway and sends it through a single local TTS queue. The queue invokes the configured system speech command, whose completion determines when the next spoken response can start.

flowchart LR
  A[Gateway chat final] --> B[Chat subscriber]
  B --> C[Single TTS queue]
  C --> D[System TTS engine]
  D --> E[Deadline decision]
  E --> F[Local speech command]
  F --> G[Spoken response or error log]
Loading

Decision needed

Question Recommendation
What timeout policy should system TTS use so hung children are bounded without truncating legitimate long speech or custom commands? Use a length-aware bounded deadline: Derive the deadline from text length and configured rate with a conservative upper cap, then test both healthy long speech and hung-child recovery.

Why: The PR changes established speech behavior for every existing system-TTS user, and choosing a fixed, configurable, or length-aware budget is a compatibility contract rather than a mechanical correction.

Before merge

  • Preserve healthy long speech instead of a fixed cutoff (P1) - The configured system command receives unrestricted final chat text, but this new constant kills every invocation after 30 seconds. At the default 180 words per minute, normal responses around 90 words can reach that limit, and custom commands have no timeout setting; derive a speech-aware budget or obtain explicit compatibility approval before cutting output off.
  • Resolve merge risk (P1) - Merging the fixed 30-second deadline will cut off legitimate long chat responses and slower custom system-TTS commands, with no user setting or length-aware allowance to recover the rest of the speech.
  • Complete next step (P2) - Agree on a long-utterance timeout policy, then revise the fixed cutoff and cover both healthy long speech and a hung child before merge.
  • 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
Focused process change 2 files; production +11/-2, tests +22 The small diff isolates the timeout policy, so the user-visible compatibility change is the principal merge decision.

Merge-risk options

Maintainer options:

  1. Adopt a length-aware deadline (recommended)
    Retain cancellation for hung children while allowing a budget that scales for legitimate speech, with focused coverage for both cases.
  2. Accept the fixed cutoff explicitly
    Merge the 30-second limit only after maintainers confirm that truncating slower or longer system-TTS output is acceptable.

Technical review

Best possible solution:

Keep the hang guard, but derive a bounded speech budget from text length and configured rate with a conservative ceiling, then prove both a hung child and a healthy utterance exceeding 30 seconds behave as intended.

Do we have a high-confidence way to reproduce the issue?

Yes. The supplied terminal trace exercises the production Speak path with a real sleeping child, and source inspection shows current main remains unbounded; this review did not execute the trace locally.

Is this the best way to solve the issue?

No. CommandContext is an appropriate hang mechanism, but a fixed 30-second budget is not the narrowest compatible solution because final chat text and custom system commands have no corresponding duration bound.

Full review comments:

  • [P1] Preserve healthy long speech instead of a fixed cutoff — cmd/clawgo/main.go:911
    The configured system command receives unrestricted final chat text, but this new constant kills every invocation after 30 seconds. At the default 180 words per minute, normal responses around 90 words can reach that limit, and custom commands have no timeout setting; derive a speech-aware budget or obtain explicit compatibility approval before cutting output off.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P1: The fixed deadline can interrupt normal spoken chat responses for existing default and custom system-TTS users.
  • merge-risk: 🚨 compatibility: A new unconditional process cutoff changes established speech completion behavior without an upgrade-safe long-output policy.
  • 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 changed production owner is the system TTS engine's local child-process call; the PR body supplies a terminal trace where its 200 ms deadline kills a real sleep child and lets the serial speech flow continue. That is sufficient proof of the intended hung-child recovery, though it does not address the separate long-speech compatibility finding.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the system TTS engine's local child-process call; the PR body supplies a terminal trace where its 200 ms deadline kills a real sleep child and lets the serial speech flow continue. That is sufficient proof of the intended hung-child recovery, though it does not address the separate long-speech compatibility finding.

Evidence

What I checked:

  • Verified introduced behavior: The verified PR delta replaces the default branch's unbounded process invocation with a fixed 30-second context deadline; current main is the PR base c6e4679, so this fix is not already implemented there. (cmd/clawgo/main.go:911, e8df603a82e9)
  • Long-response path has no bound: Final chat content is forwarded directly to the TTS queue, and accumulated delta text is held in an unrestricted strings.Builder; no current maximum speech duration or text-length policy protects valid long responses from the new fixed cutoff. (cmd/clawgo/main.go:1005, e8df603a82e9)
  • Configured command has no timeout control: The CLI exposes voice, rate, and custom system-command settings, but no timeout setting; the new deadline therefore applies equally to the default engine and user-configured commands. (cmd/clawgo/main.go:175, e8df603a82e9)
  • Real behavior proof: The PR body includes terminal evidence that a real production Speak invocation using sleep is killed at about 200 ms and that a serial queue proceeds to its next item instead of waiting two seconds. (e8df603a82e9)
  • Area history: File history identifies Mariano Belinky's f601408 as an earlier change to the central command file; available partial-clone objects did not permit line-level introduction verification. (cmd/clawgo/main.go, f60140892c55)
  • Release and current-main check: The local repository has no release tags, and main remains at c6e4679 rather than this unmerged head. (c6e46796a1c8)

Likely related people:

  • Mariano Belinky: 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.

  • Obtain maintainer agreement on the long-utterance timeout policy.
  • Add focused coverage for a healthy system-TTS command exceeding 30 seconds as well as the hung-child path.

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 (1 earlier review cycle)
  • reviewed 2026-09-02T17:06:59.544Z sha e8df603 :: found issues 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