Agent-Hellboy / Agent-Hellboy/mcp-runtime
Review follow-up: shutdown data loss, silent audit drops, and unbounded login-attempt state
- 主要言語
- Go
- スター
- 6
- フォーク
- 1
- 平均マージ
- 11時間 33分
- マージ済み PR(30日)
- 13
説明
## Summary
A static code review of the current repository found four operational correctness issues in the runtime services:
1. `services/processor` can drop the final in-memory audit batch during normal shutdown.
2. `services/mcp-proxy` silently drops analytics/audit events when its bounded queue fills.
3. `services/mcp-proxy` has no graceful shutdown path, so queued analytics and deferred cleanup are skipped on `SIGTERM`.
4. The API and UI login throttlers retain attacker-controlled keys indefinitely, allowing unbounded memory growth.
## Findings
### 1. Processor shutdown can lose the final batch
The shutdown path in `services/processor/main.go` calls `flush()` after the root context has already been canceled by `signal.NotifyContext`.
Relevant code:
- `services/processor/main.go:199`
- `services/processor/main.go:144`
- `services/processor/main.go:152`
Impact:
- On a normal rollout or pod termination, the final buffered ClickHouse batch can fail to insert.
- The process then exits, dropping the tail of the audit/event stream.
Suggested fix:
- Use a fresh timeout-bounded shutdown context for the final flush and final Kafka commit path instead of reusing the canceled root context.
### 2. Proxy silently drops audit events under load
`emitIfEnabled` sends into a bounded queue and silently discards on overflow.
Relevant code:
- `services/mcp-proxy/main.go:1304`
- `services/mcp-proxy/main.go:1312`
Impact:
- Bursty traffic or a temporary ingest slowdown can create invisible gaps in allow/deny audit records.
- This is especially risky for governance/compliance use cases because the loss is silent.
Suggested fix:
- At minimum, count and log dropped events.
- Prefer a backpressure/retry strategy or a bounded loss policy with explicit metrics.
### 3. Proxy lacks graceful shutdown
`services/mcp-proxy` blocks directly on `ListenAndServe()` and only stops the analytics dispatcher if that call returns.
Relevant code:
- `services/mcp-proxy/main.go:227`
- `services/mcp-proxy/main.go:237`
- `services/mcp-proxy/main.go:1295`
Impact:
- On Kubernetes `SIGTERM`, the proxy does not run a coordinated shutdown path.
- In-flight requests, queued analytics, and deferred cleanup can be lost on routine restarts/rollouts.
Suggested fix:
- Mirror the other services: use `signal.NotifyContext`, call `http.Server.Shutdown`, and drain/stop the analytics dispatcher explicitly during shutdown.
### 4. Login-attempt trackers can grow without bound
Both the API and UI login throttlers keep attacker-controlled keys in maps with no eviction.
Relevant code:
- API tracker structure: `services/api/platform_auth.go:44`
- API key creation/use path: `services/api/platform_auth.go:300`
- UI tracker structure: `services/ui/main.go:687`
- UI key creation/use path: `services/ui/main.go:333`
Impact:
- Repeated attempts with random emails or client identifiers can grow these maps indefinitely.
- This is a straightforward memory-growth / low-grade DoS vector on public login surfaces.
Suggested fix:
- Add TTL-based eviction or periodic sweeping for idle/expired entries.
- Consider capping retained state and discarding stale clients aggressively.
## Notes
This issue comes from a repository-wide static review. I was not able to complete local `go test` execution in the current sandbox because the repository requires Go `1.26.0` and toolchain download is blocked in that environment, so the issue is based on code inspection rather than a reproducer run.
コントリビューションガイド
このリポジトリのコントリビューションガイドは索引されていません
評価
この issue はまだ評価されていません。