Bug: downloaded media files deleted before async consumer reads them

[ex-takashima]

Description

Channel handlers (telegram.go, line.go, slack.go, onebot.go) download media files to a local temp directory and then call HandleMessage, which publishes the message (including local file paths) to an async message bus via PublishInbound. However, each handler also has a defer os.Remove() that deletes those files when the handler function returns — before the agent loop goroutine has a chance to read them.

This race condition was originally identified by @DevEverything01 in #543.

Impact

Voice transcription, image analysis, and document processing via the agent loop silently fail because the files no longer exist by the time they are accessed.

Proposed Fix

1. Remove the premature defer os.Remove() from the four affected channel handlers.
2. Introduce a background MediaCleaner that periodically scans the temp media directory and removes files older than 30 minutes, preventing unbounded temp file accumulation.

Note: discord.go is not affected — it processes voice files synchronously before returning, so there is no race condition.

[nikolasdehor]

This is a classic TOCTOU race condition. The channel handler downloads media to a temp file, publishes the message path to the async bus, then defer os.Remove() fires when the handler returns — before the agent goroutine reads the file.

The proposed two-part fix is correct:

1. Remove defer os.Remove() from telegram.go, line.go, slack.go, and onebot.go handlers. The file lifecycle should be owned by the consumer (agent loop), not the producer (channel handler).

2. Background MediaCleaner — a goroutine with a ticker that scans the temp media directory and removes files older than a configurable TTL (30 min is reasonable). This prevents unbounded disk usage.

A simpler alternative to step 2: have the agent loop call os.Remove() after it finishes processing each media file. This is more deterministic than time-based cleanup:

func (a *Agent) processMediaMessage(msg Message) {
    defer func() {
        if msg.MediaPath != "" {
            os.Remove(msg.MediaPath)
        }
    }()
    // ... process file
}

The background cleaner would then only serve as a safety net for orphaned files (e.g. if the agent crashes mid-processing).

Note that the issue body correctly identifies that discord.go is not affected since it processes voice synchronously. Worth adding a code comment in discord.go explaining why it doesn't need the same treatment, to prevent future regressions.

Related: #543 (original report by @DevEverything01).

[ex-takashima]

Thanks for the detailed analysis, @nikolasdehor — completely agree with the TOCTOU characterization.

Regarding the agent-loop-side cleanup suggestion: I looked into it and it's definitely feasible — msg.Media is already available in the Run() loop, and processMessage() has a clear completion point where os.Remove() could be called.

However, I think that's better suited as a separate enhancement rather than part of this bug fix PR (#620). It touches the core agent loop processing path, which has a broader impact surface and would benefit from review by maintainers more familiar with that area. The current PR keeps the fix minimal: remove the premature defer, add a configurable background cleaner as a safety net.

Happy to open a follow-up issue for the deterministic cleanup if that would be useful.

[alexhoshina]

Hello, we are currently working on a channel refactoring plan. Your issue and PR are included in our refactoring plan. In PR #662, I have created a new Media mechanism that uses a file registry bound to a scope, allowing the file lifecycle to be determined by the consumer.
However, I am still exploring possible better designs for the Media mechanism, and I currently do not support file persistence. If you are willing to help, we can have further discussions.

[ex-takashima]

Thanks for the heads-up about PR #662, @alexhoshina. I took a look — the scope-based MediaStore design makes sense as the primary cleanup mechanism.

One thought: since the registry is in-memory, a crash or restart would leave orphaned files with no way to reclaim them. PR #620's TTL-based scanner could serve as a lightweight safety net for that scenario. Happy to leave it open or close it — your call.

On a different note, my original interest was in contributing around the LINE channel area (I've also been watching PR #500). With #662 restructuring the channel system, is there a specific area — LINE-related or otherwise — where help would be most useful right now?

[nikolasdehor]

@ex-takashima Your approach of keeping #620 minimal (remove premature defer + background cleaner) and deferring the agent-loop-side cleanup to a follow-up is the right call. Agreed that touching the core Run() loop is a broader change that deserves its own review.

@alexhoshina The scope-based FileMediaStore in #662 is the right long-term design. As @ex-takashima noted, the TTL scanner from #620 could serve as a crash-recovery safety net for orphaned files when the in-memory registry is lost on restart. Worth considering as a complementary mechanism rather than a replacement.

Good to see this converging. The #662 refactor is the clear path forward for media lifecycle management.

[alexhoshina]

hi @ex-takashima , I have opened a refactor branch. Perhaps we can add your MediaCleaner as a supplement to MediaStore in the refactor branch, along with the specific implementation of the Line channel. Maybe you could take a look to see if there are any areas that need modification, as I am not very familiar with Line.

[alexhoshina]

Fixed