From 19eb88dd3861bc28b83c8162191b7c4192edfbe5 Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Sun, 22 Mar 2026 11:51:33 +0900 Subject: [PATCH] fix: keep URL-downloaded temp file alive for MediaStore resolution MediaStore.Store() only saves a path reference, not a copy. The previous defer os.Remove() deleted the file before Telegram could read it, causing silent send failures. Now downloads go to media.TempDir()/sendfile/ and persist until CleanupTempFiles() removes them after 1 hour. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/tools/send_file.go | 33 ++++++++++++++++++++++----------- pkg/tools/send_file_test.go | 35 ++++++++++++++++++----------------- 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/pkg/tools/send_file.go b/pkg/tools/send_file.go index 6e737ce5f..b9655fa0b 100644 --- a/pkg/tools/send_file.go +++ b/pkg/tools/send_file.go @@ -21,11 +21,17 @@ import ( ) const ( - sendFileTempDir = "/tmp/picoclaw-sendfile" sendFileTempMaxAge = 1 * time.Hour sendFileDownloadLimit = 50 * 1024 * 1024 // 50 MB download limit ) +// sendFileTempDir returns the directory for URL-downloaded temp files. +// Uses the shared media temp directory so that MediaStore can resolve paths +// after Execute returns (MediaStore only stores a path reference, not a copy). +func sendFileTempDir() string { + return filepath.Join(media.TempDir(), "sendfile") +} + // SendFileTool allows the LLM to send a local file (image, document, etc.) // to the user on the current chat channel via the MediaStore pipeline. type SendFileTool struct { @@ -118,15 +124,17 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // Handle URL downloads var resolved string - var tempFile string // non-empty when we downloaded a URL + var isURL bool if isHTTPURL(path) { var err error resolved, err = downloadToTemp(ctx, path) if err != nil { return ErrorResult(fmt.Sprintf("download failed: %v", err)) } - tempFile = resolved - defer os.Remove(tempFile) + isURL = true + // Do NOT delete the temp file here — MediaStore only stores a path + // reference, so the file must survive until the channel sends it. + // CleanupTempFiles() handles stale files periodically. } else { var err error resolved, err = validatePathWithAllowPaths(path, t.workspace, t.restrict, t.allowPaths) @@ -151,7 +159,7 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe filename, _ := args["filename"].(string) if filename == "" { - if tempFile != "" { + if isURL { filename = filenameFromURL(path) } else { filename = filepath.Base(resolved) @@ -178,10 +186,12 @@ func isHTTPURL(path string) bool { return strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") } -// downloadToTemp downloads a URL to a temporary file under sendFileTempDir. -// The caller is responsible for removing the returned file. +// downloadToTemp downloads a URL to a temporary file under sendFileTempDir(). +// The file must persist until the channel has sent it; cleanup is handled by +// CleanupTempFiles (periodic) rather than the caller. func downloadToTemp(ctx context.Context, rawURL string) (string, error) { - if err := os.MkdirAll(sendFileTempDir, 0o700); err != nil { + dir := sendFileTempDir() + if err := os.MkdirAll(dir, 0o700); err != nil { return "", fmt.Errorf("create temp dir: %w", err) } @@ -206,7 +216,7 @@ func downloadToTemp(ctx context.Context, rawURL string) (string, error) { return "", err } ext := extFromURL(rawURL) - tmpPath := filepath.Join(sendFileTempDir, fmt.Sprintf("dl_%x%s", randBytes, ext)) + tmpPath := filepath.Join(dir, fmt.Sprintf("dl_%x%s", randBytes, ext)) f, err := os.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0o600) if err != nil { @@ -254,7 +264,8 @@ func filenameFromURL(rawURL string) string { // CleanupTempFiles removes old temp files from the sendfile temp directory. // Files older than sendFileTempMaxAge are deleted. func CleanupTempFiles() { - entries, err := os.ReadDir(sendFileTempDir) + dir := sendFileTempDir() + entries, err := os.ReadDir(dir) if err != nil { return } @@ -268,7 +279,7 @@ func CleanupTempFiles() { continue } if info.ModTime().Before(cutoff) { - os.Remove(filepath.Join(sendFileTempDir, e.Name())) + os.Remove(filepath.Join(dir, e.Name())) } } } diff --git a/pkg/tools/send_file_test.go b/pkg/tools/send_file_test.go index 9ab5fc794..c8e62d5a1 100644 --- a/pkg/tools/send_file_test.go +++ b/pkg/tools/send_file_test.go @@ -239,7 +239,7 @@ func TestSendFileTool_URLDownloadHTTPError(t *testing.T) { } } -func TestSendFileTool_URLTempFileCleanedUp(t *testing.T) { +func TestSendFileTool_URLTempFilePersistsForMediaStore(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("data")) })) @@ -249,31 +249,32 @@ func TestSendFileTool_URLTempFileCleanedUp(t *testing.T) { tool := NewSendFileTool(t.TempDir(), false, 0, store) tool.SetContext("telegram", "chat123") - // Execute should download and clean up temp file - tool.Execute(context.Background(), map[string]any{ + result := tool.Execute(context.Background(), map[string]any{ "path": srv.URL + "/photo.jpg", }) - - // Verify no temp files remain with dl_ prefix in the temp dir - entries, _ := os.ReadDir(sendFileTempDir) - for _, e := range entries { - if strings.HasPrefix(e.Name(), "dl_") { - info, _ := e.Info() - // Only flag files created very recently (within this test) - if info != nil && time.Since(info.ModTime()) < 5*time.Second { - t.Errorf("temp file not cleaned up: %s", e.Name()) - } - } + if result.IsError { + t.Fatalf("unexpected error: %s", result.ForLLM) } + + // The temp file must still exist so MediaStore can resolve and read it + ref := result.Media[0] + resolved, err := store.Resolve(ref) + if err != nil { + t.Fatalf("Resolve failed: %v", err) + } + if _, err := os.Stat(resolved); err != nil { + t.Errorf("temp file should persist for MediaStore, but got: %v", err) + } + t.Cleanup(func() { os.Remove(resolved) }) } func TestCleanupTempFiles(t *testing.T) { - if err := os.MkdirAll(sendFileTempDir, 0o700); err != nil { + if err := os.MkdirAll(sendFileTempDir(), 0o700); err != nil { t.Fatal(err) } // Create an "old" temp file - oldFile := filepath.Join(sendFileTempDir, "dl_test_old.tmp") + oldFile := filepath.Join(sendFileTempDir(), "dl_test_old.tmp") if err := os.WriteFile(oldFile, []byte("old"), 0o600); err != nil { t.Fatal(err) } @@ -282,7 +283,7 @@ func TestCleanupTempFiles(t *testing.T) { os.Chtimes(oldFile, past, past) // Create a "new" temp file - newFile := filepath.Join(sendFileTempDir, "dl_test_new.tmp") + newFile := filepath.Join(sendFileTempDir(), "dl_test_new.tmp") if err := os.WriteFile(newFile, []byte("new"), 0o600); err != nil { t.Fatal(err) }