ronantakizawa/codereview-bench huggingface dataset:
Repository: prometheus/prometheus
File: tsdb/db.go (Go)

Code before:
	var dirs []string

	for _, f := range files {
		if isBlockDir(f) {
			dirs = append(dirs, filepath.Join(dir, f.Name()))
		}
	}
	return dirs, nil
}

func exponential(d, minD, maxD time.Duration) time.Duration {
	d *= 2
	if d < minD {
		d = minD
	}
	if d > maxD {
		d = maxD
	}
	return d
}

// CloseAll closes all given closers while recording error in MultiError.
func CloseAll(cs []io.Closer) error {
	var errs error
	for _, c := range cs {
		errs = errors.Join(errs, c.Close())
	}
	return errs
}


Review comment (suggestion):
This should probably collect the errors via `append` then `Join` at the end.

Code after:

// CleanTombstones re-writes any blocks with tombstones.
func (db *DB) CleanTombstones() (err error) {
	db.cmtx.Lock()
	defer db.cmtx.Unlock()

	start := time.Now()
	defer func() {
		db.metrics.tombCleanTimer.Observe(time.Since(start).Seconds())
	}()

	cleanUpCompleted := false
	// Repeat cleanup until there is no tombstones left.
	for !cleanUpCompleted {
		cleanUpCompleted = true

		for _, pb := range db.Blocks() {
			uids, safeToDelete, cleanErr := pb.CleanTombstones(db.Dir(), db.compactor)
			if cleanErr != nil {
				return fmt.Errorf("clean tombstones: %s: %w", pb.Dir(), cleanErr)
			}
			if !safeToDelete {
				// There was nothing to clean.
				continue
			}

			// In case tombstones of the old block covers the whole block,
			// then there would be no resultant block to tell the parent.
			// The lock protects against race conditions when deleting blocks
			// during an already running reload.
			db.mtx.Lock()
			pb.meta.Compaction.Deletable = safeToDelete
			db.mtx.Unlock()
			cleanUpCompleted = false
			if err = db.reloadBlocks(); err == nil { // Will try to delete old block.
				// Successful reload will change the existing blocks.
				// We need to loop over the new set of blocks.
				break
			}

			// Delete new block if it was created.
			for _, uid := range uids {
				dir := filepath.Join(db.Dir(), uid.String())
				if err := os.RemoveAll(dir); err != nil {
					db.logger.Error("failed to delete block after failed `CleanTombstones`", "dir", dir, "err", err)
				}
			}

			// This should only be reached if an error occurred.
			return fmt.Errorf("reload blocks: %w", err)
		}
Repository: prometheus/prometheus
File: tsdb/index/index.go (Go)

Code before:
	idxStageNone indexWriterStage = iota
	idxStageSymbols
	idxStageSeries
	idxStageDone
)

func (s indexWriterStage) String() string {
	switch s {
	case idxStageNone:
		return "none"
	case idxStageSymbols:
		return "symbols"
	case idxStageSeries:
		return "series"
	case idxStageDone:
		return "done"
	}
	return "<unknown>"
}

// ErrPostingsOffsetTableTooLarge is returned when the postings offset table length
// would exceed 4 bytes (table would exceed the 4GB limit).
var ErrPostingsOffsetTableTooLarge = errors.New("length size exceeds 4 bytes")

// ErrIndexExceeds64GiB is returned when the index file would exceed the 64GiB limit.
var ErrIndexExceeds64GiB = errors.New("exceeds max size of 64GiB")

// The table gets initialized with sync.Once but may still cause a race
// with any other use of the crc32 package anywhere. Thus we initialize it
// before.
var castagnoliTable *crc32.Table

func init() {
	castagnoliTable = crc32.MakeTable(crc32.Castagnoli)
}

// newCRC32 initializes a CRC32 hash with a preconfigured polynomial, so the
// polynomial may be easily changed in one location at a later time, if necessary.
func newCRC32() hash.Hash32 {
	return crc32.New(castagnoliTable)
}

type PostingsEncoder func(*encoding.Encbuf, []uint32) error

type PostingsDecoder func(encoding.Decbuf) (int, Postings, error)

// Writer implements the IndexWriter interface for the standard
// serialization format.
type Writer struct {
	ctx context.Context


Review comment (suggestion):
I suggest not changing the error message, to not break anyone string matching it:
```suggestion
var ErrIndexExceeds64GiB = errors.New("exceeding max size of 64GiB")
```

Code after:
	idxStageNone indexWriterStage = iota
	idxStageSymbols
	idxStageSeries
	idxStageDone
)

func (s indexWriterStage) String() string {
	switch s {
	case idxStageNone:
		return "none"
	case idxStageSymbols:
		return "symbols"
	case idxStageSeries:
		return "series"
	case idxStageDone:
		return "done"
	}
	return "<unknown>"
}

// ErrPostingsOffsetTableTooLarge is returned when the postings offset table length
// would exceed 4 bytes (table would exceed the 4GB limit).
var ErrPostingsOffsetTableTooLarge = errors.New("length size exceeds 4 bytes")

// ErrIndexExceeds64GiB is returned when the index file would exceed the 64GiB limit.
var ErrIndexExceeds64GiB = errors.New("exceeding max size of 64GiB")

// The table gets initialized with sync.Once but may still cause a race
// with any other use of the crc32 package anywhere. Thus we initialize it
// before.
var castagnoliTable *crc32.Table

func init() {
	castagnoliTable = crc32.MakeTable(crc32.Castagnoli)
}

// newCRC32 initializes a CRC32 hash with a preconfigured polynomial, so the
// polynomial may be easily changed in one location at a later time, if necessary.
func newCRC32() hash.Hash32 {
	return crc32.New(castagnoliTable)
}

type PostingsEncoder func(*encoding.Encbuf, []uint32) error

type PostingsDecoder func(encoding.Decbuf) (int, Postings, error)

// Writer implements the IndexWriter interface for the standard
// serialization format.
type Writer struct {
	ctx context.Context
Repository: prometheus/prometheus
File: util/teststorage/appender.go (Go)

Code before:

	next storage.Appender
	a    *Appendable
}

func (a *appender) checkErr() error {
	a.a.mtx.Lock()
	defer a.a.mtx.Unlock()

	return a.err
}

func (a *Appendable) Appender(ctx context.Context) storage.Appender {
	ret := &appender{a: a}
	if a.openAppenders.Inc() > 1 {
		ret.err = errors.New("teststorage.Appendable.Appender() concurrent use is not supported; attempted opening new Appender() without Commit/Rollback of the previous one. Extend the implementation if concurrent mock is needed")
	}

	if a.next != nil {
		ret.next = a.next.Appender(ctx)
	} else if a.nextV2 != nil {
		ret.err = errors.Join(ret.err, errors.New("teststorage.Appendable.Appender() invoked with .ThenV2 but no .Then was supplied; likely bug"))
	}
	if a.appendExemplarsError != nil {
		ret.err = errors.Join(ret.err, errors.New("teststorage.Appendable.Appender() invoked with .WithErr exemplar partial error injection. This is not supported for Appender V1 flow"))
	}
	return ret
}

func (*appender) SetOptions(*storage.AppendOptions) {}

func (a *appender) Append(ref storage.SeriesRef, ls labels.Labels, t int64, v float64) (storage.SeriesRef, error) {
	if err := a.checkErr(); err != nil {
		return 0, err
	}

	if a.a.appendErrFn != nil {
		if err := a.a.appendErrFn(ls); err != nil {
			return 0, err
		}
	}

	a.a.mtx.Lock()
	a.a.pendingSamples = append(a.a.pendingSamples, Sample{L: ls, T: t, V: v})
	a.a.mtx.Unlock()

	if a.next != nil {
		return a.next.Append(ref, ls, t, v)
	}

	return computeOrCheckRef(ref, ls)

Review comment (security):
Really? Line 322 says otherwise. I think we can reuse the same error injection for the errors in AppendExemplar and v2 Append.

```suggestion
```

Code after:

	nextTr storage.AppenderTransaction
	a      *Appendable
}

func (a *baseAppender) checkErr() error {
	a.a.mtx.Lock()
	defer a.a.mtx.Unlock()

	return a.err
}

func (a *baseAppender) Commit() error {
	if err := a.checkErr(); err != nil {
		return err
	}
	defer a.a.openAppenders.Dec()

	if a.a.commitErr != nil {
		return a.a.commitErr
	}

	a.a.mtx.Lock()
	a.a.resultSamples = append(a.a.resultSamples, a.a.pendingSamples...)
	a.a.pendingSamples = a.a.pendingSamples[:0]
	a.err = errClosedAppender
	a.a.mtx.Unlock()

	if a.nextTr != nil {
		return a.nextTr.Commit()
	}
	return nil
}

func (a *baseAppender) Rollback() error {
	if err := a.checkErr(); err != nil {
		return err
	}
	defer a.a.openAppenders.Dec()

	a.a.mtx.Lock()
	a.a.rolledbackSamples = append(a.a.rolledbackSamples, a.a.pendingSamples...)
	a.a.pendingSamples = a.a.pendingSamples[:0]
	a.err = errClosedAppender
	a.a.mtx.Unlock()

	if a.nextTr != nil {
		return a.nextTr.Rollback()
	}
	return nil
}
Repository: prometheus/prometheus
File: notifier/manager_test.go (Go)

Code before:
		errc      = make(chan error, 1)
	)

	server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
		// Notify the test function that we have received something.
		select {
		case called <- struct{}{}:
		case <-done:
			return
		}

		// Wait for the test function to unblock us.
		select {
		case expected := <-expectedc:
			var alerts []*Alert

			b, err := io.ReadAll(r.Body)
			if err != nil {
				panic(err)
			}

			err = json.Unmarshal(b, &alerts)
			if err == nil {
				err = alertsEqual(expected, alerts)
			}
			select {
			case errc <- err:
			default:
			}
		case <-done:
		}
	}))
	defer func() {
		close(done)
		server.Close()
	}()

	reg := prometheus.NewRegistry()
	h := NewManager(
		&Options{
			QueueCapacity: 3 * DefaultMaxBatchSize,
			MaxBatchSize:  DefaultMaxBatchSize,
			Registerer:    reg,
		},
		model.UTF8Validation,
		nil,
	)

	h.alertmanagers = make(map[string]*alertmanagerSet)

	am1Cfg := config.DefaultAlertmanagerConfig

