fix: API quality fixes from code review (HAN-398) #17

Merged
multica-agent merged 1 commits from fix/han-398-api-quality into main 2026-06-07 18:52:20 +00:00
2 changed files with 43 additions and 19 deletions
Showing only changes of commit a4d7b1514e - Show all commits
+28 -11
View File
@@ -86,10 +86,8 @@ func (j *TraversalJob) subscribe() <-chan ProgressEvent {
return ch return ch
} }
// publish sends an event to all current subscribers. // publishLocked sends ev to all current subscribers. Caller must hold j.mu.
func (j *TraversalJob) publish(ev ProgressEvent) { func (j *TraversalJob) publishLocked(ev ProgressEvent) {
j.mu.Lock()
defer j.mu.Unlock()
for _, ch := range j.subs { for _, ch := range j.subs {
select { select {
case ch <- ev: case ch <- ev:
@@ -99,6 +97,18 @@ func (j *TraversalJob) publish(ev ProgressEvent) {
} }
} }
// unsubscribe removes ch from the subscriber list.
func (j *TraversalJob) unsubscribe(ch <-chan ProgressEvent) {
j.mu.Lock()
defer j.mu.Unlock()
for i, s := range j.subs {
if s == ch {
j.subs = append(j.subs[:i], j.subs[i+1:]...)
return
}
}
}
// closeSubscribers drains and closes all subscriber channels. // closeSubscribers drains and closes all subscriber channels.
func (j *TraversalJob) closeSubscribers() { func (j *TraversalJob) closeSubscribers() {
j.mu.Lock() j.mu.Lock()
@@ -153,7 +163,7 @@ type Handler struct {
mux *http.ServeMux mux *http.ServeMux
} }
func newHandler() *Handler { func newHandler(ctx context.Context) *Handler {
h := &Handler{ h := &Handler{
st: newStore(), st: newStore(),
mux: http.NewServeMux(), mux: http.NewServeMux(),
@@ -164,12 +174,17 @@ func newHandler() *Handler {
h.mux.HandleFunc("GET /api/traverse/{id}", h.getTraversal) h.mux.HandleFunc("GET /api/traverse/{id}", h.getTraversal)
h.mux.HandleFunc("GET /api/health", h.health) h.mux.HandleFunc("GET /api/health", h.health)
// periodic cleanup // periodic cleanup; exits when ctx is cancelled (e.g. on Server.Shutdown).
go func() { go func() {
t := time.NewTicker(10 * time.Minute) t := time.NewTicker(10 * time.Minute)
defer t.Stop() defer t.Stop()
for range t.C { for {
h.st.cleanup() select {
case <-ctx.Done():
return
case <-t.C:
h.st.cleanup()
}
} }
}() }()
@@ -189,6 +204,7 @@ func (h *Handler) health(w http.ResponseWriter, _ *http.Request) {
// startTraversal handles POST /api/traverse. // startTraversal handles POST /api/traverse.
func (h *Handler) startTraversal(w http.ResponseWriter, r *http.Request) { func (h *Handler) startTraversal(w http.ResponseWriter, r *http.Request) {
r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1 MB limit
var req TraverseRequest var req TraverseRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil { if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error()) writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error())
@@ -285,8 +301,10 @@ func (h *Handler) streamTraversal(w http.ResponseWriter, r *http.Request) {
} }
// Subscribe before snapshotting progress so we don't miss events between // Subscribe before snapshotting progress so we don't miss events between
// the two operations. // the two operations. Unsubscribe when the client disconnects so stale
// channels don't accumulate.
sub := job.subscribe() sub := job.subscribe()
defer job.unsubscribe(sub)
// Replay events already recorded. // Replay events already recorded.
job.mu.RLock() job.mu.RLock()
@@ -381,9 +399,8 @@ func (h *Handler) runTraversal(ctx context.Context, job *TraversalJob, domain st
job.mu.Lock() job.mu.Lock()
job.Progress = append(job.Progress, ev) job.Progress = append(job.Progress, ev)
job.publishLocked(ev) // inside lock: no race with subscribe+replay
job.mu.Unlock() job.mu.Unlock()
job.publish(ev)
}, },
} }
+15 -8
View File
@@ -26,8 +26,9 @@ var staticFiles embed.FS
// Server is the HTTP API server. // Server is the HTTP API server.
type Server struct { type Server struct {
addr string addr string
srv *http.Server srv *http.Server
cancel context.CancelFunc
} }
// NewServer creates a new Server that listens on addr (e.g. ":8080"). // NewServer creates a new Server that listens on addr (e.g. ":8080").
@@ -39,7 +40,9 @@ func NewServer(addr string) *Server {
// server has accepted its first connection or the address is bound. // server has accepted its first connection or the address is bound.
// Call Shutdown to stop gracefully. // Call Shutdown to stop gracefully.
func (s *Server) Start() error { func (s *Server) Start() error {
h := newHandler() ctx, cancel := context.WithCancel(context.Background())
s.cancel = cancel
h := newHandler(ctx)
sub, err := fs.Sub(staticFiles, "static") sub, err := fs.Sub(staticFiles, "static")
if err != nil { if err != nil {
@@ -48,11 +51,12 @@ func (s *Server) Start() error {
h.registerStatic(sub) h.registerStatic(sub)
s.srv = &http.Server{ s.srv = &http.Server{
Addr: s.addr, Addr: s.addr,
Handler: corsMiddleware(h.mux), Handler: corsMiddleware(h.mux),
ReadTimeout: 30 * time.Second, ReadHeaderTimeout: 10 * time.Second,
WriteTimeout: 0, // SSE streams need no write timeout ReadTimeout: 30 * time.Second,
IdleTimeout: 120 * time.Second, WriteTimeout: 0, // SSE streams need no write timeout
IdleTimeout: 120 * time.Second,
} }
ln, err := net.Listen("tcp", s.addr) ln, err := net.Listen("tcp", s.addr)
@@ -78,6 +82,9 @@ func (s *Server) Addr() string {
// Shutdown gracefully stops the server, waiting up to timeout for in-flight // Shutdown gracefully stops the server, waiting up to timeout for in-flight
// requests to complete. // requests to complete.
func (s *Server) Shutdown(timeout time.Duration) error { func (s *Server) Shutdown(timeout time.Duration) error {
if s.cancel != nil {
s.cancel()
}
if s.srv == nil { if s.srv == nil {
return nil return nil
} }