🐛 fix(lqa): stop qa_chunk_reviews penalty_points drifting from qa_entries - #4690
Open
mauretto78 wants to merge 2 commits into
Open
🐛 fix(lqa): stop qa_chunk_reviews penalty_points drifting from qa_entries#4690mauretto78 wants to merge 2 commits into
mauretto78 wants to merge 2 commits into
Conversation
Picks up revision:check-penalty-drift and revision:recount-drifted (matecat/internal_scripts#46), needed by the qa_chunk_reviews penalty_points drift fix in this PR.
…ries qa_chunk_reviews.penalty_points was drifting from the live sum of qa_entries.penalty_points in production. Four contributing causes, ranked by probability during investigation: - TranslationIssueModel::delete() pre-checked a stale, non-locking read of the current total before deciding whether to subtract, silently skipping the decrement when it looked like it would go negative, while the qa_entries row was already unconditionally soft-deleted. The DAO's atomic GREATEST(...,0) clamp already handles this safely, so the guard is removed and the subtract now always runs. - ChunkReviewDao::passFailCountsAtomicUpdate() returned early — writing nothing at all — whenever a project had no LQA model, permanently freezing penalty_points/reviewed_words_count/total_tte. Counters now always write; only the is_pass clause (which needs the model's force_pass_at threshold) is skipped without one. - Job split/merge deletes and recreates qa_chunk_reviews rows with no locking, racing against concurrent single-issue add/edit/delete on the same job. Both paths now serialize per job_id via a new best-effort Redis lock (Utils\LQA\ChunkReviewJobLock, built on the previously-unused RedisHandler::tryLock/unlock) — it fails open on Redis errors/timeouts so this can never make core review functionality hard-depend on Redis availability. - None of the counter/pass-fail write paths invalidated the Redis caches on findChunkReviews/findByProjectId/ findByJobIdReviewPasswordAndSourcePage, so pages could show a stale score/pass-fail badge after a correct write. All four write paths now call the new ChunkReviewDao::destroyCachesFor(). Also adds ChunkReviewDao::findPenaltyPointsMismatches(), the shared detection query behind the two new CLI tasks in the internal_scripts submodule (revision:check-penalty-drift, revision:recount-drifted).
🧪 Test-Guard ReportCoverage Analysis: ❌ FAILChanged lines: 59.0% covered (threshold: 80%) 📋 7 files: 3 ❌ fail, 4 ✅ pass
Test File Matching: ❌ FAILFile matching: 3 pass, 3 warning, 1 fail 📋 7 files: 1 ❌ fail, 3
|
| File | Verdict | Reason |
|---|---|---|
lib/Model/LQA/ChunkReviewDao.php |
✅ pass | Test file modified in PR: tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoTest.php |
lib/Model/QualityReport/QualityReportModel.php |
Test file exists (tests/unit/Core/Model/QualityReport/QualityReportModelTest.php) but was not modified in this PR | |
lib/Plugins/Features/AbstractRevisionFeature.php |
Test file exists (tests/unit/Core/Features/AbstractRevisionFeatureTest.php) but was not modified in this PR | |
lib/Plugins/Features/ReviewExtended/ChunkReviewModel.php |
Test file exists (tests/unit/Core/Plugins/Features/ReviewExtended/ChunkReviewModelTest.php) but was not modified in this PR | |
lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php |
❌ fail | No matching test file found |
lib/Plugins/Features/ReviewExtended/TranslationIssueModel.php |
✅ pass | Test file modified in PR: tests/unit/Core/Plugins/Features/ReviewExtended/TranslationIssueModelTest.php |
lib/Utils/LQA/ChunkReviewJobLock.php |
✅ pass | Test file modified in PR: tests/unit/Core/Utils/LQA/ChunkReviewJobLockTest.php |
Per-File Evaluation: ⚠️ WARNING
Evaluated 7 files: 3 via AI (1 batch), 4 via shortcuts.
📋 7 files: 3 ⚠️ warning, 4 ✅ pass
| File | Verdict | Reason |
|---|---|---|
lib/Model/LQA/ChunkReviewDao.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/ChunkReviewModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/TranslationIssueModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Utils/LQA/ChunkReviewJobLock.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Model/QualityReport/QualityReportModel.php |
Update calls destroyCachesFor to cover cache invalidation; coverage is 0%. | |
lib/Plugins/Features/AbstractRevisionFeature.php |
Test coverage is partial (20%) and covers only some code paths, especially around locking and review processing. | |
lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php |
This is a new class with no existing tests; coverage is 0%. |
Result:
Why this WARNING?
- Coverage: lib/Model/QualityReport/QualityReportModel.php has 0% coverage; update calls to destroyCachesFor are missing, leading to cache invalidation issues.
- Coverage: lib/Plugins/Features/AbstractRevisionFeature.php has only 20% coverage, mainly around locking and review processing; more comprehensive tests are needed.
- Test Matching: lib/Plugins/Features/ReviewExtended/PenaltyPointsDriftAlertEmail.php has no matching test file; this class is new and requires test coverage.
- File Matching: Several files (e.g., QualityReportModel.php, AbstractRevisionFeature.php, ChunkReviewModel.php) have existing tests but were not modified in this PR, leading to warnings.
To resolve: add tests for lib/Plugins/Features/ReviewExtended/PenaltyPointsDriftAlertEmail.php and increase coverage for QualityReportModel.php and AbstractRevisionFeature.php.
Contributor
Author
|
@Ostico could you take a look at this one when you have a chance? Summary:
Also added 239 tests / 904 assertions passing, PHPStan clean on every changed file. Full analysis + design tradeoffs are in the PR description. Thanks in advance! |
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
1 participant
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
qa_chunk_reviews.penalty_pointswas drifting away from the liveSUM(qa_entries.penalty_points)in production only. Root-cause analysis foundtwo production-only application-level causes plus two related weaknesses,
fixed together with new detection/repair tooling.
Type
feat— new user-facing featurefix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coverageChanges
lib/Plugins/Features/ReviewExtended/TranslationIssueModel.phpGREATEST(...,0)clamp at zero. Wraps save/delete/editFrom in the new per-job lock.lib/Model/LQA/ChunkReviewDao.phppassFailCountsAtomicUpdate()no longer early-returns (and writes nothing) when a project has no LQA model — counters always write, only theis_passclause is conditional. AddeddestroyCachesFor()andfindPenaltyPointsMismatches().lib/Utils/LQA/ChunkReviewJobLock.phpRedisHandler::tryLock/unlock) serializing the single-issue write path against job split/merge. Fails open on Redis errors/timeouts.lib/Plugins/Features/AbstractRevisionFeature.phppostJobSplitted()/postJobMerged()now useChunkReviewJobLock;alterChunkReviewStruct()now busts caches after its write.lib/Plugins/Features/ReviewExtended/ChunkReviewModel.php_updatePassFailResult,recountAndUpdatePassFailResult) now bust caches after writing.lib/Model/QualityReport/QualityReportModel.phpupdateChunkReview()now busts caches after writing (used byresetScore()).lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php,lib/View/Emails/ReviewExtended/penalty_points_drift_alert.htmlinternal_scriptsPR.internal_scripts(submodule bump)revision:check-penalty-driftandrevision:recount-driftedfrom matecat/internal_scripts#46.Testing
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstanpasses (0 errors, with baseline)239 tests / 904 assertions passing across the touched suites
(
TranslationIssueModelTest,ChunkReviewDaoTest,ChunkReviewDaoRealSqlTest,AbstractRevisionFeatureTest,ChunkReviewModelTest,QualityReportModelTest,new
ChunkReviewJobLockTest). PHPStan clean (0 errors) on every changed file.Companion submodule PR (matecat/internal_scripts#46) has its own 34/34 passing
suite for the two new CLI tasks.
AI Disclosure
Claude Code (claude-sonnet-5)
Notes
Depends on matecat/internal_scripts#46 — merge that first (already reflected
by this PR's submodule pointer bump commit).
This fixes the code paths that cause future drift; it does not repair
qa_chunk_reviewsrows that already drifted in production. Once merged, runrevision:check-penalty-driftto find currently-affected jobs andrevision:recount-drifted --liveto repair them (dry-run by default).