Review comment (refactor):
The top check was removed since it is essentially doing the same check that we are doing at the bottom but without blocking which was causing synchronisation issues with the bottom checks.
It is removed as a duplicate.

Code after:
	assertAlerts := func(expected []*Alert) {
		t.Helper()
		for {
			select {
			case <-called:
				expectedc <- expected
			case err := <-errc:
				require.NoError(t, err)
				return
			case <-time.After(5 * time.Second):
				require.FailNow(t, "Alerts were not pushed.")
			}
		}
	}

	// Send one batch, wait for it to arrive and block the server so the queue fills up.
	h.Send(alerts[:DefaultMaxBatchSize]...)
	<-called

	// Send several batches while the server is still blocked so the queue
	// fills up to its maximum capacity (3*DefaultMaxBatchSize). Then check that the
	// queue is truncated in the front.
	h.Send(alerts[1*DefaultMaxBatchSize : 2*DefaultMaxBatchSize]...) // This batch should be dropped.
	h.Send(alerts[2*DefaultMaxBatchSize : 3*DefaultMaxBatchSize]...)
	h.Send(alerts[3*DefaultMaxBatchSize : 4*DefaultMaxBatchSize]...)

	// Send the batch that drops the first one.
	h.Send(alerts[4*DefaultMaxBatchSize : 5*DefaultMaxBatchSize]...)

	// Unblock the server.
	expectedc <- alerts[:DefaultMaxBatchSize]
	select {
	case err := <-errc:
		require.NoError(t, err)
	case <-time.After(5 * time.Second):
		require.FailNow(t, "Alerts were not pushed.")
	}

	// Verify that we receive the last 3 batches.
	for i := 2; i < 5; i++ {
		assertAlerts(alerts[i*DefaultMaxBatchSize : (i+1)*DefaultMaxBatchSize])
	}
}

