Skip to content

fix(order): prevent duplicate order numbers under concurrent submit - #399

Merged
biz87 merged 3 commits into
betafrom
fix/issue-380-order-num-race
Jul 26, 2026
Merged

biz87 merged 3 commits into
betafrom
fix/issue-380-order-num-race

Conversation

@Ibochkarev

Copy link
Copy Markdown
Member

Описание

Параллельный checkout/finalize мог выдать один и тот же msOrder.num: оба потока делали SELECT … LIKE prefix% ORDER BY id DESC, увеличивали счётчик и сохраняли без блокировки.

Теперь номер выделяется и сохраняется под MySQL GET_LOCK в OrderNumberGenerator::runWithNextNumber(), для черновиков num остаётся NULL, на колонку вешается UNIQUE (миграция чистит '' и разводит существующие дубликаты). Submit и finalize менеджера ходят через этот сервис.

Тип изменений

  • Исправление бага (non-breaking change)
  • Новая функциональность (non-breaking change)
  • Breaking change (изменение, ломающее обратную совместимость)
  • Рефакторинг (без изменения функциональности)
  • Документация
  • Другое (опишите):

Связанные Issues

Closes #380

Как это было протестировано?

  • Ручное тестирование
  • Автоматические тесты (PHPStan, ESLint)
  • Тестирование на разных версиях PHP/MODX

Конфигурация тестирования:

  • MiniShop3: ветка fix/issue-380-order-num-race
  • MODX: n/a (unit)
  • PHP: локальный CLI

Команды (exit 0):

  • php -l на всех затронутых PHP-файлах
  • php tests/OrderNumberGeneratorTest.php
  • php tests/PhinxMigrationTablePrefixTest.php (+ остальные tests/*.php)

Миграцию на живой БД в этом прогоне не применяли. Нагрузочный concurrent checkout не гоняли.

Скриншоты (если применимо)

До После
n/a n/a

Чеклист

  • Код соответствует стилю проекта
  • Добавлены/обновлены комментарии в сложных местах
  • Изменения не ломают существующую функциональность
  • Лексиконы добавлены на двух языках (ru/en)
  • PHPStan проходит без новых ошибок
  • ESLint проходит без ошибок (для JS/Vue изменений)
  • Обновлён CHANGELOG.md (для значимых изменений)

Дополнительные заметки

  • После деплоя нужна Phinx-миграция 20260716120000_add_unique_index_on_order_num.
  • Legacy Order::getNewOrderNum() / generate() помечены deprecated (WARN в лог): allocate без persist всё ещё подвержен гонке у плагинов; основные пути submit/finalize используют runWithNextNumber().
  • Атомарность смены статуса (соседний bug из issue) — вне scope этого PR.
  • Review: 1 fix-loop; blockers closed (silent counter fallback, schema sync, migration dedupe).

@Ibochkarev Ibochkarev added priority: medium Средний приоритет bug Something isn't working labels Jul 16, 2026
@Ibochkarev
Ibochkarev requested a review from biz87 July 16, 2026 03:46
@Ibochkarev
Ibochkarev force-pushed the fix/issue-380-order-num-race branch from ebdd886 to 83647f9 Compare July 23, 2026 16:07
Allocate and persist msOrder.num under MySQL GET_LOCK, add UNIQUE index
with NULL drafts, and centralize generation in OrderNumberGenerator.
@Ibochkarev
Ibochkarev force-pushed the fix/issue-380-order-num-race branch from 83647f9 to ce8cfab Compare July 24, 2026 02:15
biz87 added 2 commits July 27, 2026 00:01
PHPStan flagged `throw $lastError ?? new RuntimeException(...)` — $lastError
is always assigned before the throw is reachable (every attempt must land in
catch to exit the loop without return), so the ?? fallback was dead.

Seed $lastError with the RuntimeException up front and throw it directly.
Same behavior, no new PHPStan error. No logic change.
@biz87

biz87 commented Jul 26, 2026

Copy link
Copy Markdown
Member

Race-condition fix проверен детально. Тройная защита:

  1. GET_LOCK (primary) — runWithNextNumber() сериализует allocate+persist под MySQL advisory-lock (per-prefix, timeout 10s, finally releaseLock). Одновременных выдач номера больше нет.
  2. UNIQUE index (secondary) — миграция вешает UNIQUE на num; draft = NULL (множественные NULL допустимы, черновики не конфликтуют). Ловит вставки вне лока.
  3. Retry (tertiary) — при UNIQUE-коллизии до 3 попыток с пересчётом max counter.

Проверил:

  • GET_LOCK impl корректна (finally releaseLock, === 1 для timeout/NULL, prepared, fetchMaxCounter исключает NULL/'').
  • Миграция образцовая: ''→NULL, dedup дубликатов с -d{id} + existence-check + 20-char лимит, assertNoDuplicateNums перед index (refuse если остались), idempotent guards, корректный down().
  • Model + schema + migration синхронны (num default→null, UNIQUE везде).
  • Persist-callback атомарен (num+costs+save под одним локом).
  • Legacy getNewOrderNum/generate deprecated с WARN — не удалены, обратная совместимость.
  • behavioral-тест OK, 13 smoke зелёные, merge с beta бесконфликтный.

Внёс сам (коммит b4fc5dd): 1 PHPStan-ошибка (throw $lastError ?? new RuntimeException — ??-fallback недостижим). Seed $lastError до цикла. Логика не изменена, PHPStan чист, тест зелёный.

Оговорки (не блокеры): GET_LOCK привязан к соединению (persistent connections — finally+timeout защищают); named locks не реплицируются (multi-master вне scope); concurrent load-тест не гоняли (GET_LOCK — проверенный паттерн, code review + unit достаточны).

После деплоя нужна Phinx-миграция add_unique_index_on_order_num.

Для CHANGELOG: order numbers атомарны под GET_LOCK + UNIQUE; draft num = NULL. Требуется миграция.

Мержим.

@biz87
biz87 merged commit b3ef4ff into beta Jul 26, 2026
2 checks passed
@biz87
biz87 deleted the fix/issue-380-order-num-race branch July 26, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Средний приоритет

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Генерация номера заказа без блокировки — дубликаты num

2 participants