fix(job): scope lock to --run, fix exit code, remove dead code#238
Open
icciaaron wants to merge 2 commits intoFreePBX:release/17.0from
Open
fix(job): scope lock to --run, fix exit code, remove dead code#238icciaaron wants to merge 2 commits intoFreePBX:release/17.0from
icciaaron wants to merge 2 commits intoFreePBX:release/17.0from
Conversation
Three issues in the fwconsole job command:
1. Lock acquired too early: the LockableTrait lock was acquired at
the top of execute(), blocking --list, --enable, and --disable
when a --run was already in progress. These non-destructive
operations do not need mutual exclusion.
2. Lock contention returned exit code 0 (success). Cron and scripts
could not detect that the job run was skipped. Return 1 and wrap
the message in <error> tags for console visibility.
3. Dead code: duplicate if($input->getOption('disable')) block at
line 61 (copy-paste of the block at line 56) was unreachable and
contained unrelated runJobs() logic.
Move the lock to immediately before the --run dispatch so that
--list, --enable, and --disable execute without contention.
Reported-by: Aaron Salsitz <asalsitz@icci.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
The lock must only be acquired when --run is present. Without this gate, running `fwconsole job` with no arguments (help display) would unnecessarily contend for the lock and show an error instead of help when another --run is in progress. Consolidate the two --run branches (run-all vs run-specific) into a single --run block for clarity. Co-Authored-By: Claude Code <noreply@anthropic.com>
Author
|
Hi @jissphilip @kapilgupta01 — this one scopes the --run lock so it doesn't block other fwconsole job operations, fixes an incorrect exit code, and drops some dead code. We hit this while debugging a cron issue on customer systems. Happy to adjust anything that doesn't match your preferences. |
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.
Summary
Three issues in
fwconsole jobthat combine to silently prevent time-sensitive jobs (like time condition BLF updates) from executing.1. Lock scope too broad
The
LockableTraitlock is acquired at the top ofexecute(), blocking all operations — including--list,--enable, and--disable— when a--runis already in progress. These non-destructive operations do not modify job state or interfere with running jobs.Fix: Move the lock to immediately before the
--rundispatch.--list,--enable, and--disablenow execute without contention.2. Lock contention returns exit code 0
When a second
fwconsole job --runcannot acquire the lock, it returns0(success). Cron, scripts, and monitoring cannot distinguish a successful run from a completely skipped one.Fix: Return
1(failure) and wrap the message in<error>tags for console visibility.3. Dead code block
Lines 61–64 contain a duplicate
if(\$input->getOption('disable'))check that is identical to the block at lines 56–59 — making it unreachable. The body references\$input->getOption('run')suggesting it was intended as the--rundispatch but was never corrected.Fix: Remove the dead block.
Impact
On busy PBXact/FreePBX systems with 27+ registered jobs, a single slow job (calendar sync, firewall update, SangomaConnect keepalive) can hold the process-wide lock past the 60-second cron interval. The next
fwconsole job --runsilently exits with code 0, skipping all jobs including time-sensitive ones like the timeconditionsschedtcjob that updates BLF hint states.With the lock scoped to
--runonly, administrators can still query and manage jobs (--list,--enable,--disable) during a long-running job execution.Environment
LockableTrait)Related
Discovered during fleet maintenance by Aaron Salsitz (Master Bitherder) at ICCI, LLC. Analysis and fix developed with Claude Code.