Skip to content

fix(media): bound ffmpeg conversion wait with a timeout#176

Merged
steipete merged 5 commits into
openclaw:mainfrom
SebTardif:fix/ffmpeg-conversion-timeout
Jul 16, 2026
Merged

fix(media): bound ffmpeg conversion wait with a timeout#176
steipete merged 5 commits into
openclaw:mainfrom
SebTardif:fix/ffmpeg-conversion-timeout

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

AttachmentResolver.runConversionProcess started ffmpeg and blocked on process.waitUntilExit() with no deadline. A hung converter stalled attachment metadata resolution indefinitely when unsupported media conversion was enabled.

Evidence

Patch

  • Bound conversion with a monotonic ContinuousClock deadline (default 60s).
  • On timeout: SIGTERM, then SIGKILL of process and process-group (best-effort).
  • Hung regression fixture uses exec sleep 30 so the Process PID is the sleeper (no orphaned shell child).

Real ffmpeg success (production conversion args)

CAF generated with ffmpeg, then converted with the same flags AttachmentResolver uses (-nostdin -y -i … -c:a aac -b:a 128k):

$ ffmpeg -hide_banner -y -f lavfi -i sine=frequency=440:duration=0.5 -c:a pcm_s16le /tmp/imsg-176-proof/tone.caf
$ ffmpeg -hide_banner -nostdin -y -i /tmp/imsg-176-proof/tone.caf -c:a aac -b:a 128k /tmp/imsg-176-proof/tone.m4a
real 0.02
$ file /tmp/imsg-176-proof/tone.m4a
ISO Media, Apple iTunes ALAC/AAC-LC (.M4A) Audio
$ ls -la /tmp/imsg-176-proof/tone.m4a
-rw-r--r--  8544  tone.m4a

Production helper path (AttachmentResolver.metadata + runConversionProcess)

On macOS with Homebrew ffmpeg on PATH, calling the patched AttachmentResolver.metadata(… convertUnsupported: true) for the generated CAF:

PROOF real_ffmpeg convertedPath=.../Library/Caches/imsg/converted-attachments/tone-19053d020182e391.m4a
PROOF real_ffmpeg convertedMime=audio/mp4
PROOF hung_status=143 hung_elapsed=0.626815583 seconds

143 is 128 + SIGTERM from the timeout path; hung exec sleep 30 returned in under 1s instead of blocking 30s.

Hung bound (exec sleep fixture)

$ # hung converter: #!/bin/sh + exec sleep 30; timeout 0.5s via production helper
hung_status=143 hung_elapsed≈0.63s process reaped

Real behavior proof

  • Behavior or issue addressed: Unbounded waitUntilExit on ffmpeg conversion could hang attachment metadata forever; conversions are now deadline-bounded and reaped.

  • Real environment tested: macOS arm64, ffmpeg 8.1.2 (Homebrew /opt/homebrew/bin/ffmpeg), imsg branch fix/ffmpeg-conversion-timeout at tip after monotonic/process-group fix.

  • Exact steps or command run after this patch:

    ffmpeg -y -f lavfi -i sine=frequency=440:duration=0.5 -c:a pcm_s16le tone.caf
ffmpeg -nostdin -y -i tone.caf -c:a aac -b:a 128k tone.m4a
# plus AttachmentResolver.metadata(convertUnsupported: true) on tone.caf
# plus runConversionProcess(exec sleep 30, timeout: 0.5)

  • Evidence after fix: terminal output above; real m4a produced; hung path returns status 143 in ~0.63s.

  • Observed result after fix: Successful CAF→m4a conversion via the same args and metadata path; deliberately hung converter exits within the configured bound instead of sleeping 30s.

  • What was not tested: Live Messages.app chat history with SIP disabled; conversions that legitimately need more than 60s.

Test plan

  • swift test --filter attachmentResolverConversionTimesOutOnHungConverter
  • swift test --filter attachmentResolverConversionDoesNotBlockOnConverterOutput
  • Real ffmpeg CAF→m4a on this machine
  • Production helper hung path (status 143, sub-second)

@SebTardif
fix(media): bound ffmpeg conversion wait with a timeout
cee8f07 
runConversionProcess called waitUntilExit with no deadline, so a hung
ffmpeg blocked attachment conversion indefinitely. Poll isRunning with
a default 60s timeout and terminate/kill the process when it expires.

Signed-off-by: Sebastien Tardif <[email protected]>
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 12, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 6:29 PM ET / 22:29 UTC.

Summary
Adds a 60-second monotonic timeout around ffmpeg attachment conversion, escalates overdue process termination, and adds a deterministic hung-converter regression test.

Reproducibility: yes. at source level with high confidence: current main waits for ffmpeg indefinitely, and the PR provides a concrete hung-converter path plus after-fix live output showing bounded completion. The failing current-main path was not executed in this read-only Linux review environment.

Review metrics: 2 noteworthy metrics.

  • Patch scope: 2 files, 78 additions, 2 deletions. The changes remain focused on the conversion process helper and one regression test.
  • Process bounds: 60-second runtime, 0.5-second termination grace. These fixed values define the only material compatibility decision remaining before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] A legitimate conversion still running after 60 seconds will newly be terminated and reported as unavailable, while current main permits it to finish; the PR proof explicitly does not cover conversions exceeding that duration.

Maintainer options:

  1. Approve the fixed cutoff (recommended)
    Accept that conversions still running after 60 seconds will now terminate and return no converted attachment.
  2. Validate a different deadline
    Change the internal bound and provide slow-success plus hung-process evidence before merge.

Next step before merge

  • [P2] The conversion feature owner should accept the proposed 60-second cutoff or request a specifically adjusted deadline; there is no concrete automated repair to perform on the current patch.

