Fix SQLite deadlock with recurring tasks #1053
Conversation
WalkthroughSQLite connection pool limits increased from 1 to 4 in main.go. Attachment thumbnail creation now early-returns if disabled, batches "attachment_created" pubsub messages using a slice with fmt.Appendf, sending them after processing. Logs database OpenConnections during image processing and adjusts error handling to align with the batched message send approach. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant API as API Handler
participant Repo as AttachmentRepo
participant DB as SQLite DB
participant PubSub as Pub/Sub
API->>Repo: CreateMissingThumbnails(ctx)
alt thumbnails disabled
Repo-->>API: return 0
else thumbnails enabled
Repo->>DB: Query attachments missing thumbnails
DB-->>Repo: attachments list
loop per attachment
Repo->>Repo: CreateThumbnail / processThumbnailFromImage
Note right of Repo: Log DB OpenConnections before fetch group
Repo->>Repo: Build message body (fmt.Appendf)
Repo->>Repo: Append message to slice
end
Repo->>PubSub: Send batch of messages
PubSub-->>Repo: ack / error (logged, but do not fail)
Repo-->>API: return count of messages sent
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Security recommendations
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
715-726: Critical: Resource leak - PubSub topic opened but never closed.The
pubsub.OpenTopic()call on lines 715-722 opens a connection, but whenr.thumbnail.Enabledisfalse(lines 724-726), the function returns early without closing the topic. This creates a resource leak.Additionally, even in the success path (line 750), the topic is never explicitly closed.
Apply this diff to fix the resource leak:
func (r *AttachmentRepo) CreateMissingThumbnails(ctx context.Context, groupId uuid.UUID) (int, error) { + if !r.thumbnail.Enabled { + return 0, nil + } + attachments, err := r.db.Attachment.Query(). Where( attachment.HasItemWith(item.HasGroupWith(group.ID(groupId))), attachment.TypeNEQ("thumbnail"), ). All(ctx) if err != nil { return 0, err } pubsubString, err := utils.GenerateSubPubConn(r.pubSubConn, "thumbnails") if err != nil { log.Err(err).Msg("failed to generate pubsub connection string") + return 0, err } topic, err := pubsub.OpenTopic(ctx, pubsubString) if err != nil { log.Err(err).Msg("failed to open pubsub topic") + return 0, err } + defer func() { + if err := topic.Shutdown(ctx); err != nil { + log.Err(err).Msg("failed to close pubsub topic") + } + }() - if !r.thumbnail.Enabled { - return 0, nil - } - var messages []*pubsub.Message
🧹 Nitpick comments (2)
backend/internal/data/repo/repo_item_attachments.go (1)
732-732: Consider using fmt.Sprintf for clarity.While
fmt.Appendf(nil, ...)is slightly more efficient thanfmt.Sprintf(...), the performance gain is negligible for this use case, andfmt.Sprintfis more readable and idiomatic.Consider this simpler alternative:
if !attachment.QueryThumbnail().ExistX(ctx) { msg := &pubsub.Message{ - Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()), + Body: []byte(fmt.Sprintf("attachment_created:%s", attachment.ID.String())), Metadata: map[string]string{ "group_id": groupId.String(), "attachment_id": attachment.ID.String(), "title": attachment.Title, "path": attachment.Path, }, }backend/app/api/main.go (1)
143-145: Confirm connection pool size and security settings
- The DSN in conf_database.go already sets
journal_mode=WAL,busy_timeout=999, and foreign keys.- Verify that 4 max open/idle connections fits your workload; consider 2–3 if write contention persists and document your choice.
- Security: ensure the SQLite database file (
.data/homebox.db) has restrictive permissions to prevent unauthorized access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/main.go(1 hunks)backend/internal/data/repo/repo_item_attachments.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/internal/data/repo/repo_item_attachments.go (1)
backend/internal/data/ent/client.go (1)
Debug(123-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
🔇 Additional comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
864-864: LGTM! Helpful debug logging.The debug log showing
OpenConnectionswill be valuable for troubleshooting concurrency issues and verifying the connection pool fix.
| var messages []*pubsub.Message | ||
| for _, attachment := range attachments { | ||
| if r.thumbnail.Enabled { | ||
| if !attachment.QueryThumbnail().ExistX(ctx) { | ||
| if count > 0 && count%100 == 0 { | ||
| time.Sleep(2 * time.Second) | ||
| } | ||
| err = topic.Send(ctx, &pubsub.Message{ | ||
| Body: []byte(fmt.Sprintf("attachment_created:%s", attachment.ID.String())), | ||
| Metadata: map[string]string{ | ||
| "group_id": groupId.String(), | ||
| "attachment_id": attachment.ID.String(), | ||
| "title": attachment.Title, | ||
| "path": attachment.Path, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| log.Err(err).Msg("failed to send message to topic") | ||
| continue | ||
| } else { | ||
| count++ | ||
| } | ||
| if !attachment.QueryThumbnail().ExistX(ctx) { | ||
| msg := &pubsub.Message{ | ||
| Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()), | ||
| Metadata: map[string]string{ | ||
| "group_id": groupId.String(), | ||
| "attachment_id": attachment.ID.String(), | ||
| "title": attachment.Title, | ||
| "path": attachment.Path, | ||
| }, | ||
| } | ||
| messages = append(messages, msg) | ||
| } | ||
| } | ||
| for _, msg := range messages { | ||
| err = topic.Send(ctx, msg) | ||
| if err != nil { | ||
| log.Err(err).Msg("failed to send 'attachment_created' message to topic 'thumbnails'") | ||
| } | ||
| } | ||
|
|
||
| return count, nil | ||
| return len(messages), nil |
There was a problem hiding this comment.
Major: Silent message loss on send failures.
The current implementation collects messages in a slice and sends them individually (line 744). When topic.Send() fails (line 745), the error is only logged (lines 746-747), and the message is silently dropped. This means thumbnail generation requests can be lost without any indication to the user.
Consider one of these approaches:
- Fail fast: Return an error on first failure
- Aggregate errors: Collect all errors and return them
- Use batch send: If the pubsub implementation supports it
Apply this diff to fail fast on send errors:
for _, msg := range messages {
err = topic.Send(ctx, msg)
if err != nil {
log.Err(err).Msg("failed to send 'attachment_created' message to topic 'thumbnails'")
+ return 0, fmt.Errorf("failed to send message to thumbnails topic: %w", err)
}
}
return len(messages), nilAlternatively, to aggregate errors and return partial success count:
+ sent := 0
for _, msg := range messages {
err = topic.Send(ctx, msg)
if err != nil {
log.Err(err).Msg("failed to send 'attachment_created' message to topic 'thumbnails'")
+ } else {
+ sent++
}
}
- return len(messages), nil
+ if sent < len(messages) {
+ return sent, fmt.Errorf("failed to send %d of %d messages", len(messages)-sent, len(messages))
+ }
+ return sent, nil🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_item_attachments.go around lines 728-750 the
code currently logs failures from topic.Send and silently drops messages; change
this to fail fast by returning the error immediately when topic.Send(ctx, msg)
fails. Update the error handling to wrap/annotate the returned error with
context (e.g., group_id and attachment_id or the message body) and return the
current count of successfully queued messages or simply return the error
(depending on calling conventions), instead of only logging; ensure no message
send failure is ignored.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
743-751: Don’t silently drop failed sends; return partial/fail-fast errors and close the topicThis was flagged earlier; echoing with a concrete fix. Also consider returning the actual sent count, not len(messages).
- for _, msg := range messages { - err = topic.Send(ctx, msg) - if err != nil { - log.Err(err).Msg("failed to send 'attachment_created' message to topic 'thumbnails'") - } - } - - return len(messages), nil + sent := 0 + for _, msg := range messages { + if err := topic.Send(ctx, msg); err != nil { + log.Err(err). + Str("group_id", groupId.String()). + Str("attachment_id", msg.Metadata["attachment_id"]). + Msg("failed to send 'attachment_created' message to topic 'thumbnails'") + } else { + sent++ + } + } + if sent < len(messages) { + return sent, fmt.Errorf("failed to send %d of %d messages to 'thumbnails'", len(messages)-sent, len(messages)) + } + return sent, nil
🧹 Nitpick comments (2)
backend/internal/data/repo/repo_item_attachments.go (2)
724-726: Early-return check is too late; move it to function start and handle topic lifecycleCurrently you:
- Query DB and open pubsub even when thumbnails are disabled.
- Risk a nil-pointer deref later if OpenTopic fails (you log but keep using topic).
- Never close the topic (resource leak).
Action:
- Move the check to the top of the function.
- Only open the topic if there’s at least one message to send.
- Treat pubsub setup errors as fatal.
- Always shutdown the topic.
Apply within this hunk (remove here; add at top as shown below):
- if !r.thumbnail.Enabled { - return 0, nil - }Then restructure the function like:
func (r *AttachmentRepo) CreateMissingThumbnails(ctx context.Context, groupId uuid.UUID) (int, error) { if !r.thumbnail.Enabled { return 0, nil } // ... build messages first if len(messages) == 0 { return 0, nil } pubsubString, err := utils.GenerateSubPubConn(r.pubSubConn, "thumbnails") if err != nil { return 0, fmt.Errorf("generate pubsub conn: %w", err) } topic, err := pubsub.OpenTopic(ctx, pubsubString) if err != nil { return 0, fmt.Errorf("open pubsub topic: %w", err) } defer topic.Shutdown(ctx) // ... send messages with robust error handling (see next comment) }
441-579: Keep DB transactions short; avoid holding a Tx across heavy I/O/CPUCreateThumbnail holds a transaction while decoding images and uploading blobs. With SQLite’s locking, long-lived transactions increase lock contention even with a larger connection pool.
Refactor to:
- Do decoding/thumbnail generation and blob upload outside a DB tx.
- Use a short tx only to insert the thumbnail row and link it to the attachment.
This reduces deadlock risk and improves concurrency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/internal/data/ent/client.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/external.gois excluded by!backend/internal/data/ent/**
📒 Files selected for processing (1)
backend/internal/data/repo/repo_item_attachments.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
🔇 Additional comments (2)
backend/internal/data/repo/repo_item_attachments.go (2)
446-446: Good: propagate transaction open errorReturning the tx error instead of nil is correct.
733-738: Security: metadata may expose sensitive infoYou publish user-provided title and storage path. If the bus/subscribers aren’t fully trusted, this can leak PII or internal paths. Consider minimizing or hashing path, and ensure topic ACLs/log retention policies are restrictive.
| if !attachment.QueryThumbnail().ExistX(ctx) { | ||
| msg := &pubsub.Message{ | ||
| Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()), | ||
| Metadata: map[string]string{ | ||
| "group_id": groupId.String(), | ||
| "attachment_id": attachment.ID.String(), | ||
| "title": attachment.Title, | ||
| "path": attachment.Path, | ||
| }, | ||
| } | ||
| messages = append(messages, msg) | ||
| } |
There was a problem hiding this comment.
Avoid Ent’s ExistX (panics on error); handle existence check errors
ExistX will panic on DB errors. Use Exist(ctx) and handle err.
- if !attachment.QueryThumbnail().ExistX(ctx) {
+ exists, err := attachment.QueryThumbnail().Exist(ctx)
+ if err != nil {
+ log.Err(err).
+ Str("group_id", groupId.String()).
+ Str("attachment_id", attachment.ID.String()).
+ Msg("failed to check thumbnail existence")
+ return 0, err
+ }
+ if !exists {
msg := &pubsub.Message{
Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()),
Metadata: map[string]string{
"group_id": groupId.String(),
"attachment_id": attachment.ID.String(),
"title": attachment.Title,
"path": attachment.Path,
},
}
messages = append(messages, msg)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !attachment.QueryThumbnail().ExistX(ctx) { | |
| msg := &pubsub.Message{ | |
| Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()), | |
| Metadata: map[string]string{ | |
| "group_id": groupId.String(), | |
| "attachment_id": attachment.ID.String(), | |
| "title": attachment.Title, | |
| "path": attachment.Path, | |
| }, | |
| } | |
| messages = append(messages, msg) | |
| } | |
| exists, err := attachment.QueryThumbnail().Exist(ctx) | |
| if err != nil { | |
| log.Err(err). | |
| Str("group_id", groupId.String()). | |
| Str("attachment_id", attachment.ID.String()). | |
| Msg("failed to check thumbnail existence") | |
| return 0, err | |
| } | |
| if !exists { | |
| msg := &pubsub.Message{ | |
| Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()), | |
| Metadata: map[string]string{ | |
| "group_id": groupId.String(), | |
| "attachment_id": attachment.ID.String(), | |
| "title": attachment.Title, | |
| "path": attachment.Path, | |
| }, | |
| } | |
| messages = append(messages, msg) | |
| } |
🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_item_attachments.go lines 730 to 741, replace
the use of attachment.QueryThumbnail().ExistX(ctx) with
attachment.QueryThumbnail().Exist(ctx) and properly handle the returned error.
Check if an error occurred during the existence check, and handle it accordingly
instead of letting the code panic. Proceed with the logic only if no error
occurs and the thumbnail exists or not.
|
|
||
| // Get the group for uploading the thumbnail | ||
| log.Debug().Msg(fmt.Sprintf("OpenConnections = %d", r.db.Debug().Sql().Stats().OpenConnections)) | ||
| group, err := r.db.Group.Get(ctx, groupId) |
There was a problem hiding this comment.
Compile-time issue: invalid call chain to DB stats
ent.Client doesn’t expose Sql(); use DB() to access database/sql stats.
- log.Debug().Msg(fmt.Sprintf("OpenConnections = %d", r.db.Debug().Sql().Stats().OpenConnections))
+ log.Debug().Int("open_connections", r.db.DB().Stats().OpenConnections).Msg("DB stats")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| group, err := r.db.Group.Get(ctx, groupId) | |
| // Replace the invalid Sql() call on ent.Client with DB() | |
| log.Debug(). | |
| Int("open_connections", r.db.DB().Stats().OpenConnections). | |
| Msg("DB stats") | |
| group, err := r.db.Group.Get(ctx, groupId) |
🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_item_attachments.go at line 865, the call
chain to access database statistics is incorrect because ent.Client does not
have a Sql() method. Replace the use of Sql() with DB() to correctly access the
underlying database/sql instance and its stats. Adjust the code to call DB() on
the ent.Client or relevant DB object instead of Sql().
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/internal/data/repo/repo_item_attachments.go (3)
644-655: Replace ExistX with Exist to handle errors gracefully.
ExistXpanics on database errors (line 644), which can crash the application. This is particularly problematic given the PR's goal of fixing concurrency issues with SQLite.Apply this diff to handle errors properly:
var messages []*pubsub.Message for _, attachment := range attachments { - if !attachment.QueryThumbnail().ExistX(ctx) { + exists, err := attachment.QueryThumbnail().Exist(ctx) + if err != nil { + log.Err(err). + Str("group_id", groupId.String()). + Str("attachment_id", attachment.ID.String()). + Msg("failed to check thumbnail existence") + return 0, err + } + if !exists { msg := &pubsub.Message{ Body: fmt.Appendf(nil, "attachment_created:%s", attachment.ID.String()),Based on learnings.
657-662: Propagate send failures instead of silently continuing.When
topic.Send()fails (line 658-659), the error is only logged and processing continues. This means thumbnail generation requests can be silently lost, contradicting the function's return value which reports all messages as sent.Apply this diff to fail fast on send errors:
for _, msg := range messages { err = topic.Send(ctx, msg) if err != nil { log.Err(err).Msg("failed to send 'attachment_created' message to topic 'thumbnails'") + return 0, fmt.Errorf("failed to send message to thumbnails topic: %w", err) } } return len(messages), nilBased on learnings.
778-778: Fix invalid method call: use DB() instead of Sql().The call chain
r.db.Debug().Sql().Stats()is incorrect becauseent.Clientdoes not expose aSql()method. This will cause a compilation error.Apply this diff to use the correct method:
- log.Debug().Msg(fmt.Sprintf("OpenConnections = %d", r.db.Debug().Sql().Stats().OpenConnections)) + log.Debug().Int("open_connections", r.db.DB().Stats().OpenConnections).Msg("DB connection stats")Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/internal/data/repo/repo_item_attachments.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Backend Server Tests / Go
backend/internal/data/repo/repo_item_attachments.go
[failure] 447-447:
Error return value of tx.Rollback is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: build (linux/arm/v7)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Frontend Tests / Lint
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
🔇 Additional comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
610-615: LGTM: Transaction commit handling is correct.The simplified error handling after commit aligns with the deferred rollback pattern introduced earlier. The transaction will be properly rolled back by the defer if commit fails.
|
|
||
| // Cleanup: always rollback the transaction and ignore errors to prevent leaving connections open | ||
| defer func() { | ||
| if v := recover(); v != nil { | ||
| err := tx.Rollback() | ||
| if err != nil { | ||
| return | ||
| } | ||
| } | ||
| tx.Rollback() | ||
| }() | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Handle or explicitly ignore the Rollback error.
The deferred tx.Rollback() on line 447 ignores its error return value, which the static analysis tool correctly flags. While calling Rollback() after a successful Commit() is a no-op, the error should be explicitly ignored or handled to satisfy linters and make the intent clear.
Apply this diff to explicitly ignore the error:
// Cleanup: always rollback the transaction and ignore errors to prevent leaving connections open
defer func() {
- tx.Rollback()
+ _ = tx.Rollback()
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cleanup: always rollback the transaction and ignore errors to prevent leaving connections open | |
| defer func() { | |
| if v := recover(); v != nil { | |
| err := tx.Rollback() | |
| if err != nil { | |
| return | |
| } | |
| } | |
| tx.Rollback() | |
| }() | |
| if err != nil { | |
| return err | |
| } | |
| // Cleanup: always rollback the transaction and ignore errors to prevent leaving connections open | |
| defer func() { | |
| _ = tx.Rollback() | |
| }() | |
| if err != nil { | |
| return err | |
| } |
🧰 Tools
🪛 GitHub Check: Backend Server Tests / Go
[failure] 447-447:
Error return value of tx.Rollback is not checked (errcheck)
🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_item_attachments.go around lines 444 to 452,
the deferred call to tx.Rollback() ignores its returned error value. Update the
deferred function to capture the error returned by tx.Rollback() and explicitly
ignore it by assigning it to the blank identifier or handle it appropriately, to
clarify intent and satisfy linters.
| if !r.thumbnail.Enabled { | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the thumbnail enabled check before pubsub setup.
The early return for disabled thumbnails (lines 638-640) occurs after establishing the pubsub connection (lines 629-636). This wastes resources by opening a topic that won't be used.
Apply this diff to check the flag first:
if err != nil {
return 0, err
}
- pubsubString, err := utils.GenerateSubPubConn(r.pubSubConn, "thumbnails")
- if err != nil {
- log.Err(err).Msg("failed to generate pubsub connection string")
- }
- topic, err := pubsub.OpenTopic(ctx, pubsubString)
- if err != nil {
- log.Err(err).Msg("failed to open pubsub topic")
- }
-
if !r.thumbnail.Enabled {
return 0, nil
}
+ pubsubString, err := utils.GenerateSubPubConn(r.pubSubConn, "thumbnails")
+ if err != nil {
+ log.Err(err).Msg("failed to generate pubsub connection string")
+ return 0, err
+ }
+ topic, err := pubsub.OpenTopic(ctx, pubsubString)
+ if err != nil {
+ log.Err(err).Msg("failed to open pubsub topic")
+ return 0, err
+ }
+
var messages []*pubsub.Message📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !r.thumbnail.Enabled { | |
| return 0, nil | |
| } | |
| if err != nil { | |
| return 0, err | |
| } | |
| // bail out early if thumbnails are disabled, before opening any topics | |
| if !r.thumbnail.Enabled { | |
| return 0, nil | |
| } | |
| pubsubString, err := utils.GenerateSubPubConn(r.pubSubConn, "thumbnails") | |
| if err != nil { | |
| log.Err(err).Msg("failed to generate pubsub connection string") | |
| return 0, err | |
| } | |
| topic, err := pubsub.OpenTopic(ctx, pubsubString) | |
| if err != nil { | |
| log.Err(err).Msg("failed to open pubsub topic") | |
| return 0, err | |
| } | |
| var messages []*pubsub.Message |
🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_item_attachments.go around lines 629 to 640,
move the check for whether thumbnails are enabled (lines 638-640) to before the
pubsub setup (lines 629-636). This avoids opening the pubsub topic unnecessarily
when thumbnails are disabled. To fix, place the if !r.thumbnail.Enabled { return
0, nil } condition before any pubsub connection or topic initialization occurs.
1 participant
What type of PR is this?
What this PR does / why we need it:
When running the thumbnail generation task, it attempts to create a transaction which locks SQLite databases (since the maximum number of allowed connections is 1, and the main thread already has an open connection).
This PR increases the maximum number of connections to 4, which fixes the deadlock. Also function
CreateThumbnailwas upadted to guarantee that the transaction is closed, even when returning an error.Which issue(s) this PR fixes:
Fixes #1051
Special notes for your reviewer:
-
Testing
-
Summary by CodeRabbit
Refactor
Chores