Merged
Conversation
Reviewer's GuideSwitches background task processing from a SQLite-backed Huey setup to a filesystem-based Huey configuration with a custom cross-platform file-lock implementation, and removes now-unused SQLite-related setup and maintenance code, along with a small client-side cleanup and version/requirements updates. Sequence diagram for filesystem-based Huey background task processingsequenceDiagram
actor User
participant WebApp
participant HueyClient
participant FileHuey
participant FileLock
participant TaskFiles
User->>WebApp: Trigger_endpoint_requiring_background_work
WebApp->>HueyClient: enqueue_task(args)
HueyClient->>FileHuey: add_task(task_data)
FileHuey->>FileLock: __enter__()
activate FileLock
FileLock-->>FileHuey: acquired_lock
FileHuey->>TaskFiles: write_task_file()
FileHuey->>FileLock: __exit__()
deactivate FileLock
Note over FileHuey,TaskFiles: Task is now persisted in filesystem-backed store
participant HueyConsumer
HueyConsumer->>FileHuey: poll_for_tasks()
FileHuey->>FileLock: __enter__()
activate FileLock
FileLock-->>FileHuey: acquired_lock
FileHuey->>TaskFiles: read_and_pop_next_task()
FileHuey->>FileLock: __exit__()
deactivate FileLock
FileHuey-->>HueyConsumer: task_data
HueyConsumer->>HueyConsumer: execute_task()
Class diagram for custom FileLock and Huey integrationclassDiagram
class FileLock {
-filename : str
-fd : int
+FileLock(filename : str)
+acquire() void
+release() void
+__enter__() FileLock
+__exit__(exc_type, exc_val, exc_tb) void
}
class HueyUtils {
<<module>>
+FileLock
}
class SettingsHUEY {
<<settings_dict>>
+name : str
+huey_class : str
+path : str
+immediate : bool
+use_thread_lock : bool
+consumer_workers : int
+consumer_initial_delay : float
}
HueyUtils ..> FileLock : assigned_as_FileLock
SettingsHUEY ..> FileLock : used_for_file_locking_on_win32
SettingsHUEY ..> FileHuey : configures
class FileHuey {
<<Huey_backend>>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The custom FileLock implementation unlinks any existing lock file in init, which can break an active lock held by another process; consider moving any cleanup into acquire with proper checks so multiple processes cannot silently invalidate each other’s locks.
- os.makedirs(dirname) without exist_ok=True combined with a separate os.path.exists check is racy under concurrent startup; it’s safer to call os.makedirs(dirname, exist_ok=True) and drop the explicit existence check.
- In the HUEY settings, removing the explicit worker_type may change behavior depending on Huey defaults; if a specific concurrency model is required, keep worker_type set explicitly to avoid subtle runtime differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The custom FileLock implementation unlinks any existing lock file in __init__, which can break an active lock held by another process; consider moving any cleanup into acquire with proper checks so multiple processes cannot silently invalidate each other’s locks.
- os.makedirs(dirname) without exist_ok=True combined with a separate os.path.exists check is racy under concurrent startup; it’s safer to call os.makedirs(dirname, exist_ok=True) and drop the explicit existence check.
- In the HUEY settings, removing the explicit worker_type may change behavior depending on Huey defaults; if a specific concurrency model is required, keep worker_type set explicitly to avoid subtle runtime differences.
## Individual Comments
### Comment 1
<location path="conreq/__init__.py" line_range="19-21" />
<code_context>
+ elif os.path.exists(self.filename):
+ os.unlink(self.filename)
+
+ def acquire(self):
+ flags = os.O_CREAT | os.O_TRUNC | os.O_RDWR
+ self.fd = os.open(self.filename, flags)
+
+ def release(self):
</code_context>
<issue_to_address>
**issue (bug_risk):** Current FileLock.acquire implementation does not provide actual mutual exclusion across processes.
Because the file is opened with `os.O_CREAT | os.O_TRUNC | os.O_RDWR` only, multiple processes can open the same lock file at once, so this does not enforce mutual exclusion (especially on Windows, where several processes can think they hold the lock). Please use `os.O_EXCL` or a platform-specific locking primitive (`msvcrt.locking` on Windows, `fcntl` on POSIX, etc.) so acquisition fails or blocks when another process already holds the lock, matching Huey’s `FileLock` semantics.
</issue_to_address>
### Comment 2
<location path="conreq/__init__.py" line_range="13-16" />
<code_context>
+ self.filename = filename
+ self.fd = None
+
+ dirname = os.path.dirname(filename)
+ if not os.path.exists(dirname):
+ os.makedirs(dirname)
+ elif os.path.exists(self.filename):
+ os.unlink(self.filename)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditionally deleting an existing lock file in __init__ can interfere with other processes and may raise errors for empty dirnames.
Two issues here:
1) Deleting `self.filename` in `__init__` can remove an active lock held by another process and break locking semantics. A lock implementation should not unconditionally delete a potentially valid lock file without verifying ownership.
2) If `filename` has no directory (e.g. `"lockfile"`), `os.path.dirname(filename)` is `''`, so `os.makedirs(dirname)` will be called with an empty string and fail. Guard against this with something like `if dirname and not os.path.exists(dirname):` and reconsider whether the eager unlink is needed here at all.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+19
to
+21
| def acquire(self): | ||
| flags = os.O_CREAT | os.O_TRUNC | os.O_RDWR | ||
| self.fd = os.open(self.filename, flags) |
There was a problem hiding this comment.
issue (bug_risk): Current FileLock.acquire implementation does not provide actual mutual exclusion across processes.
Because the file is opened with os.O_CREAT | os.O_TRUNC | os.O_RDWR only, multiple processes can open the same lock file at once, so this does not enforce mutual exclusion (especially on Windows, where several processes can think they hold the lock). Please use os.O_EXCL or a platform-specific locking primitive (msvcrt.locking on Windows, fcntl on POSIX, etc.) so acquisition fails or blocks when another process already holds the lock, matching Huey’s FileLock semantics.
Comment on lines
+13
to
+16
| dirname = os.path.dirname(filename) | ||
| if not os.path.exists(dirname): | ||
| os.makedirs(dirname) | ||
| elif os.path.exists(self.filename): |
There was a problem hiding this comment.
issue (bug_risk): Unconditionally deleting an existing lock file in init can interfere with other processes and may raise errors for empty dirnames.
Two issues here:
-
Deleting
self.filenamein__init__can remove an active lock held by another process and break locking semantics. A lock implementation should not unconditionally delete a potentially valid lock file without verifying ownership. -
If
filenamehas no directory (e.g."lockfile"),os.path.dirname(filename)is'', soos.makedirs(dirname)will be called with an empty string and fail. Guard against this with something likeif dirname and not os.path.exists(dirname):and reconsider whether the eager unlink is needed here at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Modify background task manager to utilize filesystem rather than unstable sqlite implementation.
Checklist
Please update this checklist as you complete each item:
By submitting this pull request I agree that all contributions comply with this project's open source license(s).
Summary by Sourcery
Switch background task management from a SQLite-backed Huey configuration to a filesystem-based Huey implementation with cross-platform file locking support.
Enhancements: