Skip to content

Comments

fix: remove premature media file cleanup in channel handlers#543

Open
DevEverything01 wants to merge 3 commits intosipeed:mainfrom
DevEverything01:fix/remove-premature-media-file-cleanup
Open

fix: remove premature media file cleanup in channel handlers#543
DevEverything01 wants to merge 3 commits intosipeed:mainfrom
DevEverything01:fix/remove-premature-media-file-cleanup

Conversation

@DevEverything01
Copy link
Contributor

Problem

Media files (photos, voice messages, audio, documents) downloaded by channel handlers are immediately deleted via defer when handleMessage returns. However, HandleMessage publishes the message (including media file paths) to an async message bus (PublishInbound), and the agent consumes it in a separate goroutine.

Result: By the time the agent goroutine tries to access the media files, they have already been deleted. Voice/image/document processing is effectively non-functional.

Root Cause

// handleMessage downloads files to temp paths...
defer func() {
    for _, file := range localFiles {
        os.Remove(file)  // ← deleted BEFORE agent consumes them
    }
}()
// ...then publishes to async bus
c.HandleMessage(...)  // → PublishInbound(msg) → channel send
return nil            // ← defer fires here, files gone

Fix

Remove the premature defer cleanup from all 4 affected channels:

  • pkg/channels/telegram.go
  • pkg/channels/discord.go
  • pkg/channels/slack.go
  • pkg/channels/line.go

Media files live in os.TempDir()/picoclaw_media/ with UUID-prefixed names, managed by the OS temp directory lifecycle.

Verification

  • go build ./...
  • go test ./pkg/channels/...
  • go vet ./...

Media files (photos, voice, audio, documents) downloaded by channel
handlers were immediately deleted via defer when handleMessage returned.
However, HandleMessage publishes to an async message bus, so the agent
goroutine would attempt to access already-deleted files.

Remove the eager defer cleanup from telegram, discord, slack, and line
channels. Temp files in os.TempDir()/picoclaw_media/ are managed by the
OS temp directory lifecycle.

Fixes media/voice/document processing being non-functional across all
affected channels.
- Remove defer cleanup of LocalFiles in onebot channel handler, same
  async race condition as the other channels.
- Remove the now-unused LocalFiles field from parseMessageResult.
- Add downloaded audio files to mediaPaths in discord channel so they
  are tracked (previously only tracked via the now-removed localFiles).
@ex-takashima
Copy link
Contributor

Thanks for identifying this race condition — the analysis in the PR description is spot on. 👍

I ran into the same issue and built on your findings in #620, which takes a slightly different approach: instead of only removing the defer cleanup, it also adds a background MediaCleaner that periodically removes files older than 30 minutes from the temp directory. This way we avoid the race condition without risking unbounded temp file accumulation over time.

Would love to hear your thoughts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants