Files
SQS-service/HISTORY/2026-10-11-session-log.md
T

97 lines
9.2 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# История правок 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`.