152 lines
11 KiB
Markdown
152 lines
11 KiB
Markdown
# История правок shared-sqs — 2026-10-07 (P1.2 + фикс утечки блокировки)
|
||
|
||
> Запись по требованию «документировать всё, включая собственные ошибки».
|
||
> Предыдущие записи: `doc/thinking/*-2026-10-0[56].md`, `HISTORY/2026-08-1*-session-log.md`.
|
||
|
||
## 1. Задача
|
||
|
||
Пользователь: «доделать ПОЛНОСТЬЮ и протестировать SQS». Незакрытым по плану
|
||
`plan-for-flash-2026-10-05.md` оставался единственный пункт — **P1.2
|
||
(индексация сообщений)**, отложенный 2026-10-06 с обоснованием.
|
||
|
||
## 2. Что сделано
|
||
|
||
### 2.1. Дизайн (коммит `b58e958`)
|
||
|
||
`doc/thinking/p1.2-message-index-design-2026-10-07.md`.
|
||
|
||
Ключевое решение: **tombstone + индексы + амортизированная компакция**.
|
||
Удаление не сдвигает срез (O(1)), поэтому индексы `receipt→index` и `uuid→index`
|
||
остаются валидными. Физическая чистка — `Compact`/`CompactIfNeeded` в `processQueueTick`.
|
||
Redis-схема не меняется (сообщения и так лежат в per-message HASH).
|
||
|
||
### 2.2. Код (коммит `22e9bc8`)
|
||
|
||
- `models.Queue`: `deadIdx`, `receiptIdx`, `uuidIdx`, `liveCount`, `deadCount` (все `json:"-"`)
|
||
+ примитивы: `AppendMessage`, `RemoveIndex`, `Compact`, `CompactIfNeeded`, `Reset`,
|
||
`EnsureIndexes`/`RebuildIndexes`, `IndexOfReceipt`, `IndexOfUUID`, `MarkInFlight`,
|
||
`ClearInFlight`, `InFlightIndexes`, `IsDead`, `LiveCount`.
|
||
- `DeleteMessage` / `ChangeMessageVisibility(±Batch)` — поиск по индексу вместо скана среза;
|
||
удаление без сдвига остатка.
|
||
- `DeleteMessageBatch` — tombstone вместо полной пересборки среза.
|
||
- `processQueueTick` — visibility/DLQ по индексу in-flight (O(in-flight)), retention — через
|
||
tombstone, в конце — `CompactIfNeeded`.
|
||
- send-пути — `AppendMessage`/`LiveCount`; admin/metrics/persistence — учёт живых сообщений;
|
||
`LoadAllQueues` — `RebuildIndexes` после загрузки.
|
||
|
||
### 2.3. Тесты (коммиты `e05af04`, `5b3fb51`)
|
||
|
||
- `app/models/message_index_test.go` — 6 тестов примитивов (tombstone, компакция, индексы, Reset, порог).
|
||
- `app/gosqs/delete_middle_test.go` — удаление СЕРЕДИНЫ очереди через API: остальные не теряются,
|
||
удалённое не возвращается.
|
||
- `app/gosqs/tenant_flow_test.go` — существующие тесты переведены на `LiveCount()`; добавлен
|
||
`TestReceiveEmptyQueue_DoesNotLeakLock`.
|
||
|
||
## 3. НАЙДЕННЫЙ ДЕФЕКТ (моя регрессия) — утечка per-queue блокировки
|
||
|
||
### 3.1. Проявление
|
||
|
||
Стресс (2000 сообщений × циклы): `ReadTimeout`, всплеск 1.9 с на `delete_batch`,
|
||
дальше — зависание сервиса (health при этом отвечает).
|
||
|
||
### 3.2. Диагностика (факты, не догадки)
|
||
|
||
- Снят стек горутин по `SIGQUIT`: десятки горутин в `sync.RWMutex.RLock`
|
||
(`models.go:101 Queue.RLockMu`), включая `metrics.updateGauges` (ждала 15 минут)
|
||
и HTTP-хендлеры `GetQueueAttributesV1`.
|
||
- Аудит всех `GetLocked`/`GetRLocked`: дефект ровно один.
|
||
- `receive_message.go`:
|
||
```go
|
||
queue := models.SyncQueues.GetLocked(key) // Lock взят
|
||
if queue == nil || queue.LiveCount() == 0 {
|
||
return nil, false // ← возврат БЕЗ Unlock
|
||
}
|
||
defer queue.UnlockMu()
|
||
```
|
||
|
||
### 3.3. Причина и почему проявилось именно сейчас
|
||
|
||
Возврат до `defer` оставлял `queue.mu` захваченным навсегда (в Go мьютекс не
|
||
освобождается при выходе из функции). Прежнее условие `len(queue.Messages) == 0`
|
||
срабатывало только на полностью пустой очереди; после перехода на tombstone
|
||
появился новый случай `LiveCount()==0` при `len(Messages)>0` (все сообщения удалены,
|
||
компакция ещё не прошла) — типовой для последовательных циклов «залить → выгрести».
|
||
|
||
### 3.4. Фикс
|
||
|
||
`defer queue.UnlockMu()` — до любой проверки/возврата. Регрессионный тест
|
||
`TestReceiveEmptyQueue_DoesNotLeakLock` проверен на дефекте (падает за 2 с)
|
||
и на фиксе (зелёный).
|
||
|
||
## 4. Результаты проверок (после фикса)
|
||
|
||
| Проверка | Результат |
|
||
|---|---|
|
||
| `go build ./...`, `go vet ./...` | OK |
|
||
| `go test -race ./app/...` | зелёные |
|
||
| `tests/api_test.sh` (aws CLI, локальный сервис) | PASS=22 FAIL=0 |
|
||
| `tests/sdk_test.py` (boto3 1.43.108) | PASS=15 FAIL=0 |
|
||
| `tests/fifo_dlq_probe.py` | fifo_order / fifo_dedup / dlq = PASS |
|
||
| Стресс 3×2000 (send-batch → receive → delete-batch) | send ≈0.6 с, drain ≈1.2 с на цикл, `left=0/0`, вызовов >0.5 с нет |
|
||
|
||
## 5. Инфраструктурные заметки (для повторного прогона)
|
||
|
||
- Локальный сервис: `SHARED_SQS_SEED_DEMO=true /tmp/shared-sqs --admin-token=... --port=4100`
|
||
(memory-only без `REDIS_ADDR`; креды демо-тенанта печатаются в лог при старте).
|
||
- `boto3` в системе НЕ установлен (PEP 668) и `python3.12-venv` отсутствует →
|
||
wheels скачаны через `pip download` и распакованы в `/tmp/sqlibs_unpacked`,
|
||
запуск с `PYTHONPATH=/tmp/sqlibs_unpacked`.
|
||
- Лимит очередей тенанта — 10: падавшие скрипты оставляют очереди, перед прогоном
|
||
чистить (`aws sqs delete-queue`).
|
||
- `ENDPOINT` (api_test.sh) и `ENDPOINT_URL` (python-тесты) — разные имена переменных.
|
||
|
||
## 6. Уроки
|
||
|
||
1. Стресс-проверка обязательна: api/sdk/fifo проходили и с дефектом, он проявлялся
|
||
только на циклах полного опустошения очереди.
|
||
2. `SIGQUIT` — быстрый и точный способ найти зависание (кто держит мьютекс), дешевле догадок.
|
||
3. При замене условия на новый критерий (`len` → `LiveCount`) надо перечитывать
|
||
весь блок, включая порядок `defer` — старый код содержал латентную утечку лока,
|
||
которую новый критерий сделал массовой.
|
||
4. Не запускать сервис с `| head` — SIGPIPE убивает процесс на середине теста.
|
||
|
||
## 7. Код-ревью (роль ПРО) и фиксы — коммит `48c38c4`
|
||
|
||
Проведён архитектурный разбор всех затронутых файлов. Найдено и исправлено:
|
||
|
||
1. **🔥 Батч-видимость не заводила в DLQ** (`change_message_visibility_batch.go`).
|
||
При `VisibilityTimeout=0` батч лишь инкрементировал `Retry` и НЕ переносил
|
||
сообщение в DLQ — логика была только в одиночной версии, счётчик и перенос
|
||
расходились. Вынесен единый метод `models.Queue.ResetVisibilityAndTryDLQ`
|
||
(сброс видимости + DLQ-перенос), обе версии приведены к одному пути.
|
||
Тест `TestChangeVisibilityBatch_MovesToDLQ` (падает на старой логике, зелёный на новой).
|
||
2. **TOCTOU лимит в `SendMessageBatch`**: проверка OOM-лимита была только по
|
||
устаревшему снимку (RLock → Lock). Добавлена повторная проверка под per-queue Lock.
|
||
3. Инварианты `InFlightIndexes` (снимок может содержать устаревшие индексы —
|
||
потребитель обязан перепроверять) и `ensureIndexes` (проверяет согласованность
|
||
чисел, а не валидность map-индексов → `Messages` нельзя мутировать вне методов)
|
||
зафиксированы в док-комментариях.
|
||
4. `Compact` упрощён: один `RebuildIndexes` на выходе + `ensureIndexes` на входе
|
||
(убран дублирующий блок), `deadCount` пересчитывается внутри.
|
||
|
||
Осталось осознанно не тронуто: `DeleteMessageBatch` итерация с `RemoveIndex` внутри
|
||
безопасна (tombstone не меняет длину) — покрыто тестами.
|
||
|
||
## 8. Полный код-ревью сервиса (2026-10-08) — коммит `5b0e89a`
|
||
|
||
Разобран весь сервис: auth (SigV4 + JWT), router, billing, tenant, admin/UI API,
|
||
создание очередей. Критичных проблем не найдено. Исправлено три замечания:
|
||
|
||
1. **TOCTOU лимита тенантов** — `TenantStore.Create`: проверка `MaxTenantsGlobal`
|
||
была только под RLock, вставка — позже под Lock без повторной проверки. Добавлена
|
||
проверка под Lock (как уже было в `CreateFromJWT`).
|
||
2. **Потеря `NubesSub` при рестарте** — `CreateFromJWT`: обновлённый `NubesSub`
|
||
сохранялся в Redis только внутри ветки смены `MaxQueues`; теперь персист при
|
||
любом реальном изменении, запись только если что-то изменилось.
|
||
3. **`http.Client` на каждый вызов** — `auth.PingNubesAPI` переведён на общий
|
||
клиент (keep-alive).
|
||
|
||
UI/безопасность проверены отдельно: `listTenants`/`getTenant`/IDOR для UI-тенанта —
|
||
видит только себя, чужие `{id}` отсекаются, удаление через UI запрещено,
|
||
`SecretKey` отдаётся только по явному `/ui/api/credentials`.
|