[SI-610]: Add task queue maintenance script - #190
Open
tatsianazh wants to merge 12 commits into
Open
Conversation
…nd vacuum tq_task_log
📝 WalkthroughWalkthroughIntroduces a new maintenance script extending ScriptAction that provides cron-oriented operations for task queue management. Supports archive, delete, unblock, and vacuum operations with configurable retention periods for completed tasks (30 days), archived tasks (180 days), and stuck tasks (2 days). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script as TaskQueueMaintenance
participant TaskLog as TaskLogInterface
participant Broker as Task Log Broker
User->>Script: Execute archive operation
Script->>TaskLog: Query COMPLETED/FAILED tasks<br/>older than 30 days
TaskLog-->>Script: Return matched tasks
alt No tasks found
Script-->>User: Return success report
else Tasks found
Script->>Broker: Archive collection
Broker-->>Script: Confirmation
Script-->>User: Return success with count
end
sequenceDiagram
actor User
participant Script as TaskQueueMaintenance
participant TaskLog as TaskLogInterface
participant QueueDisp as QueueDispatcherInterface
participant RdsBroker as RdsQueueBroker
participant DB as Persistence/DB
User->>Script: Execute unblock operation
Script->>TaskLog: Query RUNNING/ENQUEUED tasks<br/>with updated_at ≤ cutoff
TaskLog-->>Script: Return matched tasks
alt Empty collection
Script-->>User: Return success report
else Tasks found
loop For each task log entry
Script->>QueueDisp: Get available queues
QueueDisp-->>Script: Return queue list
loop For each queue
Script->>RdsBroker: Get task by task log ID
alt Task found in queue
RdsBroker->>DB: Query visibility from tq_queue table
DB-->>RdsBroker: Return visible status
alt Not visible
Script->>DB: Set visibility to true
DB-->>Script: Confirmation
Script->>Script: Count as unblocked
else Already visible
Script->>Script: Count as already visible
end
else Task not found
Script->>Script: Count as orphaned
end
end
end
Script-->>User: Return aggregated stats report
end
sequenceDiagram
actor User
participant Script as TaskQueueMaintenance
participant Driver as Persistence Driver
participant PDO as PDO Connection
User->>Script: Execute vacuum operation
Script->>Driver: Verify driver is SQL with PostgreSQL
alt Not compatible
Script-->>User: Return error report
else Compatible
Script->>PDO: Execute VACUUM FULL tq_task_log
PDO-->>Script: Confirmation
Script-->>User: Return success report
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
siwane
reviewed
Dec 5, 2025
tatsianazh
marked this pull request as ready for review
December 5, 2025 16:59
tatsianazh
marked this pull request as draft
December 5, 2025 17:01
Front-end summary Node 18
|
tatsianazh
marked this pull request as ready for review
January 7, 2026 13:45
tikhanovichA
reviewed
Jan 7, 2026
tatsianazh
marked this pull request as draft
February 6, 2026 09:24
added 2 commits
February 23, 2026 16:12
…into feat/SI-610/task-queue-maintenance
tatsianazh
marked this pull request as ready for review
March 27, 2026 17:01
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tools/TaskQueueMaintenance.php`:
- Around line 40-373: The PR adds TaskQueueMaintenance with substantial logic
but no tests; add unit tests that exercise archiveCompletedAndFailed,
deleteOldArchived, unblockStuckTasks, and runVacuum (positive flows) and
negative/failure paths (empty collections, broker delete failures, persistence
query returning no row, decorator null, changeTaskVisibility failures, and
non-pg driver for VACUUM). Write tests that instantiate TaskQueueMaintenance and
mock/getServiceLocator to return mocked TaskLogInterface (search,
archiveCollection, getBroker), TaskLogBrokerInterface (deleteById),
QueueDispatcherInterface (getQueues), Queue and RdsQueueBroker (getBroker,
getTaskByTaskLogId, changeTaskVisibility), and
common_persistence_Manager/common_persistence_SqlPersistence (query/exec/driver)
to simulate all branches and assert Report messages and counts; include tests
for retention parameter handling and error reports from runVacuum when driver is
not common_persistence_sql_Driver or driver param driver != 'pdo_pgsql'. Ensure
each test verifies both Report success/error contents and that mocked methods
(archiveCollection, deleteById, changeTaskVisibility, exec) are called the
expected number of times.
- Around line 369-371: Wrap the call to $persistence->exec('VACUUM FULL
tq_task_log;') in a try/catch so database failures do not abort the cron: try
executing the VACUUM, and on success return
Report::createSuccess('[TaskQueueMaintenance] Vacuum flow finished (VACUUM FULL
tq_task_log).'); catch the DB exception (e.g. PDOException or generic Exception)
and return Report::createError(...) containing a clear message and the exception
message/details so the cron returns an error report instead of crashing.
- Around line 125-127: After casting the options to ints ($completedRetention,
$archivedRetention, $stuckRetention) validate that none are negative; if any
value is < 0, short-circuit before computing cutoff dates by returning/raising a
clear error/report (use the existing task tool error reporting mechanism—e.g.
the class' error/report method or return a non‑zero status) with a message
naming the offending option(s) and expected >= 0 constraint; ensure this
validation sits immediately after the getOption casts so invalid inputs cannot
produce bad date modifiers.
- Around line 311-315: The code concatenates $queue->getName() into $tableName
and injects it into SQL for persistence->query, which risks SQL injection
because identifiers cannot be parameterized; instead validate or map the queue
name to a safe identifier before building the SQL: enforce a strict whitelist or
regex (e.g. only lowercase letters, digits and underscores) for
$queue->getName() (or use a known mapping of queue names to table names), then
construct $tableName as 'tq_' . $validatedName (or lookup) and only after that
use persistence->query with parameterized values (keeping $queueRowId as a
parameter); update the code around $tableName, $queue->getName(), and
persistence->query to implement this validation/mapping and throw/log on invalid
queue names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: https://raw.githubusercontent.com/oat-sa/tao-code-quality/main/coderabbit/php/authoring/v1/.coderabbit.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: c5913621-0fc4-47ba-828f-b3133d907c20
📒 Files selected for processing (1)
scripts/tools/TaskQueueMaintenance.php
Comment on lines
+40
to
+373
| class TaskQueueMaintenance extends ScriptAction implements ServiceLocatorAwareInterface | ||
| { | ||
| use ServiceLocatorAwareTrait; | ||
|
|
||
| protected function provideDescription(): string | ||
| { | ||
| return 'Cron-oriented maintenance script for task queue.'; | ||
| } | ||
|
|
||
| protected function provideUsage(): array | ||
| { | ||
| return [ | ||
| 'prefix' => 'h', | ||
| 'longPrefix' => 'help', | ||
| 'description' => 'Display this help message.' | ||
| ]; | ||
| } | ||
|
|
||
| protected function provideOptions(): array | ||
| { | ||
| return [ | ||
| 'archive' => [ | ||
| 'prefix' => 'a', | ||
| 'longPrefix' => 'archive', | ||
| 'flag' => true, | ||
| 'required' => false, | ||
| 'description' => 'Archive completed/failed tasks older than retention.' | ||
| ], | ||
| 'delete' => [ | ||
| 'prefix' => 'd', | ||
| 'longPrefix' => 'delete', | ||
| 'flag' => true, | ||
| 'required' => false, | ||
| 'description' => 'Delete archived tasks older than retention.' | ||
| ], | ||
| 'vacuum' => [ | ||
| 'prefix' => 'v', | ||
| 'longPrefix' => 'vacuum', | ||
| 'flag' => true, | ||
| 'required' => false, | ||
| 'description' => 'Run VACUUM FULL on tq_task_log.' | ||
| ], | ||
| 'unblock' => [ | ||
| 'prefix' => 'u', | ||
| 'longPrefix' => 'unblock', | ||
| 'flag' => true, | ||
| 'required' => false, | ||
| 'description' => 'Unblock stuck tasks older than retention.' | ||
| ], | ||
| 'completedRetention' => [ | ||
| 'prefix' => 'cr', | ||
| 'longPrefix' => 'completed-retention', | ||
| 'cast' => 'int', | ||
| 'required' => false, | ||
| 'defaultValue' => 30, | ||
| 'description' => 'Retention in days for completed/failed tasks before archiving.', | ||
| ], | ||
| 'archivedRetention' => [ | ||
| 'prefix' => 'ar', | ||
| 'longPrefix' => 'archived-retention', | ||
| 'cast' => 'int', | ||
| 'required' => false, | ||
| 'defaultValue' => 180, | ||
| 'description' => 'Retention in days for archived tasks before deletion.', | ||
| ], | ||
| 'stuckRetention' => [ | ||
| 'prefix' => 'sr', | ||
| 'longPrefix' => 'stuck-retention', | ||
| 'cast' => 'int', | ||
| 'required' => false, | ||
| 'defaultValue' => 2, | ||
| 'description' => 'Age in days for running/enqueued tasks to be considered stuck.', | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Entry point when called via: | ||
| * sudo -u www-data php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' | ||
| */ | ||
| protected function run() | ||
| { | ||
| $report = Report::createInfo('[TaskQueueMaintenance] Maintenance run summary.'); | ||
| $hasAction = false; | ||
|
|
||
| $completedRetention = (int) $this->getOption('completedRetention'); | ||
| $archivedRetention = (int) $this->getOption('archivedRetention'); | ||
| $stuckRetention = (int) $this->getOption('stuckRetention'); | ||
|
|
||
| if ($this->getOption('archive')) { | ||
| $hasAction = true; | ||
| $report->add($this->archiveCompletedAndFailed($completedRetention)); | ||
| } | ||
|
|
||
| if ($this->getOption('delete')) { | ||
| $hasAction = true; | ||
| $report->add($this->deleteOldArchived($archivedRetention)); | ||
| } | ||
|
|
||
| if ($this->getOption('unblock')) { | ||
| $hasAction = true; | ||
| $report->add($this->unblockStuckTasks($stuckRetention)); | ||
| } | ||
|
|
||
| if ($this->getOption('vacuum')) { | ||
| $hasAction = true; | ||
| $report->add($this->runVacuum()); | ||
| } | ||
|
|
||
| if (!$hasAction) { | ||
| return Report::createInfo('No option provided. Use --help to see usage.'); | ||
| } | ||
|
|
||
| return $report; | ||
| } | ||
|
|
||
| /** | ||
| * Move Completed and Failed tasks older than N days to Archived status in the tq_task_log table. | ||
| */ | ||
| private function archiveCompletedAndFailed(int $completedRetention): Report | ||
| { | ||
| /** @var TaskLogInterface $taskLog */ | ||
| $taskLog = $this->getServiceLocator()->get(TaskLogInterface::SERVICE_ID); | ||
|
|
||
| $cutoffDate = (new DateTimeImmutable()) | ||
| ->modify(sprintf('-%d days', $completedRetention)); | ||
|
|
||
| $cutoffDateString = $cutoffDate->format('Y-m-d H:i:s'); | ||
|
|
||
| $filter = new TaskLogFilter(); | ||
|
|
||
| $filter | ||
| ->in( | ||
| TaskLogBrokerInterface::COLUMN_STATUS, | ||
| [ | ||
| TaskLogInterface::STATUS_COMPLETED, | ||
| TaskLogInterface::STATUS_FAILED, | ||
| ] | ||
| ) | ||
| ->lt( | ||
| TaskLogBrokerInterface::COLUMN_UPDATED_AT, | ||
| $cutoffDateString | ||
| ); | ||
|
|
||
| $collection = $taskLog->search($filter); | ||
|
|
||
| if ($collection->isEmpty()) { | ||
| return Report::createSuccess('[TaskQueueMaintenance] Archive: nothing to archive.'); | ||
| } | ||
|
|
||
| $taskLog->archiveCollection($collection); | ||
|
|
||
| return Report::createSuccess(sprintf( | ||
| '[TaskQueueMaintenance] Archive flow finished. Affected tasks: %d', | ||
| $collection->count() | ||
| )); | ||
| } | ||
|
|
||
| /** | ||
| * Delete archived tasks older than N days. | ||
| */ | ||
| private function deleteOldArchived(int $archivedRetention): Report | ||
| { | ||
| /** @var TaskLogInterface $taskLog */ | ||
| $taskLog = $this->getServiceLocator()->get(TaskLogInterface::SERVICE_ID); | ||
|
|
||
| $cutoffDate = (new DateTimeImmutable()) | ||
| ->modify(sprintf('-%d days', $archivedRetention)); | ||
|
|
||
| $cutoffDateString = $cutoffDate->format('Y-m-d H:i:s'); | ||
|
|
||
| $filter = new TaskLogFilter(); | ||
|
|
||
| $filter | ||
| ->eq( | ||
| TaskLogBrokerInterface::COLUMN_STATUS, | ||
| TaskLogInterface::STATUS_ARCHIVED | ||
| ) | ||
| ->lt( | ||
| TaskLogBrokerInterface::COLUMN_UPDATED_AT, | ||
| $cutoffDateString | ||
| ); | ||
|
|
||
| $collection = $taskLog->search($filter); | ||
|
|
||
| if ($collection->isEmpty()) { | ||
| return Report::createSuccess('[TaskQueueMaintenance] Delete: nothing to delete.'); | ||
| } | ||
|
|
||
| $deleted = 0; | ||
|
|
||
| $broker = $taskLog->getBroker(); | ||
|
|
||
| foreach ($collection as $entity) { | ||
| /** @var \oat\tao\model\taskQueue\TaskLog\Entity\TaskLogEntityInterface|\ArrayAccess $entity */ | ||
| if ($broker->deleteById($entity->getId())) { | ||
| $deleted++; | ||
| } | ||
| } | ||
|
|
||
| return Report::createSuccess(sprintf( | ||
| '[TaskQueueMaintenance] Delete archived flow finished. Tasks deleted: %d', | ||
| $deleted | ||
| )); | ||
| } | ||
|
|
||
| /** | ||
| * Unblock stuck tasks in Running/Enqueued status older than $stuckRetention. | ||
| */ | ||
| private function unblockStuckTasks(int $stuckRetention): Report | ||
| { | ||
| /** @var TaskLogInterface $taskLog */ | ||
| $taskLog = $this->getServiceLocator()->get(TaskLogInterface::SERVICE_ID); | ||
|
|
||
| /** @var QueueDispatcherInterface $dispatcher */ | ||
| $dispatcher = $this->getServiceLocator()->get(QueueDispatcherInterface::SERVICE_ID); | ||
|
|
||
| /** @var \common_persistence_SqlPersistence $persistence */ | ||
| $persistence = $this->getServiceLocator() | ||
| ->get(common_persistence_Manager::SERVICE_ID) | ||
| ->getPersistenceById('default'); | ||
|
|
||
| $cutoff = (new DateTimeImmutable()) | ||
| ->modify(sprintf('-%d days', $stuckRetention)) | ||
| ->format('Y-m-d H:i:s'); | ||
|
|
||
| $filter = (new TaskLogFilter()) | ||
| ->in( | ||
| TaskLogBrokerInterface::COLUMN_STATUS, | ||
| [ | ||
| TaskLogInterface::STATUS_RUNNING, | ||
| TaskLogInterface::STATUS_ENQUEUED, | ||
| ] | ||
| ) | ||
| ->lte( | ||
| TaskLogBrokerInterface::COLUMN_UPDATED_AT, | ||
| $cutoff | ||
| ); | ||
|
|
||
| $collection = $taskLog->search($filter); | ||
|
|
||
| $stats = [ | ||
| 'stuckFound' => $collection->count(), | ||
| 'unblocked' => 0, | ||
| 'alreadyVisible' => 0, | ||
| 'orphan' => 0, | ||
| ]; | ||
|
|
||
| if ($collection->isEmpty()) { | ||
| return Report::createSuccess('[TaskQueueMaintenance] Unblock: nothing to unblock.'); | ||
| } | ||
|
|
||
| /** @var \oat\tao\model\taskQueue\Queue[] $queues */ | ||
| $queues = $dispatcher->getQueues(); | ||
|
|
||
| foreach ($collection as $taskLogEntity) { | ||
| $taskLogId = $taskLogEntity->getId(); | ||
| $foundInQueue = false; | ||
|
|
||
| foreach ($queues as $queue) { | ||
| $broker = $queue->getBroker(); | ||
|
|
||
| /** @var RdsQueueBroker $broker */ | ||
| $decorator = $broker->getTaskByTaskLogId($taskLogId); | ||
| if ($decorator === null) { | ||
| continue; | ||
| } | ||
|
|
||
| $foundInQueue = true; | ||
|
|
||
| $queueRowId = (int) $decorator->getTaskId(); | ||
| $tableName = 'tq_' . $queue->getName(); | ||
|
|
||
| $row = $persistence->query( | ||
| 'SELECT visible FROM ' . $tableName . ' WHERE id = ?', | ||
| [$queueRowId] | ||
| )->fetch(PDO::FETCH_ASSOC); | ||
|
|
||
| if (!$row) { | ||
| continue; | ||
| } | ||
|
|
||
| if ((bool) $row['visible'] === false) { | ||
| $broker->changeTaskVisibility((string) $queueRowId, true); | ||
| $stats['unblocked']++; | ||
| } else { | ||
| $stats['alreadyVisible']++; | ||
| } | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| if (!$foundInQueue) { | ||
| $stats['orphan']++; | ||
| } | ||
| } | ||
|
|
||
| return Report::createSuccess(sprintf( | ||
| '[TaskQueueMaintenance] Unblock finished. found=%d already_visible=%d unblocked=%d orphan=%d', | ||
| $stats['stuckFound'], | ||
| $stats['alreadyVisible'], | ||
| $stats['unblocked'], | ||
| $stats['orphan'] | ||
| )); | ||
| } | ||
|
|
||
| /** | ||
| * Run VACUUM FULL on the tq_task_log table. | ||
| */ | ||
| private function runVacuum(): Report | ||
| { | ||
| /** @var common_persistence_Manager $pm */ | ||
| $pm = $this->getServiceLocator()->get(common_persistence_Manager::SERVICE_ID); | ||
| $persistence = $pm->getPersistenceById('default'); | ||
|
|
||
| /** @var common_persistence_Driver $driver */ | ||
| $driver = $persistence->getDriver(); | ||
|
|
||
| if (!($driver instanceof common_persistence_sql_Driver)) { | ||
| return Report::createError('VACUUM FULL is only supported on PostgreSQL (pdo_pgsql)'); | ||
| } | ||
|
|
||
| $conn = $driver->getDbalConnection()->getParams(); | ||
| $driverType = $conn['driver'] ?? null; | ||
|
|
||
| if ($driverType !== 'pdo_pgsql') { | ||
| return Report::createError('VACUUM FULL is only supported on PostgreSQL (pdo_pgsql)'); | ||
| } | ||
|
|
||
| $persistence->exec('VACUUM FULL tq_task_log;'); | ||
|
|
||
| return Report::createSuccess('[TaskQueueMaintenance] Vacuum flow finished (VACUUM FULL tq_task_log).'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add unit tests for all maintenance flows and failure paths.
This new script introduces substantial logic but no corresponding tests are included in this PR context (archive/delete/unblock/vacuum, including negative cases and broker/persistence failures).
As per coding guidelines, "All new code MUST include appropriate unit tests" and "Tests MUST cover both positive and negative scenarios."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tools/TaskQueueMaintenance.php` around lines 40 - 373, The PR adds
TaskQueueMaintenance with substantial logic but no tests; add unit tests that
exercise archiveCompletedAndFailed, deleteOldArchived, unblockStuckTasks, and
runVacuum (positive flows) and negative/failure paths (empty collections, broker
delete failures, persistence query returning no row, decorator null,
changeTaskVisibility failures, and non-pg driver for VACUUM). Write tests that
instantiate TaskQueueMaintenance and mock/getServiceLocator to return mocked
TaskLogInterface (search, archiveCollection, getBroker), TaskLogBrokerInterface
(deleteById), QueueDispatcherInterface (getQueues), Queue and RdsQueueBroker
(getBroker, getTaskByTaskLogId, changeTaskVisibility), and
common_persistence_Manager/common_persistence_SqlPersistence (query/exec/driver)
to simulate all branches and assert Report messages and counts; include tests
for retention parameter handling and error reports from runVacuum when driver is
not common_persistence_sql_Driver or driver param driver != 'pdo_pgsql'. Ensure
each test verifies both Report success/error contents and that mocked methods
(archiveCollection, deleteById, changeTaskVisibility, exec) are called the
expected number of times.
Comment on lines
+125
to
+127
| $completedRetention = (int) $this->getOption('completedRetention'); | ||
| $archivedRetention = (int) $this->getOption('archivedRetention'); | ||
| $stuckRetention = (int) $this->getOption('stuckRetention'); |
There was a problem hiding this comment.
Validate retention inputs before cutoff-date computation.
Negative retention values are currently accepted and can produce invalid or unintended date modifiers. Reject < 0 values early and return an error report.
Proposed fix
$completedRetention = (int) $this->getOption('completedRetention');
$archivedRetention = (int) $this->getOption('archivedRetention');
$stuckRetention = (int) $this->getOption('stuckRetention');
+
+ foreach (
+ [
+ 'completedRetention' => $completedRetention,
+ 'archivedRetention' => $archivedRetention,
+ 'stuckRetention' => $stuckRetention,
+ ] as $name => $value
+ ) {
+ if ($value < 0) {
+ return Report::createError(sprintf('%s must be >= 0 days.', $name));
+ }
+ }📝 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.
Suggested change
| $completedRetention = (int) $this->getOption('completedRetention'); | |
| $archivedRetention = (int) $this->getOption('archivedRetention'); | |
| $stuckRetention = (int) $this->getOption('stuckRetention'); | |
| $completedRetention = (int) $this->getOption('completedRetention'); | |
| $archivedRetention = (int) $this->getOption('archivedRetention'); | |
| $stuckRetention = (int) $this->getOption('stuckRetention'); | |
| foreach ( | |
| [ | |
| 'completedRetention' => $completedRetention, | |
| 'archivedRetention' => $archivedRetention, | |
| 'stuckRetention' => $stuckRetention, | |
| ] as $name => $value | |
| ) { | |
| if ($value < 0) { | |
| return Report::createError(sprintf('%s must be >= 0 days.', $name)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tools/TaskQueueMaintenance.php` around lines 125 - 127, After casting
the options to ints ($completedRetention, $archivedRetention, $stuckRetention)
validate that none are negative; if any value is < 0, short-circuit before
computing cutoff dates by returning/raising a clear error/report (use the
existing task tool error reporting mechanism—e.g. the class' error/report method
or return a non‑zero status) with a message naming the offending option(s) and
expected >= 0 constraint; ensure this validation sits immediately after the
getOption casts so invalid inputs cannot produce bad date modifiers.
Comment on lines
+311
to
+315
| $tableName = 'tq_' . $queue->getName(); | ||
|
|
||
| $row = $persistence->query( | ||
| 'SELECT visible FROM ' . $tableName . ' WHERE id = ?', | ||
| [$queueRowId] |
There was a problem hiding this comment.
Avoid raw SQL identifier interpolation from queue names.
$tableName is built from $queue->getName() and concatenated into SQL. Even for internal values, this should be strictly validated before query construction.
Proposed fix
$queueRowId = (int) $decorator->getTaskId();
- $tableName = 'tq_' . $queue->getName();
+ $queueName = $queue->getName();
+ if (!preg_match('/^[A-Za-z0-9_]+$/', $queueName)) {
+ return Report::createError(
+ sprintf('[TaskQueueMaintenance] Unsupported queue name: %s', $queueName)
+ );
+ }
+ $tableName = 'tq_' . $queueName;
$row = $persistence->query(
- 'SELECT visible FROM ' . $tableName . ' WHERE id = ?',
+ sprintf('SELECT visible FROM %s WHERE id = ?', $tableName),
[$queueRowId]
)->fetch(PDO::FETCH_ASSOC);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tools/TaskQueueMaintenance.php` around lines 311 - 315, The code
concatenates $queue->getName() into $tableName and injects it into SQL for
persistence->query, which risks SQL injection because identifiers cannot be
parameterized; instead validate or map the queue name to a safe identifier
before building the SQL: enforce a strict whitelist or regex (e.g. only
lowercase letters, digits and underscores) for $queue->getName() (or use a known
mapping of queue names to table names), then construct $tableName as 'tq_' .
$validatedName (or lookup) and only after that use persistence->query with
parameterized values (keeping $queueRowId as a parameter); update the code
around $tableName, $queue->getName(), and persistence->query to implement this
validation/mapping and throw/log on invalid queue names.
Comment on lines
+369
to
+371
| $persistence->exec('VACUUM FULL tq_task_log;'); | ||
|
|
||
| return Report::createSuccess('[TaskQueueMaintenance] Vacuum flow finished (VACUUM FULL tq_task_log).'); |
There was a problem hiding this comment.
Handle DB failures in vacuum execution.
$persistence->exec() is not wrapped in exception handling. A failure here can terminate the whole cron run instead of returning an error report.
Proposed fix
- $persistence->exec('VACUUM FULL tq_task_log;');
+ try {
+ $persistence->exec('VACUUM FULL tq_task_log;');
+ } catch (\Throwable $exception) {
+ return Report::createError(
+ sprintf('[TaskQueueMaintenance] Vacuum failed: %s', $exception->getMessage())
+ );
+ }📝 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.
Suggested change
| $persistence->exec('VACUUM FULL tq_task_log;'); | |
| return Report::createSuccess('[TaskQueueMaintenance] Vacuum flow finished (VACUUM FULL tq_task_log).'); | |
| try { | |
| $persistence->exec('VACUUM FULL tq_task_log;'); | |
| } catch (\Throwable $exception) { | |
| return Report::createError( | |
| sprintf('[TaskQueueMaintenance] Vacuum failed: %s', $exception->getMessage()) | |
| ); | |
| } | |
| return Report::createSuccess('[TaskQueueMaintenance] Vacuum flow finished (VACUUM FULL tq_task_log).'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tools/TaskQueueMaintenance.php` around lines 369 - 371, Wrap the call
to $persistence->exec('VACUUM FULL tq_task_log;') in a try/catch so database
failures do not abort the cron: try executing the VACUUM, and on success return
Report::createSuccess('[TaskQueueMaintenance] Vacuum flow finished (VACUUM FULL
tq_task_log).'); catch the DB exception (e.g. PDOException or generic Exception)
and return Report::createError(...) containing a clear message and the exception
message/details so the cron returns an error report instead of crashing.
Version
There are 0 BREAKING CHANGE, 4 features, 6 fixes |
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
3 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.
https://oat-sa.atlassian.net/browse/SI-610
Adds TaskQueueMaintenance script that is supposed to be run by a cron-job.
--archive: archives completed/failed tasks older than period of time specified number of days
--unblock: unblocks stuck tasks (set visible=='t') older than specified number of days
--vacuum: runs VACUUM FULL tq_task_log.
--delete: deletes archived tasks older than period of time specified number of days
How to run:
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --archive
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --unblock
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --vacuum
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --delete
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --archive -cr [n-days]
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --unblock -sr [n-days]
php index.php 'oat\taoTaskQueue\scripts\tools\TaskQueueMaintenance' --delete -ar [n-days]
Summary by CodeRabbit
Release Notes