diff --git a/HISTORY/2026-10-11-session-log.md b/HISTORY/2026-10-11-session-log.md new file mode 100644 index 0000000..a5adbc6 --- /dev/null +++ b/HISTORY/2026-10-11-session-log.md @@ -0,0 +1,96 @@ +# История правок 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` | + +### Осознанно НЕ менялось (решения владельца/архитектуры) + +- 🟡34 (атрибуты «0 = не передано») — не менять совместимость. +- 🟡37 (дефолт `SHARED_SQS_MAX_HEAP_MB`) — число зависит от RAM ВМ, решает владелец. +- 🟡28 (не держать `queue.mu` во время сетевых вызовов) — **не сделано**: см. п.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** (`send_message.go:150`, `delete_message.go`, `gosqs.go`): синхронный + `SaveMessageSync` (до 3 с) выполняется под `queue.mu`. Простой вынос сети за + `mu` **вносит регрессию**: между `Unlock` и повторным `Lock` появляется окно, + в котором два конкурентных `SendMessage` с одним `deduplicationId` оба пройдут + `IsDuplicate` (дубль), либо при откате `dedup` теряется. Корректное решение + требует более сложной синхронизации — отложено до отдельного захода. +- 🟡34/🟡37 — см. п.2. + +## 5. Верификация + +- `go build ./...`, `go vet ./...`, `go test -race ./...` — зелёные на всех коммитах. +- Push: `2c6d2d7..7a2340d` в `origin/master`.