type alertmanagerMock struct {
	urlf func() string
}

func (a alertmanagerMock) url() *url.URL {
	u, err := url.Parse(a.urlf())
	if err != nil {
Repository: prometheus/prometheus
File: web/api/v1/api_test.go (Go)

Code before:
	} {
		t.Run(tc.name, func(t *testing.T) {
			es := s
			ctx := context.Background()

			appendExemplars(t, es, tc.exemplars)

			req, err := request(http.MethodGet, tc.query)
			require.NoError(t, err)
			res := tc.api.queryExemplars(req.WithContext(ctx))
			assertAPIError(t, res.err, tc.expectedErrorType)

			if tc.expectedErrorType == errorNone {
				assertAPIResponse(t, res.data, tc.exemplars)
			}
		})
	}
}

func TestLabelNames(t *testing.T) {
	// TestEndpoints doesn't have enough label names to test api.labelNames
	// endpoint properly. Hence we test it separately.
	s := promqltest.LoadedStorage(t, `
		load 1m
			test_metric1{foo1="bar", baz="abc"} 0+100x100
			test_metric1{foo2="boo"} 1+0x100
			test_metric2{foo="boo"} 1+0x100
			test_metric2{foo="boo", xyz="qwerty"} 1+0x100
			test_metric2{foo="baz", abc="qwerty"} 1+0x100
	`)

	api := &API{
		Queryable: s,
	}
	request := func(method, limit string, matchers ...string) (*http.Request, error) {
		u, err := url.Parse("http://example.com")
		require.NoError(t, err)
		q := u.Query()
		for _, matcher := range matchers {
			q.Add("match[]", matcher)
		}
		if limit != "" {
			q.Add("limit", limit)
		}
		u.RawQuery = q.Encode()

		r, err := http.NewRequest(method, u.String(), nil)
		if method == http.MethodPost {
			r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
		}
		return r, err

Review comment (suggestion):
nit: please don't mix style changes with logic changes

```suggestion
	storage := promqltest.LoadedStorage(t, `
```

Code after:
				"end":   []string{"4"},
			},
		},
	} {
		t.Run(tc.name, func(t *testing.T) {
			es := s
			ctx := context.Background()

			appendExemplars(t, es, tc.exemplars)

			req, err := request(http.MethodGet, tc.query)
			require.NoError(t, err)
			res := tc.api.queryExemplars(req.WithContext(ctx))
			assertAPIError(t, res.err, tc.expectedErrorType)

			if tc.expectedErrorType == errorNone {
				assertAPIResponse(t, res.data, tc.exemplars)
			}
		})
	}
}

func TestLabelNames(t *testing.T) {
	// TestEndpoints doesn't have enough label names to test api.labelNames
	// endpoint properly. Hence we test it separately.
	s := promqltest.LoadedStorage(t, `
		load 1m
			test_metric1{foo1="bar", baz="abc"} 0+100x100
			test_metric1{foo2="boo"} 1+0x100
			test_metric2{foo="boo"} 1+0x100
			test_metric2{foo="boo", xyz="qwerty"} 1+0x100
			test_metric2{foo="baz", abc="qwerty"} 1+0x100
	`)

	api := &API{
		Queryable: s,
	}
	request := func(method, limit string, matchers ...string) (*http.Request, error) {
		u, err := url.Parse("http://example.com")
		require.NoError(t, err)
		q := u.Query()
		for _, matcher := range matchers {
			q.Add("match[]", matcher)
		}
		if limit != "" {
			q.Add("limit", limit)
		}
		u.RawQuery = q.Encode()

		r, err := http.NewRequest(method, u.String(), nil)
		if method == http.MethodPost {
Repository: prometheus/prometheus
File: web/api/v1/api_test.go (Go)

Code before:
			},
			expect: nil,
			err:    errors.New(`error parsing lookback delta duration: cannot parse "invalid" to a valid duration`),
		},
	}

	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			req := &http.Request{Form: test.form}
			opts, err := extractQueryOpts(req)
			require.Equal(t, test.expect, opts)
			if test.err == nil {
				require.NoError(t, err)
			} else {
				require.EqualError(t, err, test.err.Error())
			}
		})
	}
}

// Test query timeout parameter.
func TestQueryTimeout(t *testing.T) {
	s := promqltest.LoadedStorage(t, `
		load 1m
			test_metric1{foo="bar"} 0+100x100
	`)

	now := time.Now()

	for _, tc := range []struct {
		name   string
		method string
	}{
		{
			name:   "GET method",
			method: http.MethodGet,
		},
		{
			name:   "POST method",
			method: http.MethodPost,
		},
	} {
		t.Run(tc.name, func(t *testing.T) {
			engine := &fakeEngine{}
			api := &API{
				Queryable:             s,
				QueryEngine:           engine,
				ExemplarQueryable:     s,
				alertmanagerRetriever: testAlertmanagerRetriever{}.toFactory(),
				flagsMap:              sampleFlagMap,
				now:                   func() time.Time { return now },

Review comment (suggestion):
nit: please don't mix style changes with logic changes

```suggestion
	storage := promqltest.LoadedStorage(t, `
```

Code after:
			name: "with invalid lookback delta",
			form: url.Values{
				"lookback_delta": []string{"invalid"},
			},
			expect: nil,
			err:    errors.New(`error parsing lookback delta duration: cannot parse "invalid" to a valid duration`),
		},
	}

	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			req := &http.Request{Form: test.form}
			opts, err := extractQueryOpts(req)
			require.Equal(t, test.expect, opts)
			if test.err == nil {
				require.NoError(t, err)
			} else {
				require.EqualError(t, err, test.err.Error())
			}
		})
	}
}

