Code Review Horror Stories. Часть 2: API, ошибки и graceful shutdown
- вторник, 12 мая 2026 г. в 00:00:18
Продолжение разбора реального кода с собеседования. В первой части разобрали 8 проблем concurrency и memory: race conditions, утечки горутин, проигнорированный mutex, TOCTOU. Это была первая половина из 21 бага в одном сервисе на 150 строк.
Сегодня — вторая часть. Тут нет страшных race conditions, но есть то, что выдаёт уровень разработчика на собесе: отношение к ошибкам, валидация, API design, graceful shutdown, observability. Эти баги не упадут “вдруг” в продакшене — они будут тихо пилить вам костыль за костылём, пока кто-то не сядет переписывать. Актуально для Go 1.26.
Напомню итог первой части: из 8 багов про concurrency на интервью нашёл 7, пропустил только TOCTOU race. В этой части из 13 багов пропустил два: package applike с func main() (то, что код не компилируется — банально не посмотрел на объявление пакета) и отсутствие slog (просто не зацепился за log.Println, а зря). Остальные 11 — поймал. Расскажу, какими паттернами в чтении кода я их вылавливал.
Микросервис трекинга рекламных кликов на Gin с in-memory storage. Принимает POST с (user_uuid, campaign_uuid, country), генерирует tracking link, дедуплицирует за 5 секунд, паблишит в очередь.
В первой части мы починили mutex’ы, утечки и каналы. Теперь смотрим на всё остальное.
Смотрю сигнатуру GetRecent:
func (r *InMemoryClickRepository) GetRecent( userUUID, campaignUUID string, maxAge time.Duration, ) (error, *CampaignClick, bool) { // ... }
На сигнатуре глаз сам зацепился — она читается криво, как иностранный акцент в Go-коде. Озвучиваю: “В Go конвенция — error всегда последний. Тут он первый, и при этом функция вообще никогда его не возвращает (везде return nil, ...). Лишний слот в сигнатуре — выкинуть”.
В Go это негласная конвенция: error всегда последний. Это не просто стилевое — это влияет на читаемость, на работу с linter’ами (errcheck ожидает error в конце), на то, как люди в команде будут это вызывать. Каждый Go-разработчик на автомате пишет:
val, err := someFunc()
А тут получается:
err, click, ok := repo.GetRecent(...)
И ты каждый раз спотыкаешься. Плюс — зачем тут вообще error? Функция никогда его не возвращает (везде return nil, ...). Лишний слот в сигнатуре, который путает.
Правильно:
func (r *InMemoryClickRepository) GetRecent( userUUID, campaignUUID string, maxAge time.Duration, ) (*CampaignClick, bool) { // если error реально не возвращается — убираем его }
Мораль: Конвенции языка существуют не просто так. error последний, context.Context первый, ok bool для двойного возврата с map/cast. Нарушаешь — твой код выглядит как написанный человеком, который Go не знает.
Смотрю на handler:
c.repo.Save(click) go c.publisher.Publish(click) ctx.JSON(http.StatusOK, ClickResponse{ ClickUUID: click.ClickUUID, TrackingLink: click.TrackingLink, })
Save объявлен как func Save(c *CampaignClick) error. То есть по контракту может вернуть ошибку. А мы её просто игнорируем и отвечаем клиенту 200 OK, как будто всё сохранилось.
Сегодня in-memory — действительно всегда nil. Но завтра кто-то заменит на PostgreSQL/Redis/Kafka, Save начнёт возвращать реальные ошибки, а handler как ни в чём не бывало рапортует “успех”. Клиент получает clickUUID, идёт по tracking link, но на стороне аналитики этого клика нет. Hello, broken funnel.
В Go 1.26 завезли errors.AsType[T] — типобезопасный generic-вариант errors.As. Удобно для чистой обработки доменных ошибок:
if err := c.repo.Save(ctx.Request.Context(), click); err != nil { if dbErr, ok := errors.AsType[*DBError](err); ok && dbErr.Retriable { // retry или fallback } log.Printf("save click failed: %v", err) ctx.JSON(http.StatusInternalServerError, gin.H{"error": "internal"}) return }
Мораль: Если функция возвращает error — обработай его. Всегда. Даже если “сейчас он всегда nil”. Сигнатура — это контракт на будущее. errcheck linter в CI закроет 90% таких дыр автоматически.
Смотрю handler:
func (c *ClickController) Post(ctx *gin.Context) { var req struct { UserUUID string `json:"user_uuid"` CampaignUUID string `json:"campaign_uuid"` Country string `json:"country"` } _ = ctx.ShouldBindJSON(&req) // дальше используем req... }
Заметили? = ctx.ShouldBindJSON(&req). Ошибка явно проигнорирована через "_ ". То есть если придёт невалидный JSON, мы получим пустую структуру, а потом сохраним в репозиторий запись с пустыми UUID’ами. Затем сгенерируем tracking link https://tracker.example/click?campaign=&user= и отправим клиенту. Hello broken analytics.
И валидации нет вообще никакой. UUID мог быть строкой "drop table users", страна — пустой, или "zz", или "🇷🇺". Всё это улетит в БД (когда in-memory заменят на постгрес) и в metrics.
Правильно — Gin умеет валидировать сам через binding теги:
func (c *ClickController) Post(ctx *gin.Context) { var req struct { UserUUID string `json:"user_uuid" binding:"required,uuid"` CampaignUUID string `json:"campaign_uuid" binding:"required,uuid"` Country string `json:"country" binding:"required,len=2,alpha"` } if err := ctx.ShouldBindJSON(&req); err != nil { ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } // дальше можно доверять req }
Мораль: Любой external input — это враждебная среда. Валидируй на границе системы. И НИКОГДА не игнорируй ошибки через "_". Это всегда сразу триггер на code review: либо явно ошибка проигнорирована и это ОК (и тогда нужен комментарий — почему), либо ошибка проигнорирована и это НЕ ОК. Третьего не дано.
Даже если добавить binding:"len=2,alpha", это пропустит "XQ", "ZZ", "AA" — формально 2 буквы, но не существующие коды стран. В аналитике потом эти “страны” сидят отдельной строкой и портят графики.
Правильно — кастомный validator с whitelist’ом:
import "github.com/biter777/countries" func validateCountry(fl validator.FieldLevel) bool { code := fl.Field().String() return countries.ByName(code).IsValid() } // при инициализации: if v, ok := binding.Validator.Engine().(*validator.Validate); ok { v.RegisterValidation("iso3166", validateCountry) } // в struct'е: Country string `binding:"required,iso3166"`
Либо хранить enum’ом и парсить:
type CountryCode string func (c *CountryCode) UnmarshalJSON(data []byte) error { var s string if err := json.Unmarshal(data, &s); err != nil { return err } if !isValidISO3166(s) { return fmt.Errorf("invalid country code: %q", s) } *c = CountryCode(s) return nil }
Мораль: “Это же просто строка” — главный источник мусора в таблицах. Если значение из конечного набора — заведи enum или валидатор. Domain types > primitive types.
Внимательно смотрю на:
func generateTrackingLink(campaignUUID, userUUID string) string { return "https://tracker.example/click?campaign=" + campaignUUID + "?user=" + userUUID }
Видите? Два знака ? в URL. Должен быть ? для первого параметра и & для остальных. То есть фактически в ?campaign=...?user=... параметр campaign будет содержать значение <uuid>?user=<uuid>, а параметра user вообще не будет на стороне tracker’а.
Это даже не баг concurrency или производительности. Это бизнес-баг: трекер не сможет атрибутировать клик пользователю. Деньги рекламодателей улетают в никуда, потому что аналитика битая.
Мораль: Не клей URL’ы и SQL-запросы строками. Для URL — net/url, для SQL — параметризованные запросы. Это правило 1990-х, которое почему-то всё ещё нарушают в 2026-м.
Допустим, мы починили прошлый баг и собираем URL руками: ?campaign= + &user=. Всё ещё плохо: значения не экранируются. Если в userUUID прилетит строка с & или = (через тот самый отсутствующий валидатор из бага #12), мы сломаем разбор query string на стороне tracker’а:
?campaign=abc&user=evil&fake=injected
Параметр user стал evil, а в URL появился левый fake. Если tracker этому доверяет — у нас injection.
Правильно — net/url:
import "net/url" func generateTrackingLink(campaignUUID, userUUID string) string { u, _ := url.Parse("https://tracker.example/click") q := u.Query() q.Set("campaign", campaignUUID) q.Set("user", userUUID) u.RawQuery = q.Encode() // автоматический escape return u.String() }
url.Values.Encode() сделает URL-escaping автоматически. Заодно правильно расставит ? и &.
Мораль: Любые user-controlled значения, попадающие в URL/SQL/HTML — должны проходить через соответствующий escape. net/url, database/sql (placeholders), html/template (а не text/template) — это базовая гигиена. Не делай руками то, что стандартная библиотека делает корректно.
Handler всегда возвращает http.StatusOK:
ctx.JSON(http.StatusOK, ClickResponse{...})
И для нового клика, и для дедуплицированного возврата из кеша. Семантически это неверно:
Новый клик → создан ресурс → 201 Created
Дедуплицированный → старый ресурс → 200 OK (или даже 409 Conflict, если хотим явно сигналить дубль)
Клиенту обычно всё равно (он смотрит только на 2xx), но как только включается тележка observability — графики “сколько новых vs дубликатов” построить нельзя без отдельной метрики, потому что HTTP-логи не различают эти случаи.
Правильно:
if cached, ok := c.repo.GetRecent(...); ok { ctx.JSON(http.StatusOK, toResponse(cached)) return } // ... создание нового ctx.JSON(http.StatusCreated, toResponse(click))
Мораль: HTTP status codes — это API. Используй их по назначению. 201 Created для создания, 204 No Content для DELETE без тела, 409 Conflict для дубликатов. Это бесплатная документация для клиентов и метрика для observability.
Где использование контекста для управления жизненным циклом? Нигде. Метод Save не принимает context.Context. Publish тоже. Если запрос отменился (клиент отвалился) — мы всё равно сохраним клик и попытаемся опубликовать.
Для in-memory это не критично. Но как только репозиторий заменят на PostgreSQL — db.Exec(...) без контекста будет висеть до победного, даже если клиент уже ушёл. Ресурсы DB connection pool кончатся, сервис ляжет.
Правильная сигнатура:
func (r *InMemoryClickRepository) Save(ctx context.Context, c *CampaignClick) error { select { case <-ctx.Done(): return ctx.Err() default: } r.mu.Lock() defer r.mu.Unlock() r.clicks = append(r.clicks, c) return nil }
И в handler’е:
if err := c.repo.Save(ctx.Request.Context(), click); err != nil { // ... }
Мораль: context.Context — первый аргумент любой функции, которая делает I/O или может занять заметное время. Даже если сейчас оно “быстро” — завтра кто-то заменит in-memory на сетевой вызов, и без контекста это будет бомба.
Смотрю в main:
r := gin.Default() r.POST("/v1/campaign/click", ctrl.Post) if err := r.Run(":8080"); err != nil { panic(err) }
r.Run() блокирует main-горутину до ошибки. Когда придёт SIGTERM (а это происходит при каждом деплое в Kubernetes), процесс просто убьётся. Все запросы, которые были в обработке — потеряются. Дедуп-кеш в памяти — потеряется. Канал publisher.out с накопленными сообщениями — потеряется.
Правильно — через http.Server напрямую и signal handling:
srv := &http.Server{ Addr: ":8080", Handler: r, ReadHeaderTimeout: 5 * time.Second, // защита от Slowloris ReadTimeout: 30 * time.Second, WriteTimeout: 30 * time.Second, IdleTimeout: 120 * time.Second, } go func() { if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { log.Fatalf("server: %v", err) } }() // Ждём сигнал quit := make(chan os.Signal, 1) signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM) <-quit // Даём 30 секунд на завершение in-flight запросов ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() if err := srv.Shutdown(ctx); err != nil { log.Printf("forced shutdown: %v", err) } // И только теперь закрываем publisher, repository и т.д. publisher.Close(ctx)
Мораль: В production graceful shutdown — это обязательно, не “nice to have”. Без него каждый деплой = потерянные запросы = недовольные пользователи.
Заметили в примере выше четыре поля? ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout. По умолчанию у http.Server они все равны нулю = “без ограничений”. Это знаменитая ловушка Go.
Что это значит на практике:
Один медленный клиент держит соединение бесконечно (атака Slowloris)
Каждое такое соединение — занятый worker
На N соединений сервис ложится при N == file descriptor limit
gin.Default().Run(...) под капотом тоже использует http.Server без таймаутов. Это часть бага #18, но отдельно — потому что на собесе про это спрашивают вне контекста shutdown.
Минимальный конфиг для production:
srv := &http.Server{ ReadHeaderTimeout: 5 * time.Second, // обязательно — защита от Slowloris ReadTimeout: 30 * time.Second, // полный read с body WriteTimeout: 30 * time.Second, // запись ответа IdleTimeout: 120 * time.Second, // keep-alive }
Если у вас бывают долгие запросы (стриминг, большой upload) — ReadTimeout/WriteTimeout ставьте побольше или используйте http.ResponseController для управления per-request.
Мораль: http.Server{} с дефолтами — это бомба замедленного действия. Минимум ReadHeaderTimeout должен быть всегда. Это знают на каждом собесе на Senior, и не знать про это — стыдно.
Этот я тоже промахнул на интервью. По всему коду:
log.Println("post error:", err) log.Println("click:", string(bodyBytes))
Глаз скользнул по log.Println как по чему-то нейтральному — мол, ну логирует и логирует. А зря. Уже при подготовке этого поста, перечитывая, сразу бросилось: 2026 год, в стандартной библиотеке давно log/slog, а тут ничем не лучше fmt.Println-обёртки.
Это плохо по нескольким причинам:
Не структурировано — в production система мониторинга (Loki, Datadog, ELK) не сможет нормально парсить
Нет уровней — нельзя отфильтровать DEBUG от ERROR
Нет контекста — нет request_id, trace_id, user_id
Глобальный logger — нельзя подменить на тестах
Со Go 1.21 в стандартной библиотеке есть log/slog — структурированный логгер. В Go 1.26 он стал ещё стабильнее.
Правильно:
import "log/slog" logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{ Level: slog.LevelInfo, })) logger.Error("publish failed", slog.String("click_uuid", click.ClickUUID), slog.String("user_uuid", click.UserUUID), slog.Any("err", err), )
Output:
{"time":"2026-05-04T12:00:00Z","level":"ERROR","msg":"publish failed","click_uuid":"...","user_uuid":"...","err":"..."}
Мораль: Переходите на log/slog. log.Println хорош только для quick’n’dirty CLI-утилит. В сервисе — структурированные логи с контекстом, всегда.
Самый вишенко-на-торте баг. В main() запускается фоновая горутина, которая каждую секунду шлёт тестовый запрос:
go func() { ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() for range ticker.C { payload := struct { UserUUID string `json:"user_uuid"` CampaignUUID string `json:"campaign_uuid"` Country string `json:"country"` }{ UserUUID: uuid.NewString(), // ❌ новый UUID каждый раз! CampaignUUID: uuid.NewString(), // ❌ новый UUID каждый раз! Country: "US", } b, _ := json.Marshal(payload) body := bytes.NewBuffer(b) resp, err := http.Post("http://localhost:8080/v1/campaign/click", "application/json", body) if err != nil { log.Println("post error:", err) continue } bodyBytes, _ := io.ReadAll(resp.Body) log.Println("click:", string(bodyBytes)) _ = resp.Body.Close() } }()
Вот тут букет проблем:
A. Random UUIDs — дедуп никогда не триггерится. Пара (user, campaign) всегда уникальная. То есть половина функциональности кода не покрыта этим тестом. Зачем он тогда?
B. http.Post без таймаута. http.DefaultClient не имеет timeout — connection может висеть до победного. Под нагрузкой first thing to break.
C. Игнорирование ошибок — b, := json.Marshal(...) и bodyBytes, := io.ReadAll(...). Кстати, в Go 1.26 io.ReadAll стал в 2 раза быстрее и кушает на 50% меньше памяти — приятный бонус, но не отменяет необходимости проверять ошибку.
D. Race на старте. Горутина запускается до r.Run(). Если планировщик решит так — первый POST стрельнёт раньше, чем сервер начал слушать порт. В логах увидите connection refused.
E. Тестовый код в main. Это вообще не место для smoke-теста. Должен быть отдельный бинарь cmd/load-tester/ или внешний инструмент (vegeta, k6, hey).
Правильно (если уж нужно оставить в main):
testUsers := []string{uuid.NewString(), uuid.NewString(), uuid.NewString()} testCampaigns := []string{uuid.NewString(), uuid.NewString()} httpClient := &http.Client{ Timeout: 5 * time.Second, } go func() { // Ждём, пока сервер поднимется time.Sleep(500 * time.Millisecond) ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() i := 0 for range ticker.C { payload := struct{ /* ... */ }{ UserUUID: testUsers[i%len(testUsers)], // повторы → дедуп сработает! CampaignUUID: testCampaigns[i%len(testCampaigns)], Country: "US", } b, err := json.Marshal(payload) if err != nil { slog.Error("marshal", "err", err) continue } resp, err := httpClient.Post( "http://localhost:8080/v1/campaign/click", "application/json", bytes.NewBuffer(b), ) if err != nil { slog.Error("post", "err", err) continue } bodyBytes, err := io.ReadAll(resp.Body) resp.Body.Close() if err != nil { slog.Error("read body", "err", err) continue } slog.Info("click", "response", string(bodyBytes)) i++ } }()
Лучше вообще выкинуть эту горутину из main и положить в cmd/load-tester/main.go или заменить на k6 script.
Мораль: Если ты пишешь “тестовый код” — убедись, что он реально тестирует то, что нужно. Random data часто означает отсутствие повторяемости и пропуск кейсов с пересечениями. И никогда не используй http.Post/http.Get без явного клиента с timeout — это первое, что упадёт под нагрузкой.
# | Проблема | Тип |
|---|---|---|
9 |
| API convention |
10 |
| Error handling |
11 |
| Input validation |
12 | Country без проверки на ISO 3166-1 | Input validation |
13 | URL с двумя | Business bug |
14 | URL без | Security/correctness |
15 | Всегда | API design |
16 | Нет | Cancellation |
17 | Нет graceful shutdown | Reliability |
18 | Нет HTTP timeout’ов на сервере | Slowloris |
19 |
| Observability |
20 | Тест с random UUIDs + http.Post без timeout + ignored errors + race | Test quality |
Из 13 багов в этой части на интервью я нашёл 11. Когда писал пост - увидел что один из багов который я нашел первоначально - багом не является. Так что багов оказалось меньше чем писал в первой части. Пропустил #19 (log.Println вместо slog — глаз не зацепился, был сфокусирован на функциональных багах, не на code style). Если суммировать с первой частью: 18 из 20 за 30 минут. Три пропущенных — TOCTOU race, compile error и slog — каждый по своему характерный, и каждый научил отдельному паттерну “куда смотреть в следующий раз”.
errors.AsType[T] — типобезопасный generic-вариант errors.As. Чище парсятся доменные ошибки (см. баг #11).
io.ReadAll ×2 быстрее, −50% памяти — те, кто читает HTTP body, получают бесплатный буст (см. баг #21).
HTTP/2 StrictMaxConcurrentRequests — теперь можно нормально ограничивать concurrent requests на уровне транспорта.
HTTP 307 для trailing slash redirects (вместо 301) — breaking change, проверьте редиректы в API.
net/url.Parse стал строже к malformed URL’ам с двоеточиями в host. Если что-то сломалось — GODEBUG=urlstrictcolons=0 вернёт старое поведение.
Полный список из обеих частей:
Concurrency & Memory (Часть 1):
Забытый Lock в Save (data race)
Канал без consumer’а (goroutine leak)
Publisher по значению (channel duplication)
PurgeOld не вызывается (memory leak)
PurgeOld O(N²) под Lock (latency spike)
TOCTOU race в дедупликации
O(N) поиск в горячем пути
go publisher.Publish() без context
API & Reliability (Часть 2):
error не последний в return
Save() error игнорируется
Нет валидации входных данных
Country не проверяется на ISO 3166-1
Битый URL (два ?)
URL без escaping
Status 200 вместо 201
Нет context.Context
Нет graceful shutdown
Нет HTTP timeout’ов на сервере
log.Println вместо slog
Тестовая горутина бесполезна (random UUIDs + http.Post без timeout)
Раз. Code review — это не “найди опечатку”. Это “представь, что это твой код в продакшене на 1000 RPS, что сломается?”. Думай в режиме “как я буду это дебажить в субботу ночью”.
Два. Идти сверху вниз не работает. Нашёл первый баг — выписал, идёшь дальше. Иначе залипнешь на одной строке и потеряешь время.
Три. Категории ошибок, по которым стоит проходиться явно:
Compilation: package, импорты, объявления (это часто пропускают, и зря)
Concurrency: где Lock? где race? где забытый channel close?
Resource leaks: горутины, каналы, файлы, connections, память
Error handling: где проигнорированы errors? где nil panic’нет?
Boundaries: валидация на input, escaping на output
API design: status codes, error returns, context propagation
Reliability: shutdown, timeouts, retries, observability
Performance hot paths: O(N) в handler — это плохо
Четыре. Озвучивай мысли вслух. Это сильно помогает: интервьюеру важно понять, как ты думаешь, а не только что нашёл. Я начал с “так, у нас репозиторий с RWMutex, проверю, все ли write-методы его берут” — и сразу поймал баг #1. Дальше шёл по чеклисту вслух: “теперь канал — кто consumer? нет consumer’а — баг #2”, и так далее. Минусом — на чеклисте легко провалить вещи, которые в чеклист не попали (как у меня с TOCTOU и slog).
Пять. Если что-то непонятно — спрашивай. “А этот сервис будет под нагрузкой?”, “А Publisher куда пишет в реальности?”, “Нужна ли exactly-once гарантия?”. Контекст меняет приоритеты багов.
Все эти разборы — часть подготовки к серии материалов про реальный Senior Go. Не теория из книжек, а вот такие куски кода, которые реально дают на интервью топ-компании.
В Telegram-канале @go_interview_prep_ru регулярно выкладываю:
📝 Code review challenges — кусок кода, найди баги (с разборами через 2 дня)
🧠 Go quiz по 1 вопросу в день — то, что реально спрашивают
💡 Patterns & anti-patterns из production
🔧 Готовые snippet’ы для интервью (worker pool, rate limiter, и т.д.)
Так же готовлю курс на Stepik — Go Senior Interview. В нем хочу рассмотреть несколько моментов:
GMP Scheduler (готов!)
Memory Model & Escape Analysis (готов!)
Garbage Collector Internals
Interfaces & Reflection
Generics in Production
Context & Error Patterns
Два модуля уже сделал. Остальные готовлю. Курс как и телеграмм канал БЕСПЛАТНЫЙ - если кто опять этот пост решит как реклама обозначить.
На этом я свои посты не заканчиваю, буду дальше выкладывать интересные моменты которые встречал на собеседовании. И как прочитал в одной соцсети - всем хороших офферов и толковых интервьюеров, а компаниям - квалифицированных кандидатов.