preserve media store across config reloads#2040
preserve media store across config reloads#2040Harshit-shrivastav wants to merge 1 commit intosipeed:mainfrom
Conversation
## Bug Fix Prevents loss of uploaded file references when using `/reload` command. ## Problem When calling `/reload`, a new `FileMediaStore` was created with an empty `refs` map, causing all previously uploaded files to become inaccessible with error: media store: unknown ref: media://... ## Solution Reuse the existing `MediaStore` instance instead of recreating it on every reload. The media store is now only created once on initial startup. ## Changes - `pkg/gateway/gateway.go` (line 468): Wrap media store creation in `if runningServices.MediaStore == nil` ## Testing 1. Start picoclaw with Telegram enabled 2. Upload a file to the bot 3. Run `/reload` command 4. Ask bot to read the previously uploaded file 5. File should now be accessible (previously would fail with "unknown ref" error) ## Impact - Users can now safely use `/reload` without breaking file uploads - No breaking changes - Backward compatible
|
|
There was a problem hiding this comment.
Pull request overview
Fixes /reload behavior in the gateway so previously uploaded media refs remain resolvable by avoiding recreation of the FileMediaStore during service restarts.
Changes:
- Guard
FileMediaStorecreation inrestartServices()so the existing store instance is reused across reloads.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if runningServices.MediaStore == nil { | ||
| runningServices.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ | ||
| Enabled: cfg.Tools.MediaCleanup.Enabled, | ||
| MaxAge: time.Duration(cfg.Tools.MediaCleanup.MaxAge) * time.Minute, | ||
| Interval: time.Duration(cfg.Tools.MediaCleanup.Interval) * time.Minute, | ||
| }) | ||
| if fms, ok := runningServices.MediaStore.(*media.FileMediaStore); ok { | ||
| fms.Start() | ||
| } | ||
| } |
There was a problem hiding this comment.
stopAndCleanupServices(..., isReload=true) still calls fms.Stop() for the existing FileMediaStore, but this restart path only creates/starts the media store when runningServices.MediaStore == nil. Since FileMediaStore.Stop() permanently closes the stop channel and Start() is guarded by startOnce, cleanup cannot be restarted after a /reload, leaving media cleanup disabled (and new cleanup settings ignored). Consider not stopping the media store on reload, or recreating it while migrating existing refs / adding a reconfigurable cleaner.
| if runningServices.MediaStore == nil { | |
| runningServices.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ | |
| Enabled: cfg.Tools.MediaCleanup.Enabled, | |
| MaxAge: time.Duration(cfg.Tools.MediaCleanup.MaxAge) * time.Minute, | |
| Interval: time.Duration(cfg.Tools.MediaCleanup.Interval) * time.Minute, | |
| }) | |
| if fms, ok := runningServices.MediaStore.(*media.FileMediaStore); ok { | |
| fms.Start() | |
| } | |
| } | |
| runningServices.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ | |
| Enabled: cfg.Tools.MediaCleanup.Enabled, | |
| MaxAge: time.Duration(cfg.Tools.MediaCleanup.MaxAge) * time.Minute, | |
| Interval: time.Duration(cfg.Tools.MediaCleanup.Interval) * time.Minute, | |
| }) | |
| if fms, ok := runningServices.MediaStore.(*media.FileMediaStore); ok { | |
| fms.Start() | |
| } |
|
|
||
| if runningServices.MediaStore == nil { | ||
| runningServices.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ | ||
| Enabled: cfg.Tools.MediaCleanup.Enabled, | ||
| MaxAge: time.Duration(cfg.Tools.MediaCleanup.MaxAge) * time.Minute, | ||
| Interval: time.Duration(cfg.Tools.MediaCleanup.Interval) * time.Minute, | ||
| }) | ||
| if fms, ok := runningServices.MediaStore.(*media.FileMediaStore); ok { | ||
| fms.Start() | ||
| } | ||
| } |
There was a problem hiding this comment.
This block’s indentation/bracing doesn’t match Go formatting (spaces instead of tabs and the closing } alignment). Please run gofmt on this section to keep the file consistent and avoid noisy diffs.
| if runningServices.MediaStore == nil { | |
| runningServices.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ | |
| Enabled: cfg.Tools.MediaCleanup.Enabled, | |
| MaxAge: time.Duration(cfg.Tools.MediaCleanup.MaxAge) * time.Minute, | |
| Interval: time.Duration(cfg.Tools.MediaCleanup.Interval) * time.Minute, | |
| }) | |
| if fms, ok := runningServices.MediaStore.(*media.FileMediaStore); ok { | |
| fms.Start() | |
| } | |
| } | |
| if runningServices.MediaStore == nil { | |
| runningServices.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ | |
| Enabled: cfg.Tools.MediaCleanup.Enabled, | |
| MaxAge: time.Duration(cfg.Tools.MediaCleanup.MaxAge) * time.Minute, | |
| Interval: time.Duration(cfg.Tools.MediaCleanup.Interval) * time.Minute, | |
| }) | |
| if fms, ok := runningServices.MediaStore.(*media.FileMediaStore); ok { | |
| fms.Start() | |
| } | |
| } |
yinwm
left a comment
yinwm
left a comment
There was a problem hiding this comment.
Thanks for the fix — the problem is real and the approach is correct.
Two things to address before we can merge:
1. Indentation (blocking Linter)
The new code uses spaces instead of tabs. Please run gofmt -w pkg/gateway/gateway.go to fix, or re-indent using tabs.
2. Rebase needed
This PR is based on pre-#2082 code. After rebase onto latest main, you'll also need to adjust the context since restartServices was refactored in #2082. The MediaStore creation block itself is unchanged, so the fix should be straightforward — just wrap it in the if nil guard as you did.
Once these are fixed, I'll approve and merge.
Bug Fix
Prevents loss of uploaded file references when using
/reloadcommand.Problem
When calling
/reload, a newFileMediaStorewas created with an emptyrefsmap, causing all previously uploaded files to become inaccessible with error: media store: unknown ref: media://...Solution
Reuse the existing
MediaStoreinstance instead of recreating it on every reload. The media store is now only created once on initial startup.Changes
pkg/gateway/gateway.go(line 468): Wrap media store creation inif runningServices.MediaStore == nilTesting
/reloadcommandImpact
/reloadwithout breaking file uploads📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist