🐛 fix(lqa): stop qa_chunk_reviews penalty_points drifting from qa_entries - #4690
🐛 fix(lqa): stop qa_chunk_reviews penalty_points drifting from qa_entries#4690mauretto78 wants to merge 4 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.
|
@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! |
There was a problem hiding this comment.
Summary
The diagnosis behind this PR is right, and several parts of the fix are genuinely good. Three of the
sub-fixes are correct and I mutation-checked each one (restoring the old behaviour makes tests fail,
so they are held by the suite rather than passing by accident):
- Removing the
lqaModel === nullearly return inpassFailCountsAtomicUpdate(). A project with no
LQA model previously skipped the entire counter update, sopenalty_pointsnever accumulated at
all. That was a real drift source. - Removing the stale-read guard in
TranslationIssueModel::delete(). The old guard decided whether
to decrement based on an in-memory struct value read outside any lock, and silently skipped the
decrement when it looked wrong. destroyCachesFor(). None of the counter write paths busted the cached reads before, so the UI
could show a stale score for up to an hour.
The ON DUPLICATE KEY UPDATE delta statement itself is sound — a single self-referential statement
under the row lock is the correct shape for the increment/decrement path.
Full suite on this branch is green: OK (9217 tests, 30036 assertions).
That said, I do not think the concurrency fix holds up, and I found one arithmetic bug that stops
the new repair command from ever converging. Details and suggested fixes below.
Blocking
1. The lock is released before the transaction commits, so it does not serialize anything
This is the main one. ChunkReviewJobLock::run() releases the lock in its finally block, and every
call site sits inside an already-open transaction:
| Path | Transaction | Lock taken |
|---|---|---|
| create issue | SegmentTranslationIssueController.php begin():81 … commit():93 |
inside save():91 |
| update issue | begin():123 … commit():196 |
inside delete():174 and again inside save():183 |
| delete issue | begin():214 … commit():232 |
inside delete():231 |
| merge | JobSplitMergeService.php dispatch :635 … commit():641 |
inside the event handler |
So the sequence is: acquire → write → release → commit. A second process can take the lock the
instant our callback returns, while our writes are still uncommitted. It then reads a state that does
not include our change and writes an absolute value on top of it after we commit. That is exactly the
lost update this PR is trying to eliminate.
The update path is the clearest case: the decrement (delete()) and the increment (save()) take
and release the lock separately, so they are never under a single hold. Another writer can interleave
between them.
Worth double-checking the split path too — I could not confirm from the line numbers whether
postJobSplitted dispatch is inside the same transaction as the merge one is, so please verify.
Remediation. The lock has to wrap the transaction, not the other way round:
// SegmentTranslationIssueController::update()
ChunkReviewJobLock::run($idJob, function () {
$this->getDatabase()->begin();
try {
$model->delete();
$struct = $model->save();
$this->getDatabase()->commit();
} catch (Throwable $e) {
$this->getDatabase()->rollback();
throw $e;
}
return $struct;
});That gives one hold spanning both the decrement and the increment, and the lock is only released once
the work is visible to everyone else.
2. getPenaltyPointsForChunk() returns int and truncates the sum
lib/Model/LQA/ChunkReviewDao.php:144:
public function getPenaltyPointsForChunk(JobStruct $chunk, ?int $source_page = null): intBut the column is double(20, 2) (INSTALL/matecat.sql:882 and :922), and
EntryStruct::$penalty_points is ?float. There is no declare(strict_types=1) in the file, so
"7.50" silently becomes int(7).
This matters more than a rounding nit, because of where the value lands:
ChunkReviewModel::recountAndUpdatePassFailResult()writes it as an absolute value — so the
recount actively corrupts a row that was previously correct.- That recount is what
revision:recount-drifted --liveruns. - The new detector compares
ROUND(actual, 2) != ROUND(recorded, 2).
Net result on any chunk with fractional penalty points: the repair writes 7 where the truth is
7.50, the detector immediately flags the same row again, and the alert email reports it on every
run forever. The two new commands work against each other.
The same truncation exists at ReviewedWordCountModel.php:435
(getPenaltyPointsForSourcePage(): int), which is on the segment-status-change path.
Remediation. Return float from both, and cast explicitly rather than relying on coercion:
public function getPenaltyPointsForChunk(JobStruct $chunk, ?int $source_page = null): float
{
// ...
return (float)($count[0] ?? 0);
}Please add a real-SQL test with fractional penalties (two entries of 2.75, say) asserting that the
recount and the detector agree afterwards. Without one this will regress quietly.
3. The lock primitive is not safe enough for a data-integrity guarantee
ChunkReviewJobLock is the only caller of RedisHandler::tryLock() — it was unused before this PR,
so it is worth looking at closely now that it is load-bearing.
The TTL is the wait budget, not a lease. tryLock($key, $wait_time_seconds) sets the key's expiry
to the same value it uses as the acquisition timeout. ChunkReviewJobLock defaults to 5s, and
split/merge passes 10s. So the lock expires 5 or 10 seconds after acquisition regardless of how long
the work takes. postJobSplitted/postJobMerged do deleteByJobId plus N × createRecord plus
N × recountAndUpdatePassFailResult (three aggregate queries each). Going past 10s on a large job
under load is entirely plausible, at which point a second process enters the critical section and
nothing anywhere reports it.
Acquisition is not atomic. setnx and expire are two separate round trips. If the process dies
between them — fatal error, OOM, php-fpm timeout, deploy restart — the key is left with no TTL,
and unlock() only deletes on an identifier match that no future process will have. From then on
every issue operation on that job burns its full 5–10s wait, logs, and proceeds unlocked. Permanently,
and silently. That is worse than having no lock at all.
Remediation. Use one atomic command, and separate the lease from the wait:
// acquire: one round trip, TTL set atomically
$acquired = $conn->set($key, $identifier, 'EX', $leaseSeconds, 'NX');and give tryLock() a separate $leaseSeconds parameter sized to the work (say 30s for issue CRUD,
120s for split/merge) rather than reusing the caller's wait budget. unlock() should also be a Lua
compare-and-delete rather than GET then DEL, which can delete a successor's lock if the TTL lapsed
in between.
4. Several writers of the same rows never take the lock
ChunkReviewJobLock::run has exactly five call sites. These write qa_chunk_reviews.penalty_points
and are not among them:
BatchReviewProcessor.php:150— delta, on the segment approve/reject path (the highest-volume
writer in the product)BatchReviewProcessor.php:119— absolute recountAbstractRevisionFeature.php:358→QualityReportModel::resetScore()— absolute, sets the row to 0AbstractRevisionFeature.php:367alterChunkReviewStruct()— absolute, restores fromundo_datainternal_scriptsFixChunkReviewPenaltyPointsDrift.php:126— absolute recount
All the absolute ones are read-modify-write, so a delta landing between their read and their write is
lost. A lock that only two of the writer families take does not establish mutual exclusion — a
split/merge can still run against rows that BatchReviewProcessor is mid-transaction on.
The last one deserves special attention: the repair command is itself an unlocked lost-update
writer, and it also opens no transaction, so under ProxySQL its SELECTs are replica-routed while
its UPDATE goes to the primary. Run --live against live traffic and it can read a lagging replica
sum and write it authoritatively over a newer delta. The tool built to fix drift can introduce it.
Remediation. Either bring these paths under the same per-job lock, or scope the claim down and
lean on the atomic SQL plus the detector. Either way, the repair command needs the lock and needs its
reads pinned to the primary (wrapping it in a transaction is enough for ProxySQL persistence). The
docblock at ChunkReviewJobLock.php:10-13 currently describes coverage the code does not have.
5. The INSERT branch has no GREATEST clamp
In passFailCountsAtomicUpdate() the clamp only exists in the ON DUPLICATE KEY UPDATE clause. The
VALUES(...) list binds :penalty_points raw. If the row is absent when the statement lands — which
the deleteByJobId + recreate window in split/merge makes reachable — a subtract inserts a row with a
negative penalty_points.
Remediation. Clamp on insert too:
VALUES( :id, :id_job, :id_project, :password, :review_password,
GREATEST( :penalty_points, 0 ),
GREATEST( :reviewed_words_count, 0 ),
GREATEST( :total_tte, 0 ) )Related: save() and editFrom() use the ChunkReviewStruct loaded in the constructor, outside the
lock, while delete() re-reads inside it. Worth making those consistent.
6. The alert email tells operators to run a flag that does not exist
lib/View/Emails/ReviewExtended/penalty_points_drift_alert.html:27 says:
Run
revision:recount-drifted --dry-runto review, or without--dry-run…
FixChunkReviewPenaltyPointsDrift defines only two options, min-job-id and live. Symfony Console
will error on the undefined --dry-run, and the polarity is inverted anyway — running with no flag is
already the dry run, and --live is what performs the repair.
Remediation. Change the text to revision:recount-drifted to review and
revision:recount-drifted --live to apply.
Non-blocking, but worth addressing
The clamp hides the problem instead of reporting it. GREATEST(x, 0) discards the surplus with
nothing logged. When a decrement lands before its matching increment the row floors at 0 and the
difference is gone permanently — only a full recount fixes it, and per issue 2 the recount currently
truncates. On a data-integrity path this should be observable when it happens, not inferred from the
nightly email.
Fail-open cannot distinguish "Redis is down" from "another writer holds the lock." Both produce the
same non-error log line at ChunkReviewJobLock.php:35. The second case is the single most valuable
signal this class could emit — it means we are about to run unprotected, concurrently, right now. The
fail-open policy itself is defensible; it just needs to be visible. Consider logging at error level
with the exception class and elapsed wait, plus a caller-supplied context label.
The detection query is unbounded. findPenaltyPointsMismatches() joins qa_chunk_reviews to
jobs to qa_entries on e.id_segment BETWEEN j.job_first_segment AND j.job_last_segment — a range
join — then groups over the whole result with no LIMIT. With the default $minJobId = null this
scans every chunk review ever created, and the full result set is rendered one row per <tr> into an
email. Please EXPLAIN it against production-sized data before it runs on a schedule, and cap the
email body.
destroyCachesFor() runs inside the open transaction with no re-bust after commit. The window
between the DEL and the COMMIT is exactly when a concurrent reader can repopulate the 1-hour-TTL
caches with the pre-write value, which then sticks for the full hour. Busting after commit would be
safer.
Test coverage
The changed-line coverage gate reports 59% against an 80% threshold, with QualityReportModel at 0%,
AbstractRevisionFeature at 20% (139 changed lines) and PenaltyPointsDriftAlertEmail at 0%. The
drift CLI tests do exist, but they live in the internal_scripts submodule where the gate cannot see
them — worth noting in the PR description so it does not read as untested.
Both repos have to deploy together, since internal_scripts holds the only consumers of
findPenaltyPointsMismatches() and PenaltyPointsDriftAlertEmail.
The new ChunkReviewJobLockTest cases are meaningful — they would fail if the class were deleted —
but they only cover the single-threaded path. Nothing asserts that a second caller is actually
excluded while the first is inside the callback, which is the one property the class exists for, and
nothing asserts the lock outlives a long critical section. A test with a 2s callback under
waitSeconds = 1 that then checks the key still exists would have caught the TTL issue. The
wall-clock timing assertions will also likely flake on loaded CI.
Also worth a second look: the PR description says 239 tests / 904 assertions across the touched
suites; running the ones I could identify sums to 175 / 677. Probably the submodule tests making up
the difference, but worth confirming.
Suggested direction
Drop the Redis lock entirely rather than repair it — SELECT ... FOR UPDATE inside the transaction already there gives real mutual exclusion, commit-aligned release, and no fail-open mode, so the whole TTL/atomicity/coverage class of problems disappears instead of being patched.
Incremental deltas plus a best-effort external lock is a difficult shape for this invariant given
ProxySQL, the transaction boundaries above, and the number of writers spread across web, worker and
CLI. Two alternatives that would remove most of the blocking issues outright:
-
Make
penalty_pointsderived. Recompute in a single self-contained statement at each write
boundary:UPDATE qa_chunk_reviews r SET r.penalty_points = ( SELECT COALESCE(SUM(e.penalty_points), 0) FROM qa_entries e JOIN jobs j ON j.id = r.id_job AND j.password = r.password WHERE e.id_job = j.id AND e.id_segment BETWEEN j.job_first_segment AND j.job_last_segment AND e.source_page = r.source_page AND e.deleted_at IS NULL ) WHERE r.id = :id
Atomic, no lock, no clamp, and no PHP-side float handling to get wrong.
-
Keep the deltas and serialize on the row itself with
SELECT ... FOR UPDATEon the
qa_chunk_reviewsrow inside the existing transaction. Real mutual exclusion, released exactly at
commit, and no fail-open mode.
Happy to talk either through if useful.
…-drift Resolve conflicts between the penalty-points drift fix and develop's acting-user threading (d90bb2d), which made UserStruct $actingUser a required argument on ChunkReviewModel's recount and penalty-point methods. - AbstractRevisionFeature: keep the ChunkReviewJobLock wrapper and pass $event->actingUser, capturing $event in both closures so it is in scope - TranslationIssueModel: thread $this->actingUser through all three call sites; keep the removal of the protected subtractPenaltyPoints helper, whose >= 0 guard is the drift this branch fixes - internal_scripts: bump to the matching master merge, which gives revision:recount-drifted the same required uid argument develop added to revision:recount
🧪 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 |
Missing tests for the added cache invalidation logic. | |
lib/Plugins/Features/AbstractRevisionFeature.php |
Critical job locking logic wrap and cache invalidation lack specific test coverage. | |
lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php |
New email class has no corresponding test coverage for generation or sending. |
Result:
Why this WARNING?
- Coverage Analysis:
QualityReportModel.php,AbstractRevisionFeature.php, andPenaltyPointsDriftAlertEmail.phpfailed to meet the 80% threshold for changed lines → Action required: add unit tests for the new logic in these files. - Test File Matching:
PenaltyPointsDriftAlertEmail.phplacks a corresponding test file, whileQualityReportModel.phpandAbstractRevisionFeature.phphave existing tests that were not updated for the new changes → Action required: create a new test for the email class and update existing tests for the models. - AI Evaluation: Identified missing test coverage for cache invalidation logic in
QualityReportModel.php, job locking logic inAbstractRevisionFeature.php, and email generation/sending inPenaltyPointsDriftAlertEmail.php→ Action required: implement specific test cases for these identified gaps.
To resolve: Add or update unit tests to cover the new logic in QualityReportModel.php, AbstractRevisionFeature.php, and PenaltyPointsDriftAlertEmail.php.
…ting penalty points Addresses the review on #4690. Replace the Redis advisory lock with SELECT ... FOR UPDATE on the job's qa_chunk_reviews rows. The old lock released in a `finally` while every caller's transaction was still open, so a second process could enter the critical section, read state without our uncommitted change, and write an absolute value over it — the lost update the lock was meant to prevent. The update path was worse: delete() and save() took and released it separately, so they never spanned one hold. - ChunkReviewDao::lockByJobId() locks by id_job, not row id: split/merge deletes and recreates the rows, so no stable row exists. Under REPEATABLE READ the id_job index range also gap-locks, which closes the delete/recreate window. Throws outside a transaction, where FOR UPDATE would take the locks and drop them again immediately. - Taken inside ChunkReviewModel's recount and delta paths rather than at each call site, so the previously unlocked writers — BatchReviewProcessor, split/merge, the repair CLI — are covered. resetScore() and alterChunkReviewStruct() lock explicitly as they bypass ChunkReviewModel. - Delete ChunkReviewJobLock and its test. RedisHandler::tryLock() is unused again; document its TTL-as-wait-budget and non-atomic setnx+expire defects so it is not adopted as-is. Return float from getPenaltyPointsForChunk() and getPenaltyPointsForSourcePage(). penalty_points is double(20,2) and PDO returns SUM() as a string, so an int return silently made "7.50" into 7. The recount writes that back as an absolute while the detector compares to 2dp, so the repair corrupted correct rows and re-flagged them forever. Clamp the INSERT branch of passFailCountsAtomicUpdate, reachable for a subtract via the split/merge recreate window. The deltas are bound separately as :*_delta rather than read back with VALUES(penalty_points): VALUES() yields the value that would have been inserted, so clamping the list would turn every decrement into GREATEST(-3,0) = 0 and silently stop all subtraction. Correct the drift alert email, which named a --dry-run flag that does not exist, inverted the polarity, and omitted the now-required uid argument.
|
Thanks — this was a genuinely useful review. I checked all six blocking points against the code and they all hold. Pushed in ba2457c (plus 06b261a9 in Heads-up: remediation 5 as written breaks every decrementWorth flagging before anyone applies it elsewhere. Clamping the I applied your version verbatim first to check, and the decrement is a total no-op: So the insert clamp is in, but the deltas are bound a second time under their own The split path you asked me to verifyConfirmed defective, same as merge. Took your suggested directionDropped the Redis lock rather than repairing it. Two things worth calling out:
On your point 4: rather than patching each call site, the lock is taken inside Your point 3 dissolves with the lock gone, but it's slightly worse than described and I left a note on it: the identifier embeds a per-instance The repair CLI also had no transaction at all, so I added one; that fixes the ProxySQL replica-read hazard you flagged. CoverageYou were right that nothing asserted the actual exclusion property. Added Also added the fractional-penalty real-SQL test you asked for (two Not done yetDeliberately left your non-blocking items for a follow-up rather than growing this PR further — the unbounded One note on the diff: the truncation fix and the locking change both touch Both repos have to deploy together, as you noted. |
|
@Ostico when you have a moment — re-requested your review above. No rush. The one bit I would especially value your eyes on is the Also flagging again in case it is useful elsewhere: clamping the |
🧪 Test-Guard Report❌ FAIL — Some changed source files lack adequate test coverage. Coverage Analysis: ❌ FAILChanged lines: 79.0% covered (threshold: 80%) 📋 8 files: 3 ❌ fail, 5 ✅ pass
Test File Matching: ❌ FAILFile matching: 3 pass, 4 warning, 1 fail 📋 8 files: 1 ❌ fail, 4
|
| 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 |
✅ pass | Test file modified in PR: tests/unit/Core/Features/AbstractRevisionFeatureTest.php |
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/ReviewedWordCountModel.php |
Test file exists (tests/unit/Core/Features/ReviewExtended/ReviewedWordCountModelTest.php) but was not modified in this PR | |
lib/Plugins/Features/ReviewExtended/TranslationIssueModel.php |
✅ pass | Test file modified in PR: tests/unit/Core/Plugins/Features/ReviewExtended/TranslationIssueModelTest.php |
lib/Utils/Redis/RedisHandler.php |
Test file exists (tests/unit/Core/Utils/Redis/RedisHandlerTest.php) but was not modified in this PR |
Per-File Evaluation: ❌ FAIL
Evaluated 8 files: 2 via AI (1 batch), 6 via shortcuts.
📋 8 files: 1 ❌ fail, 2 ⚠️ warning, 4 ✅ pass, 1 ⏭️ skip
| File | Verdict | Reason |
|---|---|---|
lib/Model/LQA/ChunkReviewDao.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/AbstractRevisionFeature.php |
❌ fail | shortcut → coverage 50% < 80%, relevant tests exist but insufficient |
lib/Plugins/Features/ReviewExtended/ChunkReviewModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/ReviewedWordCountModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/TranslationIssueModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Utils/Redis/RedisHandler.php |
⏭️ skip | shortcut → trivial change (whitespace/comments only) |
lib/Model/QualityReport/QualityReportModel.php |
Missing verification of database locking and cache invalidation logic. | |
lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php |
New email class implementation lacks any tests for content generation or sending behavior. |
Result: ❌ FAIL
Why this FAIL?
- Coverage:
QualityReportModel.php,AbstractRevisionFeature.php, andPenaltyPointsDriftAlertEmail.phpfall below the 80% threshold → Action needed to increase test coverage. - Test File Matching:
PenaltyPointsDriftAlertEmail.phplacks a corresponding test file, while several other files have existing tests that were not updated despite code changes → Action needed to create/update test files. - AI Analysis:
QualityReportModel.phplacks verification for database locking and cache invalidation;PenaltyPointsDriftAlertEmail.phplacks tests for email content generation → Action needed to implement these specific test cases.
To resolve: Add unit tests for PenaltyPointsDriftAlertEmail.php and extend existing test suites for QualityReportModel.php and AbstractRevisionFeature.php to cover the identified logic gaps.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic Password | ba2457c | tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoRealSqlTest.php | View secret |
| - | - | Generic Password | ba2457c | tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoRealSqlTest.php | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
@Ostico one question on Test-Guard before I write more tests — it's your gate, so I'd rather follow your preference than guess. The gate is the only red check (
The first two are clear and I'll just fix them: make one The email is the one I want your call on.
I lean towards 2, since it also gives Unrelated, for the record: the |
2 participants
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).