108 lines
10 KiB
Markdown
108 lines
10 KiB
Markdown
# История правок shared-sqs — 2026-10-11 (внешнее код-ревью и фиксы)
|
||
|
||
> Запись по требованию «документировать всё, включая собственные ошибки» (приказ 2026-09-24).
|
||
> Предыдущие: `HISTORY/2026-10-10-session-log.md`, `doc/thinking/*-2026-10-08.md`.
|
||
|
||
## 1. Задача
|
||
|
||
Провести **внешнее код-ревью** двух сервисов (SQS-service + IoT) моделью Claude Sonnet
|
||
(в режиме Plan — правки не вносит, только находит), затем своими силами исправить
|
||
найденное. Промпт для ревью — `CLAUDE-REVIEW-PROMPT.md` в корне монорепо
|
||
(со списком файлов, фокус-точками и строгим форматом ответа; явно запрещены
|
||
правки, оценка тестов/UI/legacy/стиля).
|
||
|
||
Ревью дало **40 пунктов** (10 🔴 / 29 🟡 / 1 🟢). Разбор вопросов ревьюера и
|
||
уточнение некорректных выводов — в п.3.
|
||
|
||
## 2. Что исправлено (SQS-service)
|
||
|
||
### 🔴 Критичные (10/10)
|
||
|
||
| Пункт | Файл | Суть | Коммит |
|
||
|-------|------|------|--------|
|
||
| 🔴1 | `app/admin/admin.go` | `jwtMiddleware` пропускал любой путь с суффиксом `/auth` (обход JWT + IDOR на `DELETE .../queues/auth`). Теперь — точное сравнение `/ui/api/auth` | `1241093` |
|
||
| 🔴2 | `app/gosqs/queue_attributes.go` | Запрет self-/циклического `RedrivePolicy`: перенос в DLQ берёт второй `mu` под первым → deadlock | `9b9bbe1` |
|
||
| 🔴3 | `app/gosqs/gosqs.go`, `models.go`, `change_message_visibility*.go` | Перенос в DLQ не персистился (сообщение «воскрешало» в исходной и пропадало из DLQ) | `1e334c2` |
|
||
| 🔴4 | `app/persistence/redis.go`, `app/tenant/tenant_store.go` | `DeleteTenant` не чистил индекс `ssq:tenant_ak` — отозванный тенант возвращался через read-through | `f68b0a4` |
|
||
| 🔴5 | `app/persistence/redis.go`, `app/cmd/goaws.go` | Глобальный `Client` перезаписывался в `reconnect()` без синхронизации (data race). → `atomic.Pointer` | `d03ce36` |
|
||
| 🔴6 | `app/conf/config.go` | `return ports` без `SyncQueues.Unlock()` при ошибке `setQueueRedrivePolicy` → вечный deadlock | `1520753` |
|
||
| 🔴7 | `app/billing/billing.go` | `Close()` писал `db=nil` параллельно с `usageWorker` (nil-паника). → мьютекс + остановка воркера до `db.Close` | `4e36ba0` |
|
||
| 🔴10 | `app/auth/auth_middleware.go` | `X-Amz-Content-Sha256` не сверялся с телом — подмена тела под подписанным заголовком | `2918a41` |
|
||
|
||
(🔴8/🔴9 — на стороне IoT, см. `IoT/HISTORY/2026-10-11-session-log.md`.)
|
||
|
||
### 🟡 Важные
|
||
|
||
| Пункт | Файл | Суть | Коммит |
|
||
|-------|------|------|--------|
|
||
| 🟡20 | `app/admin/admin.go` | `createTenantQueue` не валидировал имя очереди (`:`,`/` ломали ключ) | `efd8c8e` |
|
||
| 🟡23 | `app/auth/jwt.go` | Токен без `exp` принимался бессрочно → требуем `exp` | `efd8c8e` |
|
||
| 🟡24 | `app/leader/leader.go` | При сбое `RefreshLock` инстанс оставался лидером (split-brain) → сброс лидерства после TTL | `efd8c8e` |
|
||
| 🟡21 | `app/tenant/tenant_store.go` | Запрет `:` в BYOC `access_key` (коллизии ключа `{ak}:{name}`) | `dc4815d` |
|
||
| 🟡22 | `app/admin/admin.go` | `sendMessageToQueue` без лимитов + async persist → лимит `MaxMessagesForQueue` + `SaveMessageSync` | `dc4815d` |
|
||
| 🟡33 | `app/models/models.go` | `InitDuplicatation` падал на nil-map `Duplicates` → ленивая инициализация | `dc4815d` |
|
||
| 🟡35 | `app/models/models.go` | `Reset()` не чистил `FIFOMessages` (после purge группы не выдавались) | `dc4815d` |
|
||
| 🟡36 | `app/gosqs/receive_message.go` | `ApproximateReceiveCount` был завышен на 1 | `dc4815d` |
|
||
| 🟡38 | `app/cmd/goaws.go` | `billing.Close()` вызывался ДО `Shutdown` серверов → перенесён после | `dc4815d` |
|
||
| 🟡26 | `app/gosqs/delete_message_batch.go` | Ключ map по `ReceiptHandle` (два entry с одним handle теряли Id) → по `Id` | `e0b5f24` |
|
||
| 🟡27 | `app/gosqs/send_message.go` | `DelaySeconds` не валидировался → 0–900 | `e0b5f24` |
|
||
| 🟡25 | `app/gosqs/send_message_batch.go` | Dedup регистрировался после цикла (два entry с одним id оба добавлялись); `NextSequenceNumber` до проверки дубликата; per-entry `DelaySeconds` не применялся | `6f69755` |
|
||
| 🟡29 | `app/gosqs/delete_message*.go`, `persistence/redis.go` | Ошибка `HDel` только логировалась (клиент получал 200, после рестарта сообщение воскрешало) → `DeleteMessage(s)Persist` возвращает ошибку, 500 клиенту | `5d38118` |
|
||
| 🟡31 | `app/persistence/redis.go`, `app/cmd/goaws.go` | Ошибка загрузки сообщений очереди давала `continue` (очередь стартовала пустой) → fail-fast (`log.Fatalf`) | `5d38118` |
|
||
| 🟡30 | `app/persistence/redis.go` | Async `SaveMessage` без порядковой нумерации (старое состояние могло перезаписать новое) → seq по `{queueKey}|{uuid}` + очистка при удалении | `7a2340d` |
|
||
| 🟡32 | `app/gosqs/gosqs.go` | `PeriodicTasks` без `recover` (паника убивала процесс) | `dc4815d` |
|
||
| 🟢 | `app/utils/utils.go` | `SqsErrors[errKey]` без проверки → nil-deref на неизвестном ключе | `5d38118` |
|
||
| (🟡 new) | `app/gosqs/create_queue.go` | Очередь из API получала `VisibilityTimeout=0` и пр. (дефолты не подставлялись) | `5d38118` |
|
||
|
||
### Осознанно НЕ менялось — РЕШЕНИЯ (2026-10-11)
|
||
|
||
- 🟡34 (атрибуты «0 = не передано»): **решение — не менять** совместимость.
|
||
Различать «не передано» и явный 0 потребовало бы перевести `QueueAttributes`
|
||
на указатели — ломает JSON-протокол и Terraform-drift. Явный 0 — редкий сценарий.
|
||
- 🟡37 (дефолт `SHARED_SQS_MAX_HEAP_MB`): **решение — оставить выключенным (0)**.
|
||
Безопасное число зависит от RAM конкретной ВМ и числа тенантов — выбор владельца,
|
||
не хардкод в коде.
|
||
- 🟡28 (сеть под `queue.mu`): **решение — оставить как есть (вариант A)**. См. п.4.
|
||
|
||
## 3. Уточнение выводов ревьюера
|
||
|
||
Ревьюер задал вопросы по коду, ответы найдены в коде (файл:строка):
|
||
|
||
- **ACL EMQX — единственный барьер для топиков** `{ns}/telemetry/{deviceId}`? Да:
|
||
`bridge/handler.go` берёт `ns`/`deviceID` из топика без сверки принадлежности,
|
||
барьер — EMQX authz (`emqx/emqx.conf`). Добавлена defense-in-depth проверка
|
||
формата (IoT).
|
||
- **Self-/циклический `RedrivePolicy` реально задаётся через API?** Да
|
||
(`setQueueAttributesV1` не проверял) — отсюда фикс 🔴2.
|
||
- **Пустой `adminToken`?** Недопустим — `goaws.go:~74` `log.Fatal` при отсутствии.
|
||
- **Дефолты очереди расходятся** (`create_queue.go` vs `conf/config.go:100-109`)?
|
||
Да — отсюда фикс (🟡 new).
|
||
- **`utils.go` паника на неизвестном ключе?** Текущей паники нет (все ключи есть
|
||
в `models.SqsErrors`), но добавлена защита.
|
||
|
||
Также ревьюер отметил некорректный пункт (удалён из списка по лимиту 40): nil-map
|
||
`Duplicates` (🟡33) фактически не воспроизводился в текущем коде — но защита
|
||
добавлена как дешёвая страховка.
|
||
|
||
## 4. 🟡28 — РЕШЕНИЕ (2026-10-11)
|
||
|
||
- **Проблема**: синхронный `SaveMessageSync` (до 3 с) выполняется под `queue.mu`
|
||
(`send_message.go:150`, `delete_message.go`, `admin.go`). Медленный Redis блокирует
|
||
всю очередь.
|
||
- **Варианты**: A) оставить; B) укоротить таймаут Redis-записи; C) полный рефакторинг
|
||
с per-message синхронизацией.
|
||
- **Выбрано A — оставить.** Обоснование:
|
||
1. in-memory ядро очередей — одиночка, блокировка `queue.mu` локальна (одна очередь).
|
||
2. Синхронный персист под `mu` = гарантия «нет потери подтверждённого сообщения
|
||
при крахе пода» — приоритет проекта (надёжность, а не масштабирование).
|
||
3. Вынос сети за `mu` вносит окно между `Unlock`/`Lock`: два конкурентных
|
||
`SendMessage` с одним `deduplicationId` оба пройдут `IsDuplicate` (дубль),
|
||
либо при откате `dedup` теряется сообщение — хуже кратковременной блокировки.
|
||
4. Redis-авария — глобальная: деградирует всё, 2–3 с на очередь не критично.
|
||
- 🟡34/🟡37 — см. п.2.
|
||
|
||
## 5. Верификация
|
||
|
||
- `go build ./...`, `go vet ./...`, `go test -race ./...` — зелёные на всех коммитах.
|
||
- Push: `2c6d2d7..7a2340d` в `origin/master`.
|