905c241daa
- Fix metrics: change MessagesTotal, ConnectionsTotal, DisconnectionsTotal from Gauge to Counter with proper _total naming convention - Fix broadcast write-error handling: failed clients now get properly removed with accurate metrics updates - Add graceful shutdown: SIGINT/SIGTERM handling with 10s timeout, CloseGoingAway frame sent to clients before disconnect - Add integration tests: 11 tests using real WebSocket connections covering connect, broadcast, disconnect, concurrency, and shutdown - Fix example client port: changed from 8000 to 8443 to match config - Rewrite README.md to reflect current features and usage - Add AGENTS.md and .agents/summary/ documentation for AI assistants
76 lines
3.6 KiB
Markdown
76 lines
3.6 KiB
Markdown
# Documentation Review Notes
|
|
|
|
## Consistency Check Results
|
|
|
|
### ✅ Consistent
|
|
|
|
- Configuration documented in `data_models.md` matches actual `config.go` struct
|
|
- Hub methods documented in `components.md` match actual implementation
|
|
- CLI flags documented in `interfaces.md` match `main.go`
|
|
- Build commands in `codebase_info.md` match `Makefile`
|
|
|
|
### ⚠️ Inconsistencies Found
|
|
|
|
| Issue | Location | Details |
|
|
|-------|----------|---------|
|
|
| **Port mismatch in example client** | `example/index.html` | Uses `ws://localhost:8000/` but config default is port `8443` |
|
|
| **Metrics type mismatch** | `internal/metrics/metrics.go` | `MessagesTotal`, `ConnectionsTotal`, `DisconnectionsTotal` are defined as `Gauge` but semantically represent counters (monotonically increasing values). Should be `Counter` type. |
|
|
| **Silent client removal** | `internal/hub/hub.go` | During broadcast, write errors cause client removal without going through the `unregister` channel, meaning `DisconnectionsTotal` and `ConnectedClients` metrics won't be updated correctly. |
|
|
| **README port reference** | `README.md` | Mentions TLS is enabled by default, but `config.yaml` has `tls.enabled: false` |
|
|
|
|
---
|
|
|
|
## Completeness Check Results
|
|
|
|
### ✅ Well-Documented Areas
|
|
|
|
- Core WebSocket relay logic
|
|
- Configuration structure and loading
|
|
- Build and deployment pipeline
|
|
- Metrics definitions
|
|
|
|
### ❌ Documentation Gaps
|
|
|
|
| Gap | Severity | Recommendation |
|
|
|-----|----------|----------------|
|
|
| **No graceful shutdown** | Medium | Document that the server lacks graceful shutdown — connections are terminated abruptly on SIGTERM |
|
|
| **No rate limiting** | Medium | Document absence of rate limiting and implications for production use |
|
|
| **No message size limits** | Medium | No `ReadLimit` set on WebSocket connections — potential DoS vector |
|
|
| **No health check endpoint** | Low | No `/health` or `/ready` endpoint for orchestrators |
|
|
| **No connection limits** | Medium | No max client count — server could be overwhelmed |
|
|
| **No logging configuration** | Low | Uses default `log` package with no level control |
|
|
| **No deployment docs** | Medium | No systemd unit file, Docker instructions, or k8s manifests |
|
|
| **Missing test coverage** | Medium | `internal/metrics` has no tests; hub integration tests (actual WebSocket connections) missing |
|
|
|
|
---
|
|
|
|
## Language Support
|
|
|
|
| Aspect | Support Level | Notes |
|
|
|--------|--------------|-------|
|
|
| Go source analysis | ✅ Full | All Go code fully analyzed |
|
|
| HTML/JS (example) | ✅ Full | Simple single-file client analyzed |
|
|
| YAML configs | ✅ Full | Configuration fully documented |
|
|
| Makefile | ✅ Full | All targets documented |
|
|
| Gitea Actions YAML | ✅ Full | CI/CD pipelines documented |
|
|
|
|
---
|
|
|
|
## Recommendations
|
|
|
|
### High Priority
|
|
1. **Fix metrics types**: Change `MessagesTotal`, `ConnectionsTotal`, `DisconnectionsTotal` from `Gauge` to `Counter`
|
|
2. **Fix broadcast disconnect handling**: Route write-error disconnections through the unregister channel to maintain accurate metrics
|
|
3. **Add message size limits**: Set `conn.SetReadLimit()` to prevent memory exhaustion
|
|
|
|
### Medium Priority
|
|
4. **Add graceful shutdown**: Use `context.Context` and `http.Server.Shutdown()`
|
|
5. **Add health endpoint**: Simple `/health` returning 200 OK
|
|
6. **Add integration tests**: Test actual WebSocket connections end-to-end
|
|
7. **Fix example port**: Update `example/index.html` to use port 8443
|
|
|
|
### Low Priority
|
|
8. **Add structured logging**: Replace `log` with `slog` or `zerolog`
|
|
9. **Add connection limits**: Max concurrent connections configuration
|
|
10. **Add Docker support**: Dockerfile and docker-compose for easy deployment
|