Nail/SQS-service
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
582ad4a83295513d9f98a48b823c2f8170f3291e
SQS-service/doc/thinking/opus-report-2026-10-10.md
T

25 KiB
Raw Blame History

Код-ревью 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.
Reference in New Issue View Git Blame Copy Permalink