// Test query timeout parameter.
func TestQueryTimeout(t *testing.T) {
	s := promqltest.LoadedStorage(t, `
		load 1m
			test_metric1{foo="bar"} 0+100x100
	`)

	now := time.Now()

	for _, tc := range []struct {
		name   string
		method string
	}{
		{
			name:   "GET method",
			method: http.MethodGet,
		},
		{
			name:   "POST method",
			method: http.MethodPost,
		},
	} {
		t.Run(tc.name, func(t *testing.T) {
			engine := &fakeEngine{}
			api := &API{
				Queryable:             s,
				QueryEngine:           engine,
				ExemplarQueryable:     s,
Repository: prometheus/prometheus
File: scrape/manager_test.go (Go)

Code before:
	default:
	}

	// Apply a configuration for which the reload succeeds.
	err = scrapeManager.ApplyConfig(cfg3)
	require.NoError(t, err, "Unable to apply configuration.")
	select {
	case <-ch:
	default:
		require.FailNow(t, "Reload didn't happen.")
	}

	// Re-applying the same configuration shouldn't trigger a reload.
	err = scrapeManager.ApplyConfig(cfg3)
	require.NoError(t, err, "Unable to apply configuration.")
	select {
	case <-ch:
		require.FailNow(t, "Reload happened.")
	default:
	}
}

func TestManagerTargetsUpdates(t *testing.T) {
	opts := Options{}
	testRegistry := prometheus.NewRegistry()
	m, err := NewManager(&opts, nil, nil, nil, nil, testRegistry)
	require.NoError(t, err)

	targetSetsCh := make(chan map[string][]*targetgroup.Group)
	go m.Run(targetSetsCh)
	defer m.Stop()

	tgSent := make(map[string][]*targetgroup.Group)
	for x := range 10 {
		tgSent[strconv.Itoa(x)] = []*targetgroup.Group{
			{
				Source: strconv.Itoa(x),
			},
		}

		select {
		case targetSetsCh <- tgSent:
		case <-time.After(10 * time.Millisecond):
			require.Fail(t, "Scrape manager's channel remained blocked after the set threshold.")
		}
	}

	m.mtxScrape.Lock()
	tsetActual := m.targetSets
	m.mtxScrape.Unlock()


Review comment (performance):
FYI: Previously I left nil on purpose for clarity, because normally those test does not require to append anything. However with slow CI, on some tests (one of the fixed here) did scrape and panic. 

For flakiness reasons, and safety of `NewManager` users, let's validate and expect at least one appendable.


Code after:
	default:
	}

	// Apply a configuration for which the reload succeeds.
	err = scrapeManager.ApplyConfig(cfg3)
	require.NoError(t, err, "Unable to apply configuration.")
	select {
	case <-ch:
	default:
		require.FailNow(t, "Reload didn't happen.")
	}

	// Re-applying the same configuration shouldn't trigger a reload.
	err = scrapeManager.ApplyConfig(cfg3)
	require.NoError(t, err, "Unable to apply configuration.")
	select {
	case <-ch:
		require.FailNow(t, "Reload happened.")
	default:
	}
}

func TestManagerTargetsUpdates(t *testing.T) {
	opts := Options{}
	testRegistry := prometheus.NewRegistry()
	m, err := NewManager(&opts, nil, nil, nil, teststorage.NewAppendable(), testRegistry)
	require.NoError(t, err)

	targetSetsCh := make(chan map[string][]*targetgroup.Group)
	go m.Run(targetSetsCh)
	defer m.Stop()

	tgSent := make(map[string][]*targetgroup.Group)
	for x := range 10 {
		tgSent[strconv.Itoa(x)] = []*targetgroup.Group{
			{
				Source: strconv.Itoa(x),
			},
		}

		select {
		case targetSetsCh <- tgSent:
		case <-time.After(10 * time.Millisecond):
			require.Fail(t, "Scrape manager's channel remained blocked after the set threshold.")
		}
	}

	m.mtxScrape.Lock()
	tsetActual := m.targetSets
	m.mtxScrape.Unlock()
Repository: prometheus/prometheus
File: storage/interface_append.go (Go)

Code before:
		return ""
	}
	return errs.Error()
}

// ToError returns AppendPartialError as error, returning nil
// if there are no errors.
func (e *AppendPartialError) ToError() error {
	if e == nil {
		return nil
	}
	return e
}

// Is implements method that's expected by errors.Is.
func (*AppendPartialError) Is(target error) bool {
	// This does not need to handle wrapped errors as AppendPartialError.Is should be used
	// via errors.Is.
	_, ok := target.(*AppendPartialError)
	return ok
}

// Handle handles the given err that may be an AppendPartialError.
// If the err is nil or not an AppendPartialError it returns err.
// Otherwise, partial errors are aggregated.
func (e *AppendPartialError) Handle(err error) (_ *AppendPartialError, _ error) {
	if err == nil {
		return e, nil
	}

	// Fast, alloc-free path first for non-partial error cases.
	if !errors.Is(err, e) {
		return e, err
	}
	var pErr *AppendPartialError
	if !errors.As(err, &pErr) {
		return e, err
	}

	if e == nil {
		// Lazy allocation.
		e = &AppendPartialError{}
	}
	e.ExemplarErrors = append(e.ExemplarErrors, pErr.ExemplarErrors...)
	return e, nil
}

var _ error = &AppendPartialError{}

// AppenderV2 provides appends against a storage for all types of samples.
// It must be completed with a call to Commit or Rollback and must not be reused afterwards.

Review comment (suggestion):
nit:

```suggestion
func (e *AppendPartialError) Handle(err error) (*AppendPartialError, error) {
```

Code after:
		return ""
	}
	return errs.Error()
}

