---
name: qstash-review
description: |
  Code review with QStash-specific best practices for qstash-stress project.
  Use when user asks to: (1) review code changes, (2) check for issues before commit,
  (3) review a PR, (4) validate new code follows project patterns, (5) find potential bugs.
  Triggers: "review this", "check my changes", "review code", "PR review", "code review".
---

# qstash-review

Code review checklist for qstash-stress with project-specific patterns.

## Review Checklist

### 1. Thread Safety

**Tracker access:**
- [ ] Use `atomic.AddInt64`/`LoadInt64` for counters
- [ ] Lock `TrackingInfo.mu` before accessing fields
- [ ] Use `sync.Map` methods (Load, Store, Range) correctly

**Metrics access:**
- [ ] Lock `m.mu` before modifying samples
- [ ] Use RLock for read-only operations

**Behaviors access:**
- [ ] Lock `b.mu` before accessing/modifying config

**Example - Correct:**
```go
info.mu.Lock()
info.Status = "delivered"
info.mu.Unlock()
atomic.AddInt64(&t.totalDelivered, 1)
```

**Example - Wrong:**
```go
info.Status = "delivered"  // No lock!
t.totalDelivered++          // Not atomic!
```

### 2. Error Handling

**HTTP responses:**
- [ ] Call `drainAndClose(resp.Body)` to enable connection reuse
- [ ] Check `io.ReadAll` errors

**JSON operations:**
- [ ] Check `json.Marshal` and `json.Unmarshal` errors
- [ ] Handle parse errors gracefully

**Example - Correct:**
```go
defer drainAndClose(resp.Body)
body, err := io.ReadAll(resp.Body)
if err != nil {
    return fmt.Errorf("read body: %w", err)
}
```

### 3. QStash Header Compliance

When adding new headers, verify:
- [ ] Header name follows `Upstash-*` convention
- [ ] Value format matches QStash API spec
- [ ] Added to `PublishOptions` struct
- [ ] Set in `Publish()` method
- [ ] Test case added in appropriate suite

Reference: `../qstash/features/*.mdx` for header specifications.

### 4. Memory Bounds

- [ ] New collections have size limits
- [ ] Long-running operations use reservoir sampling pattern
- [ ] No unbounded slice growth in loops

**Reservoir sampling pattern:**
```go
const maxSamples = 10000
if len(samples) < maxSamples {
    samples = append(samples, value)
} else {
    // Random replacement
    idx := rand.Intn(len(samples))
    samples[idx] = value
}
```

### 5. Test Coverage

For new features:
- [ ] Unit test in `*_test.go`
- [ ] Integration test case in `test-suites/*.yaml`
- [ ] Test passes with `go test -race`

For bug fixes:
- [ ] Regression test added
- [ ] Related tests still pass

### 6. Code Style

- [ ] No unused imports or variables
- [ ] Error messages are lowercase (Go convention)
- [ ] Context is propagated for cancellation
- [ ] Timeouts are configurable

## Common Issues to Flag

### Data Race Potential

```go
// BAD: Reading without lock
status := info.Status

// GOOD: Lock before read
info.mu.RLock()
status := info.Status
info.mu.RUnlock()
```

### Resource Leak

```go
// BAD: Body not drained
resp, _ := http.Get(url)
return resp.StatusCode

// GOOD: Drain and close
resp, _ := http.Get(url)
defer drainAndClose(resp.Body)
return resp.StatusCode
```

### Missing Error Check

```go
// BAD: Error ignored
body, _ := json.Marshal(data)

// GOOD: Error checked
body, err := json.Marshal(data)
if err != nil {
    return fmt.Errorf("marshal: %w", err)
}
```

## File-Specific Patterns

| File | Key Patterns |
|------|--------------|
| `tracker/tracker.go` | sync.Map, atomic ops, per-message mutex |
| `tracker/metrics.go` | Reservoir sampling, RWMutex |
| `publisher/client.go` | drainAndClose, QStash headers |
| `receiver/handlers.go` | Signature verification, body limits |
| `runner/runner.go` | Shared tracker with --serve mode |