Repository navigation
O3-5765: Automatically clear queue entries on a schedule - #119
Merged
wikumChamith merged 15 commits intoOct 5, 2026
Merged
Conversation
dkayiwa
reviewed
Jul 21, 2026
UjjawalPrabhat
force-pushed
the
O3-5765-scheduled-queue-clear
branch
from
July 24, 2026 13:23
b9b4520 to
fb2a13d
Compare
dkayiwa
reviewed
Jul 26, 2026
dkayiwa
reviewed
Jul 27, 2026
dkayiwa
left a comment
dkayiwa
left a comment
Member
There was a problem hiding this comment.
The PR description still describes an earlier version of this. It says the default is 23:59, warns that existing deployments will begin clearing all queues at end of day once deployed, and describes the task as acting once the clock passes the configured time. None of that holds any more: the property defaults to blank, and the task works back from the most recent occurrence of the configured time. Could you refresh it? It is the first thing anyone coming to this PR reads, and as written it advertises the behaviour that got changed.
dkayiwa
reviewed
Jul 28, 2026
ibacher
reviewed
Jul 31, 2026
Member
There was a problem hiding this comment.
While I realize this wasn't added by this module, it would be better to use core's task scheduling features rather than ScheduledExecutorTask. That said, if we are going to use Spring scheduling, then do not call Context.getService() but inject the service from Spring.
Both tasks are now AbstractTasks registered as TaskDefinitions when the module starts, following the pattern the reference application and chartsearchai activators use. That retires QueueTaskExecutor and QueueTimerTask along with the daemon token plumbing, since core runs scheduled tasks as the daemon user itself, and replaces the two hand rolled re-entrancy flags with the isExecuting guard AbstractTask provides. Implementers get both tasks on the Manage Scheduler page, where the interval can be changed or a task stopped. Nothing here overrides that afterwards: the scheduler starts tasks with startOnStartup at server startup and restores them across a module being started or stopped, so registration only happens once. The definition is scheduled as it is created, which is the one case the scheduler does not cover, of this module being installed into a running server. Services stay behind Context lookups because a task built by TaskFactory is not a Spring bean, which is what core's own AutoCloseVisitsTask does.
Contributor
Author
@ibacher Tried out using core's scheduling feat....have a look!! |
The unit tests stub the services the task talks through, so nothing checked that the search criteria filter the way they assume, that the save survives validation, or that endedAt reaches the database. This builds the task the way the scheduler does, from the class name on a TaskDefinition, and reads the entry back from the database rather than from the session the task used.
|
@claude review |
jwnasambu
reviewed
Aug 20, 2026
NethmiRodrigo
left a comment
NethmiRodrigo
left a comment
Contributor
There was a problem hiding this comment.
Thanks @UjjawalPrabhat! A few suggestions -
Close queue entries through a new QueueEntryService.closeQueueEntry, which reloads the entry and writes it with optimistic locking, so an entry transitioned between the query and its turn in the loop is left alone instead of being overwritten from a stale snapshot. Both tasks use it, and the unreachable closeActiveQueueEntries is removed. Also flush and clear the session every 250 entries so the first sweep after a close time is configured stays linear, wrap the whole task registration so a lookup failure cannot skip the second task, and tighten a couple of log messages.
…e its visit closeQueueEntry used dao.get, which returns the instance already in the task's session, so its state checks saw the copy loaded at the start of the run and a transition made in the meantime was overwritten. The entry and its visit are now refreshed from the database first. The direct UPDATE in updateIfUnmodified skips QueueEntryValidator, so the visit rule is applied in closeQueueEntry: an entry whose visit stopped before the close time ends at the visit stop time. If that leaves no time after the entry started, the entry is left alone with one warning instead of a rejected write every minute. The catch (ValidationException) blocks in both tasks could not fire on this path and are removed. Integration tests cover the concurrent end and the stopped visit against the real session.
Contributor
|
@wikumChamith can you review this? |
ibacher
reviewed
Sep 18, 2026
ibacher
left a comment
ibacher
left a comment
Member
There was a problem hiding this comment.
Thanks @UjjawalPrabhat! A random smattering of curmudgeonly comments.
Contributor
|
@UjjawalPrabhat let me know if you'll be addressing the review, else I'll pick it up :D |
Set date_changed / changed_by in QueueEntryDaoImpl.updateIfUnmodified, which the bulk update bypasses AuditableInterceptor for, and refuse to write an end over a row another transaction has already ended. Re-opening an entry deliberately targets an ended row, so it stays exempt. The query is now built with the JPA Criteria API rather than by string munging. Restore the deprecated QueueEntryService.closeActiveQueueEntries() rather than deleting public API someone may be relying on. Resolve queue references in queue.autoCloseQueueEntriesForQueues by name as well as uuid, the way concept and location references already resolve, so the property can be configured in terms an implementer can read. Parse the configured close time with a static DateTimeFormatter instead of SimpleDateFormat, and trim the README section down.
Name the criteria field literals that updateIfUnmodified repeated, and return an Optional from getQueuesToClear rather than null. An empty Optional means no queues are configured and every queue is swept; a present but empty list means queues were configured and none of them resolved, which is the case that must not turn into a sweep of everything. Drop the evict-and-continue test in each task that only repeated its neighbour with a different exception class, now that the two tasks catch exceptions in one place, and stop the warning in closeQueueEntry from blaming a visit that may not exist.
Contributor
|
Ping @wikumChamith can you re-review this? |
Read the lazy visit before stopping it in the visit-refresh test, stop the scheduled tasks after the activator tests, drop tests that only exercised their own stubs, and shorten the README entries.
dkayiwa
reviewed
Oct 1, 2026
wikumChamith
approved these changes
Oct 1, 2026
wikumChamith
left a comment
wikumChamith
left a comment
Member
There was a problem hiding this comment.
A few minor nits with the code style. Not merge blocking.
|
Member
|
@UjjawalPrabhat what about the 4 new issues Sonar is pointing out? |
Contributor
Author
@wikumChamith 2 of them are just reminders to remove the deprecated code someday. The other 2 are code quality improvements and aren't touched by this PR. I did rewrite them, but Daniel suggested keeping the PR scoped, so I put them back. |
dkayiwa
added a commit
to dkayiwa/openmrs-module-queue
that referenced
this pull request
Oct 7, 2026
Brings in openmrs#119 (O3-5765) and the 3.1.0 release, and ports openmrs#119 to Platform 3: - updateIfUnmodified keeps openmrs#119's CriteriaUpdate, on jakarta.persistence and createMutationQuery. - openmrs#119's tests move to JUnit 5. QueueModuleActivatorTest and AutoCloseQueueEntryTaskIntegrationTest extend the Jupiter BaseModuleContextSensitiveTest, and the integration test runs the task through core's LegacyTask, which is how the JobRunr scheduler runs a task definition now that TaskFactory is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
6 participants
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.
Summary
Adds
AutoCloseQueueEntryTask. Each minute it ends active entries whosestartedAtis on or before the most recent occurrence of the configured time of day. So a run missed at that time is caught up by the next one, and entries started since then stay in the queue.Both this and the existing visit-close task are now registered with core's scheduler as
TaskDefinitionsfromQueueModuleActivator, replacing the module's ownQueueTaskExecutorandQueueTimerTask. They appear on the Manage Scheduler page, where the interval can be changed or a task stopped.Two new global properties:
queue.autoCloseQueueEntriesAtTime-HH:mm, blank by default, which disables clearing. An outpatient clinic would set 23:59.queue.autoCloseQueueEntriesForQueues- comma-separated queue uuids; blank means all queues.Blank by default so nothing changes for an existing deployment until an implementer opts in (O3-2443).
Related Issue
O3-5765