🐛 fix(lqa): stop qa_chunk_reviews penalty_points drifting from qa_entries - #4690
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 | |
|---|---|---|---|---|---|
| 35970300 | Triggered | Generic Password | 3707ce8 | tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoRealSqlTest.php | View secret |
| 35970301 | Triggered | Generic Password | 3707ce8 | tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoTest.php | View secret |
| 35970303 | Triggered | Generic Password | 3707ce8 | tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoRealSqlTest.php | View secret |
| 35970302 | Triggered | Generic Password | 3707ce8 | 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 |
Ostico
left a comment
Ostico
left a comment
There was a problem hiding this comment.
Review
This is a good revision. The change of approach since the last round is the right one, and it solved the hard part of the problem rather than working around it. Below I've written out what the new design gets right, what changed since the previous review, and the things I'd like fixed before this merges — with the reasoning for each, so the fixes are easy to apply confidently.
The approach is right
The original problem: qa_chunk_reviews.penalty_points is a running total, and several different parts of the system write it. Some writes are deltas ("add 3", "subtract 2.5"), others are absolute ("recount everything and set the total to 12"). When a delta and an absolute write overlap, the absolute one reads the total, computes a new value, and writes it back — and any delta that landed in between is silently erased. That is the drift.
The previous version tried to fix this with a Redis advisory lock. The problem was the release timing: the lock was released in a finally block, but the caller's database transaction was still open. So the sequence was acquire → write → release → commit. Another process could take the lock the instant the callback returned, read a state that did not yet include the uncommitted change, and write over it after the commit landed. The lock existed but did not actually exclude anything.
The new version replaces it with ChunkReviewDao::lockByJobId():
SELECT id FROM qa_chunk_reviews WHERE id_job = :id_job ORDER BY id FOR UPDATEThis is a much better fit, for reasons worth spelling out:
- The lock releases at commit, not before. InnoDB holds row locks until the transaction ends. That removes the entire class of bug above — there is no window where the lock is gone but the write is not yet visible.
- There is no fail-open path. The Redis lock logged and continued when Redis was unavailable, which meant it could silently stop protecting anything. Row locks either work or the query fails loudly.
- It covers writers automatically. Because the lock lives in the DAO and the models call it, most callers get it without having to remember to opt in.
Guarding it with if (!$conn->inTransaction()) throw is also the right instinct. Outside a transaction, autocommit would drop the locks the moment the SELECT returned, so the code would look protected while protecting nothing. Failing loudly beats that. The guard does need two call sites fixed before merge — see item 1.
I checked the schema: qa_chunk_reviews has KEY id_job (id_job), so this is an index range scan and not a whole-table lock. Good.
What improved since the last round
All six blocking items from the previous review are addressed:
- Lock/commit ordering — resolved by the design change above.
- Truncated penalty points —
getPenaltyPointsForChunk()andReviewedWordCountModel::getPenaltyPointsForSourcePage()now returnfloatinstead ofint. This mattered more than a rounding nit: the column isdouble(20,2), so7.50was becoming7, the repair command was writing that truncated value back as the authoritative total, and the detector then re-flagged the same row forever. The two tools were fighting each other; now they agree. - The unsafe Redis lock primitive — removed along with the lock itself.
- Writers that never took the lock — locking now happens inside
ChunkReviewModel::_updatePassFailResult()and::recountAndUpdatePassFailResult(),QualityReportModel::resetScore(),alterChunkReviewStruct()and the split/merge handlers. Putting it there rather than at each call site is the better choice:BatchReviewProcessor, the highest-volume writer, is covered without touching it. - The missing
GREATESTclamp on theINSERTbranch — added. I want to call out the:*_deltahandling specifically, because it is subtle and the code gets it right:VALUES(col)returns the value that would have been inserted, which is the already-clampedGREATEST(:penalty_points, 0). Reading the delta back that way would turn a-3into0and silently drop every decrement. Binding the raw signed deltas under separate names avoids that. The explanatory comment is worth keeping. - The alert email's
--dry-runflag — corrected.
The internal_scripts bump also pins the repair loop's reads to the primary inside a transaction, which closes the ProxySQL replica-read hole where the repair tool could read a lagging replica and write a stale total authoritatively.
Static analysis is clean: PHPStan at level 8 with no baseline reports [OK] No errors across the changed lib/ files. The test suite is at parity with develop.
Please fix before merge
1. Two paths reach lockByJobId() without a transaction, so they throw
lockByJobId() throws when no transaction is open. I traced every path that reaches it; eleven are fine, two are not.
1a — chunk completion returns 500.
POST /api/app/set-chunk-completed lib/Routes/app_routes.php:217
SetChunkCompletedController::complete() no begin()
ProjectCompletion\Model\EventModel::save() no begin()
FeatureSet::dispatch(ProjectCompletionEventSavedEvent)
AbstractRevisionFeature::projectCompletionEventSaved() :361
QualityReportModel::resetScore() :152
ChunkReviewDao::lockByJobId() → RuntimeException
None of the controller, EventModel, or ChunkCompletionEventDao opens a transaction, and projectCompletionEventSaved() isn't overridden — so both ReviewExtended and SecondPassReview inherit it. Marking a chunk complete fails for every revision-enabled project. It's deterministic rather than a race, so it will show up immediately in QA. I confirmed it with a test that drives resetScore() against a connection reporting inTransaction() === false; it throws with the expected message.
Suggested fix — wrap it in EventModel::save(), so the event insert and the dispatch are one unit:
public function save(): void
{
$this->_checkStatusIsValid();
$db->begin();
try {
$this->chunkCompletionEventId = (int)$this->chunkCompletionEventDao->createFromChunk(
$this->chunk, $this->eventStruct
);
$project = $this->projectDao->findById($this->chunk->id_project)
?? throw new Exception('Project not found for chunk ' . $this->chunk->id_project);
$this->featureSet->loadForProject($project);
$this->featureSet->dispatch(new ProjectCompletionEventSavedEvent(
$this->chunk, $this->eventStruct, (int)$this->chunkCompletionEventId
));
$db->commit();
} catch (Throwable $e) {
$db->rollback();
throw $e;
}
}CompletionEventController already does exactly this for the undo direction, so it matches existing practice. It's also correct independently of the lock: resetScore() snapshots the pre-reset values into undo_data, and right now that snapshot and the completion event row can drift apart if anything downstream fails.
1b — revision:recount throws on every invocation.
internal_scripts/tasks/CommandLineTasks/SecondPassReview/FixChunkReviewRecordCounts.php:112-117 loops over the job's chunk reviews calling recountAndUpdatePassFailResult(), which now takes the lock at ChunkReviewModel.php:196. The file contains no begin(), and the submodule's AbstractTask has no begin/commit/transaction anywhere either — I grepped for it. So the guard fires on the first iteration.
This one is easy to miss because the task lives in the other repository, but it's worth catching now: revision:recount is the tool operators already use to repair drift by hand, so as things stand the PR ships a drift detector and a drift repairer while breaking the existing repair command. The fix is the same shape as the new task — wrap the loop in begin() / commit() / rollback().
1c — the test stub hides both of these.
tests/unit/TestHelpers/AbstractTest.php:97-101 stubs inTransaction() to true for the whole suite, with a comment stating that every production path that writes qa_chunk_reviews runs inside a transaction. That statement is what 1a and 1b disprove — and because the stub asserts it globally, no unit test can discover the exception. AbstractRevisionFeatureTest::setUp() was changed the same way, so the assertion at :239-270 passes only because of that stub.
This is the change that let the two broken paths through, so it's the most valuable one to undo. Could you default the stub to false and let individual tests opt in where they genuinely simulate a transactional path? A small test asserting that lockByJobId() throws outside a transaction would be worth adding too, since that guard is load-bearing and nothing exercises it today.
Worth noting why static analysis couldn't have caught either path: lockByJobId() declares @throws RuntimeException, but every caller up the chain already carried a catch-all @throws Exception, which satisfies missingCheckedExceptionInThrows without proving anything about transaction state. The annotations are correct; they just can't express this. Tests are the only place it could have been caught.
2. The INSERT branch omits source_page
passFailCountsAtomicUpdate() builds its INSERT column list at ChunkReviewDao.php:738-749 without source_page, even though the struct carries it and the same method uses it two lines up. So a genuine insert produces a row with source_page = NULL.
To be clear about how often this happens: the statement binds an explicit :id, so when that primary key already exists the row collides and ON DUPLICATE KEY UPDATE runs — the normal path, where the existing row keeps its source_page and nothing is lost. The insert branch only fires when the struct's id isn't in the table: a struct built in memory (getOrCreateChunkReviews), or a row deleted between load and write, which is the split/merge delete-and-recreate window. So this is an edge path, and it's pre-existing — the column was missing before this PR too. What changes is that the PR adds two consumers that depend on the column, which turns a dormant wart into a permanent one:
UNIQUE KEY job_pw_source_page (id_job, password, source_page)stops constraining them. In MySQL a unique index treats eachNULLas distinct from every otherNULL, so that key imposes nothing on a row whosesource_pageis NULL. Repeat calls with the same id still collide on the primary key and update correctly — but two different ids for the same(id_job, password, source_page)would normally be rejected by that unique key, and here both survive. That's the guard which would otherwise catch the create race in item 4, disabled for exactly the rows this branch produces.- The new detector joins
e.source_page = r.source_page(ChunkReviewDao.php:201), andNULLmatches nothing. So these rows are reported as drifted on every scan, and no repair can ever clear them.
That's the same never-converges failure mode the last round rejected, reached by a different route.
Suggested fix. ChunkReviewStruct.php:23 declares public int $source_page; — typed and non-nullable — so this is three lines with no cast or null-guard needed. Column list and VALUES:
$sql = "INSERT INTO
qa_chunk_reviews ( id, id_job, id_project, password, review_password, source_page, penalty_points, reviewed_words_count, total_tte )
VALUES(
:id,
:id_job,
:id_project,
:password,
:review_password,
:source_page,
GREATEST( :penalty_points, 0 ),
GREATEST( :reviewed_words_count, 0 ),
GREATEST( :total_tte, 0 )
) ON DUPLICATE KEY UPDATE
" . implode(",\n ", $setClauses) . ";";and the binding:
'review_password' => $chunkReview->review_password,
'source_page' => $chunkReview->source_page,
'password' => $chunkReview->password,Worth not adding source_page to $setClauses: on the update branch the row already holds the right value, and the column is part of the unique key, so rewriting it there is a no-op at best. The typed int also means an unset struct raises a loud Error rather than quietly binding NULL, which is the failure mode you want.
The new test already drives this branch, so extending its assertion is enough to keep it honest — right now it creates a NULL-source_page row and only checks the clamped counters:
$this->assertSame($chunkReview->source_page, (int)$row['source_page']);One thing the patch doesn't cover: rows already written with source_page = NULL stay invisible to the detector. Worth checking the size of that before deploying —
SELECT COUNT(*) FROM qa_chunk_reviews WHERE source_page IS NULL;If that comes back non-zero, backfilling needs a rule for deciding which page each orphan belonged to. That's a separate call from this PR, and your judgement rather than mine — but it's better to know the number now than to find it in the first alert email.
3. The repair CLI drops chunks on split jobs
FixChunkReviewPenaltyPointsDrift.php:113-116:
$jobs = [];
foreach ($mismatches as $row) {
$jobs[$row['id_job']] = $row['password'];
}In MateCat a split job is several jobs rows sharing the same id, distinguished by password — which is why qa_chunk_reviews keys on (id_job, password, source_page). Using id_job as the array key keeps only the last password seen and silently discards the others.
That matters because the lookup on the next line is password-scoped: findChunkReviews() resolves through _findChunkReviewsStatement(), whose condition is jobs.id = ? AND jobs.password = ?. So on a split job the task repairs one chunk, prints FIXED job N, and leaves the rest drifted — after which the check task alerts on them again on the next run. A green repair log sitting next to a permanent alert is a confusing thing to debug.
Suggested fix — key by both, e.g. $jobs["{$row['id_job']}-{$row['password']}"] = $row;, and iterate over id/password pairs. The comment at :111-112 needs a small correction too: it says the dedup is safe because recountAndUpdatePassFailResult() recomputes every source_page of the job at once, but that method recomputes only its own source_page (ChunkReviewModel.php:198-200). What actually makes the per-source_page dedup safe is the loop over $allChunkReviews below it.
4. Two code paths take the same two tables in opposite orders
Deadlocks happen when two transactions need the same two resources and grab them in different orders — each ends up holding what the other is waiting for. That's the situation here, and it's new in this revision:
| Path | Takes first | Then |
|---|---|---|
| Saving a translation | qa_entries — BatchReviewProcessor::process():152 → deleteIssues() → EntryDao::deleteEntry() |
qa_chunk_reviews — lockByJobId() at :158 |
| Creating/deleting an issue | qa_chunk_reviews — TranslationIssueModel::save():165, delete() |
qa_entries — createEntry() / deleteEntry() |
Before this change, delete() wrote qa_entries first, which matched the translate path. Hoisting the lock to the top of save()/delete() is what flipped the order.
Concretely, on one job: a translator's save marks an issue row deleted and holds that qa_entries lock; a reviewer's issue-delete takes the job's chunk-review locks; then the translator asks for the chunk-review locks and the reviewer asks for that same qa_entries row. MySQL detects the cycle and kills one of them — and since translate() rolls back and rethrows, the translator's segment save fails. This is ordinary concurrent use, not an exotic scenario.
Suggested fix — take the lock once at the very top of BatchReviewProcessor::process(), above both the getOrCreateChunkReviews() call and the deleteIssues loop. That gives you three things at once: a single qa_chunk_reviews → qa_entries order everywhere; no more redundant re-locking once per chunk review per event inside the loop; and it closes the create race at :105-137, where two concurrent requests can both miss the unlocked findChunkReviews() at :107 and both call createRecord() at :126. (With fix 2 applied the unique key would at least make the second one fail loudly; with the lock hoisted, neither happens.)
5. Redis and email work happen while the lock is held
_updatePassFailResult() takes the job-wide lock and then, still holding it, does three Redis round trips in destroyCachesFor() and dispatches ChunkReviewUpdatedEvent through FeatureSet — which runs plugin code that may enqueue to ActiveMQ. On the branch that creates a missing chunk review, BatchReviewProcessor:132 also sends an email synchronously.
The concern is hold time. This is the highest-volume write path in the product, the lock covers the whole job, and it's inside the web request's transaction. Every translator and reviewer working on that job queues behind it. If Redis or the mail server is slow, that turns into innodb_lock_wait_timeout waits — 50 seconds by default — on ordinary segment saves.
Suggested fix — move destroyCachesFor() to after the commit and enqueue the email rather than sending it inline. General rule worth carrying forward: no network calls inside a database lock.
6. Caches are invalidated before the commit
Related to the above but a correctness issue in its own right. destroyCachesFor() deletes the cache keys while the transaction is still open, and nothing re-busts them afterwards. In the window between the DEL and the COMMIT, a concurrent reader can read the old value from the database and repopulate the cache with it — and because the TTL is an hour, that stale value then outlives the commit and sticks around.
That's the same "displayed score doesn't match reality" symptom this PR is fixing, just moved from the database into Redis. Busting after the commit fixes it, and pairs naturally with fix 5. While you're there: createRecord() (:614) and deleteByJobId() (:658) bust nothing at all, so a create or delete leaves a stale findChunkReviews entry behind.
7. The drift detector query is unbounded
findPenaltyPointsMismatches() joins qa_chunk_reviews to jobs to qa_entries on a segment-id range, groups over the whole result, and has no LIMIT — $minJobId is a starting watermark, not a cap. Every row it returns becomes one <tr> in the alert email.
The awkward part is that the first run after deploy is exactly the run most likely to return a large set, and it's the one that goes out by email. Could you add a LIMIT plus a separate total count, and render something like "showing 50 of 812"? An EXPLAIN against production-sized data before this goes on a schedule would be worth the ten minutes.
One related detail: the scan itself runs outside any transaction in both new tasks (FixChunkReviewPenaltyPointsDrift.php:103 and CheckChunkReviewPenaltyPointsDrift.php:67), which under ProxySQL means it can be answered by a replica. The per-job repair transaction below it is correctly pinned to the primary, so the writes are safe — but replica lag can still produce alerts for rows that are already correct. Wrapping the scan in a transaction too would make the report match reality.
Smaller things, not blocking
- The gap-lock reasoning depends on the isolation level, and nothing says so. The
lockByJobId()docblock explains that gap locking blocks inserts during the split/merge delete-and-recreate window. That's true under REPEATABLE READ, and I confirmed the server is there today —@@global.transaction_isolationand@@session.transaction_isolationboth reportREPEATABLE-READon 5.7.44-48-log — so this is correct as shipped, not a bug. The point is that nothing inlib/,inc/orINSTALL/sets or asserts it; the guarantee rests on InnoDB's default holding forever. Under READ COMMITTED, InnoDB disables gap locking, so aFOR UPDATEmatching zero rows takes no locks at all and returns success — precisely the window this is meant to cover, failing silently. That matters here because this PR adds job-wide gap-lock contention on the hottest write path (item 5), and "switch to READ COMMITTED to reduce gap locking" is a plausible future response to that contention which would quietly disarm the guard. The docblock does mention REPEATABLE READ at:678-680, but as a description of a nice property rather than as a prerequisite. Promoting it to a stated requirement is the whole fix — something like:
* Requires REPEATABLE READ, InnoDB's default and what this installation runs. The gap lock is
* what covers the recreate window: while the job's rows are deleted the SELECT matches nothing,
* so there are no record locks to take, and the gap lock is the only thing standing between two
* concurrent recreates. READ COMMITTED disables gap locking, so the same SELECT would lock
* nothing and still return success — this method would degrade to a silent no-op precisely in
* the window it exists to protect. Do not lower the isolation level to relieve contention here
* without first replacing this with a lock on a row that always exists, e.g.
* SELECT id FROM jobs WHERE id = :id_job FOR UPDATE, which takes a real record lock at any
* isolation level.The last sentence is the load-bearing one: it puts the warning in front of the person most likely to break this — someone chasing lock contention, who would otherwise read the isolation level as free to change. Making that swap to jobs now would remove the dependency altogether, but it moves the lock to another table and could contend with unrelated job-level work, so I'd treat it as your call rather than an obvious win.
ORDER BY iddoesn't do what the comment says. Locks are acquired as rows are scanned, before any sort is applied, so the clause doesn't control acquisition order. And since every caller locks a singleid_jobwith the same predicate, they already scan the same rows in the same order. Harmless to keep, but the comment promises a guarantee the clause doesn't provide.editFrom()has no caller.SegmentTranslationIssueController::update()callsdelete()thensave(); nothing outside tests callseditFrom(). So the lock added at:126and its comment describe a flow that doesn't run. Worth deleting the method or moving that comment somewhere accurate.RedisHandler::tryLock()/unlock()are now unused. With the advisory lock gone they have no production caller. Their known issues are harmless while unused, but the next person who needs a lock may reach for them. Deleting them (and their tests) is cleaner than leaving a warning comment.- The null-LQA behaviour change isn't pinned by a test. Removing the early return means projects with no QA model now get a
qa_chunk_reviewsrow where previously they got noINSERTat all, andforce_pass_atis no longer computed viaReviewUtils::filterLQAModelLimit. MeanwhileChunkReviewModel.php:206-208setsis_pass = truefor that same case whilepassFailCountsAtomicUpdate()leaves itNULL— two paths, two answers for the same row.passFailCountsAtomicUpdateReturnsEarlyWhenLqaModelIsNullwas deleted without a replacement, so nothing records which behaviour is intended now. Worth picking one and writing the test. - Float comparison in the detector.
ROUND(a,2) != ROUND(b,2)onDOUBLEvalues can differ in the last bit even when logically equal, especially since one side accumulates incrementally and the other comes from a singleSUM().ABS(a - b) > 0.005is safer, and a false positive here recreates the flag-forever loop. - No test for
PenaltyPointsDriftAlertEmail. It's the piece an operator actually sees, and it's the only new class without one. - Coverage. The changed-line gate reads 59% against 80%, largely because the drift CLI's tests live in
internal_scriptswhere the gate can't see them. A line in the PR description saying so would stop it reading as untested. Also worth noting there that both repositories need to deploy together, sinceinternal_scriptsholds the only consumers offindPenaltyPointsMismatches()and the alert email.
Items 1, 2 and 3 are the ones that break things outright; 4 through 7 are the ones I'd want sorted before this sees production traffic. None of them touch the central design decision, which is the part that was hardest to get right — serialising on the rows themselves instead of an external lock is the correct call, and the rest is follow-through. Happy to talk any of this through.
…ck safe to hold Addresses the second review on #4690. QualityReportModel::resetScore() takes the job's qa_chunk_reviews row locks, but nothing between SetChunkCompletedController and it opened a transaction — so marking a chunk complete threw for every revision-enabled project. Wrap EventModel::save() with TransactionalTrait so the event row and the listeners it dispatches are one unit, matching what CompletionEventController already does for the undo direction. It is also what makes resetScore()'s undo_data snapshot atomic with the completion-event row it describes. Default createDatabaseMock()'s inTransaction() stub to false. It returned true globally on the premise that every writer of qa_chunk_reviews was already transactional, which is exactly what the above disproves, and is why no unit test could find it. Tests that drive a real ChunkReviewDao now opt in explicitly; the flip also surfaced OauthResponseHandlerControllerTest, which depends on MembershipDao::createList()'s own transaction guard. passFailCountsAtomicUpdate() omitted source_page from its INSERT, so a genuine insert wrote NULL — exempting the row from UNIQUE KEY job_pw_source_page, since MySQL treats every NULL as distinct, and hiding it from the detector's source_page join so no recount could ever clear it. createRecord() read its id from lastInsertId(), which MySQL leaves at 0 on the ON DUPLICATE KEY UPDATE update branch; it now re-reads the row, which is the upstream trigger for that same spurious insert. Take the job lock once at the top of BatchReviewProcessor::process(). It wrote qa_entries first and locked qa_chunk_reviews second, the opposite order to TranslationIssueModel — an ABBA deadlock under ordinary concurrent use, newly reachable now that the lock is a row lock held to commit rather than a Redis advisory lock outside InnoDB's wait-for graph. Hoisting it also closes the unlocked find-then-create race in getOrCreateChunkReviews() and drops the per-event re-locking. Add IDatabase::onCommit() and defer every cache bust and both mail enqueues past the commit. Busting inside the transaction let a concurrent reader repopulate from the pre-commit row, and that stale value outlived the commit for the full TTL — the same wrong-score symptom this PR fixes, moved into Redis. createRecord() and deleteByJobId() busted nothing at all, so split/merge left the 10-minute ProjectUrls cache serving revise URLs for deleted review_passwords. The ChunkReviewUpdatedEvent dispatches stay inside the transaction: a plugin listener may write rows that must be atomic with the counter update. Bound the detector with an optional limit plus countPenaltyPointsMismatches(), and compare with ABS(a - b) > 0.005 rather than ROUND(a,2) != ROUND(b,2) — rounded doubles are still floats, and a bit of residue re-flags a row the repair cannot settle. The alert email reports "showing N of M" and lists the password, without which two drifted chunks of a split job render identically. Recount now leaves is_pass NULL when the project has no LQA model, agreeing with passFailCountsAtomicUpdate() and with QualitySummary, which already reads NULL as "no verdict"; true asserted a verdict that was never computed. CattoolController no longer renders that as 'fail'. State REPEATABLE READ as a prerequisite on lockByJobId() — the gap lock covering the split/merge recreate window is the whole guarantee, and READ COMMITTED would silently reduce the method to a no-op there — with a real-SQL test asserting the level. Correct the ORDER BY comment, which claimed a guarantee the clause cannot provide: locks are taken during the index scan, before any sort. Delete TranslationIssueModel::editFrom() and RedisHandler::tryLock()/unlock() with their tests. Neither has a production caller, and the Redis primitives were documented as having correctness defects, which a warning cannot stop the next person from adopting.
…ob repair fixes Picks up matecat/internal_scripts#46: revision:recount now runs inside a transaction (it threw on every invocation once the recount started taking the qa_chunk_reviews row locks), revision:recount-drifted repairs every chunk of a split job instead of only the last password seen, both mismatch scans are pinned to the primary, and both commands are bounded by --limit with an uncapped total.
|
Thanks — all seven blocking items plus the smaller ones are addressed. Two commits here What changed
Smaller items: REPEATABLE READ promoted to a stated prerequisite (plus a real-SQL test asserting Three places I diverged from the review, with reasoning
I also left the Two things worth your eyeThe test schema is not the production schema, and both divergences hide exactly the bugs in item 2
The NULL that disables One test I could not make fail. Every other new test was verified to fail against the unfixed code, not merely to pass. Verification9646 tests / 31088 assertions; the only failures are the four Pre-deploy items are in the PR description: the |
🧪 Test-Guard Report❌ FAIL — Some changed source files lack adequate test coverage. Coverage Analysis: ❌ FAILChanged lines: 93.0% covered (threshold: 80%) 📋 14 files: 3 ❌ fail, 11 ✅ pass
Test File Matching: ❌ FAILFile matching: 10 pass, 2 warning, 2 fail 📋 14 files: 2 ❌ fail, 2
|
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/SetChunkCompletedController.php |
Test file exists (tests/unit/Core/Controllers/SetChunkCompletedControllerTest.php) but was not modified in this PR | |
lib/Controller/Views/CattoolController.php |
❌ fail | No matching test file found |
lib/Model/DataAccess/Database.php |
✅ pass | Test file modified in PR: tests/unit/Core/Model/DataAccess/DatabaseTest.php |
lib/Model/DataAccess/IDatabase.php |
❌ fail | No matching test file found |
lib/Model/LQA/ChunkReviewDao.php |
✅ pass | Test file modified in PR: tests/unit/Core/DAO/TestChunkReviewDAO/ChunkReviewDaoTest.php |
lib/Model/QualityReport/QualityReportModel.php |
✅ pass | Test file modified in PR: tests/unit/Core/Model/QualityReport/QualityReportModelTest.php |
lib/Plugins/Features/AbstractRevisionFeature.php |
✅ pass | Test file modified in PR: tests/unit/Core/Features/AbstractRevisionFeatureTest.php |
lib/Plugins/Features/ProjectCompletion/Model/EventModel.php |
✅ pass | Test file modified in PR: tests/unit/Core/Plugins/ProjectCompletion/EventModelTest.php |
lib/Plugins/Features/ReviewExtended/BatchReviewProcessor.php |
✅ pass | Test file modified in PR: tests/unit/Core/Plugins/Features/ReviewExtended/BatchReviewProcessorTest.php |
lib/Plugins/Features/ReviewExtended/ChunkReviewModel.php |
✅ pass | Test file modified in PR: tests/unit/Core/Plugins/Features/ReviewExtended/ChunkReviewModelTest.php |
lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php |
✅ pass | Test file modified in PR: tests/unit/Core/Plugins/Features/ReviewExtended/PenaltyPointsDriftAlertEmailTest.php |
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 |
✅ pass | Test file modified in PR: tests/unit/Core/Utils/Redis/RedisHandlerTest.php |
Per-File Evaluation: ❌ FAIL
Evaluated 14 files: 1 via AI (1 batch), 13 via shortcuts.
📋 14 files: 2 ❌ fail, 1 ⚠️ warning, 9 ✅ pass, 2 ⏭️ skip
| File | Verdict | Reason |
|---|---|---|
lib/Controller/API/App/SetChunkCompletedController.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Model/DataAccess/Database.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Model/DataAccess/IDatabase.php |
⏭️ skip | shortcut → in coverage report, but no executable lines changed |
lib/Model/LQA/ChunkReviewDao.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Model/QualityReport/QualityReportModel.php |
❌ fail | shortcut → coverage 25% < 80%, relevant tests exist but insufficient |
lib/Plugins/Features/AbstractRevisionFeature.php |
❌ fail | shortcut → coverage 62% < 80%, relevant tests exist but insufficient |
lib/Plugins/Features/ProjectCompletion/Model/EventModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/BatchReviewProcessor.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/ChunkReviewModel.php |
✅ pass | shortcut → coverage 100% ≥ 80% |
lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php |
✅ pass | shortcut → coverage 85% ≥ 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 → in coverage report, but no executable lines changed |
lib/Controller/Views/CattoolController.php |
Logic change for quality class rendering lacks corresponding unit test coverage. |
Result: ❌ FAIL
Why this FAIL?
- Coverage Analysis:
lib/Model/QualityReport/QualityReportModel.php(25%) andlib/Plugins/Features/AbstractRevisionFeature.php(62%) fall below the 80% threshold → Action required: increase test coverage for these files. - Test File Matching:
lib/Controller/Views/CattoolController.phplacks a corresponding unit test file → Action required: create a unit test for this controller. - Test File Matching:
lib/Model/DataAccess/IDatabase.phplacks a corresponding unit test file → No action needed: this is an interface, which does not require direct unit testing. - Per-File Evaluation:
lib/Controller/Views/CattoolController.phplogic changes lack test coverage → Action required: implement unit tests to cover the new logic.
To resolve: Increase unit test coverage for QualityReportModel.php and AbstractRevisionFeature.php, and add a new unit test file for CattoolController.php.
| // enough to report a row as drifted — which the repair cannot settle, because it writes the | ||
| // sum and the column rounds it back, so the row is flagged again on the next scan. 0.005 is | ||
| // half the smallest storable unit at 2dp, i.e. "differs by at least one storable cent". | ||
| $sql = "SELECT |
There was a problem hiding this comment.
In production, penalty point is NOT double(10,2) but correctly penalty_points decimal(11,2) DEFAULT NULL,`
Ostico
left a comment
Ostico
left a comment
There was a problem hiding this comment.
I set the PR in a request change state. Just to block its merging before the security patch since this review must be reviewed after that patch.
| * @throws PDOException | ||
| * @throws ReflectionException | ||
| */ | ||
| public function destroyCachesFor(ChunkReviewStruct $chunkReview): void |
There was a problem hiding this comment.
This method for sure will have conflicts with the new security branch fixes. Expect conflicts here.
| // takes the insert branch and creates a duplicate row. Both branches leave exactly one row | ||
| // identified by job_pw_source_page, so read it back; the lookup is uncached, so it sees the | ||
| // row this statement just wrote inside the caller's transaction. | ||
| $struct = $this->findByIdJobAndPasswordAndSourcePage( |
There was a problem hiding this comment.
This probably also will have conflicts.
This comment was marked as outdated.
This comment was marked as outdated.
develop removed findByJobIdReviewPasswordAndSourcePage() and its eviction when the revision phase stopped being resolved from a client declared value, so destroyCachesFor() was busting a key family that no longer exists while leaving the per source page reads it does not cover. It now composes the credential keyed door, which evicts those, with the project keyed one, which a credential cannot reach. The two tests for the removed branch are replaced by one over the new body.
e726f3a to
c45a53c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…the verdict
The View infix is house style rather than disambiguation here - only ActivityLogController shares a
basename with a second source file. The adequacy gate globs tests/**/{name}Test.php against the
source basename, so the drift read as a missing test file.
The verdict branch of overallQualityClass had no coverage either: the existing cases only ever fed a
NULL is_pass, which is the branch that returns an empty class.
alterChunkReviewStruct had no coverage at all before the lock was added to it, and the two onCommit deferrals were asserted nowhere - a bust moved back inside the transaction would have gone unnoticed. Each new case was checked against the unfixed line, not only against the fixed one.
🧪 Test-Guard Report✅ PASS — All changed source files have adequate test coverage. Coverage Analysis: ✅ PASSChanged lines: 98.0% covered (threshold: 80%) 📋 14 files: 14 ✅ pass
Result: ✅ PASS |
adfaf13 to
3729412
Compare
🧪 Test-Guard Report✅ PASS — All changed source files have adequate test coverage. Coverage Analysis: ✅ PASSChanged lines: 98.0% covered (threshold: 80%) 📋 14 files: 14 ✅ pass
Result: ✅ PASS |
🧪 Test-Guard Report✅ PASS — All changed source files have adequate test coverage. Coverage Analysis: ✅ PASSChanged lines: 98.0% covered (threshold: 80%) 📋 14 files: 14 ✅ pass
Result: ✅ PASS |
…w-penalty-points-drift
🧪 Test-Guard Report✅ PASS — All changed source files have adequate test coverage. Coverage Analysis: ✅ PASSChanged lines: 98.0% covered (threshold: 80%) 📋 14 files: 14 ✅ pass
Result: ✅ PASS |
3 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.
The write path is serialised on the rows themselves —
ChunkReviewDao::lockByJobId()takesSELECT ... FOR UPDATEon the job'sqa_chunk_reviewsrows, so the lock releases at commit ratherthan on a timer and every writer is covered without opting in.
Type
feat— new user-facing featurefix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coverageChanges
lib/Model/LQA/ChunkReviewDao.phplockByJobId()(SELECT ... FOR UPDATEonid_job, throws outside a transaction) as the serialisation point for every writer.passFailCountsAtomicUpdate()no longer early-returns when a project has no LQA model, and now writessource_pageon its INSERT branch — omitting it leftNULL, which exempts the row fromUNIQUE KEY job_pw_source_pageand hides it from the detector'ssource_pagejoin.createRecord()re-reads the row instead of trustinglastInsertId(), which MySQL leaves at 0 on the ODKU update branch.deleteByJobId()/createRecord()now invalidate caches, which they never did. AddedfindPenaltyPointsMismatches()(bounded, withcountPenaltyPointsMismatches()) anddestroyCachesFor().lib/Model/DataAccess/Database.php,IDatabase.phponCommit(): defers work until the transaction commits, discards it on rollback, runs it inline when no transaction is open, and logs rather than propagates a callback failure. Cache invalidation and mail enqueues now run through it.lib/Plugins/Features/ReviewExtended/TranslationIssueModel.phppenalty_pointsdecrement on delete; now always subtracts and lets the DAO's atomicGREATEST(...,0)clamp at zero. Wraps save/delete in the per-job lock. DeletededitFrom(), which had no production caller.lib/Plugins/Features/ReviewExtended/BatchReviewProcessor.phpprocess(). It previously wroteqa_entriesfirst and lockedqa_chunk_reviewssecond — the opposite order toTranslationIssueModel, i.e. an ABBA deadlock under ordinary concurrent use. Hoisting it also closes the unlocked find-then-create race ingetOrCreateChunkReviews(). Both mail sends are deferred past the commit.lib/Plugins/Features/ProjectCompletion/Model/EventModel.phpsave()wraps the event insert and the dispatch in one transaction viaTransactionalTrait. Without itPOST /api/app/set-chunk-completedthrew for every revision-enabled project, because the dispatch reachesresetScore()→lockByJobId(). Also makes theundo_datasnapshot atomic with the completion-event row.lib/Plugins/Features/AbstractRevisionFeature.phpcreateQualityReportModel()extracted so the reset is observable in tests.lib/Plugins/Features/ReviewExtended/ChunkReviewModel.phpis_passNULL when the project has no LQA model, agreeing withpassFailCountsAtomicUpdate()and withQualitySummary, which already reads NULL as "no verdict".lib/Controller/Views/CattoolController.phpis_passas'fail'.lib/Model/QualityReport/QualityReportModel.phpresetScore()locks before reading;updateChunkReview()defers its cache bust.lib/Utils/Redis/RedisHandler.phptryLock()/unlock()and the instance-identifier cluster that existed only to name a lock owner. No production caller since the advisory lock was withdrawn, and both had documented correctness defects.lib/Plugins/Features/ReviewExtended/Email/PenaltyPointsDriftAlertEmail.php,lib/View/Emails/ReviewExtended/penalty_points_drift_alert.htmlshowing N of Mwhen the result is capped, and listspassword— without it two drifted chunks of a split job render identically.internal_scripts(submodule bump)revision:check-penalty-driftandrevision:recount-drifted, therevision:recounttransaction fix, and the split-job repair fix from matecat/internal_scripts#46.Testing
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstanpasses (0 errors; the project has no baseline)9646 tests / 31088 assertions. The only failures are the four
CommentControllerTest::*broker_unavailable*cases, which are environmental: they assert a throwwhen ActiveMQ is unreachable, and it is reachable in the container. PHPStan level 8 reports 0 errors
across
lib,pluginsandinternal_scripts/tasks.The companion submodule PR (matecat/internal_scripts#46) has its own 43/43 suite.
Each regression test was verified to fail against the unfixed code, not merely to pass:
revision:recountthrowslockByJobId requires an open transaction, the split-job repair leaves thesecond chunk drifted, and the
source_pagetests report the column default instead of the intendedpage.
Coverage gate
The changed-line coverage gate reads low because the tests for
revision:check-penalty-drift,revision:recount-driftedandrevision:recountlive in the companion repository, atinternal_scripts/tasks/tests/CommandLineTasks/SecondPassReview/PenaltyPointsDriftTasksTest.php.test-guardreadsphp-coverage.xml, whichphpunit.xmlwhitelists to./libonly, so thesubmodule's own PHPUnit run is invisible to the gate however thorough it is. The parts that do live
in
lib/— the DAO query, the lock, the alert email and its template — are covered here byChunkReviewDaoRealSqlTest,ChunkReviewDaoTest,DatabaseTestand the newPenaltyPointsDriftAlertEmailTest.Two things the test schema cannot reproduce
Worth knowing when reading the tests, because both are cases where production and
tests/inc/unittest_matecat_local.sqldisagree:qa_chunk_reviews.source_pageisint(11) DEFAULT NULLin production buttinyint(3) unsigned NOT NULL DEFAULT '2'in the test schema. The NULL that disabled the uniquekey cannot occur there, so the tests use a non-default
source_pageand pin that the caller'svalue is bound rather than defaulted.
penalty_pointsisdouble(20,2)in production butdecimal(11,2)in the test schema, so thefloat residue the new
ABS(a - b) > 0.005comparison absorbs is not reproducible. The tests pinthe threshold's direction, symmetry and magnitude, not the residue tolerance.
Aligning the test schema to production would close both gaps and deserves its own PR — it touches a
fixture schema shared by the whole real-SQL suite.
AI Disclosure
Claude Code (claude-opus-5)
Notes
Both repositories must deploy together.
internal_scriptsholds the only callers ofChunkReviewDao::findPenaltyPointsMismatches()/countPenaltyPointsMismatches()and the onlysender of
PenaltyPointsDriftAlertEmail; the submodule ahead of the main repo calls methods that donot exist, and the main repo ahead of the submodule ships a detector nothing runs. Merge
matecat/internal_scripts#46 first, then this PR with its pointer bump.
This fixes the code paths that cause future drift; it does not repair
qa_chunk_reviewsrows thatalready drifted in production. Once merged, run
revision:check-penalty-driftto find affected jobsand
revision:recount-drifted <uid>to review them — that is the dry run and writes nothing — thenadd
--liveto repair.<uid>is the operator the repair is attributed to. Both commands cap whatthey scan (
--limit, default 50 for the report and 500 for the repair) and report the true totalfrom an uncapped count; resume with
--min-job-id.Before deploying
Count the rows the patch cannot retro-fix:
A non-zero result means those rows are invisible to the drift detector and do not participate in
UNIQUE KEY job_pw_source_page. Backfilling them needs a rule for which review stage each orphanbelonged to, and must land before the
source_pagechange or the restored unique key will reject it.Also run
EXPLAINonfindPenaltyPointsMismatches()against production-sized data before puttingrevision:check-penalty-drifton a schedule.--limitapplies afterGROUP BY/HAVING, so itbounds rendering and repair work, not the aggregation.
Known follow-ups, deliberately not in this PR
createRecord()'sON DUPLICATE KEY UPDATElist includesreview_password, whilesetDefaults()generates a fresh one — so calling it for an existing chunk rotates the reviewer's URL password.
User-visible and independent of this bug.
EntryDao::modifyEntry()has no production caller now thatTranslationIssueModel::editFrom()isgone.
QualityReportModel::resetScore()still setsis_pass = trueunconditionally; preserving NULLthere would be consistent but changes
undo_dataround-tripping.