// ToError returns AppendPartialError as error, returning nil
// if there are no errors.
func (e *AppendPartialError) ToError() error {
	if e == nil {
		return nil
	}
	return e
}

// Is implements method that's expected by errors.Is.
func (*AppendPartialError) Is(target error) bool {
	// This does not need to handle wrapped errors as AppendPartialError.Is should be used
	// via errors.Is.
	_, ok := target.(*AppendPartialError)
	return ok
}

// Handle handles the given err that may be an AppendPartialError.
// If the err is nil or not an AppendPartialError it returns err.
// Otherwise, partial errors are aggregated.
func (e *AppendPartialError) Handle(err error) (*AppendPartialError, error) {
	if err == nil {
		return e, nil
	}

	// Fast, alloc-free path first for non-partial error cases.
	if !errors.Is(err, e) {
		return e, err
	}
	var pErr *AppendPartialError
	if !errors.As(err, &pErr) {
		return e, err
	}

	if e == nil {
		// Lazy allocation.
		e = &AppendPartialError{}
	}
	e.ExemplarErrors = append(e.ExemplarErrors, pErr.ExemplarErrors...)
	return e, nil
}

var _ error = &AppendPartialError{}

// AppenderV2 provides appends against a storage for all types of samples.
// It must be completed with a call to Commit or Rollback and must not be reused afterwards.