-
-
Notifications
You must be signed in to change notification settings - Fork 397
fix: resolve issue with S3 objects not deleting on /full route #269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d4f27ec
580fcd0
3dc4b53
92e2d14
1a28af2
2fe69d4
aa12c5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,11 @@ type MyClient struct { | |
| s *server | ||
| } | ||
|
|
||
| // ensureS3ClientForUser loads S3 config from DB and initializes client if not already present (lazy init for reconnect-after-restart) | ||
| func ensureS3ClientForUser(userID string) { | ||
| GetS3Manager().EnsureClientFromDB(userID) | ||
| } | ||
|
|
||
| func sendToGlobalWebHook(jsonData []byte, token string, userID string) { | ||
| jsonDataStr := string(jsonData) | ||
|
|
||
|
|
@@ -295,49 +300,7 @@ func (s *server) connectOnStartup() { | |
|
|
||
| // Initialize S3 client if configured | ||
| go func(userID string) { | ||
| var s3Config struct { | ||
| Enabled bool `db:"s3_enabled"` | ||
| Endpoint string `db:"s3_endpoint"` | ||
| Region string `db:"s3_region"` | ||
| Bucket string `db:"s3_bucket"` | ||
| AccessKey string `db:"s3_access_key"` | ||
| SecretKey string `db:"s3_secret_key"` | ||
| PathStyle bool `db:"s3_path_style"` | ||
| PublicURL string `db:"s3_public_url"` | ||
| RetentionDays int `db:"s3_retention_days"` | ||
| } | ||
|
|
||
| err := s.db.Get(&s3Config, ` | ||
| SELECT s3_enabled, s3_endpoint, s3_region, s3_bucket, | ||
| s3_access_key, s3_secret_key, s3_path_style, | ||
| s3_public_url, s3_retention_days | ||
| FROM users WHERE id = $1`, userID) | ||
|
|
||
| if err != nil { | ||
| log.Error().Err(err).Str("userID", userID).Msg("Failed to get S3 config") | ||
| return | ||
| } | ||
|
|
||
| if s3Config.Enabled { | ||
| config := &S3Config{ | ||
| Enabled: s3Config.Enabled, | ||
| Endpoint: s3Config.Endpoint, | ||
| Region: s3Config.Region, | ||
| Bucket: s3Config.Bucket, | ||
| AccessKey: s3Config.AccessKey, | ||
| SecretKey: s3Config.SecretKey, | ||
| PathStyle: s3Config.PathStyle, | ||
| PublicURL: s3Config.PublicURL, | ||
| RetentionDays: s3Config.RetentionDays, | ||
| } | ||
|
|
||
| err = GetS3Manager().InitializeS3Client(userID, config) | ||
| if err != nil { | ||
| log.Error().Err(err).Str("userID", userID).Msg("Failed to initialize S3 client on startup") | ||
| } else { | ||
| log.Info().Str("userID", userID).Msg("S3 client initialized on startup") | ||
| } | ||
| } | ||
| GetS3Manager().EnsureClientFromDB(userID) | ||
| }(txtid) | ||
| } | ||
| } | ||
|
|
@@ -461,6 +424,9 @@ func (s *server) startClient(userID string, textjid string, token string, subscr | |
| } | ||
| clientManager.SetHTTPClient(userID, httpClient) | ||
|
|
||
| // Initialize S3 client if configured (needed when user reconnects after container restart - connectOnStartup only runs for connected=1) | ||
| GetS3Manager().EnsureClientFromDB(userID) | ||
|
|
||
| if client.Store.ID == nil { | ||
| // No ID stored, new login | ||
| qrChan, err := client.GetQRChannel(context.Background()) | ||
|
|
@@ -817,6 +783,11 @@ func (mycli *MyClient) myEventHandler(rawEvt interface{}) { | |
| s3Config.MediaDelivery = myuserinfo.(Values).Get("MediaDelivery") | ||
| } | ||
|
|
||
| // Lazy init S3 client if needed (handles reconnect-after-restart when connectOnStartup skipped this user) | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) | ||
| } | ||
|
|
||
| postmap["type"] = "Message" | ||
| dowebhook = 1 | ||
| metaParts := []string{fmt.Sprintf("pushname: %s", evt.Info.PushName), fmt.Sprintf("timestamp: %s", evt.Info.Timestamp)} | ||
|
|
@@ -867,6 +838,7 @@ func (mycli *MyClient) myEventHandler(rawEvt interface{}) { | |
|
|
||
| // Process S3 upload if enabled | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call to Please remove this line and the similar ones for other media types to improve clarity and avoid unnecessary function calls. |
||
| // Get sender JID for inbox/outbox determination | ||
| isIncoming := evt.Info.IsFromMe == false | ||
| contactJID := evt.Info.Sender.String() | ||
|
|
@@ -955,6 +927,7 @@ func (mycli *MyClient) myEventHandler(rawEvt interface{}) { | |
|
|
||
| // Process S3 upload if enabled | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) | ||
| // Get sender JID for inbox/outbox determination | ||
| isIncoming := evt.Info.IsFromMe == false | ||
| contactJID := evt.Info.Sender.String() | ||
|
|
@@ -1048,6 +1021,7 @@ func (mycli *MyClient) myEventHandler(rawEvt interface{}) { | |
|
|
||
| // Process S3 upload if enabled | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) | ||
| // Get sender JID for inbox/outbox determination | ||
| isIncoming := evt.Info.IsFromMe == false | ||
| contactJID := evt.Info.Sender.String() | ||
|
|
@@ -1130,6 +1104,7 @@ func (mycli *MyClient) myEventHandler(rawEvt interface{}) { | |
|
|
||
| // Process S3 upload if enabled | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) | ||
| // Get sender JID for inbox/outbox determination | ||
| isIncoming := evt.Info.IsFromMe == false | ||
| contactJID := evt.Info.Sender.String() | ||
|
|
@@ -1212,6 +1187,7 @@ func (mycli *MyClient) myEventHandler(rawEvt interface{}) { | |
|
|
||
| // if using S3 (same stream as other media) | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) | ||
| isIncoming := evt.Info.IsFromMe == false | ||
| contactJID := evt.Info.Sender.String() | ||
| if evt.Info.IsGroup { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a subtle bug in the
ifcondition on the next line (if err == nil && s3Enabled). Theerrvariable it checks is from theos.RemoveAlloperation on line 5296. If local file deletion fails, this will prevent S3 objects from being deleted.To fix this, the condition on line 5303 should be changed to just
if s3Enabled. Thes3Enabledflag will correctly befalseif the initial database query for it failed, so checkingerrhere is both incorrect and unnecessary.