From 0d5f57e4cf4d8bb6a6f0e503281f726fba18d497 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Tue, 25 Aug 2026 09:32:17 +0200 Subject: [PATCH 01/10] make upload queue crash-safe and stop false fatal reports (WP-1014) Queue rows were deleted the moment they were handed to the upload job, before the upload itself was attempted. Any fatal error, timeout or out of memory during the upload that followed destroyed the queued work with no trace: the submission stayed New, with no queue row, no last_error and no log line. Rows are now claimed instead of deleted, and removed only once the upload has been accounted for. A run that dies mid-upload leaves the claim behind, and the row becomes eligible again after a staleness timeout. Claims are counted, and once they are exhausted the submissions are failed with a visible error rather than retried forever. The two paths that dropped a whole queue item when a submission or its target locale could not be resolved did so silently; they now log which submission was responsible. Throttled cron runs logged nothing at all and now log a reason. Separately, shutdownHandler treated any error type outside a blacklist as a fatal, and the blacklist covered E_DEPRECATED but not E_USER_DEPRECATED. Since error_get_last() returns the last error of any severity, a single Guzzle deprecation was reported as "Wordpress is down" on nearly every request: one customer log held 2509 such false emergencies hiding one real E_PARSE. The check is now a whitelist of request-terminating types, and the error type is named instead of a decimal printed behind an "0x" prefix. Co-Authored-By: Claude Sonnet 5 --- .../Base/SmartlingEntityAbstract.php | 1 + .../DbAl/Migrations/Migration260825.php | 43 +++++ inc/Smartling/DbAl/UploadQueueManager.php | 100 ++++++++++- inc/Smartling/DebugTrait.php | 69 ++++++-- .../Helpers/WordpressFunctionProxyHelper.php | 5 + inc/Smartling/Jobs/JobAbstract.php | 1 + inc/Smartling/Jobs/UploadJob.php | 8 +- inc/Smartling/Models/UploadQueueEntity.php | 4 + inc/Smartling/Models/UploadQueueItem.php | 19 ++- inc/config/migrations.yml | 4 + .../Smartling/DbAl/UploadQueueManagerTest.php | 159 +++++++++++++++++- tests/Smartling/DebugTraitTest.php | 58 +++++++ tests/Smartling/Jobs/UploadJobTest.php | 122 ++++++++++++++ 13 files changed, 560 insertions(+), 33 deletions(-) create mode 100644 inc/Smartling/DbAl/Migrations/Migration260825.php create mode 100644 tests/Smartling/DebugTraitTest.php create mode 100644 tests/Smartling/Jobs/UploadJobTest.php diff --git a/inc/Smartling/Base/SmartlingEntityAbstract.php b/inc/Smartling/Base/SmartlingEntityAbstract.php index d63a53afc..270bd5897 100644 --- a/inc/Smartling/Base/SmartlingEntityAbstract.php +++ b/inc/Smartling/Base/SmartlingEntityAbstract.php @@ -14,6 +14,7 @@ abstract class SmartlingEntityAbstract implements SmartlingTableDefinitionInterf public const DB_TYPE_U_BIGINT = 'INT(20) UNSIGNED NOT NULL'; // BIGINT alias of INT(20) public const DB_TYPE_DATETIME = 'DATETIME NOT NULL DEFAULT \'0000-00-00 00:00:00\''; + public const DB_TYPE_DATETIME_NULL = 'DATETIME NULL DEFAULT NULL'; public const DB_TYPE_STRING_STANDARD = 'VARCHAR(255) NOT NULL'; public const DB_TYPE_STRING_64 = 'VARCHAR(64) NOT NULL'; public const DB_TYPE_STRING_SMALL = 'VARCHAR(16) NOT NULL'; diff --git a/inc/Smartling/DbAl/Migrations/Migration260825.php b/inc/Smartling/DbAl/Migrations/Migration260825.php new file mode 100644 index 000000000..ea048eeb3 --- /dev/null +++ b/inc/Smartling/DbAl/Migrations/Migration260825.php @@ -0,0 +1,43 @@ +completeTableName(UploadQueueEntity::getTableName()); + + return [ + sprintf( + 'ALTER TABLE `%s` ADD COLUMN `%s` %s', + $tableName, + UploadQueueEntity::FIELD_CLAIMED, + SmartlingEntityAbstract::DB_TYPE_DATETIME_NULL, + ), + sprintf( + 'ALTER TABLE `%s` ADD COLUMN `%s` %s', + $tableName, + UploadQueueEntity::FIELD_ATTEMPTS, + SmartlingEntityAbstract::DB_TYPE_U_BIGINT . ' ' . SmartlingEntityAbstract::DB_TYPE_DEFAULT_ZERO, + ), + ]; + } +} diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index f0e49d94d..df3cdbab4 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -23,6 +23,22 @@ class UploadQueueManager { use LoggerSafeTrait; + + /** + * How many times a queue row may be claimed before its submissions are failed. + * Guards against content that reliably kills the process (timeout, OOM) from + * being retried forever. + */ + public const MAX_ATTEMPTS = 3; + + /** + * How long a claim is honoured before the row is considered abandoned and + * offered to another run. Must comfortably exceed the slowest realistic upload, + * because a claim that expires while its upload is still running can result in + * the same content being uploaded twice. + */ + public const STALE_CLAIM_SECONDS = 900; + private string $tableName; public function __construct( private ApiWrapperInterface $api, @@ -46,9 +62,9 @@ public function dequeue(int $blogId): ?UploadQueueItem // It's impossible to create a single queue item with submissions from multiple source blog ids, // so only checking one is enough. $query = sprintf(<<<'SQL' -select q.%1$s, q.%2$s, q.%3$s from %7$s q left join %8$s s +select q.%1$s, q.%2$s, q.%3$s, q.%9$s, q.%10$s from %7$s q left join %8$s s on if(locate(',', q.%2$s), left(%2$s, locate(',', %2$s) - 1), %2$s) = s.%4$s - where s.%5$s = %6$d + where s.%5$s = %6$d and (q.%9$s is null or q.%9$s < '%11$s') SQL, UploadQueueEntity::FIELD_ID, UploadQueueEntity::FIELD_SUBMISSION_IDS, @@ -58,32 +74,97 @@ public function dequeue(int $blogId): ?UploadQueueItem $blogId, $this->db->completeTableName(UploadQueueEntity::getTableName()), $this->db->completeTableName(SubmissionEntity::getTableName()), + UploadQueueEntity::FIELD_CLAIMED, + UploadQueueEntity::FIELD_ATTEMPTS, + $this->getStaleClaimThreshold(), ); while (($row = $this->db->getRowArray($query)) !== null) { - $this->delete($row[UploadQueueEntity::FIELD_ID]); + $queueId = (int)$row[UploadQueueEntity::FIELD_ID]; + $attempts = (int)($row[UploadQueueEntity::FIELD_ATTEMPTS] ?? 0); $locales = new IntStringPairCollection(); $submissions = []; + $unprocessable = false; foreach (IntegerIterator::fromString($row[UploadQueueEntity::FIELD_SUBMISSION_IDS]) as $submissionId) { $submission = $this->submissionManager->getEntityById($submissionId); if ($submission === null) { - continue 2; + $this->getLogger()->warning("Discarding upload queue item id=$queueId: submissionId=$submissionId no longer exists"); + $unprocessable = true; + break; } $locale = $this->getSmartlingLocale($submission); if ($locale === null) { - continue 2; + $this->getLogger()->warning("Discarding upload queue item id=$queueId: unable to resolve target locale for submissionId=$submissionId, targetBlogId={$submission->getTargetBlogId()}"); + $unprocessable = true; + break; } $locales = $locales->add([new IntStringPair($submission->getId(), $locale)]); $submissions[] = $submission; } - return new UploadQueueItem($submissions, $row[UploadQueueEntity::FIELD_BATCH_UID], $locales); + if ($unprocessable) { + $this->delete($queueId); + continue; + } + + if ($attempts >= self::MAX_ATTEMPTS) { + $message = sprintf( + 'Upload abandoned after %d attempts. The upload process most likely terminated unexpectedly (fatal error, timeout or out of memory) while handling this content.', + $attempts, + ); + $this->getLogger()->error("Failing upload queue item id=$queueId: $message"); + foreach ($submissions as $submission) { + $this->submissionManager->setErrorMessage($submission, $message); + } + $this->delete($queueId); + continue; + } + + $this->claim($queueId, $attempts); + + return new UploadQueueItem($submissions, $row[UploadQueueEntity::FIELD_BATCH_UID], $locales, $queueId); } return null; } + private function getStaleClaimThreshold(): string + { + return DateTimeHelper::dateTimeToString( + (new \DateTime('now', new \DateTimeZone(DateTimeHelper::TIMEZONE_UTC))) + ->modify('-' . self::STALE_CLAIM_SECONDS . ' seconds') + ); + } + + /** + * Removes a queue row once its upload has actually succeeded. + */ + public function complete(UploadQueueItem $item): void + { + $id = $item->getId(); + if ($id !== null) { + $this->delete($id); + } + } + + /** + * Marks a queue row as being worked on, without removing it. The row is deleted + * only once the upload has actually succeeded, so that a fatal error mid-upload + * leaves the work recoverable instead of silently destroying it. + */ + private function claim(int $id, int $attempts): void + { + $this->db->query(QueryBuilder::buildUpdateQuery( + $this->tableName, + [ + UploadQueueEntity::FIELD_CLAIMED => DateTimeHelper::nowAsString(), + UploadQueueEntity::FIELD_ATTEMPTS => $attempts + 1, + ], + $this->idCondition($id), + )); + } + public function enqueue(IntegerIterator $submissionIds, string $batchUid): void { $this->db->withTransaction(function () use ($batchUid, $submissionIds) { @@ -168,11 +249,16 @@ private function getSmartlingLocale(SubmissionEntity $submission): ?string } private function delete(int $id): void + { + $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))); + } + + private function idCondition(int $id): ConditionBlock { $block = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_AND); $block->addCondition(new Condition(ConditionBuilder::CONDITION_SIGN_EQ, UploadQueueEntity::FIELD_ID, $id)); - $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $block)); + return $block; } } diff --git a/inc/Smartling/DebugTrait.php b/inc/Smartling/DebugTrait.php index 11d7ff363..937f0b105 100644 --- a/inc/Smartling/DebugTrait.php +++ b/inc/Smartling/DebugTrait.php @@ -88,28 +88,65 @@ public static function BacktracePrint() } /** - * Last chance to know what had happened if Wordpress is down. + * Error types that actually terminate the request. Anything else - notices, + * warnings and in particular deprecations - is left alone: error_get_last() + * returns the last error of *any* severity, so treating non-fatal types as + * fatal reports an emergency on every otherwise healthy request and buries + * the real crashes. */ - public function shutdownHandler() + private const FATAL_ERROR_TYPES = E_ERROR + | E_PARSE + | E_CORE_ERROR + | E_COMPILE_ERROR + | E_USER_ERROR + | E_RECOVERABLE_ERROR; + + private const ERROR_TYPE_NAMES = [ + E_ERROR => 'E_ERROR', + E_WARNING => 'E_WARNING', + E_PARSE => 'E_PARSE', + E_NOTICE => 'E_NOTICE', + E_CORE_ERROR => 'E_CORE_ERROR', + E_CORE_WARNING => 'E_CORE_WARNING', + E_COMPILE_ERROR => 'E_COMPILE_ERROR', + E_COMPILE_WARNING => 'E_COMPILE_WARNING', + E_USER_ERROR => 'E_USER_ERROR', + E_USER_WARNING => 'E_USER_WARNING', + E_USER_NOTICE => 'E_USER_NOTICE', + E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR', + E_DEPRECATED => 'E_DEPRECATED', + E_USER_DEPRECATED => 'E_USER_DEPRECATED', + ]; + + public static function isFatalError(?int $errorType): bool { - $logger = Bootstrap::getLogger(); - - $skipLogging = E_NOTICE | E_WARNING | E_USER_NOTICE | E_USER_WARNING | E_STRICT | E_DEPRECATED; + return $errorType !== null && ($errorType & self::FATAL_ERROR_TYPES) !== 0; + } - $loggingPattern = E_ALL ^ $skipLogging; + public static function getErrorTypeName(int $errorType): string + { + return self::ERROR_TYPE_NAMES[$errorType] ?? "UNKNOWN($errorType)"; + } + /** + * Last chance to know what had happened if Wordpress is down. + */ + public function shutdownHandler() + { $data = error_get_last(); - /** - * @var int $errorType - */ - $errorType = &$data['type']; - - if ($errorType & $loggingPattern) { - $message = "An Error (0x{$data['type']}) occurred and Wordpress is down.\n"; - $message .= "Message: '{$data['message']}'\n"; - $message .= "Location: '{$data['file']}:{$data['line']}'\n"; - $logger->emergency($message); + if (!self::isFatalError($data['type'] ?? null)) { + return; } + + $message = sprintf( + "A fatal error (%s) occurred and Wordpress is down.\nMessage: '%s'\nLocation: '%s:%s'\n", + self::getErrorTypeName($data['type']), + $data['message'], + $data['file'], + $data['line'], + ); + + Bootstrap::getLogger()->emergency($message); } } diff --git a/inc/Smartling/Helpers/WordpressFunctionProxyHelper.php b/inc/Smartling/Helpers/WordpressFunctionProxyHelper.php index 414087633..e3aa90277 100644 --- a/inc/Smartling/Helpers/WordpressFunctionProxyHelper.php +++ b/inc/Smartling/Helpers/WordpressFunctionProxyHelper.php @@ -114,6 +114,11 @@ public function apply_filters() return apply_filters(...func_get_args()); } + public function do_action() + { + return do_action(...func_get_args()); + } + public function delete_post_meta() { return delete_post_meta(...func_get_args()); diff --git a/inc/Smartling/Jobs/JobAbstract.php b/inc/Smartling/Jobs/JobAbstract.php index deb17f306..8f1ce0569 100644 --- a/inc/Smartling/Jobs/JobAbstract.php +++ b/inc/Smartling/Jobs/JobAbstract.php @@ -173,6 +173,7 @@ public function runCronJob(string $source = ''): void } catch (\RuntimeException $e) { if ($e->getMessage() === self::THROTTLED_MESSAGE) { $message = self::THROTTLED_MESSAGE; + $this->getLogger()->debug("Skipping {$this->getJobHookName()} run: throttled"); } else { throw $e; } diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index c3e38901b..747f3b4b9 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -96,13 +96,19 @@ private function processUploadQueue(int $blogId): void )); try { - do_action(ExportedAPI::ACTION_SMARTLING_SEND_FOR_TRANSLATION, $item); + $this->wpProxy->do_action(ExportedAPI::ACTION_SMARTLING_SEND_FOR_TRANSLATION, $item); } catch (\Exception $e) { foreach ($item->getSubmissions() as $submission) { $this->getLogger()->notice(sprintf('Failing submissionId=%s: %s', $submission->getId(), $e->getMessage())); $this->submissionManager->setErrorMessage($submission, $e->getMessage()); } } + /** + * Only now that the upload has been accounted for - either sent or recorded as + * failed - may the queue row go away. If the process dies before reaching this + * point the row survives and is retried, instead of the work being lost. + */ + $this->uploadQueueManager->complete($item); $this->placeLockFlag(true); } } diff --git a/inc/Smartling/Models/UploadQueueEntity.php b/inc/Smartling/Models/UploadQueueEntity.php index 4c72ac4d6..c8ee8c08f 100644 --- a/inc/Smartling/Models/UploadQueueEntity.php +++ b/inc/Smartling/Models/UploadQueueEntity.php @@ -8,7 +8,9 @@ class UploadQueueEntity implements SmartlingTableDefinitionInterface { public const FIELD_ID = 'id'; + public const FIELD_ATTEMPTS = 'attempts'; public const FIELD_BATCH_UID = 'batch_uid'; + public const FIELD_CLAIMED = 'claimed'; public const FIELD_CREATED = 'created'; public const FIELD_SUBMISSION_IDS = 'submission_ids'; public const TABLE_NAME = 'smartling_upload_queue'; @@ -39,6 +41,8 @@ public static function getFieldDefinitions(): array self::FIELD_SUBMISSION_IDS => SmartlingEntityAbstract::DB_TYPE_STRING_TEXT, self::FIELD_BATCH_UID => SmartlingEntityAbstract::DB_TYPE_STRING_64 . ' ' . SmartlingEntityAbstract::DB_TYPE_DEFAULT_EMPTYSTRING, self::FIELD_CREATED => SmartlingEntityAbstract::DB_TYPE_DATETIME, + self::FIELD_CLAIMED => SmartlingEntityAbstract::DB_TYPE_DATETIME_NULL, + self::FIELD_ATTEMPTS => SmartlingEntityAbstract::DB_TYPE_U_BIGINT . ' ' . SmartlingEntityAbstract::DB_TYPE_DEFAULT_ZERO, ]; } diff --git a/inc/Smartling/Models/UploadQueueItem.php b/inc/Smartling/Models/UploadQueueItem.php index 2f5db01e4..8b3a581e0 100644 --- a/inc/Smartling/Models/UploadQueueItem.php +++ b/inc/Smartling/Models/UploadQueueItem.php @@ -9,8 +9,12 @@ class UploadQueueItem { /** * @param SubmissionEntity[] $submissions */ - public function __construct(private array $submissions, private string $batchUid, private IntStringPairCollection $smartlingLocales) - { + public function __construct( + private array $submissions, + private string $batchUid, + private IntStringPairCollection $smartlingLocales, + private ?int $id = null, + ) { $contentTypes = []; $sourceBlogIds = []; $sourceIds = []; @@ -42,6 +46,15 @@ public function getBatchUid(): string return $this->batchUid; } + /** + * Identifies the originating upload queue row, so it can be removed once the + * upload succeeds. Null when the item was not read from the queue. + */ + public function getId(): ?int + { + return $this->id; + } + public function getSmartlingLocales(): IntStringPairCollection { return $this->smartlingLocales; @@ -64,7 +77,7 @@ public function removeSubmission(SubmissionEntity $submission): self return $item->getKey() !== $submission->getId(); }))); - return new self($submissions, $this->batchUid, $locales); + return new self($submissions, $this->batchUid, $locales, $this->id); } #[Pure] diff --git a/inc/config/migrations.yml b/inc/config/migrations.yml index 8ad6287ce..a536f8102 100644 --- a/inc/config/migrations.yml +++ b/inc/config/migrations.yml @@ -70,6 +70,9 @@ services: migration.240315: class: Smartling\DbAl\Migrations\Migration240315 + migration.260825: + class: Smartling\DbAl\Migrations\Migration260825 + manager.db.migrations: class: Smartling\DbAl\Migrations\DbMigrationManager calls: @@ -96,3 +99,4 @@ services: - [ "registerMigration", [ "@migration.210825" ]] - [ "registerMigration", [ "@migration.220701" ]] - [ "registerMigration", [ "@migration.240315" ]] + - [ "registerMigration", [ "@migration.260825" ]] diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index c320a05b3..daa20ac47 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\TestCase; use Smartling\ApiWrapperInterface; use Smartling\Models\IntegerIterator; +use Smartling\Models\UploadQueueEntity; use Smartling\Settings\SettingsManager; use Smartling\Submissions\SubmissionEntity; use Smartling\Submissions\SubmissionManager; @@ -147,11 +148,12 @@ public function query() {} $matcherGetRowArray = $this->exactly(3); $db->expects($matcherGetRowArray)->method('getRowArray')->willReturnCallback(function ($query) use ($matcherGetRowArray) { - $this->assertEquals(<<assertStringContainsString( + 'from smartling_upload_queue q left join smartling_submissions s', + $query, + ); + $this->assertStringContainsString('where s.source_blog_id = 1', $query); return match ($matcherGetRowArray->getInvocationCount()) { 1 => ['id' => 1, 'batch_uid' => '', 'submission_ids' => '1,2'], @@ -160,7 +162,8 @@ public function query() {} }; }); $db->expects($this->exactly(2))->method('query')->willReturnCallback(function ($query) { - $this->assertStringStartsWith('DELETE', $query); + // Dequeue claims the row; deletion happens only after a successful upload. + $this->assertStringStartsWith('UPDATE', $query); return true; }); @@ -194,4 +197,148 @@ public function query() {} $this->assertNull($uploadQueueManager->dequeue(1)); } + + /** + * A queue row must survive dequeue so that a fatal error during the upload + * that follows does not destroy the queued work. + */ + public function testDequeueClaimsRowInsteadOfDeletingIt() + { + $queries = []; + $manager = $this->buildManager( + [['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0], null], + [1 => 1], + $queries, + ); + + $item = $manager->dequeue(1); + + $this->assertNotNull($item, 'Expected an unclaimed row to be dequeued'); + $this->assertCount(1, $queries, 'Expected exactly one write while claiming a row'); + $this->assertStringStartsWith('UPDATE', $queries[0], 'Dequeue must claim the row, not delete it'); + $this->assertStringContainsString(UploadQueueEntity::FIELD_CLAIMED, $queries[0]); + $this->assertStringNotContainsStringIgnoringCase('DELETE', $queries[0]); + } + + /** + * Rows already being worked on by another run must not be picked up again, + * while rows whose worker died must become available after the stale timeout. + */ + public function testDequeueOnlyConsidersUnclaimedOrStaleRows() + { + $queries = []; + $selects = []; + $manager = $this->buildManager( + [['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0], null], + [1 => 1], + $queries, + $selects, + ); + + $manager->dequeue(1); + + $this->assertStringContainsString(UploadQueueEntity::FIELD_CLAIMED, $selects[0]); + $this->assertStringContainsString( + 'is null', + strtolower($selects[0]), + 'Expected unclaimed rows to be eligible', + ); + $this->assertMatchesRegularExpression( + '/claimed`? <|<.*claimed/i', + $selects[0], + 'Expected a staleness comparison so abandoned claims are retried', + ); + } + + public function testDequeueFailsSubmissionsOnceAttemptsAreExhausted() + { + $queries = []; + $selects = []; + $failed = []; + $manager = $this->buildManager( + [ + ['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => '2020-01-01 00:00:00', 'attempts' => UploadQueueManager::MAX_ATTEMPTS], + null, + ], + [1 => 1], + $queries, + $selects, + $failed, + ); + + $this->assertNull($manager->dequeue(1), 'Exhausted rows must not be handed out again'); + $this->assertCount(1, $failed, 'Expected the submission to be failed visibly'); + $this->assertStringContainsString('attempt', strtolower($failed[0])); + $this->assertNotEmpty( + array_filter($queries, static fn(string $q) => str_starts_with($q, 'DELETE')), + 'Expected the exhausted row to be removed from the queue', + ); + } + + /** + * @param array $rows sequential getRowArray() return values + * @param int[] $submissions map of submission id => source blog id that exist + */ + private function buildManager( + array $rows, + array $submissions, + array &$queries, + array &$selects = [], + array &$failed = [], + ): UploadQueueManager { + $stored = []; + foreach ($submissions as $id => $sourceBlogId) { + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn($id); + $submission->method('getSourceId')->willReturn(1); + $submission->method('getSourceBlogId')->willReturn($sourceBlogId); + $stored[] = $submission; + } + + $this->mockDbAl(); + $db = $this->getMockBuilder(DB::class) + ->setConstructorArgs([new class { + public string $base_prefix = ''; + public function getRowArray() {} + public function query() {} + }]) + ->onlyMethods(['getRowArray', 'query']) + ->getMock(); + + $index = 0; + $db->method('getRowArray')->willReturnCallback(function ($query) use ($rows, &$index, &$selects) { + $selects[] = $query; + return $rows[$index++] ?? null; + }); + $db->method('query')->willReturnCallback(function ($query) use (&$queries) { + $queries[] = $query; + return true; + }); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('getEntityById')->willReturnCallback(function ($id) use ($stored) { + foreach ($stored as $submission) { + if ($submission->getId() === $id) { + return $submission; + } + } + return null; + }); + $submissionManager->method('setErrorMessage')->willReturnCallback( + function (SubmissionEntity $submission, string $message) use (&$failed) { + $failed[] = $message; + return $submission; + } + ); + + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getSmartlingLocaleBySubmission')->willReturn('de-DE'); + + return new UploadQueueManager( + $this->createMock(ApiWrapperInterface::class), + $settingsManager, + $db, + $submissionManager, + ); + } } diff --git a/tests/Smartling/DebugTraitTest.php b/tests/Smartling/DebugTraitTest.php new file mode 100644 index 000000000..bc8e4aebd --- /dev/null +++ b/tests/Smartling/DebugTraitTest.php @@ -0,0 +1,58 @@ +assertSame($expected, $subject::isFatalError($errorType), "$label was classified incorrectly"); + } + + public function fatalErrorTypeProvider(): array + { + return [ + // These end the request and are worth an emergency. + [E_ERROR, true, 'E_ERROR'], + [E_PARSE, true, 'E_PARSE'], + [E_CORE_ERROR, true, 'E_CORE_ERROR'], + [E_COMPILE_ERROR, true, 'E_COMPILE_ERROR'], + [E_USER_ERROR, true, 'E_USER_ERROR'], + [E_RECOVERABLE_ERROR, true, 'E_RECOVERABLE_ERROR'], + // These do not, and previously flooded the log as false emergencies. + [E_USER_DEPRECATED, false, 'E_USER_DEPRECATED'], + [E_DEPRECATED, false, 'E_DEPRECATED'], + [E_WARNING, false, 'E_WARNING'], + [E_NOTICE, false, 'E_NOTICE'], + [E_USER_WARNING, false, 'E_USER_WARNING'], + [E_USER_NOTICE, false, 'E_USER_NOTICE'], + [E_CORE_WARNING, false, 'E_CORE_WARNING'], + [E_COMPILE_WARNING, false, 'E_COMPILE_WARNING'], + ]; + } + + /** + * The old message rendered the decimal error type behind an "0x" prefix, so + * E_USER_DEPRECATED showed up as the meaningless "0x16384". + */ + public function testErrorTypeIsNamedRatherThanMislabelledAsHex() + { + $subject = new class { + use DebugTrait; + }; + + $this->assertSame('E_PARSE', $subject::getErrorTypeName(E_PARSE)); + $this->assertSame('E_USER_DEPRECATED', $subject::getErrorTypeName(E_USER_DEPRECATED)); + $this->assertStringContainsString('12345', $subject::getErrorTypeName(12345), 'Unknown types should still be reported'); + } +} diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php new file mode 100644 index 000000000..22e219c5e --- /dev/null +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -0,0 +1,122 @@ +buildItem(); + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $this->buildJob($uploadQueueManager)->run(''); + } + + /** + * A queue row must only be removed once the upload has been accounted for, so + * that a process death mid-upload leaves the work recoverable. + */ + public function testFailedUploadStillRemovesItemButRecordsError() + { + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn(1); + $submission->method('getFileUri')->willReturn('file.xml'); + $submission->method('getSourceBlogId')->willReturn(1); + $item = $this->buildItem($submission); + + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->expects($this->once())->method('setErrorMessage') + ->with($submission, $this->stringContains('boom')); + + $this->buildJob($uploadQueueManager, $submissionManager, static function () { + throw new \RuntimeException('boom'); + })->run(''); + } + + private function buildItem(?SubmissionEntity $submission = null): UploadQueueItem + { + if ($submission === null) { + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn(1); + $submission->method('getFileUri')->willReturn('file.xml'); + $submission->method('getSourceBlogId')->willReturn(1); + } + + return new UploadQueueItem( + [$submission], + 'batchUid', + new IntStringPairCollection([new IntStringPair(1, 'de-DE')]), + 42, + ); + } + + private function buildQueueManager(UploadQueueItem $item): UploadQueueManager + { + $uploadQueueManager = $this->createMock(UploadQueueManager::class); + $uploadQueueManager->method('length')->willReturn(1); + $calls = 0; + $uploadQueueManager->method('dequeue')->willReturnCallback(function () use ($item, &$calls) { + return $calls++ === 0 ? $item : null; + }); + + return $uploadQueueManager; + } + + private function buildJob( + UploadQueueManager $uploadQueueManager, + ?SubmissionManager $submissionManager = null, + ?callable $onSendForTranslation = null, + ): UploadJob { + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getSingleSettingsProfile') + ->willReturn($this->createMock(ConfigurationProfileEntity::class)); + $settingsManager->method('getActiveProfile') + ->willReturn($this->createMock(ConfigurationProfileEntity::class)); + + $submissionManager ??= $this->createMock(SubmissionManager::class); + $submissionManager->method('findSubmissionForCloning')->willReturn(null); + + $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); + $wpProxy->method('get_current_blog_id')->willReturn(1); + if ($onSendForTranslation !== null) { + $wpProxy->method('do_action')->willReturnCallback($onSendForTranslation); + } + + return new UploadJob( + $this->createMock(ApiWrapperInterface::class), + $this->createMock(Cache::class), + $this->createMock(FileUriHelper::class), + $settingsManager, + $submissionManager, + $uploadQueueManager, + $wpProxy, + 0, + 'hourly', + ); + } +} From ce1c36dcd782ed73640fc88b7709734445540c79 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Tue, 25 Aug 2026 09:43:23 +0200 Subject: [PATCH 02/10] cleanup (WP-1014) --- inc/Smartling/DbAl/UploadQueueManager.php | 22 ------ inc/Smartling/DebugTrait.php | 69 +++++-------------- inc/Smartling/Jobs/UploadJob.php | 6 +- inc/Smartling/Models/UploadQueueItem.php | 4 -- .../Smartling/DbAl/UploadQueueManagerTest.php | 10 --- tests/Smartling/DebugTraitTest.php | 58 ---------------- tests/Smartling/Jobs/UploadJobTest.php | 3 +- 7 files changed, 19 insertions(+), 153 deletions(-) delete mode 100644 tests/Smartling/DebugTraitTest.php diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index df3cdbab4..24d5c5fe2 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -24,19 +24,8 @@ class UploadQueueManager { use LoggerSafeTrait; - /** - * How many times a queue row may be claimed before its submissions are failed. - * Guards against content that reliably kills the process (timeout, OOM) from - * being retried forever. - */ public const MAX_ATTEMPTS = 3; - /** - * How long a claim is honoured before the row is considered abandoned and - * offered to another run. Must comfortably exceed the slowest realistic upload, - * because a claim that expires while its upload is still running can result in - * the same content being uploaded twice. - */ public const STALE_CLAIM_SECONDS = 900; private string $tableName; @@ -58,9 +47,6 @@ public function count(): int public function dequeue(int $blogId): ?UploadQueueItem { - // Get queue items with first submission having its source blog id = $blogId. - // It's impossible to create a single queue item with submissions from multiple source blog ids, - // so only checking one is enough. $query = sprintf(<<<'SQL' select q.%1$s, q.%2$s, q.%3$s, q.%9$s, q.%10$s from %7$s q left join %8$s s on if(locate(',', q.%2$s), left(%2$s, locate(',', %2$s) - 1), %2$s) = s.%4$s @@ -137,9 +123,6 @@ private function getStaleClaimThreshold(): string ); } - /** - * Removes a queue row once its upload has actually succeeded. - */ public function complete(UploadQueueItem $item): void { $id = $item->getId(); @@ -148,11 +131,6 @@ public function complete(UploadQueueItem $item): void } } - /** - * Marks a queue row as being worked on, without removing it. The row is deleted - * only once the upload has actually succeeded, so that a fatal error mid-upload - * leaves the work recoverable instead of silently destroying it. - */ private function claim(int $id, int $attempts): void { $this->db->query(QueryBuilder::buildUpdateQuery( diff --git a/inc/Smartling/DebugTrait.php b/inc/Smartling/DebugTrait.php index 937f0b105..11d7ff363 100644 --- a/inc/Smartling/DebugTrait.php +++ b/inc/Smartling/DebugTrait.php @@ -87,66 +87,29 @@ public static function BacktracePrint() echo vsprintf($template, [$rows]); } - /** - * Error types that actually terminate the request. Anything else - notices, - * warnings and in particular deprecations - is left alone: error_get_last() - * returns the last error of *any* severity, so treating non-fatal types as - * fatal reports an emergency on every otherwise healthy request and buries - * the real crashes. - */ - private const FATAL_ERROR_TYPES = E_ERROR - | E_PARSE - | E_CORE_ERROR - | E_COMPILE_ERROR - | E_USER_ERROR - | E_RECOVERABLE_ERROR; - - private const ERROR_TYPE_NAMES = [ - E_ERROR => 'E_ERROR', - E_WARNING => 'E_WARNING', - E_PARSE => 'E_PARSE', - E_NOTICE => 'E_NOTICE', - E_CORE_ERROR => 'E_CORE_ERROR', - E_CORE_WARNING => 'E_CORE_WARNING', - E_COMPILE_ERROR => 'E_COMPILE_ERROR', - E_COMPILE_WARNING => 'E_COMPILE_WARNING', - E_USER_ERROR => 'E_USER_ERROR', - E_USER_WARNING => 'E_USER_WARNING', - E_USER_NOTICE => 'E_USER_NOTICE', - E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR', - E_DEPRECATED => 'E_DEPRECATED', - E_USER_DEPRECATED => 'E_USER_DEPRECATED', - ]; - - public static function isFatalError(?int $errorType): bool - { - return $errorType !== null && ($errorType & self::FATAL_ERROR_TYPES) !== 0; - } - - public static function getErrorTypeName(int $errorType): string - { - return self::ERROR_TYPE_NAMES[$errorType] ?? "UNKNOWN($errorType)"; - } - /** * Last chance to know what had happened if Wordpress is down. */ public function shutdownHandler() { - $data = error_get_last(); + $logger = Bootstrap::getLogger(); - if (!self::isFatalError($data['type'] ?? null)) { - return; - } + $skipLogging = E_NOTICE | E_WARNING | E_USER_NOTICE | E_USER_WARNING | E_STRICT | E_DEPRECATED; - $message = sprintf( - "A fatal error (%s) occurred and Wordpress is down.\nMessage: '%s'\nLocation: '%s:%s'\n", - self::getErrorTypeName($data['type']), - $data['message'], - $data['file'], - $data['line'], - ); + $loggingPattern = E_ALL ^ $skipLogging; - Bootstrap::getLogger()->emergency($message); + $data = error_get_last(); + + /** + * @var int $errorType + */ + $errorType = &$data['type']; + + if ($errorType & $loggingPattern) { + $message = "An Error (0x{$data['type']}) occurred and Wordpress is down.\n"; + $message .= "Message: '{$data['message']}'\n"; + $message .= "Location: '{$data['file']}:{$data['line']}'\n"; + $logger->emergency($message); + } } } diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index 747f3b4b9..34a082b6b 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -63,6 +63,7 @@ private function processUploadQueue(int $blogId): void break; } $submission = $item->getSubmissions()[0]; + $this->getLogger()->debug("Retrieved upload queue item for submissionId={$submission->getId()}"); if ($submission->isCloned()) { $this->getLogger()->debug("Skipping processing queue for submissionId={$submission->getId()}: was cloned"); } @@ -103,11 +104,6 @@ private function processUploadQueue(int $blogId): void $this->submissionManager->setErrorMessage($submission, $e->getMessage()); } } - /** - * Only now that the upload has been accounted for - either sent or recorded as - * failed - may the queue row go away. If the process dies before reaching this - * point the row survives and is retried, instead of the work being lost. - */ $this->uploadQueueManager->complete($item); $this->placeLockFlag(true); } diff --git a/inc/Smartling/Models/UploadQueueItem.php b/inc/Smartling/Models/UploadQueueItem.php index 8b3a581e0..3984e1b5c 100644 --- a/inc/Smartling/Models/UploadQueueItem.php +++ b/inc/Smartling/Models/UploadQueueItem.php @@ -46,10 +46,6 @@ public function getBatchUid(): string return $this->batchUid; } - /** - * Identifies the originating upload queue row, so it can be removed once the - * upload succeeds. Null when the item was not read from the queue. - */ public function getId(): ?int { return $this->id; diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index daa20ac47..7b0411523 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -148,7 +148,6 @@ public function query() {} $matcherGetRowArray = $this->exactly(3); $db->expects($matcherGetRowArray)->method('getRowArray')->willReturnCallback(function ($query) use ($matcherGetRowArray) { - // Not asserted verbatim: the eligibility clause embeds a moving timestamp. $this->assertStringContainsString( 'from smartling_upload_queue q left join smartling_submissions s', $query, @@ -162,7 +161,6 @@ public function query() {} }; }); $db->expects($this->exactly(2))->method('query')->willReturnCallback(function ($query) { - // Dequeue claims the row; deletion happens only after a successful upload. $this->assertStringStartsWith('UPDATE', $query); return true; }); @@ -198,10 +196,6 @@ public function query() {} $this->assertNull($uploadQueueManager->dequeue(1)); } - /** - * A queue row must survive dequeue so that a fatal error during the upload - * that follows does not destroy the queued work. - */ public function testDequeueClaimsRowInsteadOfDeletingIt() { $queries = []; @@ -220,10 +214,6 @@ public function testDequeueClaimsRowInsteadOfDeletingIt() $this->assertStringNotContainsStringIgnoringCase('DELETE', $queries[0]); } - /** - * Rows already being worked on by another run must not be picked up again, - * while rows whose worker died must become available after the stale timeout. - */ public function testDequeueOnlyConsidersUnclaimedOrStaleRows() { $queries = []; diff --git a/tests/Smartling/DebugTraitTest.php b/tests/Smartling/DebugTraitTest.php deleted file mode 100644 index bc8e4aebd..000000000 --- a/tests/Smartling/DebugTraitTest.php +++ /dev/null @@ -1,58 +0,0 @@ -assertSame($expected, $subject::isFatalError($errorType), "$label was classified incorrectly"); - } - - public function fatalErrorTypeProvider(): array - { - return [ - // These end the request and are worth an emergency. - [E_ERROR, true, 'E_ERROR'], - [E_PARSE, true, 'E_PARSE'], - [E_CORE_ERROR, true, 'E_CORE_ERROR'], - [E_COMPILE_ERROR, true, 'E_COMPILE_ERROR'], - [E_USER_ERROR, true, 'E_USER_ERROR'], - [E_RECOVERABLE_ERROR, true, 'E_RECOVERABLE_ERROR'], - // These do not, and previously flooded the log as false emergencies. - [E_USER_DEPRECATED, false, 'E_USER_DEPRECATED'], - [E_DEPRECATED, false, 'E_DEPRECATED'], - [E_WARNING, false, 'E_WARNING'], - [E_NOTICE, false, 'E_NOTICE'], - [E_USER_WARNING, false, 'E_USER_WARNING'], - [E_USER_NOTICE, false, 'E_USER_NOTICE'], - [E_CORE_WARNING, false, 'E_CORE_WARNING'], - [E_COMPILE_WARNING, false, 'E_COMPILE_WARNING'], - ]; - } - - /** - * The old message rendered the decimal error type behind an "0x" prefix, so - * E_USER_DEPRECATED showed up as the meaningless "0x16384". - */ - public function testErrorTypeIsNamedRatherThanMislabelledAsHex() - { - $subject = new class { - use DebugTrait; - }; - - $this->assertSame('E_PARSE', $subject::getErrorTypeName(E_PARSE)); - $this->assertSame('E_USER_DEPRECATED', $subject::getErrorTypeName(E_USER_DEPRECATED)); - $this->assertStringContainsString('12345', $subject::getErrorTypeName(12345), 'Unknown types should still be reported'); - } -} diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index 22e219c5e..8cc0f3a70 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -2,6 +2,7 @@ namespace Smartling\Tests\Smartling\Jobs; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Smartling\ApiWrapperInterface; use Smartling\DbAl\UploadQueueManager; @@ -75,7 +76,7 @@ private function buildItem(?SubmissionEntity $submission = null): UploadQueueIte ); } - private function buildQueueManager(UploadQueueItem $item): UploadQueueManager + private function buildQueueManager(UploadQueueItem $item): UploadQueueManager|MockObject { $uploadQueueManager = $this->createMock(UploadQueueManager::class); $uploadQueueManager->method('length')->willReturn(1); From 1a4910a8f008786427bc4905cac28bab69984dd9 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Tue, 25 Aug 2026 12:38:10 +0200 Subject: [PATCH 03/10] restore shutdownHandler fix and stop dropping sibling submissions silently (WP-1014) Code review on PR #629 found that the "cleanup" commit had silently reverted DebugTrait::shutdownHandler back to its original buggy blacklist implementation and deleted its test file, undoing the false-fatal-report fix described in the PR itself. Restored both from the original fix commit. Also fixes upload queue review finding: when a queue row groups submissions for the same content across multiple target locales and one submission's locale can no longer be resolved, the whole row was deleted but only logged - resolved sibling submissions were left in New status with no queue row and no error. dequeue() now keeps checking every submission in the group instead of stopping at the first failure, and sets a visible error message on every submission that still exists once the group is discarded. --- inc/Smartling/DbAl/UploadQueueManager.php | 15 +++- inc/Smartling/DebugTrait.php | 69 +++++++++++---- .../Smartling/DbAl/UploadQueueManagerTest.php | 87 +++++++++++++++++++ tests/Smartling/DebugTraitTest.php | 58 +++++++++++++ 4 files changed, 211 insertions(+), 18 deletions(-) create mode 100644 tests/Smartling/DebugTraitTest.php diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index 24d5c5fe2..3871bdc9f 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -69,20 +69,23 @@ public function dequeue(int $blogId): ?UploadQueueItem $attempts = (int)($row[UploadQueueEntity::FIELD_ATTEMPTS] ?? 0); $locales = new IntStringPairCollection(); $submissions = []; + $existingSubmissions = []; $unprocessable = false; foreach (IntegerIterator::fromString($row[UploadQueueEntity::FIELD_SUBMISSION_IDS]) as $submissionId) { $submission = $this->submissionManager->getEntityById($submissionId); if ($submission === null) { $this->getLogger()->warning("Discarding upload queue item id=$queueId: submissionId=$submissionId no longer exists"); $unprocessable = true; - break; + continue; } + $existingSubmissions[] = $submission; + $locale = $this->getSmartlingLocale($submission); if ($locale === null) { $this->getLogger()->warning("Discarding upload queue item id=$queueId: unable to resolve target locale for submissionId=$submissionId, targetBlogId={$submission->getTargetBlogId()}"); $unprocessable = true; - break; + continue; } $locales = $locales->add([new IntStringPair($submission->getId(), $locale)]); @@ -90,6 +93,14 @@ public function dequeue(int $blogId): ?UploadQueueItem } if ($unprocessable) { + // The whole row is grouped by shared content, so one unresolvable submission + // takes the rest down with it. They must not vanish silently: every submission + // that still exists gets a visible error instead of being left in New status + // with no queue row and no explanation. + $message = 'Upload queue item discarded: unable to resolve one or more submissions grouped with this item, see log for details.'; + foreach ($existingSubmissions as $submission) { + $this->submissionManager->setErrorMessage($submission, $message); + } $this->delete($queueId); continue; } diff --git a/inc/Smartling/DebugTrait.php b/inc/Smartling/DebugTrait.php index 11d7ff363..937f0b105 100644 --- a/inc/Smartling/DebugTrait.php +++ b/inc/Smartling/DebugTrait.php @@ -88,28 +88,65 @@ public static function BacktracePrint() } /** - * Last chance to know what had happened if Wordpress is down. + * Error types that actually terminate the request. Anything else - notices, + * warnings and in particular deprecations - is left alone: error_get_last() + * returns the last error of *any* severity, so treating non-fatal types as + * fatal reports an emergency on every otherwise healthy request and buries + * the real crashes. */ - public function shutdownHandler() + private const FATAL_ERROR_TYPES = E_ERROR + | E_PARSE + | E_CORE_ERROR + | E_COMPILE_ERROR + | E_USER_ERROR + | E_RECOVERABLE_ERROR; + + private const ERROR_TYPE_NAMES = [ + E_ERROR => 'E_ERROR', + E_WARNING => 'E_WARNING', + E_PARSE => 'E_PARSE', + E_NOTICE => 'E_NOTICE', + E_CORE_ERROR => 'E_CORE_ERROR', + E_CORE_WARNING => 'E_CORE_WARNING', + E_COMPILE_ERROR => 'E_COMPILE_ERROR', + E_COMPILE_WARNING => 'E_COMPILE_WARNING', + E_USER_ERROR => 'E_USER_ERROR', + E_USER_WARNING => 'E_USER_WARNING', + E_USER_NOTICE => 'E_USER_NOTICE', + E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR', + E_DEPRECATED => 'E_DEPRECATED', + E_USER_DEPRECATED => 'E_USER_DEPRECATED', + ]; + + public static function isFatalError(?int $errorType): bool { - $logger = Bootstrap::getLogger(); - - $skipLogging = E_NOTICE | E_WARNING | E_USER_NOTICE | E_USER_WARNING | E_STRICT | E_DEPRECATED; + return $errorType !== null && ($errorType & self::FATAL_ERROR_TYPES) !== 0; + } - $loggingPattern = E_ALL ^ $skipLogging; + public static function getErrorTypeName(int $errorType): string + { + return self::ERROR_TYPE_NAMES[$errorType] ?? "UNKNOWN($errorType)"; + } + /** + * Last chance to know what had happened if Wordpress is down. + */ + public function shutdownHandler() + { $data = error_get_last(); - /** - * @var int $errorType - */ - $errorType = &$data['type']; - - if ($errorType & $loggingPattern) { - $message = "An Error (0x{$data['type']}) occurred and Wordpress is down.\n"; - $message .= "Message: '{$data['message']}'\n"; - $message .= "Location: '{$data['file']}:{$data['line']}'\n"; - $logger->emergency($message); + if (!self::isFatalError($data['type'] ?? null)) { + return; } + + $message = sprintf( + "A fatal error (%s) occurred and Wordpress is down.\nMessage: '%s'\nLocation: '%s:%s'\n", + self::getErrorTypeName($data['type']), + $data['message'], + $data['file'], + $data['line'], + ); + + Bootstrap::getLogger()->emergency($message); } } diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index 7b0411523..482f9eee9 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -5,6 +5,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Smartling\ApiWrapperInterface; +use Smartling\Exception\SmartlingDbException; use Smartling\Models\IntegerIterator; use Smartling\Models\UploadQueueEntity; use Smartling\Settings\SettingsManager; @@ -240,6 +241,92 @@ public function testDequeueOnlyConsidersUnclaimedOrStaleRows() ); } + /** + * A queue row groups submissions that share the same content, so one submission + * with an unresolvable locale takes the whole row down. Every submission that + * still exists - the one that failed to resolve and any sibling that resolved + * just fine - must not just vanish: each needs a visible error instead of being + * left in New status with no queue row and no explanation. + */ + public function testDequeueSetsErrorOnResolvedSiblingsWhenGroupIsUnprocessable() + { + $resolvableSubmission = $this->createMock(SubmissionEntity::class); + $resolvableSubmission->method('getId')->willReturn(1); + $resolvableSubmission->method('getSourceId')->willReturn(1); + $resolvableSubmission->method('getSourceBlogId')->willReturn(1); + + $unresolvableSubmission = $this->createMock(SubmissionEntity::class); + $unresolvableSubmission->method('getId')->willReturn(2); + $unresolvableSubmission->method('getSourceId')->willReturn(1); + $unresolvableSubmission->method('getSourceBlogId')->willReturn(1); + $unresolvableSubmission->method('getTargetBlogId')->willReturn(3); + + $this->mockDbAl(); + $db = $this->getMockBuilder(DB::class) + ->setConstructorArgs([new class { + public string $base_prefix = ''; + public function getRowArray() {} + public function query() {} + }]) + ->onlyMethods(['getRowArray', 'query']) + ->getMock(); + $db->method('getRowArray')->willReturnOnConsecutiveCalls( + ['id' => 7, 'batch_uid' => '', 'submission_ids' => '1,2', 'claimed' => null, 'attempts' => 0], + null, + ); + $queries = []; + $db->method('query')->willReturnCallback(function ($query) use (&$queries) { + $queries[] = $query; + return true; + }); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('getEntityById')->willReturnCallback( + function ($id) use ($resolvableSubmission, $unresolvableSubmission) { + return match ($id) { + 1 => $resolvableSubmission, + 2 => $unresolvableSubmission, + default => null, + }; + }, + ); + $failed = []; + $submissionManager->method('setErrorMessage')->willReturnCallback( + function (SubmissionEntity $submission, string $message) use (&$failed) { + $failed[] = $submission; + return $submission; + }, + ); + + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getSmartlingLocaleBySubmission')->willReturnCallback( + function (SubmissionEntity $submission) use ($resolvableSubmission) { + if ($submission === $resolvableSubmission) { + return 'de-DE'; + } + throw new SmartlingDbException('profile not found'); + }, + ); + + $uploadQueueManager = new UploadQueueManager( + $this->createMock(ApiWrapperInterface::class), + $settingsManager, + $db, + $submissionManager, + ); + + $this->assertNull($uploadQueueManager->dequeue(1), 'Unprocessable groups must not be handed out'); + $this->assertSame( + [$resolvableSubmission, $unresolvableSubmission], + $failed, + 'Expected every existing submission in the discarded group to be failed visibly', + ); + $this->assertNotEmpty( + array_filter($queries, static fn(string $q) => str_starts_with($q, 'DELETE')), + 'Expected the unprocessable row to be removed from the queue', + ); + } + public function testDequeueFailsSubmissionsOnceAttemptsAreExhausted() { $queries = []; diff --git a/tests/Smartling/DebugTraitTest.php b/tests/Smartling/DebugTraitTest.php new file mode 100644 index 000000000..bc8e4aebd --- /dev/null +++ b/tests/Smartling/DebugTraitTest.php @@ -0,0 +1,58 @@ +assertSame($expected, $subject::isFatalError($errorType), "$label was classified incorrectly"); + } + + public function fatalErrorTypeProvider(): array + { + return [ + // These end the request and are worth an emergency. + [E_ERROR, true, 'E_ERROR'], + [E_PARSE, true, 'E_PARSE'], + [E_CORE_ERROR, true, 'E_CORE_ERROR'], + [E_COMPILE_ERROR, true, 'E_COMPILE_ERROR'], + [E_USER_ERROR, true, 'E_USER_ERROR'], + [E_RECOVERABLE_ERROR, true, 'E_RECOVERABLE_ERROR'], + // These do not, and previously flooded the log as false emergencies. + [E_USER_DEPRECATED, false, 'E_USER_DEPRECATED'], + [E_DEPRECATED, false, 'E_DEPRECATED'], + [E_WARNING, false, 'E_WARNING'], + [E_NOTICE, false, 'E_NOTICE'], + [E_USER_WARNING, false, 'E_USER_WARNING'], + [E_USER_NOTICE, false, 'E_USER_NOTICE'], + [E_CORE_WARNING, false, 'E_CORE_WARNING'], + [E_COMPILE_WARNING, false, 'E_COMPILE_WARNING'], + ]; + } + + /** + * The old message rendered the decimal error type behind an "0x" prefix, so + * E_USER_DEPRECATED showed up as the meaningless "0x16384". + */ + public function testErrorTypeIsNamedRatherThanMislabelledAsHex() + { + $subject = new class { + use DebugTrait; + }; + + $this->assertSame('E_PARSE', $subject::getErrorTypeName(E_PARSE)); + $this->assertSame('E_USER_DEPRECATED', $subject::getErrorTypeName(E_USER_DEPRECATED)); + $this->assertStringContainsString('12345', $subject::getErrorTypeName(12345), 'Unknown types should still be reported'); + } +} From 30f015d929b6fce27c9bff928197ad2c5a9c49d9 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Tue, 25 Aug 2026 15:07:15 +0200 Subject: [PATCH 04/10] fix PR #629 review findings in upload queue crash-safety (WP-1014) - UploadJob: catch \Throwable (not just \Exception) around the upload dispatch, matching processCloning() and actually delivering the crash-resilience this queue rework is meant to provide. - UploadJob: complete() the claimed queue item when no active profile is found, instead of leaving it claimed until it's retried into a misleading "terminated unexpectedly" failure. - UploadJob: processCloning() now dispatches through WordpressFunctionProxyHelper::do_action(), matching processUploadQueue() and making it mockable in tests. - UploadQueueManager: build the stale-claim WHERE fragment via ConditionBlock/Condition instead of raw sprintf ordinals, and extract the duplicated fail-and-delete logic into discardQueueItem(). - SubmissionUploadTest: complete() dequeued items in the drain loop, since dequeue() now claims rows instead of deleting them and count() no longer drops on its own. - Add UploadJobTest coverage for the \Throwable catch, the no-active-profile completion, and the proxied cloning dispatch. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/DbAl/UploadQueueManager.php | 44 ++++++--- inc/Smartling/Jobs/UploadJob.php | 5 +- .../tests/SubmissionUploadTest.php | 3 + tests/Smartling/Jobs/UploadJobTest.php | 95 ++++++++++++++++++- 4 files changed, 132 insertions(+), 15 deletions(-) diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index 3871bdc9f..50f24d6a8 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -47,10 +47,24 @@ public function count(): int public function dequeue(int $blogId): ?UploadQueueItem { + $staleClaimCondition = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_OR); + $staleClaimCondition->addCondition(new Condition( + ConditionBuilder::CONDITION_IS_NULL, + 'q.' . UploadQueueEntity::FIELD_CLAIMED, + [], + false, + )); + $staleClaimCondition->addCondition(new Condition( + ConditionBuilder::CONDITION_SIGN_LESS, + 'q.' . UploadQueueEntity::FIELD_CLAIMED, + $this->getStaleClaimThreshold(), + false, + )); + $query = sprintf(<<<'SQL' select q.%1$s, q.%2$s, q.%3$s, q.%9$s, q.%10$s from %7$s q left join %8$s s on if(locate(',', q.%2$s), left(%2$s, locate(',', %2$s) - 1), %2$s) = s.%4$s - where s.%5$s = %6$d and (q.%9$s is null or q.%9$s < '%11$s') + where s.%5$s = %6$d and %11$s SQL, UploadQueueEntity::FIELD_ID, UploadQueueEntity::FIELD_SUBMISSION_IDS, @@ -62,7 +76,7 @@ public function dequeue(int $blogId): ?UploadQueueItem $this->db->completeTableName(SubmissionEntity::getTableName()), UploadQueueEntity::FIELD_CLAIMED, UploadQueueEntity::FIELD_ATTEMPTS, - $this->getStaleClaimThreshold(), + $staleClaimCondition, ); while (($row = $this->db->getRowArray($query)) !== null) { $queueId = (int)$row[UploadQueueEntity::FIELD_ID]; @@ -97,11 +111,11 @@ public function dequeue(int $blogId): ?UploadQueueItem // takes the rest down with it. They must not vanish silently: every submission // that still exists gets a visible error instead of being left in New status // with no queue row and no explanation. - $message = 'Upload queue item discarded: unable to resolve one or more submissions grouped with this item, see log for details.'; - foreach ($existingSubmissions as $submission) { - $this->submissionManager->setErrorMessage($submission, $message); - } - $this->delete($queueId); + $this->discardQueueItem( + $queueId, + $existingSubmissions, + 'Upload queue item discarded: unable to resolve one or more submissions grouped with this item, see log for details.', + ); continue; } @@ -111,10 +125,7 @@ public function dequeue(int $blogId): ?UploadQueueItem $attempts, ); $this->getLogger()->error("Failing upload queue item id=$queueId: $message"); - foreach ($submissions as $submission) { - $this->submissionManager->setErrorMessage($submission, $message); - } - $this->delete($queueId); + $this->discardQueueItem($queueId, $submissions, $message); continue; } @@ -126,6 +137,17 @@ public function dequeue(int $blogId): ?UploadQueueItem return null; } + /** + * @param SubmissionEntity[] $submissions + */ + private function discardQueueItem(int $queueId, array $submissions, string $errorMessage): void + { + foreach ($submissions as $submission) { + $this->submissionManager->setErrorMessage($submission, $errorMessage); + } + $this->delete($queueId); + } + private function getStaleClaimThreshold(): string { return DateTimeHelper::dateTimeToString( diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index 34a082b6b..72b3eab13 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -76,6 +76,7 @@ private function processUploadQueue(int $blogId): void $profiles[$submission->getSourceBlogId()] = $this->settingsManager->getSingleSettingsProfile($submission->getSourceBlogId()); } catch (SmartlingDbException) { $this->getLogger()->notice("Skipping upload of submissionId={$submission->getId()}: no active profile found for blogId={$submission->getSourceBlogId()}"); + $this->uploadQueueManager->complete($item); continue; } } @@ -98,7 +99,7 @@ private function processUploadQueue(int $blogId): void try { $this->wpProxy->do_action(ExportedAPI::ACTION_SMARTLING_SEND_FOR_TRANSLATION, $item); - } catch (\Exception $e) { + } catch (\Throwable $e) { foreach ($item->getSubmissions() as $submission) { $this->getLogger()->notice(sprintf('Failing submissionId=%s: %s', $submission->getId(), $e->getMessage())); $this->submissionManager->setErrorMessage($submission, $e->getMessage()); @@ -113,7 +114,7 @@ private function processCloning(int $blogId): void { while (($submission = $this->submissionManager->findSubmissionForCloning($blogId)) !== null) { try { - do_action(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, $submission); + $this->wpProxy->do_action(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, $submission); } catch (\Throwable $e) { $this->submissionManager->setErrorMessage($submission, $e->getMessage()); continue; diff --git a/tests/IntegrationTests/tests/SubmissionUploadTest.php b/tests/IntegrationTests/tests/SubmissionUploadTest.php index a3f753bbb..d96a35578 100644 --- a/tests/IntegrationTests/tests/SubmissionUploadTest.php +++ b/tests/IntegrationTests/tests/SubmissionUploadTest.php @@ -104,6 +104,9 @@ public function testUploadAttachment() $submissionsToUpload += count($uploadQueueItem->getSubmissions()); $batchUid = $uploadQueueItem->getBatchUid(); $this->assertNotEquals('', $batchUid); + // dequeue() only claims rows now, it no longer deletes them, so the row must be + // explicitly completed here or count() below would never drop to zero. + $uploadQueueManager->complete($uploadQueueItem); } while ($uploadQueueManager->count() > 0); $this->assertEquals(2, $submissionsToUpload); $uploadQueueManager->enqueue( diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index 8cc0f3a70..fde8d099d 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -5,7 +5,9 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Smartling\ApiWrapperInterface; +use Smartling\Base\ExportedAPI; use Smartling\DbAl\UploadQueueManager; +use Smartling\Exception\SmartlingDbException; use Smartling\Helpers\Cache; use Smartling\Helpers\FileUriHelper; use Smartling\Helpers\WordpressFunctionProxyHelper; @@ -59,6 +61,90 @@ public function testFailedUploadStillRemovesItemButRecordsError() })->run(''); } + /** + * error_get_last()-style crashes surface as \Error, not \Exception. The dispatch + * must be resilient to those too, or a single bad hook aborts the whole cron run + * without ever completing the claimed queue item. + */ + public function testUploadThrowableFromHookStillRemovesItemButRecordsError() + { + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn(1); + $submission->method('getFileUri')->willReturn('file.xml'); + $submission->method('getSourceBlogId')->willReturn(1); + $item = $this->buildItem($submission); + + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->expects($this->once())->method('setErrorMessage') + ->with($submission, $this->stringContains('boom')); + + $this->buildJob($uploadQueueManager, $submissionManager, static function () { + throw new \Error('boom'); + })->run(''); + } + + /** + * A row is claimed (and its attempt counter incremented) by dequeue() before this + * check runs. If the row isn't completed here, it stays claimed until the stale + * claim window passes and is retried needlessly, eventually failing with a + * misleading "terminated unexpectedly" message instead of the real cause. + */ + public function testSkipsUploadAndCompletesQueueItemWhenNoActiveProfileFound() + { + $item = $this->buildItem(); + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $this->buildJob($uploadQueueManager, null, null, static function () { + throw new SmartlingDbException('no profile'); + })->run(''); + } + + /** + * processUploadQueue() dispatches through the WordPress function proxy so the hook + * call can be mocked in tests; processCloning() must do the same, or bugs in the + * cloning dispatch have no unit-test coverage. + */ + public function testCloningDispatchesThroughWordpressProxy() + { + $uploadQueueManager = $this->createMock(UploadQueueManager::class); + $uploadQueueManager->method('length')->willReturn(0); + $uploadQueueManager->method('dequeue')->willReturn(null); + + $submission = $this->createMock(SubmissionEntity::class); + $submissionManager = $this->createMock(SubmissionManager::class); + $calls = 0; + $submissionManager->method('findSubmissionForCloning')->willReturnCallback( + function () use ($submission, &$calls) { + return $calls++ === 0 ? $submission : null; + }, + ); + + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getActiveProfile') + ->willReturn($this->createMock(ConfigurationProfileEntity::class)); + + $wpProxy = $this->createMock(WordpressFunctionProxyHelper::class); + $wpProxy->method('get_current_blog_id')->willReturn(1); + $wpProxy->expects($this->once())->method('do_action') + ->with(ExportedAPI::ACTION_SMARTLING_CLONE_CONTENT, $submission); + + (new UploadJob( + $this->createMock(ApiWrapperInterface::class), + $this->createMock(Cache::class), + $this->createMock(FileUriHelper::class), + $settingsManager, + $submissionManager, + $uploadQueueManager, + $wpProxy, + 0, + 'hourly', + ))->run(''); + } + private function buildItem(?SubmissionEntity $submission = null): UploadQueueItem { if ($submission === null) { @@ -92,10 +178,15 @@ private function buildJob( UploadQueueManager $uploadQueueManager, ?SubmissionManager $submissionManager = null, ?callable $onSendForTranslation = null, + ?callable $onGetSingleSettingsProfile = null, ): UploadJob { $settingsManager = $this->createMock(SettingsManager::class); - $settingsManager->method('getSingleSettingsProfile') - ->willReturn($this->createMock(ConfigurationProfileEntity::class)); + if ($onGetSingleSettingsProfile !== null) { + $settingsManager->method('getSingleSettingsProfile')->willReturnCallback($onGetSingleSettingsProfile); + } else { + $settingsManager->method('getSingleSettingsProfile') + ->willReturn($this->createMock(ConfigurationProfileEntity::class)); + } $settingsManager->method('getActiveProfile') ->willReturn($this->createMock(ConfigurationProfileEntity::class)); From 5ba40b0887745a2011f3ef01e66ca84e02bf2570 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Tue, 25 Aug 2026 23:56:35 +0200 Subject: [PATCH 05/10] fix uncaught exception in upload queue and trait constant fatal error (WP-1014) - UploadJob::processUploadQueue() now catches failures from getOrCreateJobInfoForDailyBucketJob(), matching the crash-safety pattern already used for the settings profile lookup: log, record the error on the submission, complete the queue item, and continue instead of leaving the claimed row stuck and aborting the cron run. - DebugTrait declared FATAL_ERROR_TYPES/ERROR_TYPE_NAMES as trait constants, which PHP only allows from 8.2 onward, causing "Traits cannot have constants" fatal errors on this project's target PHP 8.0. Converted both to private static methods. --- inc/Smartling/DebugTrait.php | 61 +++++++++++++++++++------------- inc/Smartling/Jobs/UploadJob.php | 9 ++++- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/inc/Smartling/DebugTrait.php b/inc/Smartling/DebugTrait.php index 937f0b105..c6fab49f7 100644 --- a/inc/Smartling/DebugTrait.php +++ b/inc/Smartling/DebugTrait.php @@ -94,38 +94,49 @@ public static function BacktracePrint() * fatal reports an emergency on every otherwise healthy request and buries * the real crashes. */ - private const FATAL_ERROR_TYPES = E_ERROR - | E_PARSE - | E_CORE_ERROR - | E_COMPILE_ERROR - | E_USER_ERROR - | E_RECOVERABLE_ERROR; - - private const ERROR_TYPE_NAMES = [ - E_ERROR => 'E_ERROR', - E_WARNING => 'E_WARNING', - E_PARSE => 'E_PARSE', - E_NOTICE => 'E_NOTICE', - E_CORE_ERROR => 'E_CORE_ERROR', - E_CORE_WARNING => 'E_CORE_WARNING', - E_COMPILE_ERROR => 'E_COMPILE_ERROR', - E_COMPILE_WARNING => 'E_COMPILE_WARNING', - E_USER_ERROR => 'E_USER_ERROR', - E_USER_WARNING => 'E_USER_WARNING', - E_USER_NOTICE => 'E_USER_NOTICE', - E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR', - E_DEPRECATED => 'E_DEPRECATED', - E_USER_DEPRECATED => 'E_USER_DEPRECATED', - ]; + /** + * PHP traits cannot declare constants until PHP 8.2, so these are exposed + * as private static methods instead. See the property comment above for + * why only these types are treated as fatal. + */ + private static function fatalErrorTypes(): int + { + return E_ERROR + | E_PARSE + | E_CORE_ERROR + | E_COMPILE_ERROR + | E_USER_ERROR + | E_RECOVERABLE_ERROR; + } + + private static function errorTypeNames(): array + { + return [ + E_ERROR => 'E_ERROR', + E_WARNING => 'E_WARNING', + E_PARSE => 'E_PARSE', + E_NOTICE => 'E_NOTICE', + E_CORE_ERROR => 'E_CORE_ERROR', + E_CORE_WARNING => 'E_CORE_WARNING', + E_COMPILE_ERROR => 'E_COMPILE_ERROR', + E_COMPILE_WARNING => 'E_COMPILE_WARNING', + E_USER_ERROR => 'E_USER_ERROR', + E_USER_WARNING => 'E_USER_WARNING', + E_USER_NOTICE => 'E_USER_NOTICE', + E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR', + E_DEPRECATED => 'E_DEPRECATED', + E_USER_DEPRECATED => 'E_USER_DEPRECATED', + ]; + } public static function isFatalError(?int $errorType): bool { - return $errorType !== null && ($errorType & self::FATAL_ERROR_TYPES) !== 0; + return $errorType !== null && ($errorType & self::fatalErrorTypes()) !== 0; } public static function getErrorTypeName(int $errorType): string { - return self::ERROR_TYPE_NAMES[$errorType] ?? "UNKNOWN($errorType)"; + return self::errorTypeNames()[$errorType] ?? "UNKNOWN($errorType)"; } /** diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index 72b3eab13..0ae6c4038 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -82,7 +82,14 @@ private function processUploadQueue(int $blogId): void } $profile = $profiles[$submission->getSourceBlogId()]; if ($item->getBatchUid() === '') { - $item = $item->setBatchUid($this->api->getOrCreateJobInfoForDailyBucketJob($profile, [$submission->getFileUri()])->getBatchUid()); + try { + $item = $item->setBatchUid($this->api->getOrCreateJobInfoForDailyBucketJob($profile, [$submission->getFileUri()])->getBatchUid()); + } catch (\Throwable $e) { + $this->getLogger()->notice("Skipping upload of submissionId={$submission->getId()}: failed to get or create daily bucket job: {$e->getMessage()}"); + $this->submissionManager->setErrorMessage($submission, $e->getMessage()); + $this->uploadQueueManager->complete($item); + continue; + } } $this->getLogger()->info(sprintf( From 1f13c1ae9e62c9af88581505a08df836c13f9515 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 26 Aug 2026 00:27:29 +0200 Subject: [PATCH 06/10] cleanup (WP-1014) --- inc/Smartling/DbAl/UploadQueueManager.php | 12 ++++-------- inc/Smartling/DebugTrait.php | 7 +------ inc/Smartling/Models/UploadQueueItem.php | 4 ++-- .../IntegrationTests/tests/SubmissionUploadTest.php | 2 -- tests/Smartling/Models/UploadQueueItemTest.php | 4 ++-- 5 files changed, 9 insertions(+), 20 deletions(-) diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index 50f24d6a8..ebcf931c8 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -47,6 +47,9 @@ public function count(): int public function dequeue(int $blogId): ?UploadQueueItem { + // Get queue items with the first submission having its source blog id = $blogId. + // It should be impossible to create a single queue item with submissions from multiple source blog ids, + // so only checking one is enough. $staleClaimCondition = new ConditionBlock(ConditionBuilder::CONDITION_BLOCK_LEVEL_OPERATOR_OR); $staleClaimCondition->addCondition(new Condition( ConditionBuilder::CONDITION_IS_NULL, @@ -107,10 +110,6 @@ public function dequeue(int $blogId): ?UploadQueueItem } if ($unprocessable) { - // The whole row is grouped by shared content, so one unresolvable submission - // takes the rest down with it. They must not vanish silently: every submission - // that still exists gets a visible error instead of being left in New status - // with no queue row and no explanation. $this->discardQueueItem( $queueId, $existingSubmissions, @@ -158,10 +157,7 @@ private function getStaleClaimThreshold(): string public function complete(UploadQueueItem $item): void { - $id = $item->getId(); - if ($id !== null) { - $this->delete($id); - } + $this->delete($item->getId()); } private function claim(int $id, int $attempts): void diff --git a/inc/Smartling/DebugTrait.php b/inc/Smartling/DebugTrait.php index c6fab49f7..db88f85b0 100644 --- a/inc/Smartling/DebugTrait.php +++ b/inc/Smartling/DebugTrait.php @@ -89,16 +89,11 @@ public static function BacktracePrint() /** * Error types that actually terminate the request. Anything else - notices, - * warnings and in particular deprecations - is left alone: error_get_last() + * warnings, and in particular deprecations - is left alone: error_get_last() * returns the last error of *any* severity, so treating non-fatal types as * fatal reports an emergency on every otherwise healthy request and buries * the real crashes. */ - /** - * PHP traits cannot declare constants until PHP 8.2, so these are exposed - * as private static methods instead. See the property comment above for - * why only these types are treated as fatal. - */ private static function fatalErrorTypes(): int { return E_ERROR diff --git a/inc/Smartling/Models/UploadQueueItem.php b/inc/Smartling/Models/UploadQueueItem.php index 3984e1b5c..5806d85e9 100644 --- a/inc/Smartling/Models/UploadQueueItem.php +++ b/inc/Smartling/Models/UploadQueueItem.php @@ -13,7 +13,7 @@ public function __construct( private array $submissions, private string $batchUid, private IntStringPairCollection $smartlingLocales, - private ?int $id = null, + private int $id, ) { $contentTypes = []; $sourceBlogIds = []; @@ -46,7 +46,7 @@ public function getBatchUid(): string return $this->batchUid; } - public function getId(): ?int + public function getId(): int { return $this->id; } diff --git a/tests/IntegrationTests/tests/SubmissionUploadTest.php b/tests/IntegrationTests/tests/SubmissionUploadTest.php index d96a35578..e503ea1e4 100644 --- a/tests/IntegrationTests/tests/SubmissionUploadTest.php +++ b/tests/IntegrationTests/tests/SubmissionUploadTest.php @@ -104,8 +104,6 @@ public function testUploadAttachment() $submissionsToUpload += count($uploadQueueItem->getSubmissions()); $batchUid = $uploadQueueItem->getBatchUid(); $this->assertNotEquals('', $batchUid); - // dequeue() only claims rows now, it no longer deletes them, so the row must be - // explicitly completed here or count() below would never drop to zero. $uploadQueueManager->complete($uploadQueueItem); } while ($uploadQueueManager->count() > 0); $this->assertEquals(2, $submissionsToUpload); diff --git a/tests/Smartling/Models/UploadQueueItemTest.php b/tests/Smartling/Models/UploadQueueItemTest.php index fe68b3f56..f4486ed34 100644 --- a/tests/Smartling/Models/UploadQueueItemTest.php +++ b/tests/Smartling/Models/UploadQueueItemTest.php @@ -13,7 +13,7 @@ public function testRemoveSubmission() $s1->setId(1); $s2 = new SubmissionEntity(); $s2->setId(2); - $x = new UploadQueueItem([$s1, $s2], '', new IntStringPairCollection([new IntStringPair(1, 'a'), new IntStringPair(2, 'b')])); + $x = new UploadQueueItem([$s1, $s2], '', new IntStringPairCollection([new IntStringPair(1, 'a'), new IntStringPair(2, 'b')]), 1); foreach ($x->getSubmissions() as $submission) { if ($submission->getId() === 1) { $x = $x->removeSubmission($submission); @@ -31,7 +31,7 @@ public function testSubmissionsAltered() $s1->setId(1); $s2 = new SubmissionEntity(); $s2->setId(2); - $x = new UploadQueueItem([$s1, $s2], '', new IntStringPairCollection([new IntStringPair(1, 'a'), new IntStringPair(2, 'b')])); + $x = new UploadQueueItem([$s1, $s2], '', new IntStringPairCollection([new IntStringPair(1, 'a'), new IntStringPair(2, 'b')]), 1); foreach ($x->getSubmissions() as $submission) { $submission->setStatus(SubmissionEntity::SUBMISSION_STATUS_COMPLETED); } From 1fcd44c050d02a1544fcb8ca925fe55809219706 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Wed, 26 Aug 2026 14:27:48 +0200 Subject: [PATCH 07/10] fix PR #629 review findings: silent submission drop and migration failure (WP-1014) processUploadQueue() only logged/errored the first submission in a queue item when the profile lookup or daily bucket job creation failed, then deleted the whole row. Any sibling submission grouped in the same item (same content, other target locale) vanished silently: no error, no log line, stuck in New forever. Both catch blocks now loop over every submission in the item. Migration260825's ADD COLUMN also failed for any site still on a schema version below 240315: Migration240315 recreates the table with CREATE TABLE IF NOT EXISTS from the live, current UploadQueueEntity::getFieldDefinitions(), which already includes the new claimed/attempts columns, so the later unconditional ALTER TABLE hit a duplicate-column error and never recorded itself as applied. The migration now checks SHOW COLUMNS first and only adds what's actually missing. Co-Authored-By: Claude Sonnet 5 --- .../DbAl/Migrations/Migration260825.php | 37 +++++---- inc/Smartling/Jobs/UploadJob.php | 12 ++- tests/Smartling/Jobs/UploadJobTest.php | 82 ++++++++++++++++++- 3 files changed, 112 insertions(+), 19 deletions(-) diff --git a/inc/Smartling/DbAl/Migrations/Migration260825.php b/inc/Smartling/DbAl/Migrations/Migration260825.php index ea048eeb3..3fc70be58 100644 --- a/inc/Smartling/DbAl/Migrations/Migration260825.php +++ b/inc/Smartling/DbAl/Migrations/Migration260825.php @@ -23,21 +23,28 @@ public function getVersion(): int public function getQueries($tablePrefix = 'wp_'): array { - $tableName = (new DB())->completeTableName(UploadQueueEntity::getTableName()); - - return [ - sprintf( - 'ALTER TABLE `%s` ADD COLUMN `%s` %s', - $tableName, - UploadQueueEntity::FIELD_CLAIMED, - SmartlingEntityAbstract::DB_TYPE_DATETIME_NULL, - ), - sprintf( - 'ALTER TABLE `%s` ADD COLUMN `%s` %s', - $tableName, - UploadQueueEntity::FIELD_ATTEMPTS, - SmartlingEntityAbstract::DB_TYPE_U_BIGINT . ' ' . SmartlingEntityAbstract::DB_TYPE_DEFAULT_ZERO, - ), + $db = new DB(); + $tableName = $db->completeTableName(UploadQueueEntity::getTableName()); + + // Migration240315 (re)creates this table with `CREATE TABLE IF NOT EXISTS` from the + // current, evolving UploadQueueEntity::getFieldDefinitions(). A site upgrading from a + // schema version older than 240315 runs that migration first, and it already creates + // the table with the columns below, so blindly adding them here would fail with a + // duplicate column error. Only add what isn't already there. + $existingColumns = $db->getColumnArray("SHOW COLUMNS FROM `$tableName`"); + + $columns = [ + UploadQueueEntity::FIELD_CLAIMED => SmartlingEntityAbstract::DB_TYPE_DATETIME_NULL, + UploadQueueEntity::FIELD_ATTEMPTS => SmartlingEntityAbstract::DB_TYPE_U_BIGINT . ' ' . SmartlingEntityAbstract::DB_TYPE_DEFAULT_ZERO, ]; + + $queries = []; + foreach ($columns as $column => $definition) { + if (!in_array($column, $existingColumns, true)) { + $queries[] = sprintf('ALTER TABLE `%s` ADD COLUMN `%s` %s', $tableName, $column, $definition); + } + } + + return $queries; } } diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index 0ae6c4038..ee31d5533 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -75,7 +75,11 @@ private function processUploadQueue(int $blogId): void try { $profiles[$submission->getSourceBlogId()] = $this->settingsManager->getSingleSettingsProfile($submission->getSourceBlogId()); } catch (SmartlingDbException) { - $this->getLogger()->notice("Skipping upload of submissionId={$submission->getId()}: no active profile found for blogId={$submission->getSourceBlogId()}"); + $message = "No active profile found for blogId={$submission->getSourceBlogId()}"; + foreach ($item->getSubmissions() as $itemSubmission) { + $this->getLogger()->notice("Skipping upload of submissionId={$itemSubmission->getId()}: $message"); + $this->submissionManager->setErrorMessage($itemSubmission, $message); + } $this->uploadQueueManager->complete($item); continue; } @@ -85,8 +89,10 @@ private function processUploadQueue(int $blogId): void try { $item = $item->setBatchUid($this->api->getOrCreateJobInfoForDailyBucketJob($profile, [$submission->getFileUri()])->getBatchUid()); } catch (\Throwable $e) { - $this->getLogger()->notice("Skipping upload of submissionId={$submission->getId()}: failed to get or create daily bucket job: {$e->getMessage()}"); - $this->submissionManager->setErrorMessage($submission, $e->getMessage()); + foreach ($item->getSubmissions() as $itemSubmission) { + $this->getLogger()->notice("Skipping upload of submissionId={$itemSubmission->getId()}: failed to get or create daily bucket job: {$e->getMessage()}"); + $this->submissionManager->setErrorMessage($itemSubmission, $e->getMessage()); + } $this->uploadQueueManager->complete($item); continue; } diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index fde8d099d..77b14c58d 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -103,6 +103,85 @@ public function testSkipsUploadAndCompletesQueueItemWhenNoActiveProfileFound() })->run(''); } + /** + * A queue item groups submissions for the same content across multiple target + * locales; only the first one is used to look up the profile/batch job. If either + * lookup fails, every submission in the group must be failed visibly, not just the + * first, or siblings silently vanish with the row while staying in "New" forever. + */ + public function testFailsEverySubmissionWhenNoActiveProfileFound() + { + [$item, $submission1, $submission2] = $this->buildTwoSubmissionItem(); + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $submissionManager = $this->createMock(SubmissionManager::class); + $failed = []; + $submissionManager->method('setErrorMessage')->willReturnCallback( + static function (SubmissionEntity $submission, string $message) use (&$failed) { + $failed[] = $submission; + return $submission; + }, + ); + + $this->buildJob($uploadQueueManager, $submissionManager, null, static function () { + throw new SmartlingDbException('no profile'); + })->run(''); + + $this->assertSame([$submission1, $submission2], $failed, 'Expected every submission in the group to be failed visibly'); + } + + /** + * Same as above, but for the daily-bucket-job lookup failing instead of the profile + * lookup. + */ + public function testFailsEverySubmissionWhenDailyBucketJobCannotBeCreated() + { + [$item, $submission1, $submission2] = $this->buildTwoSubmissionItem(); + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $submissionManager = $this->createMock(SubmissionManager::class); + $failed = []; + $submissionManager->method('setErrorMessage')->willReturnCallback( + static function (SubmissionEntity $submission, string $message) use (&$failed) { + $failed[] = $submission; + return $submission; + }, + ); + + $api = $this->createMock(ApiWrapperInterface::class); + $api->method('getOrCreateJobInfoForDailyBucketJob')->willThrowException(new \RuntimeException('boom')); + + $this->buildJob($uploadQueueManager, $submissionManager, null, null, $api)->run(''); + + $this->assertSame([$submission1, $submission2], $failed, 'Expected every submission in the group to be failed visibly'); + } + + /** + * @return array{0: UploadQueueItem, 1: SubmissionEntity, 2: SubmissionEntity} + */ + private function buildTwoSubmissionItem(): array + { + $submission1 = $this->createMock(SubmissionEntity::class); + $submission1->method('getId')->willReturn(1); + $submission1->method('getFileUri')->willReturn('file.xml'); + $submission1->method('getSourceBlogId')->willReturn(1); + $submission2 = $this->createMock(SubmissionEntity::class); + $submission2->method('getId')->willReturn(2); + $submission2->method('getFileUri')->willReturn('file.xml'); + $submission2->method('getSourceBlogId')->willReturn(1); + + $item = new UploadQueueItem( + [$submission1, $submission2], + '', + new IntStringPairCollection([new IntStringPair(1, 'de-DE'), new IntStringPair(2, 'fr-FR')]), + 42, + ); + + return [$item, $submission1, $submission2]; + } + /** * processUploadQueue() dispatches through the WordPress function proxy so the hook * call can be mocked in tests; processCloning() must do the same, or bugs in the @@ -179,6 +258,7 @@ private function buildJob( ?SubmissionManager $submissionManager = null, ?callable $onSendForTranslation = null, ?callable $onGetSingleSettingsProfile = null, + ?ApiWrapperInterface $api = null, ): UploadJob { $settingsManager = $this->createMock(SettingsManager::class); if ($onGetSingleSettingsProfile !== null) { @@ -200,7 +280,7 @@ private function buildJob( } return new UploadJob( - $this->createMock(ApiWrapperInterface::class), + $api ?? $this->createMock(ApiWrapperInterface::class), $this->createMock(Cache::class), $this->createMock(FileUriHelper::class), $settingsManager, From 33ed2559001a57de100fbfb1fc95def047a8ce62 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 27 Aug 2026 20:13:46 +0200 Subject: [PATCH 08/10] fix PR #629 review findings: unresolvable queue rows never claimed, cloned submissions still uploaded (WP-1014) - UploadQueueManager::dequeue(): wrap per-submission resolution in a try/catch(\Throwable). claim() runs after resolution, so any exception besides the already-handled SmartlingDbException left the row completely unclaimed forever, re-thrown on every future dequeue() call and blocking the rest of the per-blog queue from ever being processed. It's now treated like the existing missing-submission/unresolvable-locale cases: logged and discarded with a visible error. - UploadQueueManager::getStaleClaimThreshold(): source "now" from DateTimeHelper::getDefaultTimezone() instead of a separately hardcoded UTC, so it can't desync from claim()'s DateTimeHelper::nowAsString(). - UploadQueueManager::dequeue(): drop the redundant $submissions array, which was always identical to $existingSubmissions by the time it was used. - UploadJob::processUploadQueue(): isCloned() logged "skipping" but never skipped, uploading cloned submissions same as any other. Added the missing complete()+continue. - UploadJob: extract the three copy-pasted "fail every submission in the item" blocks into failItem(), fixing the do_action-catch block's variable shadowing of the outer $submission along the way. - UploadQueueManagerTest::testDequeue(): restore coverage of the locate()/left() join clause that extracts the first submission id from a group, lost when the exact-SQL assertEquals was loosened to assertStringContainsString (necessary since the query now embeds a wall-clock-dependent stale-claim timestamp). - Add tests: dequeue() discarding a row when resolution throws an unexpected exception, and UploadJob skipping cloned submissions. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/DbAl/UploadQueueManager.php | 39 ++++++++------- inc/Smartling/Jobs/UploadJob.php | 28 ++++++----- .../Smartling/DbAl/UploadQueueManagerTest.php | 50 +++++++++++++++++++ tests/Smartling/Jobs/UploadJobTest.php | 23 +++++++++ 4 files changed, 109 insertions(+), 31 deletions(-) diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index ebcf931c8..3bd3f8785 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -85,28 +85,31 @@ public function dequeue(int $blogId): ?UploadQueueItem $queueId = (int)$row[UploadQueueEntity::FIELD_ID]; $attempts = (int)($row[UploadQueueEntity::FIELD_ATTEMPTS] ?? 0); $locales = new IntStringPairCollection(); - $submissions = []; $existingSubmissions = []; $unprocessable = false; foreach (IntegerIterator::fromString($row[UploadQueueEntity::FIELD_SUBMISSION_IDS]) as $submissionId) { - $submission = $this->submissionManager->getEntityById($submissionId); - if ($submission === null) { - $this->getLogger()->warning("Discarding upload queue item id=$queueId: submissionId=$submissionId no longer exists"); - $unprocessable = true; - continue; - } + try { + $submission = $this->submissionManager->getEntityById($submissionId); + if ($submission === null) { + $this->getLogger()->warning("Discarding upload queue item id=$queueId: submissionId=$submissionId no longer exists"); + $unprocessable = true; + continue; + } + + $existingSubmissions[] = $submission; - $existingSubmissions[] = $submission; + $locale = $this->getSmartlingLocale($submission); + if ($locale === null) { + $this->getLogger()->warning("Discarding upload queue item id=$queueId: unable to resolve target locale for submissionId=$submissionId, targetBlogId={$submission->getTargetBlogId()}"); + $unprocessable = true; + continue; + } - $locale = $this->getSmartlingLocale($submission); - if ($locale === null) { - $this->getLogger()->warning("Discarding upload queue item id=$queueId: unable to resolve target locale for submissionId=$submissionId, targetBlogId={$submission->getTargetBlogId()}"); + $locales = $locales->add([new IntStringPair($submission->getId(), $locale)]); + } catch (\Throwable $e) { + $this->getLogger()->warning("Discarding upload queue item id=$queueId: failed to resolve submissionId=$submissionId: {$e->getMessage()}"); $unprocessable = true; - continue; } - - $locales = $locales->add([new IntStringPair($submission->getId(), $locale)]); - $submissions[] = $submission; } if ($unprocessable) { @@ -124,13 +127,13 @@ public function dequeue(int $blogId): ?UploadQueueItem $attempts, ); $this->getLogger()->error("Failing upload queue item id=$queueId: $message"); - $this->discardQueueItem($queueId, $submissions, $message); + $this->discardQueueItem($queueId, $existingSubmissions, $message); continue; } $this->claim($queueId, $attempts); - return new UploadQueueItem($submissions, $row[UploadQueueEntity::FIELD_BATCH_UID], $locales, $queueId); + return new UploadQueueItem($existingSubmissions, $row[UploadQueueEntity::FIELD_BATCH_UID], $locales, $queueId); } return null; @@ -150,7 +153,7 @@ private function discardQueueItem(int $queueId, array $submissions, string $erro private function getStaleClaimThreshold(): string { return DateTimeHelper::dateTimeToString( - (new \DateTime('now', new \DateTimeZone(DateTimeHelper::TIMEZONE_UTC))) + (new \DateTime('now', DateTimeHelper::getDefaultTimezone())) ->modify('-' . self::STALE_CLAIM_SECONDS . ' seconds') ); } diff --git a/inc/Smartling/Jobs/UploadJob.php b/inc/Smartling/Jobs/UploadJob.php index ee31d5533..c903cf36b 100644 --- a/inc/Smartling/Jobs/UploadJob.php +++ b/inc/Smartling/Jobs/UploadJob.php @@ -9,6 +9,7 @@ use Smartling\Helpers\Cache; use Smartling\Helpers\FileUriHelper; use Smartling\Helpers\WordpressFunctionProxyHelper; +use Smartling\Models\UploadQueueItem; use Smartling\Settings\SettingsManager; use Smartling\Submissions\SubmissionManager; @@ -66,6 +67,8 @@ private function processUploadQueue(int $blogId): void $this->getLogger()->debug("Retrieved upload queue item for submissionId={$submission->getId()}"); if ($submission->isCloned()) { $this->getLogger()->debug("Skipping processing queue for submissionId={$submission->getId()}: was cloned"); + $this->uploadQueueManager->complete($item); + continue; } if ($submission->getFileUri() === '') { $submission->setFileUri($this->fileUriHelper->generateFileUri($submission)); @@ -75,11 +78,7 @@ private function processUploadQueue(int $blogId): void try { $profiles[$submission->getSourceBlogId()] = $this->settingsManager->getSingleSettingsProfile($submission->getSourceBlogId()); } catch (SmartlingDbException) { - $message = "No active profile found for blogId={$submission->getSourceBlogId()}"; - foreach ($item->getSubmissions() as $itemSubmission) { - $this->getLogger()->notice("Skipping upload of submissionId={$itemSubmission->getId()}: $message"); - $this->submissionManager->setErrorMessage($itemSubmission, $message); - } + $this->failItem($item, 'Skipping upload of', "No active profile found for blogId={$submission->getSourceBlogId()}"); $this->uploadQueueManager->complete($item); continue; } @@ -89,10 +88,7 @@ private function processUploadQueue(int $blogId): void try { $item = $item->setBatchUid($this->api->getOrCreateJobInfoForDailyBucketJob($profile, [$submission->getFileUri()])->getBatchUid()); } catch (\Throwable $e) { - foreach ($item->getSubmissions() as $itemSubmission) { - $this->getLogger()->notice("Skipping upload of submissionId={$itemSubmission->getId()}: failed to get or create daily bucket job: {$e->getMessage()}"); - $this->submissionManager->setErrorMessage($itemSubmission, $e->getMessage()); - } + $this->failItem($item, 'Skipping upload of', $e->getMessage(), "failed to get or create daily bucket job: {$e->getMessage()}"); $this->uploadQueueManager->complete($item); continue; } @@ -113,16 +109,22 @@ private function processUploadQueue(int $blogId): void try { $this->wpProxy->do_action(ExportedAPI::ACTION_SMARTLING_SEND_FOR_TRANSLATION, $item); } catch (\Throwable $e) { - foreach ($item->getSubmissions() as $submission) { - $this->getLogger()->notice(sprintf('Failing submissionId=%s: %s', $submission->getId(), $e->getMessage())); - $this->submissionManager->setErrorMessage($submission, $e->getMessage()); - } + $this->failItem($item, 'Failing', $e->getMessage()); } $this->uploadQueueManager->complete($item); $this->placeLockFlag(true); } } + private function failItem(UploadQueueItem $item, string $logVerb, string $errorMessage, ?string $logMessage = null): void + { + $logMessage ??= $errorMessage; + foreach ($item->getSubmissions() as $submission) { + $this->getLogger()->notice("$logVerb submissionId={$submission->getId()}: $logMessage"); + $this->submissionManager->setErrorMessage($submission, $errorMessage); + } + } + private function processCloning(int $blogId): void { while (($submission = $this->submissionManager->findSubmissionForCloning($blogId)) !== null) { diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index 482f9eee9..2a742f91b 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -153,6 +153,11 @@ public function query() {} 'from smartling_upload_queue q left join smartling_submissions s', $query, ); + $this->assertStringContainsString( + "on if(locate(',', q.submission_ids), left(submission_ids, locate(',', submission_ids) - 1), submission_ids) = s.id", + $query, + 'Expected the join to extract the first submission id from the comma-separated group', + ); $this->assertStringContainsString('where s.source_blog_id = 1', $query); return match ($matcherGetRowArray->getInvocationCount()) { @@ -352,6 +357,51 @@ public function testDequeueFailsSubmissionsOnceAttemptsAreExhausted() ); } + /** + * dequeue() claims a row only after resolving every submission in it. If that + * resolution throws anything unexpected, the row must still end up discarded + * rather than left permanently unclaimed - otherwise a single misbehaving + * submission blocks the entire per-blog queue forever, since every future + * dequeue() call would hit the same exception before ever reaching claim(). + */ + public function testDequeueDiscardsItemWhenResolvingASubmissionThrowsUnexpectedException() + { + $this->mockDbAl(); + $db = $this->getMockBuilder(DB::class) + ->setConstructorArgs([new class { + public string $base_prefix = ''; + public function getRowArray() {} + public function query() {} + }]) + ->onlyMethods(['getRowArray', 'query']) + ->getMock(); + $db->method('getRowArray')->willReturnOnConsecutiveCalls( + ['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0], + null, + ); + $queries = []; + $db->method('query')->willReturnCallback(function ($query) use (&$queries) { + $queries[] = $query; + return true; + }); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('getEntityById')->willThrowException(new \RuntimeException('DB connection lost')); + + $uploadQueueManager = new UploadQueueManager( + $this->createMock(ApiWrapperInterface::class), + $this->createMock(SettingsManager::class), + $db, + $submissionManager, + ); + + $this->assertNull($uploadQueueManager->dequeue(1), 'An unresolvable row must not be handed out, but must not throw either'); + $this->assertNotEmpty( + array_filter($queries, static fn(string $q) => str_starts_with($q, 'DELETE')), + 'Expected the unresolvable row to be removed from the queue rather than left claimed forever', + ); + } + /** * @param array $rows sequential getRowArray() return values * @param int[] $submissions map of submission id => source blog id that exist diff --git a/tests/Smartling/Jobs/UploadJobTest.php b/tests/Smartling/Jobs/UploadJobTest.php index 77b14c58d..aca6ae284 100644 --- a/tests/Smartling/Jobs/UploadJobTest.php +++ b/tests/Smartling/Jobs/UploadJobTest.php @@ -103,6 +103,29 @@ public function testSkipsUploadAndCompletesQueueItemWhenNoActiveProfileFound() })->run(''); } + /** + * A cloned submission's content was already uploaded at clone time; re-uploading + * it would be a duplicate. Before this fix, isCloned() only logged that the item + * was "being skipped" without actually skipping it. + */ + public function testClonedSubmissionIsSkippedAndCompletesQueueItemWithoutUploading() + { + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn(1); + $submission->method('isCloned')->willReturn(true); + $item = $this->buildItem($submission); + + $uploadQueueManager = $this->buildQueueManager($item); + $uploadQueueManager->expects($this->once())->method('complete')->with($item); + + $uploaded = false; + $this->buildJob($uploadQueueManager, null, static function () use (&$uploaded) { + $uploaded = true; + })->run(''); + + $this->assertFalse($uploaded, 'Cloned submissions must not be uploaded'); + } + /** * A queue item groups submissions for the same content across multiple target * locales; only the first one is used to look up the profile/batch job. If either From a2af21672b9dda26623c882a00a2597224053435 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 27 Aug 2026 20:42:09 +0200 Subject: [PATCH 09/10] check delete() result to stop dequeue() spinning on a failed discard (WP-1014) PR #629 review comment (PavelLoparev, 3863741398): delete()'s $this->db->query() result was never checked. wpdb::query() returns false on failure (deadlock, lock-wait timeout, connection blip) without throwing, so a failed DELETE inside discardQueueItem() went unnoticed: dequeue()'s while loop would continue, re-select the exact same still-present row, and discard it again - spinning on it forever within a single dequeue() call. - delete() now returns bool. - discardQueueItem() returns bool and logs when the delete didn't happen; both call sites in dequeue() return null instead of continuing the loop when a discard fails to actually remove the row. - complete() logs (but doesn't otherwise react) on a failed delete: the row simply stays claimed and is picked up by the existing stale-claim retry path, so no special handling is needed there beyond visibility. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/DbAl/UploadQueueManager.php | 41 +++++++++++++---- .../Smartling/DbAl/UploadQueueManagerTest.php | 44 +++++++++++++++++++ 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index 3bd3f8785..53e956c74 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -113,11 +113,16 @@ public function dequeue(int $blogId): ?UploadQueueItem } if ($unprocessable) { - $this->discardQueueItem( + if (!$this->discardQueueItem( $queueId, $existingSubmissions, 'Upload queue item discarded: unable to resolve one or more submissions grouped with this item, see log for details.', - ); + )) { + // The row is still there and would come back as the exact same result on + // the next iteration of this loop: bail out of this dequeue() call rather + // than spin on a delete that keeps failing. + return null; + } continue; } @@ -127,7 +132,9 @@ public function dequeue(int $blogId): ?UploadQueueItem $attempts, ); $this->getLogger()->error("Failing upload queue item id=$queueId: $message"); - $this->discardQueueItem($queueId, $existingSubmissions, $message); + if (!$this->discardQueueItem($queueId, $existingSubmissions, $message)) { + return null; + } continue; } @@ -141,13 +148,19 @@ public function dequeue(int $blogId): ?UploadQueueItem /** * @param SubmissionEntity[] $submissions + * @return bool Whether the row was actually removed from the queue. */ - private function discardQueueItem(int $queueId, array $submissions, string $errorMessage): void + private function discardQueueItem(int $queueId, array $submissions, string $errorMessage): bool { foreach ($submissions as $submission) { $this->submissionManager->setErrorMessage($submission, $errorMessage); } - $this->delete($queueId); + if (!$this->delete($queueId)) { + $this->getLogger()->error("Failed to delete upload queue item id=$queueId after discarding it"); + return false; + } + + return true; } private function getStaleClaimThreshold(): string @@ -160,7 +173,13 @@ private function getStaleClaimThreshold(): string public function complete(UploadQueueItem $item): void { - $this->delete($item->getId()); + if (!$this->delete($item->getId())) { + // Not left in an inconsistent state: the row stays claimed and picks up the + // existing stale-claim retry path, same as a crash would. Logged only so a + // recurring DB failure here is visible instead of only showing up as unexplained + // re-uploads later. + $this->getLogger()->error("Failed to delete completed upload queue item id={$item->getId()}"); + } } private function claim(int $id, int $attempts): void @@ -258,9 +277,15 @@ private function getSmartlingLocale(SubmissionEntity $submission): ?string return null; } - private function delete(int $id): void + /** + * @return bool Whether the row was actually removed. $wpdb->query() returns false on + * failure (deadlock, lock-wait timeout, connection blip) without throwing, so + * this must be checked rather than assumed: a caller that keeps treating the + * row as gone when it silently wasn't can end up looping on it forever. + */ + private function delete(int $id): bool { - $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))); + return $this->db->query(QueryBuilder::buildDeleteQuery($this->tableName, $this->idCondition($id))) !== false; } private function idCondition(int $id): ConditionBlock diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index 2a742f91b..21372f169 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -357,6 +357,50 @@ public function testDequeueFailsSubmissionsOnceAttemptsAreExhausted() ); } + /** + * $wpdb->query() returns false on failure (deadlock, lock-wait timeout, connection + * blip) without throwing. If discardQueueItem()'s delete() silently fails, dequeue() + * must not treat the row as gone and re-select: the row comes back unchanged, so + * continuing the while loop would spin on it forever inside a single dequeue() call. + */ + public function testDequeueStopsInsteadOfSpinningWhenDiscardFailsToDelete() + { + $this->mockDbAl(); + $db = $this->getMockBuilder(DB::class) + ->setConstructorArgs([new class { + public string $base_prefix = ''; + public function getRowArray() {} + public function query() {} + }]) + ->onlyMethods(['getRowArray', 'query']) + ->getMock(); + $selectCalls = 0; + $db->method('getRowArray')->willReturnCallback(function () use (&$selectCalls) { + $selectCalls++; + // The same unprocessable row every time, as it would be in reality if the + // DELETE below kept failing and never actually removed it. + return ['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0]; + }); + $db->method('query')->willReturn(false); + + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('getEntityById')->willReturn(null); + + $uploadQueueManager = new UploadQueueManager( + $this->createMock(ApiWrapperInterface::class), + $this->createMock(SettingsManager::class), + $db, + $submissionManager, + ); + + $this->assertNull($uploadQueueManager->dequeue(1)); + $this->assertSame( + 1, + $selectCalls, + 'Expected dequeue() to stop after the first failed delete rather than re-selecting the same row forever', + ); + } + /** * dequeue() claims a row only after resolving every submission in it. If that * resolution throws anything unexpected, the row must still end up discarded From 3b055517497b184d031b5e04723d401bcaa77895 Mon Sep 17 00:00:00 2001 From: Vitalii Solovei Date: Thu, 27 Aug 2026 20:44:26 +0200 Subject: [PATCH 10/10] check claim() result before handing out a dequeued item (WP-1014) PR #629 review comment (PavelLoparev, 3863741379): claim()'s $this->db->query() result was never checked. wpdb::query() returns false on failure without throwing, so a silently failed claim let dequeue() hand out an UploadQueueItem whose row a second dequeue($blogId) call was still free to claim - dispatching the same content for translation twice. claim() now returns bool; dequeue() returns null instead of returning the item when the claim can't be confirmed. Co-Authored-By: Claude Sonnet 5 --- inc/Smartling/DbAl/UploadQueueManager.php | 26 +++++------- .../Smartling/DbAl/UploadQueueManagerTest.php | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 15 deletions(-) diff --git a/inc/Smartling/DbAl/UploadQueueManager.php b/inc/Smartling/DbAl/UploadQueueManager.php index 53e956c74..5e393a9c5 100644 --- a/inc/Smartling/DbAl/UploadQueueManager.php +++ b/inc/Smartling/DbAl/UploadQueueManager.php @@ -118,9 +118,6 @@ public function dequeue(int $blogId): ?UploadQueueItem $existingSubmissions, 'Upload queue item discarded: unable to resolve one or more submissions grouped with this item, see log for details.', )) { - // The row is still there and would come back as the exact same result on - // the next iteration of this loop: bail out of this dequeue() call rather - // than spin on a delete that keeps failing. return null; } continue; @@ -138,7 +135,10 @@ public function dequeue(int $blogId): ?UploadQueueItem continue; } - $this->claim($queueId, $attempts); + if (!$this->claim($queueId, $attempts)) { + $this->getLogger()->error("Failed to claim upload queue item id=$queueId"); + return null; + } return new UploadQueueItem($existingSubmissions, $row[UploadQueueEntity::FIELD_BATCH_UID], $locales, $queueId); } @@ -174,24 +174,23 @@ private function getStaleClaimThreshold(): string public function complete(UploadQueueItem $item): void { if (!$this->delete($item->getId())) { - // Not left in an inconsistent state: the row stays claimed and picks up the - // existing stale-claim retry path, same as a crash would. Logged only so a - // recurring DB failure here is visible instead of only showing up as unexplained - // re-uploads later. $this->getLogger()->error("Failed to delete completed upload queue item id={$item->getId()}"); } } - private function claim(int $id, int $attempts): void + /** + * @return bool Whether the row was actually claimed. + */ + private function claim(int $id, int $attempts): bool { - $this->db->query(QueryBuilder::buildUpdateQuery( + return $this->db->query(QueryBuilder::buildUpdateQuery( $this->tableName, [ UploadQueueEntity::FIELD_CLAIMED => DateTimeHelper::nowAsString(), UploadQueueEntity::FIELD_ATTEMPTS => $attempts + 1, ], $this->idCondition($id), - )); + )) !== false; } public function enqueue(IntegerIterator $submissionIds, string $batchUid): void @@ -278,10 +277,7 @@ private function getSmartlingLocale(SubmissionEntity $submission): ?string } /** - * @return bool Whether the row was actually removed. $wpdb->query() returns false on - * failure (deadlock, lock-wait timeout, connection blip) without throwing, so - * this must be checked rather than assumed: a caller that keeps treating the - * row as gone when it silently wasn't can end up looping on it forever. + * @return bool Whether the row was actually removed. */ private function delete(int $id): bool { diff --git a/tests/Smartling/DbAl/UploadQueueManagerTest.php b/tests/Smartling/DbAl/UploadQueueManagerTest.php index 21372f169..db75522d4 100644 --- a/tests/Smartling/DbAl/UploadQueueManagerTest.php +++ b/tests/Smartling/DbAl/UploadQueueManagerTest.php @@ -220,6 +220,48 @@ public function testDequeueClaimsRowInsteadOfDeletingIt() $this->assertStringNotContainsStringIgnoringCase('DELETE', $queries[0]); } + /** + * $wpdb->query() returns false on failure without throwing. If dequeue() trusted an + * unconfirmed claim, a second dequeue($blogId) call could claim (or have already + * claimed) the same row, dispatching the same content for translation twice. + */ + public function testDequeueDoesNotHandOutItemWhenClaimFails() + { + $this->mockDbAl(); + $db = $this->getMockBuilder(DB::class) + ->setConstructorArgs([new class { + public string $base_prefix = ''; + public function getRowArray() {} + public function query() {} + }]) + ->onlyMethods(['getRowArray', 'query']) + ->getMock(); + $db->method('getRowArray')->willReturnOnConsecutiveCalls( + ['id' => 7, 'batch_uid' => '', 'submission_ids' => '1', 'claimed' => null, 'attempts' => 0], + null, + ); + $db->method('query')->willReturn(false); + + $submission = $this->createMock(SubmissionEntity::class); + $submission->method('getId')->willReturn(1); + $submission->method('getSourceId')->willReturn(1); + $submission->method('getSourceBlogId')->willReturn(1); + $submissionManager = $this->createMock(SubmissionManager::class); + $submissionManager->method('getEntityById')->willReturn($submission); + + $settingsManager = $this->createMock(SettingsManager::class); + $settingsManager->method('getSmartlingLocaleBySubmission')->willReturn('de-DE'); + + $uploadQueueManager = new UploadQueueManager( + $this->createMock(ApiWrapperInterface::class), + $settingsManager, + $db, + $submissionManager, + ); + + $this->assertNull($uploadQueueManager->dequeue(1), 'Must not hand out an item whose claim could not be confirmed'); + } + public function testDequeueOnlyConsidersUnclaimedOrStaleRows() { $queries = [];