From 29666530cc4d2edc2efabfea1203ac33e7351d10 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Apr 2026 05:52:24 +0000 Subject: [PATCH] Fix four bugs: packfile offset tracking, HTTP error ordering, ref ordering, and race condition - packfile.go: ReadObject now uses a countingReader to track compressed bytes consumed and properly advances r.offset, fixing sequential multi-object reads. - handlers.go: Use commitSHA from GenerateCommit directly instead of re-reading refs, which fixes both the HTTP error-after-body-written issue and ensures HEAD is always advertised first (Git protocol requirement). - commit.go: GenerateCommit now holds the repo lock for the entire read-modify-write cycle, preventing concurrent generates from reading the same parent and losing ref updates. - repo.go: Added Lock/Unlock/GetRefsLocked methods to support holding the mutex across multiple repo operations. https://claude.ai/code/session_015F9BYQoCy2P2zYZBuVeXHH --- internal/generator/commit.go | 12 ++++++++++-- internal/packfile/packfile.go | 29 +++++++++++++++++++--------- internal/repo/repo.go | 19 ++++++++++++++++++ internal/server/handlers.go | 36 +++++++++++------------------------ 4 files changed, 60 insertions(+), 36 deletions(-) diff --git a/internal/generator/commit.go b/internal/generator/commit.go index db4141c..365a97a 100644 --- a/internal/generator/commit.go +++ b/internal/generator/commit.go @@ -23,12 +23,20 @@ func New(r *repo.Repository) *Generator { } // GenerateCommit creates a new commit and updates the main branch. +// It holds the repo lock for the entire read-modify-write cycle to +// prevent concurrent generates from reading the same parent. func (g *Generator) GenerateCommit() (string, error) { // Increment counter atomically count := atomic.AddInt64(&g.counter, 1) - // Get current HEAD commit - refs, err := g.repo.GetRefs() + // Hold the repo lock for the entire operation to prevent races. + g.repo.Lock() + defer g.repo.Unlock() + + // Get current HEAD commit (use exported method is fine since + // getRefs is called internally, but we already hold the lock, + // so we call the unexported version via GetRefsLocked). + refs, err := g.repo.GetRefsLocked() if err != nil { return "", fmt.Errorf("getting refs: %w", err) } diff --git a/internal/packfile/packfile.go b/internal/packfile/packfile.go index c06efca..cbe405e 100644 --- a/internal/packfile/packfile.go +++ b/internal/packfile/packfile.go @@ -147,8 +147,9 @@ func (r *Reader) ReadObject() (objType int, data []byte, err error) { return 0, nil, err } - // Read compressed data - zr, err := zlib.NewReader(bytes.NewReader(r.data[r.offset:])) + // Wrap the remaining data in a counting reader to track compressed bytes consumed. + cr := &countingReader{reader: bytes.NewReader(r.data[r.offset:])} + zr, err := zlib.NewReader(cr) if err != nil { return 0, nil, fmt.Errorf("creating decompressor: %w", err) } @@ -159,13 +160,23 @@ func (r *Reader) ReadObject() (objType int, data []byte, err error) { return 0, nil, fmt.Errorf("decompressing object: %w", err) } - // Update offset past compressed data - // This is tricky - we need to figure out how much compressed data we read - // For now, we'll use a simple approach and skip this optimization - // In a real implementation, we'd track the compressed size properly - // For now, just consume the rest - var buf bytes.Buffer - io.Copy(&buf, zr) + // Drain the zlib reader so cr.n reflects all compressed bytes consumed. + io.Copy(io.Discard, zr) + + // Advance offset past the compressed data. + r.offset += int(cr.n) return objType, data, nil } + +// countingReader wraps an io.Reader and counts bytes read. +type countingReader struct { + reader io.Reader + n int64 +} + +func (c *countingReader) Read(p []byte) (int, error) { + n, err := c.reader.Read(p) + c.n += int64(n) + return n, err +} diff --git a/internal/repo/repo.go b/internal/repo/repo.go index ceb739e..099d31e 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -157,11 +157,30 @@ func (r *Repository) GitDir() string { return r.gitDir } +// Lock acquires the repository mutex. Use this to perform atomic +// read-modify-write operations spanning multiple repo calls. +func (r *Repository) Lock() { r.mu.Lock() } + +// Unlock releases the repository mutex. +func (r *Repository) Unlock() { r.mu.Unlock() } + // GetRefs returns the current refs in the repository. func (r *Repository) GetRefs() (map[string]string, error) { r.mu.Lock() defer r.mu.Unlock() + return r.getRefs() +} + +// GetRefsLocked is the unlocked implementation of GetRefs. +// Caller must already hold r.mu via Lock(). +func (r *Repository) GetRefsLocked() (map[string]string, error) { + return r.getRefs() +} + +// getRefs is the internal unlocked implementation of GetRefs. +// Caller must hold r.mu. +func (r *Repository) getRefs() (map[string]string, error) { refs := make(map[string]string) // Read refs from refs directory diff --git a/internal/server/handlers.go b/internal/server/handlers.go index cfb88d0..608cd5b 100644 --- a/internal/server/handlers.go +++ b/internal/server/handlers.go @@ -22,9 +22,7 @@ func (s *Server) handleInfoRefs(w http.ResponseWriter, r *http.Request) { } // Generate a new commit before advertising refs - s.mu.Lock() commitSHA, err := s.generator.GenerateCommit() - s.mu.Unlock() if err != nil { log.Error("failed to generate commit", "error", err) @@ -51,31 +49,19 @@ func (s *Server) handleInfoRefs(w http.ResponseWriter, r *http.Request) { return } - // Get current refs - refs, err := s.repo.GetRefs() - if err != nil { - log.Error("failed to get refs", "error", err) - http.Error(w, "Internal server error", http.StatusInternalServerError) + // Use the commitSHA directly from GenerateCommit rather than re-reading + // refs. This avoids a race where concurrent requests could all see the + // same latest ref, and ensures HEAD is always advertised first. + capabilities := strings.Join(s.repo.GetCapabilities(), " ") + + // Advertise HEAD first (Git protocol requirement), then refs/heads/main. + if err := pw.Writef("%s HEAD\x00%s\n", commitSHA, capabilities); err != nil { + log.Error("failed to write HEAD ref", "error", err) return } - - // Write capabilities with first ref - capabilities := strings.Join(s.repo.GetCapabilities(), " ") - first := true - - for ref, oid := range refs { - if first { - if err := pw.Writef("%s %s\x00%s\n", oid, ref, capabilities); err != nil { - log.Error("failed to write ref with capabilities", "error", err) - return - } - first = false - } else { - if err := pw.Writef("%s %s\n", oid, ref); err != nil { - log.Error("failed to write ref", "error", err) - return - } - } + if err := pw.Writef("%s refs/heads/main\n", commitSHA); err != nil { + log.Error("failed to write main ref", "error", err) + return } // Final flush