docs(review): Opus code-review report verbatim + critical analysis (confirmed 🔴1/🔴5/🔴8, open JWT and byte-limit decisions)
This commit is contained in:
@@ -152,6 +152,26 @@
|
||||
- Док (коммит `865db49`): почему C, механика, замеры, ограничения, пошаговое включение, откат,
|
||||
путь к варианту A (5 шагов) и триггеры перехода.
|
||||
|
||||
### 2.17. Код-ревью Opus + критический разбор
|
||||
- Дал промпт Opus: полное код-ревью + советы по архитектуре. Дословный ответ:
|
||||
`doc/thinking/opus-report-2026-10-10.md`; критический разбор + что подтверждено:
|
||||
`doc/thinking/opus-report-analysis-2026-10-10.md`.
|
||||
- Opus указал ~50 пунктов (🔴/🟡/🟢). Моя оценка: силён по конкуренции/durability, но смешивает
|
||||
баги / осознанный дизайн / отклонения от семантики AWS; часть 🔴 надо понизить.
|
||||
- **Подтверждено по коду (чинится без владельца):**
|
||||
- 🔴1 фатал `concurrent map iteration and map write`: `create_queue.go:84` зовёт `SaveQueue`
|
||||
под `SyncQueues.Lock` без `queue.mu`; `SaveQueue`→`json.Marshal` итерирует `Duplicates`/`FIFO*`/`Tags`,
|
||||
а мутаторы держат только `queue.mu`. Условие: идемпотентный CreateQueue существующей FIFO-очереди + SendMessage.
|
||||
- 🔴5 неполный откат dedup при сбое `SaveMessageSync` (`send_message.go:151-160`) → ретрай 200 без сохранения.
|
||||
- 🔴8 пустой `ReceiptHandle` в DeleteMessageBatch удаляет все невыданные сообщения — **только
|
||||
через JSON-протокол** (`x-amz-json-1.0`); Query-форма защищена (`requests.go:568` не на этом пути).
|
||||
- Порядок локов + I/O под глобальным локом; fail-fast на старте без Redis отсутствует.
|
||||
- **Нужны решения владельца:**
|
||||
- JWT: проверяет ли шлюз подпись (trusted-perimeter из `jwt.go:7-11`)? Независимо чинятся:
|
||||
три шлюза prod/test/dev → prod; потолок grace в `cache.go:68-75`; идентичность по `sub`, не email.
|
||||
- Лимит по байтам (OOM, 🔴14): нужны RAM ВМ + число тенантов → cap на тенанта/очередь + глобальный предохранитель по RSS.
|
||||
- Порядок исправления — §5 дока анализа.
|
||||
|
||||
## 3. Грабли (для повторного использования)
|
||||
|
||||
1. **`proxy_set_header Host $host` теряет порт** → SigV4 `SignatureDoesNotMatch`.
|
||||
@@ -216,3 +236,9 @@ pkill -x shared-sqs
|
||||
вторая ВМ, LB с проверкой `/ready`. Пока всё это не готово — флаг выключен.
|
||||
5. Дальше по мануалу: обновить устаревший `MANUAL-NEXT-2026-10-08.md` (ещё говорит «Шаг 1 — проверить
|
||||
гипотезу» и «вопрос по домену открыт»); решить вопрос push в origin (18 коммитов впереди).
|
||||
6. **JWT (решение владельца):** проверяет ли шлюз подпись и отдаёт 200 только для валидных токенов?
|
||||
От этого — 🔴 или «доверяем шлюзу». См. `opus-report-analysis-2026-10-10.md` §3.
|
||||
7. **Лимит по байтам (решение владельца):** нужны RAM ВМ 213 и целевое число тенантов для цифр cap.
|
||||
См. `opus-report-analysis-2026-10-10.md` §4.
|
||||
8. Подтверждённый критический набор к исправлению (🔴1/🔴5/🔴8 + lock order + fail-fast) —
|
||||
ожидает команды на правку.
|
||||
|
||||
@@ -0,0 +1,231 @@
|
||||
# Код-ревью shared-sqs (Opus, 2026-10-10)
|
||||
|
||||
> Ответ Opus на промпт «сделай код-ревью и посоветуй по архитектуре».
|
||||
> Дословно, без правок.
|
||||
|
||||
Все файлы из списка прочитаны. Строки взяты из листингов, часть пунктов опирается на прочитанное ранее. Пункты, которые стоит перепроверить перед правкой, помечены «проверить». Теги [Ф1–Ф6] указывают на ваши фокусные области.
|
||||
|
||||
## 🔴 Критично: потеря данных, падение процесса, обход безопасности
|
||||
|
||||
1. **[Ф1] Fatal `concurrent map iteration and map write` при повторном CreateQueue.** `create_queue.go:84` вызывает `SaveQueue` для существующей очереди под глобальным локом, но без `queue.mu`. `SaveQueue` маршалит `Duplicates`, `FIFO*` и `Tags`, а `send_message` пишет в них под `queue.mu`. Это не panic, а fatal runtime, который `recover` не ловит и который убивает весь процесс. Идемпотентный CreateQueue — частый вызов SDK. Тот же класс ошибки: `admin.go:498-535` и `seed.go`.
|
||||
2. **[Ф1] `list_queue_tags.go:49-60` отдаёт в ответ ссылку на `queue.Tags`.** `RUnlock` срабатывает по `defer` при return, а XML-маршалинг идёт позже, в router. Параллельный `TagQueue` даёт тот же fatal. Нужна копия мапы.
|
||||
3. **[Ф1] Инверсия порядка локов и дедлок через ожидающего писателя.**
|
||||
- `queue.mu` → `SyncQueues.RLock`: `set_queue_attributes.go:48,63` → `queue_attributes.go:78`.
|
||||
- `SyncQueues.RLock` → `queue.RLock`: скан в `admin.go:435-486` (подтверждено), `gauge_updater` (проверить, держит ли глобальный RLock).
|
||||
- Любой писатель в `SyncQueues` (`create_queue.go:73`, `delete_queue.go:45`, admin) блокирует новых читателей Go RWMutex. Получается цикл, и сервис встаёт целиком.
|
||||
- Отдельно: `models.go:410` — ABBA и self-дедлок на DLQ (цикл A→B→A или DLQ=self не отсекается в `queue_attributes.go:85`).
|
||||
- `conf/config.go:126-134` — `return` без `Unlock` и дедлок на старте.
|
||||
4. **[Ф1, Ф3] Redis-I/O под глобальным локом.** `delete_queue.go:45-56` выполняет синхронный `DeleteQueue` под `SyncQueues.Lock`. Комментарии «асинхронно» (`:24,55`) устарели. Redis-пауза 3–5 с останавливает все тенанты. То же в `admin.go:498` и `gosqs.go:107`.
|
||||
5. **[Ф3] Подтверждённое сообщение теряется при сбое Redis.**
|
||||
- `send_message.go:157` и `send_message_batch.go:133-139,153-158`: при ошибке `SaveMessage(s)Sync` откатывается только `RemoveIndex`.
|
||||
- `Duplicates` и `FIFOSequenceNumbers` не откатываются, `InitDuplicatation` уже сработал.
|
||||
- Ретрай клиента с тем же dedupId получает 200 без сохранения.
|
||||
- Таймаут Redis неоднозначен: запись могла пройти, и после рестарта появится фантом.
|
||||
6. **[Ф3] Receive, visibility-return и DLQ-перенос не персистятся.**
|
||||
- Затронуты `gosqs.go:124-154`, `change_message_visibility*.go`, `receive_message.go`.
|
||||
- DLQ-копия не пишется (`models.go:409-413`), а `change_message_visibility.go:72-81` делает только Delete в источнике.
|
||||
- После рестарта сообщение воскресает в источнике и пропадает из DLQ.
|
||||
- Порядок при загрузке теряется: `redis.go:504-513` итерирует HGETALL без сортировки, FIFO ломается.
|
||||
7. **[Ф1, Ф2] Зависание FIFO-групп навсегда.**
|
||||
- `ResetVisibilityAndTryDLQ` (`models.go:389-417`) не делает `UnlockGroup`, путь из `change_message_visibility.go:64` и batch-версии.
|
||||
- `purge_queue.go:63-64` сбрасывает `Duplicates`, но не `FIFOMessages`.
|
||||
- После рестарта `FIFOMessages` из Redis (`redis.go:163,448`) не согласован с in-flight, а тик снимает лок только по `receiptIdx`.
|
||||
8. **DeleteMessageBatch с пустым `ReceiptHandle` удаляет все неполученные сообщения.**
|
||||
- `delete_message_batch.go:79-97` сопоставляет handle сканом. У невыданных сообщений handle `""` и совпадает с записью `""`.
|
||||
- Query-парсер (`requests.go:568`) отсекает пустой handle, JSON-протокол — нет.
|
||||
- Одиночный delete безопасен: индекс пропускает `""`.
|
||||
9. **[Ф4] HA — это не HA.**
|
||||
- `leader.go:106-126`: ошибка Refresh не демотирует лидера, lease-проверки нет.
|
||||
- `goaws.go:171-191`: пассив выполняет `PeriodicTasks` и пишет в Redis. Handlers лидерство не проверяют, защита — только `/ready`.
|
||||
- При повышении пассива состояние из Redis не перечитывается: он работает со снимком своего старта.
|
||||
- Без Redis `TryAcquireLock` и `RefreshLock` возвращают true (`redis.go:666-715`), каждый инстанс становится «лидером».
|
||||
- Нет fencing-токена.
|
||||
- `redis.go:61-85`: при неудачном Connect клиент остаётся nil навсегда, reconnect не запускается.
|
||||
10. **[Ф3, Ф4] Старт с пустым состоянием.** `goaws.go:103-106,125-134` при недоступном Redis или ошибке `LoadAllQueues` продолжает работу. Следом `SaveQueue` перезапишет Redis пустотой. Нужен fail-fast.
|
||||
11. **[Ф6] Тело подменяется при валидной подписи.** `auth_middleware.go:166-169,234-243` берёт `X-Amz-Content-Sha256` из заголовка и не сверяет с реальным хешем тела.
|
||||
12. **[Ф6] JWT без проверки подписи.**
|
||||
- `jwt.go`: идентичность берётся из непроверенного payload, `Exp==0` пропускается.
|
||||
- Валидность подтверждает GET на любой из трёх шлюзов, включая dev и test.
|
||||
- `tenant_store.go:213-291`: тенант находится по email из такого токена, возможен захват чужого тенанта.
|
||||
- `cache.go:68-75`: при ошибке сети grace продлевает отозванный токен.
|
||||
13. **[Ф3] Удалённый тенант воскресает.** `redis.go:594-609` `DeleteTenant` не чистит `ssq:tenant_ak`, а read-through в auth (`:63-74`) его возвращает. Найденный тенант кэшируется навсегда.
|
||||
14. **[Ф5] OOM-потолок на уровне процесса.**
|
||||
- Лимит только по числу сообщений: 120k × 256 КБ = ~30 ГБ на одну очередь (`validation.go:43-44`).
|
||||
- Лимитов по байтам или памяти нет ни на очередь, ни на тенанта, ни глобально.
|
||||
- Размер атрибутов не входит в проверку (`MaxMessageAttributeSize` объявлен и не используется).
|
||||
- Один тенант валит процесс всем.
|
||||
15. **[Ф3] `redis.go:33,116-133`: `Client` глобален, `reconnect()` подменяет его без синхронизации.** Это data race, старый клиент не закрывается. go-redis и так переподключается сам, `reconnect()` не нужен.
|
||||
|
||||
## 🟡 Существенно
|
||||
|
||||
**Гонки и конкурентность**
|
||||
- **[Ф1]** `globals.go:77-100`: `GetLocked` отпускает глобальный лок до `LockMu`, и после взятия лока нет проверки, что очередь ещё в map.
|
||||
- Запись в удалённую очередь возвращает 200, а сообщение пропало.
|
||||
- `SaveMessagesSync` пишет `ssq:msg:{key}` уже после `DeleteQueue`, это осиротевшие ключи в Redis.
|
||||
- Аналог: `send_message_batch.go:46-53` → `:105`.
|
||||
- **[Ф3]** `redis.go:215-236`: seq-guard проверяется до HSET (TOCTOU). `:394-416`: проверка seq сразу после инкремента — тавтология, запущенная async-`SaveQueue` воскрешает метаданные после `HDel`. Карты seq растут вечно (`:44-46`).
|
||||
- **[Ф3]** `purge_queue.go:68`: синхронный Redis под `queue.mu`, ошибка не возвращается, память очищена, а Redis нет. Ранее запущенные async `SaveMessage` могут записать сообщение после DEL.
|
||||
|
||||
**Tombstone и индексы**
|
||||
- **[Ф2]** `models.go:201-208`: `ensureIndexes` тавтологичен. Проверка `liveCount == len-deadCount` не ловит рассинхрон map-индексов.
|
||||
- **[Ф2]** `models.go:262-283`: `RemoveIndex` не обнуляет слот, тело сообщения живёт до компакции. Порог 1024 слота или половина — до 256 МБ мусора на очередь.
|
||||
- **[Ф2]** `RebuildIndexes` на старте разреженный, после загрузки `deadCount=0`: компакции нет, но и мусора нет. Это нормально, если порядок восстановлен сортировкой (см. 🔴6).
|
||||
- `models.go:113-141`: `FIFOSequenceNumbers` и `FIFOMessages` растут без чистки.
|
||||
- `models.go:172-180`: запись в nil-map при `Duplicates==nil`.
|
||||
|
||||
**Семантика SQS**
|
||||
- Dedup:
|
||||
- `delete_message.go:59` и `delete_message_batch.go:93` делают `delete(Duplicates)` при удалении, окно 5 мин нарушено.
|
||||
- `requests.go:422-423`: `ContentBasedDeduplication` принимается и игнорируется, FIFO без dedupId не дедуплицируется.
|
||||
- Дубль возвращает новый MessageId.
|
||||
- Receive и счётчики:
|
||||
- `Retry++` на таймауте видимости (`gosqs.go:142`): `MaxReceiveCount` считается по таймаутам.
|
||||
- `receive_message.go:203,247`: `ApproximateReceiveCount` завышен на 1.
|
||||
- `ReceiptTime` перезаписывается, `FirstReceiveTimestamp` неверен.
|
||||
- `models.go:405`: `ChangeMessageVisibility(0)` скрывает сообщение ещё на `q.VisibilityTimeout`, а должно быть видно сразу (проверить по receive).
|
||||
- `change_message_visibility_batch.go:72`: нет проверки отрицательных значений.
|
||||
- `queue_attributes.go:41-70`: 0 означает «не передан», и поэтому нельзя вернуть `DelaySeconds`, `ReceiveMessageWaitTimeSeconds` или `VisibilityTimeout` в 0.
|
||||
- `requests.go:35-37,30`: невалидные значения и пустая строка молча игнорируются вместо ошибки.
|
||||
- `queue_attributes.go:73-87`:
|
||||
- нет проверки self-ссылки, цикла, совпадения FIFO-типа и диапазона `maxReceiveCount`;
|
||||
- `DeadLetterQueueName` здесь не выставляется (проверить, заполняет ли его `SaveQueue`, иначе DLQ теряется после рестарта);
|
||||
- `delete_queue.go:53` не отвязывает DLQ-ссылки других очередей.
|
||||
- `create_queue.go:73-85`: повторный CreateQueue с другими атрибутами возвращает 200 вместо `QueueNameExists`.
|
||||
- `create_queue.go:66`, `set_queue_attributes.go:59`: точное сравнение Content-Type. Для `json-1.1` или с `charset` whitelist обходится.
|
||||
- Batch:
|
||||
- `send_message_batch.go:87-103` валидирует пакет целиком, вместо per-entry `Failed`.
|
||||
- Нет суммарного лимита 256 КБ.
|
||||
- Per-message `DelaySeconds` не применяется (`:118-127`, проверить паритет с single).
|
||||
- `delete_message_batch.go:65-72`: дубль handle теряет Id из ответа.
|
||||
- `delete_message_batch.go:79` — O(N) скан вместо `IndexOfReceipt`, при том что `change_message_visibility_batch.go:84` индекс использует.
|
||||
- Код ошибки `"1"`, порядок ответа недетерминирован.
|
||||
- Query-парсеры (`requests.go:235,568,602`) молча обрезают пакет на первой пустой записи.
|
||||
- `get_queue_attributes.go:89-100`:
|
||||
- `Approximate…` включает in-flight и delayed;
|
||||
- timestamp-заглушки `"0000000000"`;
|
||||
- нет `FifoQueue`, `ContentBasedDeduplication`, `Policy`, `…Delayed`, что ломает Terraform и мониторинг.
|
||||
- `list_queues.go:34-43`: скан всех очередей сервиса под глобальным RLock при готовом tenant-индексе. Пагинации нет (`MaxResults` и `NextToken` игнорируются), порядок случайный.
|
||||
- `tag_queue.go:56-60`: нет лимитов на теги (AWS: 50 шт., ключ 128, значение 256). Теги сериализуются на каждый `SaveQueue`, это DoS-вектор.
|
||||
|
||||
**Производительность [Ф5]**
|
||||
- `receive_message.go:164-216`: два полных скана под write-локом. Long-poll опрашивает каждые 100 мс с эксклюзивным локом на каждого поллера.
|
||||
- `SaveQueue` на каждый send, delete и CMV (`send_message.go:159`, `delete_message*.go`, `change_message_visibility_batch.go:117`): O(Duplicates + FIFO + Tags) под локом.
|
||||
- `gosqs.go:50-54`: один горутин-тик на все очереди последовательно, с синхронным Redis (`:107`).
|
||||
- `redis.go:422-543`:
|
||||
- `LoadAllQueues` грузит всё последовательно, общий таймаут 30 с;
|
||||
- ошибка очереди → `continue`, очередь грузится пустой;
|
||||
- время старта и RAM пропорциональны полному состоянию.
|
||||
|
||||
**HA и жизненный цикл [Ф4]**
|
||||
- `router.go:214-228`: `/ready` не учитывает Redis, загрузку состояния и shutdown. `ReadyFunc` — глобальная переменная.
|
||||
- `goaws.go:248-268`:
|
||||
- `billing.Close()` идёт до дренажа HTTP;
|
||||
- async Redis-записи не дренируются;
|
||||
- не-ready на время дренажа нет;
|
||||
- `log.Fatalf` в `:245` не отпускает лок;
|
||||
- `leader.go:63-76`: `Stop` освобождает лок до дренажа.
|
||||
- `goaws.go:204-211`: нет `ReadTimeout` (медленное тело), лимитов соединений и rate-limit. Тело читается целиком до проверки подписи.
|
||||
|
||||
**Безопасность [Ф6]**
|
||||
- SigV4:
|
||||
- `auth_middleware.go:296-326` не требует подписи `host`, `x-amz-date`, `x-amz-target`;
|
||||
- `:158` clock skew 15 мин (в AWS — 5);
|
||||
- `:215` presigned не проверяет дату из будущего;
|
||||
- `:290` `canonicalURI` зависит от нормализации nginx и заголовка Host;
|
||||
- превышение лимита тела даёт 403 `SignatureDoesNotMatch`, а не 413 (`router.go:124-133`);
|
||||
- rate-limit по неудачным попыткам нет, ошибки раскрывают причину (`:81`).
|
||||
- `auth_middleware.go:63-74`: каждый запрос с неизвестным accessKey ходит в Redis (HGET до 3 с) без негативного кэша. `jwt.go:90-133` — до трёх последовательных запросов по 10 с на токен, без singleflight.
|
||||
- `admin.go`:
|
||||
- статичный Bearer без rate-limit;
|
||||
- `:223` `jwtMiddleware` пропускает путь по `HasSuffix("/auth")`;
|
||||
- `:263-267` IDOR-защита только если в `mux.Vars` есть id;
|
||||
- `:409-420` `deleteTenant`: in-flight handlers пишут в удалённые очереди;
|
||||
- `:194,207-212` токен и `SecretKey` отдаются в теле ответа;
|
||||
- `:640-669` `sendMessageToQueue` без лимита `MaxMessagesForQueue` и с async-сохранением.
|
||||
- `tenant_store.go`: секреты в Redis открытым текстом. Нет проверки уникальности email (`:58-108`), `MaxQueues` перезаписывается при каждом логине (`:240`), `LoadTenant` затирает свежие правки (`:194-202`).
|
||||
- `router.go:72-74`: маршруты `/{account}` и `/{account}/{queueName}` перекрывают `/admin/*` и `/health` для POST. Bypass в auth по этим путям fail-open, но nil-проверка tenant присутствует во всех 17 просмотренных handlers.
|
||||
- `goaws.go:148-150`: при `SEED_DEMO` креды демо-тенанта пишутся в лог.
|
||||
|
||||
**Прочее**
|
||||
- `router.go:250-277`: `countBatchEntries` работает только с form, для JSON-протокола возвращает 0 и биллинг батчей занижен. `:297` `msgBytes` — размер запроса, а не ответа.
|
||||
- `billing.go`:
|
||||
- канал 1024, переполнение дропает записи;
|
||||
- `db.Exec` без таймаута;
|
||||
- `Close()` обнуляет `db` без синхронизации с воркером (data race);
|
||||
- DSN собирается через Sprintf.
|
||||
- `metrics`:
|
||||
- label tenant: accessKey в gauge и `tenant.ID` в counters, accessKey утекает в метрики;
|
||||
- `Reset()` gauge даёт окно пустых значений;
|
||||
- label не чистится при удалении тенанта.
|
||||
- `conf/config.go`:
|
||||
- `:111` yaml-очереди кладутся без префикса `accessKey:` и обходят tenant-изоляцию;
|
||||
- `:189-196` при пустой RedrivePolicy `queues[deadLetterQueueName]` ищется без tenant-префикса.
|
||||
- `seed.go`: ARN собирается с `t.ID` вместо `AccountID`, очереди без FIFO/DLQ-полей, `SaveMessages` асинхронный.
|
||||
|
||||
## 🟢 Мелочи
|
||||
|
||||
- Расхождение дефолтов:
|
||||
- `build.go:19-32` (admin) задаёт `VisibilityTimeout`, `MaxSize`, `Retention` напрямую;
|
||||
- `CreateQueueV1` берёт их из `NewCreateQueueRequest` по yaml;
|
||||
- при нулях в yaml пути разойдутся.
|
||||
- Мёртвый код и дубли:
|
||||
- `validation.go`: `ClampInt`, `ValidateMessageAttributes`, `MaxRequestBodySize=1MB` (router — 3 МБ), `MaxTenantsDefault` дублирует `MaxTenantsGlobal`;
|
||||
- `AvailableQueueAttributes` и `AttrNameWhitelist` — два списка;
|
||||
- `DeduplicationPeriod` — `var`;
|
||||
- `send_message.go:73` — мёртвая ветка `mux.Vars`.
|
||||
- `BaseResponseMetadata.RequestId` — нулевой UUID во всех ответах: корреляция невозможна.
|
||||
- `models/helpers.go`: `rand.Intn(max-min)` паникует при `max<min`; `Sprintf` и `ParseDuration` на каждый вызов; `IsReadyForReceipt` пересчитывает latency на каждый вызов.
|
||||
- Устаревшие комментарии: `redis.go:1-13`, `send_message.go:38`, `goaws.yaml` (retention «not implemented»).
|
||||
- INFO-логи на каждый вызов: `delete_message.go:43` (с ReceiptHandle), `get_queue_attributes.go:64`, `list_queues.go:30`.
|
||||
- `models/errors.go`:
|
||||
- `delete_message.go:48` возвращает `MessageDoesNotExist` при отсутствии очереди;
|
||||
- `InvalidAttributeValue` и `MissingParameter` имеют жёсткий текст сообщения;
|
||||
- `QueueExists` не используется.
|
||||
- `tenant_helpers.go` и `configuration.go` не прогнаны через gofmt. Имя очереди берётся из последнего сегмента URL, `tenantID` в пути игнорируется.
|
||||
- Версия `xml.Marshal` с `http.Error` после `WriteHeader` (`router.go:175-184`).
|
||||
|
||||
## Архитектура: потолок и вынос в Redis (вариант A)
|
||||
|
||||
**Потолок сегодня.** Он определяется тремя вещами:
|
||||
- RAM процесса при отсутствии лимита по байтам;
|
||||
- время старта: полная загрузка последовательным HGETALL;
|
||||
- последовательный тик и O(N)-сканы в receive и gauge.
|
||||
|
||||
Ориентировочно потолок — десятки тысяч сообщений на горячую очередь. Измерить p99 receive при 100k in-flight и время `LoadAllQueues` на проде.
|
||||
|
||||
**Что Redis не гарантирует.** «Синхронно» в коде означает «принято Redis в память», а не fsync и не репликация. Redis на той же ВМ — общий SPOF. Для durability нужны `appendfsync always` или `everysec` с осознанной потерей до 1 с, либо `WAIT`, и отдельный реплицируемый Redis.
|
||||
|
||||
**Порядок перехода (по риску):**
|
||||
1. **Сначала дёшево, без смены архитектуры:**
|
||||
- сделать `SaveQueue` синхронным или write-behind с batch;
|
||||
- вынести FIFO-lock из сообщений в метаданные и персистить;
|
||||
- сортировать при загрузке (по seq или `SentTime`);
|
||||
- персистить receive, visibility и DLQ;
|
||||
- fail-fast при старте без Redis;
|
||||
- лимиты по байтам и по тегам.
|
||||
2. **Починить конкурентность:**
|
||||
- единый порядок локов (глобальный до per-queue, не наоборот);
|
||||
- убрать I/O под глобальным локом;
|
||||
- проверка живости очереди после `LockMu`;
|
||||
- копии при отдаче map в ответ.
|
||||
3. **Интерфейс `QueueStore`** поверх `SyncQueues`. Сейчас handlers лезут в `Messages`, индексы и `Duplicates` напрямую, поэтому без интерфейса вынос невозможен.
|
||||
4. **Redis как источник истины:**
|
||||
- Lua-скрипты (или Streams) на send, receive, delete, change-visibility, DLQ-перенос;
|
||||
- visibility через ZSET по времени;
|
||||
- dedup через ключ с TTL;
|
||||
- FIFO-групп-лок как ключ с lease.
|
||||
5. **Sweeper вместо `PeriodicTasks`**, под отдельным локом и с fencing-токеном.
|
||||
6. **Снять leader-lock.** До этого этапа HA остаётся «один активный, пассив безопасен только за `/ready`». Для безопасного active/passive нужны fencing-токен, перезагрузка состояния при повышении, запрет записи без лидерства и lease-проверка на лидере.
|
||||
|
||||
**Главный риск варианта A** — сетевой round-trip на каждую операцию и сложность FIFO и DLQ в Lua. Дешёвая промежуточная ступень — шардирование по тенантам (вариант B) с оставленным in-memory ядром. Оно не даёт HA, но снимает потолок по RAM и тику.
|
||||
|
||||
## Порядок исправления
|
||||
|
||||
1. Fatal-гонки: п. 1–2 из 🔴.
|
||||
2. Lock order и I/O под глобальным локом: п. 3–4.
|
||||
3. Откат dedup при сбое Redis: п. 5.
|
||||
4. Пустой handle в DeleteMessageBatch: п. 8.
|
||||
5. Подпись, тело и JWT: п. 11–12.
|
||||
6. Fail-fast на старте и lease у лидера: п. 9–10.
|
||||
7. Лимит по байтам: п. 14.
|
||||
8. Персист receive, visibility и DLQ вместе с порядком при загрузке: п. 6–7.
|
||||
@@ -0,0 +1,90 @@
|
||||
# Критический разбор код-ревью 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).
|
||||
Reference in New Issue
Block a user