chore: capture post-P0/P1 state for clean P2 start (working tree was dirty at task begin)
This commit is contained in:
+138
@@ -0,0 +1,138 @@
|
||||
# P0: Critical Hardening & Stability Fixes
|
||||
|
||||
**Priority**: P0 — Ship-blocking
|
||||
**Theme**: Eliminate crashes, goroutine leaks, and silent misconfigurations that make the service unreliable as a long-running LAN cache appliance.
|
||||
**Status**: Not started
|
||||
**Target**: All items resolved before next production deployment or public release.
|
||||
|
||||
## Goal
|
||||
|
||||
Make `steamcache2` safe to run continuously with real traffic under normal and adverse conditions (bad config, unreachable upstream, frequent restarts, high load).
|
||||
|
||||
## Overview
|
||||
|
||||
The current implementation has several classes of defects that can cause:
|
||||
|
||||
- Immediate panics on startup or misconfiguration
|
||||
- Deadlocks / hangs on graceful shutdown
|
||||
- Silent failure to enforce documented configuration constraints
|
||||
- Loss of error visibility in metrics and logs
|
||||
|
||||
These must be fixed before the project can be considered production-ready.
|
||||
|
||||
## Tasks
|
||||
|
||||
### P0-01: Make `New()` return an error instead of panicking on invalid sizes
|
||||
|
||||
- **Description**: `units.FromHumanSize()` failures in `New()` currently call `panic()`. This is hostile to callers (tests, embedding, future CLI refactoring) and prevents clean error handling.
|
||||
- **Impact**: Any invalid memory/disk size string (including from generated default config in edge cases) crashes the entire process with a stack trace instead of a helpful message.
|
||||
- **Affected Files**:
|
||||
- `steamcache/steamcache.go` (New function, lines ~854-863)
|
||||
- Call sites in `cmd/root.go`
|
||||
- All tests that construct `SteamCache`
|
||||
- **Approach**:
|
||||
1. Change signature to `func New(...) (*SteamCache, error)`
|
||||
2. Return wrapped error for parse failures.
|
||||
3. Update `NewWithOptions` accordingly.
|
||||
4. Update all internal construction paths and tests (use `t.Cleanup` + proper error checks).
|
||||
- **Acceptance Criteria**:
|
||||
- No more panics from `New()` for bad size strings.
|
||||
- Clear error messages: `"invalid memory size: ..."`
|
||||
- All existing tests still pass (updated for new signature).
|
||||
- `go test -race -short ./...` is green.
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Small (1-2 hours)
|
||||
|
||||
### P0-02: Fix upstream connectivity check nil pointer dereference
|
||||
|
||||
- **Description**: In `Run()`, the code does:
|
||||
```go
|
||||
resp, err := sc.client.Get(sc.upstream)
|
||||
if err != nil || resp.StatusCode != http.StatusOK {
|
||||
... use resp.StatusCode when err != nil ...
|
||||
```
|
||||
When the GET fails, `resp` is typically `nil`.
|
||||
- **Impact**: Service crashes with panic on startup whenever the configured `upstream` is unreachable or returns non-200 (very common on first setup or network hiccup).
|
||||
- **Affected Files**:
|
||||
- `steamcache/steamcache.go` (Run method, ~1025-1032)
|
||||
- **Approach**:
|
||||
1. Reorder the check: handle `err != nil` first.
|
||||
2. Only inspect `resp.StatusCode` when `resp != nil`.
|
||||
3. Always close `resp.Body` when `resp != nil`.
|
||||
4. Improve the error log message.
|
||||
5. Consider making the upstream check optional or retrying (but keep current behavior of exiting for now).
|
||||
- **Acceptance Criteria**:
|
||||
- Starting with an unreachable upstream produces a clean error log + `os.Exit(1)` instead of a panic.
|
||||
- Starting with a reachable but non-200 upstream behaves cleanly.
|
||||
- No resource leaks in the error paths.
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Trivial (< 30 min)
|
||||
|
||||
### P0-03: Call `config.Validate()` on startup and fail fast with actionable messages
|
||||
|
||||
- **Description**: A complete `Validate()` method exists in `config/config.go` but is **never invoked**.
|
||||
- **Impact**:
|
||||
- Users can start the service with invalid GC algorithm names, negative concurrency limits, or disk cache enabled with no path.
|
||||
- The service runs in a broken or surprising state instead of failing early with a clear message.
|
||||
- **Affected Files**:
|
||||
- `config/config.go`
|
||||
- `cmd/root.go` (after `LoadConfig`)
|
||||
- **Approach**:
|
||||
1. Call `cfg.Validate()` immediately after loading (and after applying CLI overrides).
|
||||
2. On error, log the problem at ERROR level with the exact field and suggestion.
|
||||
3. Exit with code 1 and print a user-friendly message to stderr.
|
||||
4. Add unit tests for `Validate()` covering all error cases (currently untested).
|
||||
- **Acceptance Criteria**:
|
||||
- Invalid `gc_algorithm`, negative limits, or missing disk path cause immediate clean failure.
|
||||
- Error messages are specific and actionable.
|
||||
- `Validate()` has ≥90% statement coverage in its own test file.
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Small (1-2 hours including tests)
|
||||
|
||||
### P0-04: Wire up error metrics and ensure all error paths are instrumented
|
||||
|
||||
- **Description**: `metrics.IncrementErrors()` exists and `Stats.Errors` is exposed, but the method is **never called** anywhere in the codebase. Many 5xx paths also fail to increment other relevant counters.
|
||||
- **Impact**: Operators have no visibility into error rates via `/metrics`. The "errors" field is always zero.
|
||||
- **Affected Files**:
|
||||
- `steamcache/metrics/metrics.go`
|
||||
- `steamcache/steamcache.go` (ServeHTTP and related methods — many locations)
|
||||
- Possibly `vfs/*` error paths that bubble up
|
||||
- **Approach**:
|
||||
1. Audit every place that returns 5xx or logs an error in the request path.
|
||||
2. Call `sc.metrics.IncrementErrors()` (and any other appropriate counters) in those paths.
|
||||
3. Ensure coalesced request error paths also record errors.
|
||||
4. Add a simple test that exercises error paths and asserts metric values.
|
||||
- **Acceptance Criteria**:
|
||||
- `/metrics` reports non-zero `errors` under induced failure conditions.
|
||||
- All current 5xx response paths increment the counter exactly once per failed request.
|
||||
- No double-counting on coalesced failures.
|
||||
- **Dependencies**: P0-02 (partially)
|
||||
- **Effort**: Medium (2-4 hours)
|
||||
|
||||
## Definition of Done (for the whole P0 milestone)
|
||||
|
||||
- [ ] All four tasks above completed and merged.
|
||||
- [ ] `go test -race -shuffle=on -timeout=5m ./...` passes cleanly.
|
||||
- [ ] Manual verification:
|
||||
- Start with bad memory size → clean error, no panic.
|
||||
- Start with unreachable upstream → clean error + exit 1, no panic.
|
||||
- Start with invalid `gc_algorithm` → fails fast with clear message.
|
||||
- Induce upstream 500s and connection errors → `/metrics` shows increasing errors count.
|
||||
- [ ] Shutdown no longer hangs due to the client limiter goroutine (see P0-05 below if split out).
|
||||
- [ ] Updated README or a new `docs/OPERATIONS.md` section documents the new strict startup validation behavior.
|
||||
|
||||
## Notes for Implementers
|
||||
|
||||
- These fixes are intentionally small and localized so they can be landed quickly.
|
||||
- Prefer adding new tests over modifying large amounts of existing test code.
|
||||
- Keep backward compatibility for the public `New` constructor as much as possible (or provide a clear migration path in comments).
|
||||
|
||||
## References
|
||||
|
||||
- Full code review (see conversation history or `plans/` directory).
|
||||
- Original locations identified in `steamcache/steamcache.go`, `config/config.go`, `cmd/root.go`.
|
||||
- Related goroutine leak in `cleanupOldClientLimiters` (may be promoted to its own P0 item if it blocks shutdown testing).
|
||||
|
||||
---
|
||||
|
||||
**Next actions after P0**: Move on to P1 items once the service can start and stop reliably without crashing.
|
||||
+147
@@ -0,0 +1,147 @@
|
||||
# P1: Hardening, Correctness & Security Improvements
|
||||
|
||||
**Priority**: P1 — Important hardening and correctness work
|
||||
**Theme**: Eliminate data integrity risks, resource exhaustion vectors, and incomplete security controls.
|
||||
**Status**: Not started
|
||||
**Depends on**: P0 (recommended — many P1 items are easier to verify once the service starts/stops cleanly)
|
||||
|
||||
## Goal
|
||||
|
||||
Make the cache **safe by default** against common failure modes, malicious or malformed input, and misconfiguration while preserving the high-performance characteristics required for Steam traffic.
|
||||
|
||||
## Overview
|
||||
|
||||
Even after P0 items are resolved, several classes of defects remain:
|
||||
|
||||
- Unbounded memory usage on large responses or cache promotion
|
||||
- Incomplete / spoofable client identification used for rate limiting
|
||||
- Overstated features (LFU, hybrid eviction) that do not actually work as documented
|
||||
- Significant "smart caching" code (adaptive/predictive) that provides no actual benefit today
|
||||
|
||||
These items directly affect correctness, security posture, and user trust.
|
||||
|
||||
## Tasks
|
||||
|
||||
### P1-01: Implement bounded / streaming response handling (prevent OOM on large bodies)
|
||||
|
||||
- **Description**: `ServeHTTP` currently does `bodyData, err := io.ReadAll(resp.Body)` for every cache miss before deciding whether to serve or cache. Promotion paths do the same. There are no size limits.
|
||||
- **Impact**:
|
||||
- A single large (or malicious) response from upstream can exhaust RAM and crash the process.
|
||||
- Steam chunks are usually small, but manifests, depots, and especially misconfigured upstreams can be very large.
|
||||
- Coalesced request buffering also keeps full bodies in memory.
|
||||
- **Affected Files**:
|
||||
- `steamcache/steamcache.go` (ServeHTTP around lines 1505-1518, reconstruct, coalesced paths)
|
||||
- `vfs/cache/cache.go` (promoteToFast)
|
||||
- Possibly disk/memory write paths
|
||||
- **Approach** (choose one or hybrid):
|
||||
1. Preferred long-term: Stream to client with `io.TeeReader` (or custom tee) directly into the VFS `Create` writer while serving. Only buffer small responses.
|
||||
2. Short-term mitigation: Add a hard per-request body cap (e.g. 64 MiB or configurable) and return 502/413 for anything larger without caching.
|
||||
3. Make coalesced request buffering also respect a size limit or use a temp file for very large objects.
|
||||
- **Acceptance Criteria**:
|
||||
- No `io.ReadAll` of unbounded upstream responses in the hot path.
|
||||
- Configurable or hard safety limit exists and is documented.
|
||||
- Large responses are still served correctly (streaming) when they fit the limit.
|
||||
- Existing Range + cache hit behavior is unaffected.
|
||||
- New integration test that attempts a > limit response and verifies graceful handling.
|
||||
- **Dependencies**: P0-04 (error metrics will help prove the new path works)
|
||||
- **Effort**: Medium-Large (4-8 hours). Streaming tee writer is the cleanest but requires care with VFS `Create` semantics.
|
||||
|
||||
### P1-02: Make client IP extraction for rate limiting configurable and safe
|
||||
|
||||
- **Description**: `getClientIP` unconditionally trusts `X-Forwarded-For` and `X-Real-IP`.
|
||||
- **Impact**:
|
||||
- Any client can spoof its IP and bypass per-client `max_requests_per_client` limits.
|
||||
- In environments with a real reverse proxy this is fine; in direct or partially proxied setups it is a DoS vector.
|
||||
- **Affected Files**:
|
||||
- `steamcache/steamcache.go` (getClientIP and getOrCreateClientLimiter)
|
||||
- `config/config.go` (new settings)
|
||||
- `cmd/root.go`
|
||||
- **Approach**:
|
||||
1. Add config options:
|
||||
- `trusted_proxies: []string` (CIDR list) or `trust_x_forwarded_for: bool`
|
||||
- Default should be conservative (`false` or empty list).
|
||||
2. When not trusting forwarded headers, fall back strictly to `r.RemoteAddr`.
|
||||
3. When trusting, implement proper "rightmost trusted proxy" logic (do not just take the first XFF entry blindly).
|
||||
4. Document the security implications clearly in README.
|
||||
- **Acceptance Criteria**:
|
||||
- Default behavior is safe (does not trust arbitrary XFF).
|
||||
- When `trusted_proxies` is configured, correct client IP is extracted.
|
||||
- Spoofing tests exist (or at least negative tests).
|
||||
- Per-client semaphore still works correctly.
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Medium (3-5 hours including tests + docs)
|
||||
|
||||
### P1-03: Implement real LFU or remove the false claim; make "hybrid" meaningful
|
||||
|
||||
- **Description**:
|
||||
- `EvictLFU` just calls `EvictBySizeAsc` (smallest first) with a TODO comment.
|
||||
- `EvictHybrid` is literally just `EvictLRU`.
|
||||
- Documentation in README and config examples heavily advertises these algorithms.
|
||||
- **Impact**: Users who select `lfu` or `hybrid` get behavior they did not ask for. This is misleading and can produce worse cache hit rates than expected.
|
||||
- **Affected Files**:
|
||||
- `vfs/eviction/eviction.go`
|
||||
- `vfs/memory/memory.go` (EvictLFU / EvictHybrid methods if they exist)
|
||||
- `vfs/disk/disk.go`
|
||||
- README.md (GC algorithm section)
|
||||
- Possibly `config/config.go` comments
|
||||
- **Approach** (two options — pick one):
|
||||
**Option A (Recommended for P1)**: Implement a real (approximate) LFU using the existing `AccessCount` field already present in `FileInfo`.
|
||||
**Option B**: Remove the non-functional choices from the public API and docs for now; keep only algorithms that actually do something different (LRU, FIFO, largest, smallest). Re-introduce LFU later under P2.
|
||||
- **Acceptance Criteria**:
|
||||
- Selecting `lfu` either does real LFU or is rejected at config validation time with a clear message.
|
||||
- "hybrid" either has a documented size+recency policy or is removed.
|
||||
- Unit tests exist that demonstrate different eviction behavior between the algorithms under controlled access patterns.
|
||||
- **Dependencies**: P0-03 (so invalid algorithm names are caught early)
|
||||
- **Effort**: Medium (if implementing real LFU: 4-6 hours; if removing: 1-2 hours)
|
||||
|
||||
### P1-04: Decide the fate of the adaptive/predictive caching subsystem
|
||||
|
||||
- **Description**: Large amounts of code (`vfs/adaptive/`, `vfs/predictive/`, plus fields and goroutines in `SteamCache`) collect access patterns but never actually change eviction strategy, promotion decisions, or GC algorithm at runtime.
|
||||
- **Impact**:
|
||||
- Wasted memory and CPU (multiple background analyzers + maps).
|
||||
- Increased goroutine count and shutdown complexity.
|
||||
- False advertising in README ("adaptive and predictive caching").
|
||||
- Maintenance burden for code that provides zero user value today.
|
||||
- **Affected Files**:
|
||||
- `vfs/adaptive/adaptive.go`
|
||||
- `vfs/predictive/predictive.go`
|
||||
- `steamcache/steamcache.go` (record* methods, manager fields, New, Shutdown)
|
||||
- `vfs/cache/cache.go` (promotion decisions)
|
||||
- **Approach** (choose one):
|
||||
1. **Prune (fast)**: Remove the unused subsystems, the recording calls, and all related goroutines/fields. Update docs. Keep the data structures in `types.FileInfo` if they are still useful for future work.
|
||||
2. **Integrate (larger)**: Wire the analyzers into actual decisions (e.g., switch promotion aggressiveness, temporarily bias toward LFU-style scoring, pre-warm on predicted sequences). This is a P2-level project.
|
||||
- **Acceptance Criteria** (for prune path):
|
||||
- No more goroutines or memory overhead from these packages at runtime.
|
||||
- `Shutdown` becomes simpler.
|
||||
- README no longer claims adaptive/predictive behavior that does not exist.
|
||||
- If kept for future, the packages are clearly marked "experimental / not yet active".
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Prune = 2-4 hours. Full integration = multi-day project (defer to P2).
|
||||
|
||||
## Definition of Done (P1 Milestone)
|
||||
|
||||
- [ ] P1-01 (streaming/bounded bodies) implemented and load-tested.
|
||||
- [ ] P1-02 (client IP trust) implemented with safe defaults + documentation.
|
||||
- [ ] P1-03 (LFU/hybrid truthfulness) resolved (either real impl or removal + doc fixes).
|
||||
- [ ] P1-04 (adaptive/predictive) decided and executed (prune is acceptable for P1).
|
||||
- [ ] All changes have accompanying tests (unit + at least one integration test per major feature).
|
||||
- [ ] `go test -race ./...` and manual long-running soak (with induced large responses and spoofed headers) pass.
|
||||
- [ ] README and any user-facing docs are updated to reflect reality (no more over-claiming).
|
||||
|
||||
## Notes for Implementers
|
||||
|
||||
- P1-01 is the highest leverage item for stability under real-world (or adversarial) traffic.
|
||||
- When implementing streaming writes, be careful with the current VFS `Create(key, declaredSize)` contract — it may need adjustment.
|
||||
- Consider adding a `max_object_size` config knob as part of P1-01.
|
||||
|
||||
## References
|
||||
|
||||
- Original full code review
|
||||
- `steamcache/steamcache.go:1506` (io.ReadAll)
|
||||
- `vfs/cache/cache.go:206` (promotion ReadAll)
|
||||
- `vfs/eviction/eviction.go:82` (LFU TODO)
|
||||
- Large unused packages in `vfs/adaptive` and `vfs/predictive`
|
||||
|
||||
---
|
||||
|
||||
**After P1**: The service should be safe to expose to untrusted Steam clients on a LAN with reasonable resource protections.
|
||||
+175
@@ -0,0 +1,175 @@
|
||||
# P2: Performance, Quality & Maintainability Improvements
|
||||
|
||||
**Priority**: P2 — Performance, refactoring, and long-term health
|
||||
**Theme**: Make the codebase faster, smaller, easier to reason about, and production-operable at scale.
|
||||
**Status**: Not started
|
||||
**Depends on**: P0 strongly recommended; P1 nice-to-have for some verification steps
|
||||
|
||||
## Goal
|
||||
|
||||
Turn a clever but monolithic prototype into a high-quality, maintainable Go project that is pleasant to work on and easy to operate.
|
||||
|
||||
## Overview
|
||||
|
||||
After the critical stability (P0) and safety (P1) work, the project still carries technical debt that affects:
|
||||
|
||||
- Runtime performance under sustained load (lock contention, unnecessary copies)
|
||||
- Developer velocity (huge source file, magic numbers, copy-paste)
|
||||
- Operational visibility (weak metrics, no benchmarks, incomplete CI)
|
||||
- Correctness confidence (very low test coverage on the storage layer)
|
||||
|
||||
These items are important for long-term success but are not immediate crash or security risks.
|
||||
|
||||
## Tasks
|
||||
|
||||
### P2-01: Refactor the monolithic `ServeHTTP` and split `steamcache/steamcache.go`
|
||||
|
||||
- **Description**: The core request handler is a single ~500+ line function with many responsibilities (authz, rate limiting, coalescing, upstream fetch, cache write, metrics, adaptive recording). The file itself is 1724 lines.
|
||||
- **Impact**:
|
||||
- Extremely hard to test individual behaviors in isolation.
|
||||
- High risk of regression when touching any part of request handling.
|
||||
- New contributors are intimidated.
|
||||
- **Affected Files**:
|
||||
- `steamcache/steamcache.go` (primary)
|
||||
- Potentially new files: `steamcache/handler.go`, `steamcache/coalescing.go`, `steamcache/upstream.go`, `steamcache/response.go`, etc.
|
||||
- **Approach**:
|
||||
1. Extract clear types for the request context (e.g. `requestContext` holding clientIP, cacheKey, service, timing, etc.).
|
||||
2. Break `ServeHTTP` into smaller focused methods: `handleCacheHit`, `handleCoalesced`, `fetchAndCache`, `writeCacheEntry`, etc.
|
||||
3. Move pure helper logic (range parsing, response reconstruction, hash generation) into separate small files if they aren't already.
|
||||
4. Keep the `SteamCache` struct as the coordinator but reduce its god-object nature over time.
|
||||
- **Acceptance Criteria**:
|
||||
- No single function in the package > 150 lines.
|
||||
- `ServeHTTP` itself becomes a thin dispatcher (< 80 lines).
|
||||
- All existing behavior (including edge cases around coalescing + errors + Ranges) still passes the test suite.
|
||||
- New unit tests become feasible for the extracted pieces.
|
||||
- **Dependencies**: None (can be done in parallel with other P2 work)
|
||||
- **Effort**: Large (8-16 hours). Best done as a series of small, reviewable refactors rather than one giant PR.
|
||||
|
||||
### P2-02: Reduce lock contention during eviction
|
||||
|
||||
- **Description**: `EvictLRU`, `EvictBySize`, etc. take the global `mu.Lock()` on the entire `MemoryFS`/`DiskFS` for the duration of the scan + deletion loop.
|
||||
- **Impact**: Under cache pressure (very common when the disk cache fills), all other operations (Open, Stat, Create) serialize behind the eviction. This can cause request latency spikes even for hot memory-tier hits.
|
||||
- **Affected Files**:
|
||||
- `vfs/memory/memory.go` (eviction methods)
|
||||
- `vfs/disk/disk.go` (eviction methods)
|
||||
- **Approach** options:
|
||||
1. Collect candidates under read lock, then do the actual deletes and size updates in a second phase or in small batches while briefly acquiring write locks.
|
||||
2. Move eviction into a dedicated background goroutine that the GC layer signals, using finer-grained coordination.
|
||||
3. Use a "generation" or "watermark" approach so readers can proceed while eviction cleans up.
|
||||
- **Acceptance Criteria**:
|
||||
- Benchmark or load test shows improved tail latencies for `Open`/`Stat` while eviction is running.
|
||||
- No data races introduced (race detector clean).
|
||||
- Total size and LRU invariants remain correct after concurrent eviction.
|
||||
- **Dependencies**: Good test coverage on the VFS layer (see P2-04)
|
||||
- **Effort**: Medium-Large (4-8 hours + measurement)
|
||||
|
||||
### P2-03: Dramatically improve test coverage on the VFS and storage layer
|
||||
|
||||
- **Description**: Most `vfs/*` packages currently have 0% coverage. The critical storage, eviction, GC, and tiering logic is almost untested in isolation.
|
||||
- **Impact**:
|
||||
- Very low confidence when changing eviction, promotion, or GC behavior.
|
||||
- Hard to catch regressions in size accounting, LRU ordering, or sharded locking.
|
||||
- Blocks safe execution of P2-02 and future performance work.
|
||||
- **Affected Areas** (need new or expanded tests):
|
||||
- `vfs/memory/*`
|
||||
- `vfs/disk/*`
|
||||
- `vfs/gc/*`
|
||||
- `vfs/cache/*`
|
||||
- `vfs/eviction/*`
|
||||
- `vfs/locks/*` and `vfs/lru/*` (at least basic)
|
||||
- **Approach**:
|
||||
1. Write focused unit tests for each VFS implementation using `t.TempDir` for disk.
|
||||
2. Add property-style or table-driven tests that verify size never exceeds capacity after many Create + Evict cycles.
|
||||
3. Test concurrent Create/Open/Delete/Delete under load (with `-race`).
|
||||
4. Test promotion, tier fallback, and lazy discovery paths.
|
||||
5. Add benchmarks (`BenchmarkMemoryFS_CreateOpen`, `BenchmarkEvictionUnderPressure`, etc.).
|
||||
- **Acceptance Criteria**:
|
||||
- Combined coverage for all `vfs/*` packages ≥ 70% (statement).
|
||||
- At least one benchmark per major component that can be run in CI or locally.
|
||||
- New tests catch at least one real bug during development (celebrated in commit message).
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Large (12-20 hours spread over multiple sessions). High leverage.
|
||||
|
||||
### P2-04: Clean up build, CI, linting, and repository hygiene
|
||||
|
||||
- **Description**:
|
||||
- Makefile `run` / `run-debug` targets are hardcoded to a Windows binary path.
|
||||
- `dist/` artifacts are committed even though `.gitignore` lists `/dist/`.
|
||||
- No golangci-lint, no `go vet` in CI, no vulnerability scanning.
|
||||
- Test target exists but coverage reporting and gates are missing.
|
||||
- **Impact**: Painful local development on non-Windows machines. Risk of shipping known-bad artifacts. Harder to maintain code quality over time.
|
||||
- **Affected Files**:
|
||||
- `Makefile`
|
||||
- `.gitea/workflows/test-pr.yaml` (and release workflow)
|
||||
- `.gitignore` (verify dist is truly ignored)
|
||||
- Possibly add `.golangci.yml`
|
||||
- **Approach**:
|
||||
1. Fix Makefile to use `go run .` or build a platform-appropriate binary.
|
||||
2. Add a `make lint` target and wire golangci-lint (with reasonable defaults + errcheck, gosec, etc.).
|
||||
3. Update Gitea workflows to run `go vet`, lint, and (optionally) `govulncheck`.
|
||||
4. Remove any committed files under `dist/` (or add them to `.gitignore` more aggressively and git-rm them).
|
||||
5. Consider adding a coverage report step (even if not enforcing a hard gate yet).
|
||||
- **Acceptance Criteria**:
|
||||
- `make test` and `make run` work cleanly on Linux and macOS.
|
||||
- CI runs lint + vet and fails the PR on new issues.
|
||||
- Repository no longer contains built binaries in its tree.
|
||||
- `go mod tidy` + build is reproducible.
|
||||
- **Dependencies**: None
|
||||
- **Effort**: Small-Medium (3-5 hours)
|
||||
|
||||
### P2-05: Use the existing rich error types consistently and improve observability
|
||||
|
||||
- **Description**: A nice `steamcache/errors` package with context, unwrap, retry classification, and client/server error helpers exists but is almost unused. Metrics are still very basic.
|
||||
- **Impact**:
|
||||
- Lost opportunity for better structured logging and error handling.
|
||||
- Harder to write generic retry / circuit-breaker logic later.
|
||||
- `/metrics` and logs give limited insight into what actually failed and why.
|
||||
- **Approach**:
|
||||
1. Audit the top 10-15 error sites in `ServeHTTP` and VFS layers.
|
||||
2. Convert the most important ones to use `NewSteamCacheError*` helpers.
|
||||
3. Wire more structured fields into zerolog calls using the error types.
|
||||
4. Expand the metrics package (per-service error counts, upstream error breakdown, cache write failures, etc.).
|
||||
5. Consider exporting Prometheus-style metrics in addition to the current text format (optional).
|
||||
- **Acceptance Criteria**:
|
||||
- At least the major error categories (upstream fetch, cache corruption, rate limit, validation) use the custom error types.
|
||||
- `/metrics` surface becomes more useful (new counters for categories of errors).
|
||||
- No behavior change for clients (still get the same HTTP status codes).
|
||||
- **Dependencies**: P0-04 (basic error counting)
|
||||
- **Effort**: Medium (4-6 hours)
|
||||
|
||||
## Definition of Done (P2 Milestone)
|
||||
|
||||
- [ ] Major refactoring (P2-01) landed in reviewable chunks; `ServeHTTP` is no longer a monster function.
|
||||
- [ ] Eviction lock contention measurably reduced (P2-02).
|
||||
- [ ] VFS/storage layer has ≥70% test coverage + benchmarks (P2-03).
|
||||
- [ ] Build/CI hygiene is excellent: cross-platform make targets, lint in CI, clean repo (P2-04).
|
||||
- [ ] Error handling and metrics are noticeably better (P2-05).
|
||||
- [ ] `go test -race -shuffle=on ./...` + `go vet` + linter are all green.
|
||||
- [ ] A new contributor can run `make help`, `make test`, `make run` on Linux/macOS without friction.
|
||||
- [ ] At least one performance or quality regression has been prevented by the new tests/benchmarks during the work.
|
||||
|
||||
## Suggested Order of Execution (within P2)
|
||||
|
||||
1. P2-04 (CI hygiene) — cheap wins, improves confidence for everything else.
|
||||
2. P2-03 (test coverage on VFS) — unblocks safe work on P2-02.
|
||||
3. P2-02 (eviction locking).
|
||||
4. P2-01 (big refactor) — do this when the test safety net is stronger.
|
||||
5. P2-05 (errors + observability) — can run in parallel with others.
|
||||
|
||||
## Notes for Implementers
|
||||
|
||||
- P2 work has the highest risk of "refactoring for its own sake." Every change should be justified by either a concrete performance win, a maintainability win that reduces future bug rate, or enabling future features.
|
||||
- Keep changes reviewable. Large refactors should be broken into multiple PRs with clear "no behavior change" invariants.
|
||||
- The custom LRU list and sharded locking are clever — make sure any refactoring preserves their performance characteristics.
|
||||
|
||||
## References
|
||||
|
||||
- `steamcache/steamcache.go:1724` (file size)
|
||||
- `vfs/disk/disk.go` and `vfs/memory/memory.go` eviction methods (global lock held)
|
||||
- `.gitea/workflows/`
|
||||
- `Makefile`
|
||||
- `steamcache/errors/errors.go` (under-used)
|
||||
|
||||
---
|
||||
|
||||
**After P2**: The project should feel like a mature, professional Go service rather than a sophisticated prototype.
|
||||
@@ -0,0 +1,41 @@
|
||||
# SteamCache2 Improvement Plans
|
||||
|
||||
This directory contains prioritized, actionable plans extracted from the full code review.
|
||||
|
||||
Use these files as the source of truth for future implementation work (via `/implement`, manual PRs, or issue tracking).
|
||||
|
||||
## Files
|
||||
|
||||
| File | Focus Area | Risk Level | Recommended Order |
|
||||
|--------|-----------------------------------|-----------------|-------------------|
|
||||
| [P0.md](./P0.md) | Critical stability, crashes, leaks, startup validation | Ship-blocking | **First** |
|
||||
| [P1.md](./P1.md) | Hardening, security, correctness, resource safety | High | After P0 |
|
||||
| [P2.md](./P2.md) | Performance, refactoring, test coverage, maintainability | Medium | After P0/P1 |
|
||||
|
||||
## How to Use These Plans
|
||||
|
||||
1. Start with **P0** items — they are prerequisites for safe operation.
|
||||
2. Each file contains:
|
||||
- Concrete numbered tasks (P0-01, P1-03, etc.)
|
||||
- Impact, affected files, suggested approach, and acceptance criteria
|
||||
- Effort estimates
|
||||
- Definition of Done for the whole milestone
|
||||
3. Many tasks are designed to be small enough for focused PRs or subagent implementation sessions.
|
||||
|
||||
## Status Tracking (suggested)
|
||||
|
||||
You may add a simple checkbox table here or in each file as work progresses:
|
||||
|
||||
- [ ] P0-01 completed
|
||||
- [ ] P0-02 completed
|
||||
- ...
|
||||
|
||||
## Related
|
||||
|
||||
- Full original code review (in conversation history)
|
||||
- `Makefile` (contains `test`, `test-race`, etc.)
|
||||
- `.gitea/workflows/` (current CI)
|
||||
|
||||
---
|
||||
|
||||
**Ready for execution.** Pick any item from P0 and go.
|
||||
Reference in New Issue
Block a user