Fix race conditions and implement queue metrics aggregation #2
+611
−97
Conversation
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
Implement queue-level metrics by aggregating job-level data to provide real-time queue performance insights for autoscaling decisions. - Add CalculateQueueMetricsAction for weighted average calculations - Add queue-metrics:calculate artisan command - Schedule automatic calculation every minute - Calculate throughput_per_minute and avg_duration_ms - Calculate failure_rate as percentage (0-100) - Support both all-queues and single-queue modes - Add comprehensive test coverage This enables the scaling package to accurately calculate worker requirements based on near real-time queue metrics.
Eliminate race conditions in Redis operations by wrapping critical sections in atomic transactions and using Lua scripts. Critical fixes: - Discovery set registration now atomic with recordStart() - Queue discovery consolidated into recordStart() transaction - Hostname metrics changed from pipeline to transaction - Failure recording fully atomic including hostname updates - Exception recording wrapped in transaction - Retry recording made atomic - Timeout recording made atomic - Throughput calculation uses Lua script for atomicity Medium priority fixes: - Add comprehensive documentation for sample methods - Clarify timestamp storage and ordering behavior This ensures data consistency under concurrent job processing without race conditions between separate Redis operations.
Remove markQueueDiscovered() mock expectations as queue discovery now happens atomically within recordStart() transaction.
Add proper type guards and remove unused dependencies to pass PHPStan level 9 analysis. - Add type guards for mixed array values in CalculateQueueMetricsAction - Add type assertions for command option values - Remove unused QueueMetricsRepository dependency from RecordJobStartAction - Handle mixed types from getMetrics() return values safely All 118 tests still passing with strict type safety.
… args Fixed critical bug in PipelineWrapper where addToSet() and removeFromSet() were passing arrays directly to PhpRedis sadd()/srem() commands instead of using spread operator. This caused job discovery to fail completely. Changes: - PipelineWrapper::addToSet(): Changed `sadd($key, $members)` to `sadd($key, ...$members)` - PipelineWrapper::removeFromSet(): Changed `srem($key, $members)` to `srem($key, ...$members)` - CalculateQueueMetricsTest: Added recordStart() calls before recordCompletion() - CalculateQueueMetricsTest: Added Redis flush in beforeEach() for clean state - CalculateQueueMetricsTest: Changed float assertions from toBe() to toEqualWithDelta() Root Cause: PhpRedis expects variadic arguments for set operations: - sadd(key, member1, member2, ...) ✅ - sadd(key, [member1, member2]) ❌ Impact: This bug prevented all job discovery tracking, causing listJobs() to return empty arrays even though job metrics were being recorded correctly. This made queue-level metric aggregation fail with 0 values.
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
This PR eliminates race conditions in Redis operations and implements queue-level metrics aggregation for real-time autoscaling decisions.
Changes
Queue Metrics Aggregation (Feature)
CalculateQueueMetricsAction- Aggregates job-level metrics into queue-level metricsqueue-metrics:calculate- Artisan command for manual/scheduled executionthroughput_per_minute- Jobs completed in last 60 secondsavg_duration_ms- Weighted average duration across all job classesfailure_rate- Percentage of failed jobs (0-100)Race Condition Fixes (Critical)
recordStart()Defensive Programming
Test Updates
RecordJobStartActionTestto reflect atomic queue discoveryTest Plan
✅ Completed Tests
RecordJobStartActionTest- All 5 tests passingCalculateQueueMetricsTest- All 6 scenarios covered (requires Redis)Test Suites Covered
tests/Unit/Actions/RecordJobStartActionTest.phptests/Feature/CalculateQueueMetricsTest.phptests/Unit/Actions/RecordJobCompletionActionTest.phptests/Unit/Actions/RecordJobFailureActionTest.phpIntegration Testing (Requires Redis)
To run Redis-dependent tests:
Breaking Changes
Failure Rate Format: The
failure_ratemetric now returns a percentage (0-100) instead of a ratio (0-1). Applications expecting the ratio format should divide by 100.Performance Impact
Migration Notes