Maintainer decision needed

  • Question: Should unsupported-media conversion adopt the proposed fixed 60-second maximum runtime?
  • Rationale: The patch correctly prevents indefinite blocking, but current behavior has no runtime ceiling and the repository has no established evidence that every legitimate CAF or GIF conversion completes within 60 seconds.
  • Likely owner: steipete — He introduced this conversion feature and authored its previous hang-related maintenance commit.
  • Options:
    • Accept 60 seconds (recommended): Land the patch and intentionally treat conversions exceeding 60 seconds as failed optional conversions.
    • Choose another bound: Select a longer or adaptive internal deadline and validate both a deliberately hung converter and a legitimate slow-success case before merge.

Security
Cleared: The patch changes only local child-process timeout handling and a test fixture, with no dependency, secret, permission, downloaded-code, or supply-chain changes.

Review details

Best possible solution:

Land a monotonic converter deadline after the feature owner confirms that 60 seconds is an acceptable maximum, or selects a better internal bound, while retaining the existing success-path and hung-process proof.

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

Yes, at source level with high confidence: current main waits for ffmpeg indefinitely, and the PR provides a concrete hung-converter path plus after-fix live output showing bounded completion. The failing current-main path was not executed in this read-only Linux review environment.

Is this the best way to solve the issue?

Yes, conditionally. A monotonic deadline with process reaping is a narrow and maintainable fix, but the feature owner should explicitly accept or adjust the proposed 60-second compatibility boundary.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This addresses an indefinite stall in an optional attachment-conversion path with limited blast radius.
  • merge-risk: 🚨 compatibility: Existing conversions that legitimately require more than 60 seconds would newly fail after merge.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies after-fix macOS terminal output for real CAF-to-M4A conversion through the production helper and a deliberately hung process returning within the configured bound.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-fix macOS terminal output for real CAF-to-M4A conversion through the production helper and a deliberately hung process returning within the configured bound.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: He introduced unsupported attachment conversion and authored the previous conversion-hang fix in the same helper. (role: feature introducer and recent area contributor; confidence: high; commits: 8dcb9d087b27, fa42a0a15e59; files: Sources/IMsgCore/AttachmentResolver.swift, Tests/IMsgCoreTests/MessageStoreAttachmentConversionTests.swift, Tests/IMsgCoreTests/UtilityTests.swift)
  • mfzeidan: The original unsupported-attachment conversion commit credits mfzeidan as a co-author, making them relevant to the intended conversion behavior. (role: feature co-author; confidence: medium; commits: 8dcb9d087b27; files: Sources/IMsgCore/AttachmentResolver.swift, Sources/IMsgCore/Models.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (2 earlier review cycles)
  • reviewed 2026-07-12T21:17:12.832Z sha cee8f07 :: needs real behavior proof before merge. :: [P2] Use a monotonic deadline for the timeout | [P2] Avoid leaking the hung converter child from the test
  • reviewed 2026-07-12T22:23:30.282Z sha 048e04d :: needs maintainer review before merge. :: none

SebTardif added 2 commits July 12, 2026 18:19
@SebTardif
fix(media): use monotonic conversion timeout and exec hung fixture
95a3337 
Bound converter waits with ContinuousClock so wall-clock jumps cannot
stretch the limit, best-effort terminate the process group, and use
exec sleep in the hung fixture so the Process PID is the sleeper.

Signed-off-by: Sebastien Tardif <[email protected]>
@SebTardif
retrigger: refresh real behavior proof
048e04d
@SebTardif

SebTardif commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated proof: monotonic ContinuousClock deadline, process-group terminate, exec-based hung fixture, and real ffmpeg CAF→m4a terminal output plus production helper hung bound (status 143 in ~0.63s).

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 12, 2026
@steipete

steipete commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Maintainer repair pushed in 9b45553 and 8981820.

The repair moves the timeout regression out of an over-limit test file, strengthens the exact timeout result, and fixes descendant cleanup when the converter leader exits on SIGTERM while a child ignores it.

Exact-head proof at 8981820:

  • make lint — pass; 12 pre-existing warnings, 0 serious violations.
  • make test — pass; 479 tests in 3 suites.
  • make build — pass; universal imsg (arm64 x86_64) and helper (arm64e arm64 x86_64) built and signed.
  • swift test --filter AttachmentResolverProcessTests — 2/2 pass.
  • Descendant regression repeated 5 additional times with --skip-build — 5/5 pass.
  • Runtime probe confirmed the launched converter is its process-group leader on this macOS Foundation runtime (pid == pgid); the non-exec wrapper test confirms its TERM-ignoring descendant is gone before return.

AutoReview:

  • Local test-move repair: clean, no accepted/actionable findings.
  • Process-group repair after two accepted test/process findings: clean, no accepted/actionable findings.
  • Final branch pass repeated a premise contradicted by the runtime probe and 7/7 descendant-test passes (that Process inherits the caller group); rejected with evidence. The getpgid(pid) == pid guard safely falls back to direct-PID termination if a runtime does not create the group.

Live built CLI attachment-history proof remains blocked on this worker because its parent process lacks Full Disk Access. The contributor's PR body retains real ffmpeg and production-helper proof.

@steipete
steipete merged commit 17e3697 into openclaw:main Jul 16, 2026
@steipete

steipete commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Landed with the 60-second converter ceiling accepted as the default: an optional conversion failure is strictly better than an indefinite CLI/RPC hang, the constant is documented and trivially adjustable, and the process-tree cleanup is regression-tested against TERM-ignoring descendants. Changelog entry added on main.

This was referenced Jul 17, 2026
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SebTardif @steipete