Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 31 of 31 files at r3, 8 of 8 files at r4, 2 of 2 files at r5, 31 of 33 files at r8, 8 of 8 files at r9, 2 of 2 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

pkg/util/must/must.go line 271 at r5 (raw file):

// handlePanic is thrown and recovered via Handle().
type handlePanic struct{ err error }

Better yet: make this an error, (with func Unwrap() error { return h.err })
Then use errors.As() in your recover handler above, like this:

if r := recover(); r != nil {
   e, ok := r.(error)
   if !ok { panic(r) }
   if unused := handlePanic{}; !errors.As(e, &unused) { panic(r) }
   err = e
}

Why this makes sense:

• the code under Handle can be complex and wrap its errors using errors.Wrap or other things.
• you don't want to discard these layers in the return value of Handle.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

pkg/util/must/must.go line 271 at r5 (raw file):

Sure, if it fits in the mid-stack inlining budget.

Does the overall approach seem reasonable though? I'm a bit lukewarm on the whole nolint:errcheck thing and lack of API safety, but seems like the lesser of evils.

Why do the must.XX APIs return errors in the first place? I don't see much of a usecase for that..

E.g. not much benefit to do

if err := must.True(x, "msg"); err != nil {
  return err
}

instead of

if !x {
  return errors.AssertionFailedf("msg")
}

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @knz)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @itsbilal, and @knz)

pkg/storage/intent_interleaving_iter.go line 1348 at r14 (raw file):

		// At least one of the iterators must be valid. The iterator's validity state
		// should match i.iterValid.
		must.True(ctx, iterValid || intentValid, "i.valid=%t but both iterators are invalid", i.valid)

This code looks much cleaner now, nice!

One thing to keep in mind though is that now all the formatting arguments get boxed into interface{} in all cases, so we will allocate a lot more - we don't want that in hot paths.

Why do the must.XX APIs return errors in the first place? I don't see much of a usecase for that..

A couple of reasons:

• We want to fatal on assertion failures in non-release builds, to make sure we notice them. We want to return assertion errors in release builds, to avoid outages over trigger-happy assertions, but still want them to notify Sentry and log an error with a stack trace at the failure site.

• For other assertions, e.g. must.Equal(a, b), we can automatically include the values in the failure messages without the caller having to manually pass them in the format string, and can generate better output like diffs in the case of large values (e.g. large structs). We can also have less trivial assertions like must.Contains(), although probably nothing too complex since we want them to be reasonably cheap. This is all purely for convenience though, and we can drop it if that's the consensus, but I find myself preferring Testify over testing.T and figured the same would hold here.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @knz, and @RaduBerinde)

pkg/storage/intent_interleaving_iter.go line 1348 at r14 (raw file):

One thing to keep in mind though is that now all the formatting arguments get boxed into interface{} in all cases, so we will allocate a lot more - we don't want that in hot paths.

Oof, yeah, good point. Not a big deal here specifically, because these are all gated on must.Expensive and compiled out in most builds, but could be a problem in general.

Given the assertions all inline, I was hoping the compiler would be clever enough to avoid the heap escape on the happy path, or that we could avoid it with something like the below, but alas.

func True(ctx context.Context, v bool, format string, args ...interface{}) error {
	if v {
		return nil
	}
	f, a := format, args
	return failDepth(ctx, 1, f, a...)
}

That said, the current approach for assertions also has this problem, so I guess it isn't strictly worse than today, but it would be nice to avoid it. Consider e.g.:

foo := "bar"
if foo != "bar" {
	log.Fatalf(ctx, "%s != bar", foo) // foo escapes
}

Both must or log.Fatalf can avoid it with something like:

foo := "bar"
if foo != "bar" {
	foo := foo // escapes on unhappy path
	must.Fail(ctx, "%s != bar", foo)
}

But I agree that this is an unfortunate API footgun. Ideas for how to improve this?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @knz, and @RaduBerinde)

pkg/storage/intent_interleaving_iter.go line 1348 at r14 (raw file):

This is only the case for format args though, with the typed convenience API we copy these internally on the unhappy path, e.g.:

foo := "bar"
err := must.Equal(ctx, foo, "bar", "failed") // does not escape
if err != nil {
	return err
}

• We want to fatal on assertion failures in non-release builds, to make sure we notice them. We want to return assertion errors in release builds, to avoid outages over trigger-happy assertions, but still want them to notify Sentry and log an error with a stack trace at the failure site.

Can't we just make sure that AssertionFailedf does these things? At least the SQL layer reports assertion errors to Sentry (via sqltelemetry.RecordError).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @itsbilal, and @knz)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @knz, and @RaduBerinde)

pkg/storage/intent_interleaving_iter.go line 1348 at r14 (raw file):

Right, that makes sense, thanks for catching this. I'll have a closer look at this when I can, but I agree, this seems like a showstopper so we may have to do something like what you're suggesting.

Can't we just make sure that AssertionFailedf does these things? At least the SQL layer reports assertion errors to Sentry (via sqltelemetry.RecordError).

We could but it's not exactly trivial. The errors constructors do not have access to context.Context, so it's hard to include meaningful detail if we fail there. Access to the ctx is what I particularly like about the must API.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @miretskiy, and @RaduBerinde)

Can't we just make sure that AssertionFailedf does these things?

An alternative could be to replace all calls to errors.AssertionFailedf and errors.AssertionErrorWithWrappedErrf to some other API that includes the ctx as argument. Then lint against use of the raw API.

Thoughts?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @miretskiy, and @RaduBerinde)

Yeah, it'd be a very small helper API, basically what must.Fail() does.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @knz, @miretskiy, and @RaduBerinde)