91 lines
7.1 KiB
Markdown
91 lines
7.1 KiB
Markdown
# Критический разбор код-ревью Opus (2026-10-10)
|
||
|
||
> Дополняет `opus-report-2026-10-10.md` (дословный отчёт Opus).
|
||
> Здесь — проверка ключевых пунктов по коду + что подтверждено/отклонено + решения, которые нужны.
|
||
|
||
## 1. Вердикт
|
||
|
||
Отчёт сильный по конкуренции и durability. Главный дефект отчёта — смешивает три категории
|
||
без разделения: **(а)** реальные баги, **(б)** осознанный дизайн (JWT trusted-perimeter),
|
||
**(в)** отклонения от семантики AWS (в основном косметика). Часть 🔴 надо понизить.
|
||
|
||
## 2. Подтверждено по коду (критично, чинится без владельца)
|
||
|
||
### 2.1. 🔴1 — фатал `concurrent map iteration and map write` в CreateQueue
|
||
- `create_queue.go:73-84`: ветка «очередь уже существует» вызывает `SaveQueue(key, queue)`
|
||
под `SyncQueues.Lock`, **без** `queue.mu`.
|
||
- `SaveQueue` → `json.Marshal` (`redis.go:208,220`) итерирует `Duplicates`, `FIFOSequenceNumbers`,
|
||
`FIFOMessages`, `Tags`.
|
||
- Мутаторы этих map (send/tag/CMV) держат только `queue.mu` (`GetLocked` берёт глобальный RLock
|
||
на миг и отпускает — `globals.go:77-88`), поэтому глобальный Lock их не блокирует.
|
||
- Условие: идемпотентный `CreateQueue` существующей FIFO-очереди (обычный вызов SDK при старте)
|
||
+ параллельный `SendMessage`. Вероятность растёт с размером map. Необратимый краш процесса
|
||
(`recover` не ловит fatal).
|
||
- Шапка `redis.go:211-213` («вызывать под `SyncQueues.Lock`») описывает неверный контракт.
|
||
|
||
### 2.2. 🔴5 — неполный откат dedup при сбое Redis
|
||
- `send_message.go:151-160`: при ошибке `SaveMessageSync` откатывается только `RemoveIndex`,
|
||
а `InitDuplicatation` выполняется всё равно → ретрай с тем же dedupId даёт 200, но сообщение
|
||
не сохранено. Аналог в `send_message_batch.go`.
|
||
- `FIFOSequenceNumbers` (инкремент в `models.go:117`) тоже не откатывается.
|
||
|
||
### 2.3. 🔴8 — пустой `ReceiptHandle` в DeleteMessageBatch (protocol-gated)
|
||
- Срабатывает **только** при `Content-Type: application/x-amz-json-1.0`: тело декодируется
|
||
напрямую (`utils.go:35-46`), валидация `receiptHandle == ""` (`requests.go:568`, Query-форма)
|
||
на этом пути не вызывается.
|
||
- `delete_message_batch.go:51-57` проверяет только уникальность `Id`, handle не валидируется.
|
||
- `deleteMessageMap[""]` совпадает с невыданными сообщениями (у них `ReceiptHandle == ""`),
|
||
цикл `:79-98` удаляет все такие сообщения очереди.
|
||
- Одиночный `DeleteMessage` безопасен: пустой handle не кладётся в `receiptIdx`
|
||
(`models.go:233,251,373`).
|
||
|
||
### 2.4. Дополнительно подтверждено (не тестировалось, статический разбор)
|
||
- Инверсия порядка локов (`queue.mu` ↔ `SyncQueues.RLock`) и Redis-I/O под глобальным локом
|
||
(`delete_queue.go:45-56`).
|
||
- Fail-fast отсутствует: старт с недоступным Redis продолжает работу и следом `SaveQueue`
|
||
перезаписывает Redis пустотой (`goaws.go:103-106,125-134`).
|
||
|
||
## 3. Решение владельца: JWT
|
||
|
||
`jwt.go:7-11` прямо документирует trusted-perimeter: «Подпись JWT НЕ проверяется — аналогично
|
||
sless», валидация через `PingNubesAPI` (GET на 3 шлюза, приём по 2xx).
|
||
|
||
**Ключевой факт, который не проверен:** проверяет ли шлюз подпись и отдаёт 200 только для
|
||
валидных токенов? От этого зависит классификация:
|
||
- шлюз проверяет подпись → «доверяем шлюзу», не уязвимость;
|
||
- нет → 🔴, чинить первым делом.
|
||
|
||
Независимо от ответа чинятся три вещи:
|
||
1. `jwt.go:90-92` — три шлюза (prod/test/dev): токен тестового стенда принимается как prod.
|
||
Оставить prod-шлюз.
|
||
2. `cache.go:68-75` — grace при сетевой ошибке продлевается повторно → отозванный токен живёт
|
||
бесконечно. Нужен потолок суммарного grace (например, 2 мин от последней успешной проверки).
|
||
3. `tenant_store.go:213-291` — идентичность по email; привязать к стабильному `sub`/`iss`,
|
||
иначе смена email на IdP отдаёт чужой тенант.
|
||
|
||
## 4. Решение владельца: лимит по байтам (OOM, 🔴14)
|
||
|
||
Сейчас лимит только по числу сообщений (`validation.go:43-44`), по байтам/памяти — нет.
|
||
Нужны входные данные владельца: RAM ВМ и целевое число тенантов.
|
||
|
||
Структура (рекомендация):
|
||
- **Cap на тенанта** (суммарные байты тел+атрибутов по всем очередям) — изоляция.
|
||
- **Cap на очередь** — производный (доля тенантского лимита).
|
||
- **Глобальный предохранитель** — отказ send при RSS > порога (напр. 70% RAM), единственная
|
||
защита от суммы тенантов.
|
||
|
||
Реализация — счётчик байт в `Queue` (append/delete/purge/compact/restore) + тенантский
|
||
аккумулятор → требует `QueueStore`-интерфейса (план A).
|
||
|
||
Минимальный шаг без решения по квотам: снизить `MaxMessagesPerQueue` + глобальный
|
||
предохранитель по RSS.
|
||
|
||
## 5. Порядок исправления (после решений владельца)
|
||
|
||
1. 🔴1 — убрать `SaveQueue` без `queue.mu` в `create_queue.go:84` (и `admin.go`/`seed.go`).
|
||
2. 🔴8 — валидация `ReceiptHandle != ""` в `delete_message_batch.go`.
|
||
3. 🔴5 — полный откат dedup/FIFO-seq при сбое `SaveMessage(s)Sync`.
|
||
4. Lock order + I/O под глобальным локом.
|
||
5. Fail-fast на старте без Redis.
|
||
6. JWT (после решения п.3) + лимит по байтам (после п.4).
|