Repository navigation
fix(data-transfer): clear the worker timeout alarm when the queue drains - #722
Merged
Merged
Conversation
singleJobDaemon() armed pcntl_alarm() before fetching a job, then broke out of the loop on an empty queue before resetTimeoutHandler() ran. The alarm outlived the command and, --timeout seconds later, the framework's timeout handler SIGKILLed whatever process had invoked it. The TIA baseline recording runs JobLauncherTest in-process and was killed with exit 137 exactly 60s after that test finished. Reset the handler in a finally block so every exit path disarms it.
Copilot started reviewing on behalf of
navneetkumar-pim-webkul
September 23, 2026 11:34
View session
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The critical and moderate test issues remain unresolved, and exception-path cleanup coverage is still missing.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes stale DataTransfer worker timeout alarms that can terminate in-process callers after the queue drains.
Changes:
- Resets timeout handlers in a
finallyblock. - Updates
ReflectionMethodsyntax. - Adds a queue-drain alarm regression test.
| File | Summary and review findings |
|---|---|
packages/Webkul/DataTransfer/src/Queue/Worker.php |
Clears alarms on loop exit and updates PHP 8.4 syntax. Nit (1 vote): Add coverage for alarm cleanup when runJob() throws. |
packages/Webkul/Admin/tests/Feature/DataTransfer/Command/JobLauncherTest.php |
Adds alarm-cleanup coverage. Critical (1 vote): Guard or skip the assertion when ext-pcntl is unavailable. Moderate (1 vote): Remove the extra run() call after assertSuccessful(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2 of 6 tasks
navneetkumar-pim-webkul
added a commit
that referenced
this pull request
Sep 24, 2026
Six merged pull requests since v3.1.1 carried no changelog entry: the gallery unsaved badge and worker timeout handler (#713), the import filter re-render (#720), the worker timeout alarm (#722), the channel name on PostgreSQL (#725), the price input height (#726), and the export view events (#717, #718). The two fixes pending in #727 and #728 are included so the cut merges after them. The model recommendation entry described the cheap-tier ranking that #724 replaced with newest-per-family; it now describes the shipped behaviour. 3.1.2 is a patch release, so everything is filed under bug fixes and improvements; no feature section. Core::VERSION, package.json and the UPGRADE.md walkthrough move to 3.1.2 together, as they did for 3.1.1.
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
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.
Issue Reference
N/A — found while investigating the failing TIA Baseline run: https://github.com/unopim/unopim/actions/runs/35849737689/job/107144309998
Description
Webkul\DataTransfer\Queue\Worker::singleJobDaemon()armspcntl_alarm()(the job timeout) before fetching a job, thenbreaks out of the loop on an empty queue beforeresetTimeoutHandler()runs. The alarm outlives theunopim:queue:workcommand and,--timeoutseconds later (default 60), the framework's timeout handler callsWorker::kill()→posix_kill(getmypid(), SIGKILL)on whatever process invoked it.The TIA Baseline workflow runs
JobLauncherTestin-process, so the whole Pest run was SIGKILLed (exit 137) exactly 60.0s after that test finished — no memory pressure, no OOM entry in dmesg. It has failed this way on every run since at least 2026-08-21.Fix: reset the timeout handler in a
finallyblock so every exit path (empty queuebreak, or an exception fromrunJob()) disarms the alarm. Behaviour while a job is running is unchanged — a hung job is still killed at--timeout.Also applies Rector's PHP 8.4
newwithout parentheses on one untouched line in the same file.Impact: running
unopim:queue:workas its own CLI process behaves the same (the process exited anyway). Callers that invoke it in-process (Artisan::call(), tests) are no longer killed 60s later. Laravel's standardqueue:workis not affected.How To Test This?
vendor/bin/pest packages/Webkul/Admin/tests/Feature/DataTransfer/Command/JobLauncherTest.phpclears the worker timeout alarm once the queue is drainedassertspcntl_alarm(0) === 0after the command.Failed asserting that 60 is identical to 0.; with the fix all 4 tests pass.3.xshould no longer be killed with exit 137.Screenshots
N/A — no UI change.
Checklist
vendor/bin/pestpasses locally (JobLauncherTest + WorkerTest, 5 passed)vendor/bin/pint --testreports no style issues3.x(patch branch, merged up tomaster)Documentation