docs: clarify runtime tag comparability checks and update test logging

Signed-off-by: Johan Lindh <johan@linkdata.se>

Author: linkdata

jaws.go

@@ -66,10 +66,10 @@
 //
 // Always run the tests with the -race flag (or -tags debug). Both set
 // deadlock.Debug, which enables the debug-gated runtime invariant checks: the
-// lock-order verification described above, the late-handler panic, and the
-// tag-comparability check in [github.com/linkdata/jaws/lib/tag]. Those branches
-// are compile-time dead in normal builds, so a plain "go test" neither
-// exercises them nor reports their statement coverage. CI builds with
+// lock-order verification described above and the late-handler panic. Those
+// branches are compile-time dead in normal builds, so a plain "go test" neither
+// exercises them nor reports their statement coverage. Runtime tag-comparability
+// checks in [github.com/linkdata/jaws/lib/tag] run in every build. CI builds with
 // -tags debug -race.
 package jaws
 
@@ -759,11 +759,9 @@ func (jw *Jaws) Broadcast(msg wire.Message) {
 // [Jaws.reportMisuse], if any tag is not comparable at runtime.
 //
 // Expanded tags become map keys in the processing loop (wantMessage's lookup in
-// Request.tagMap). A value can pass tag expansion's static comparability check yet
-// be non-comparable at runtime (a comparable struct holding e.g. a func in an
-// interface field), which panics when hashed. [tag.NewErrNotComparable] runs the
-// runtime value check that ensureUsableTag only performs in debug builds, so this
-// rejects a bad Dest before it can panic the Serve goroutine and crash the process.
+// Request.tagMap). TagExpand rejects the known runtime-non-comparable
+// struct/array case, and this guard remains as a final defense before a bad Dest
+// can panic the Serve goroutine and crash the process.
 func (jw *Jaws) dropNonComparableTags(tags []any) []any {
 	for _, tagValue := range tags {
 		if cmperr := tag.NewErrNotComparable(tagValue); cmperr != nil {
@@ -793,14 +791,13 @@ func (jw *Jaws) setDirty(tags []any) {
 // Note that if any of the tags implement [tag.TagGetter], it will be called
 // with a nil [Request]. Prefer using [Request.Dirty] which avoids this.
 //
-// A tag that is comparable statically but not at runtime (a comparable struct
-// holding e.g. a func in an interface field) panics the calling goroutine when
-// hashed as a map key, in non-debug builds where the expansion check is skipped.
-// The panic is contained to the caller (the lock is released and the [Jaws.Serve]
-// loop is unaffected); it is not logged-and-dropped the way [Jaws.Broadcast]
-// handles such a [wire.Message.Dest], because that hashing happens in the Serve
-// goroutine where a panic would crash the process. [Request.Dirty] behaves the
-// same as Dirty here.
+// A tag that somehow passes expansion but is still not hashable panics the
+// calling goroutine when hashed as a map key. The panic is contained to the
+// caller (the lock is released and the [Jaws.Serve] loop is unaffected); it is
+// not logged-and-dropped the way [Jaws.Broadcast] handles such a
+// [wire.Message.Dest], because that hashing happens in the Serve goroutine where
+// a panic would crash the process. [Request.Dirty] behaves the same as Dirty
+// here.
 func (jw *Jaws) Dirty(dirtyTags ...any) {
 	// Use TagExpand+MustLog rather than MustTagExpand: with a nil Context the
 	// latter panics on an illegal tag even in production, unlike the sibling

jaws_test.go

@@ -7,7 +7,6 @@ import (
 	"errors"
 	"html/template"
 	"io"
-	"log/slog"
 	"mime"
 	"net/http"
 	"net/http/httptest"
@@ -466,9 +465,8 @@ func TestJaws_BroadcastMultiRuntimeNonComparable(t *testing.T) {
 }
 
 // TestJaws_dropNonComparableTags exercises the Broadcast comparability guard
-// directly. Broadcast cannot reach it through tag.TagExpand under deadlock.Debug
-// (the -race build) because ensureUsableTag rejects non-comparable tags first, so
-// the guard is tested with a synthetic tag slice.
+// directly. TagExpand rejects runtime-non-comparable struct/array tags before
+// Broadcast reaches this guard, so the guard is tested with a synthetic tag slice.
 func TestJaws_dropNonComparableTags(t *testing.T) {
 	jw, err := New()
 	if err != nil {
@@ -509,10 +507,9 @@ func TestJaws_dropNonComparableTags(t *testing.T) {
 
 // TestJaws_setDirtyPanicReleasesLock verifies the documented defer-unlock in
 // setDirty: a tag comparable statically but not at runtime panics when used as a
-// map key, yet jw.mu is still released so the instance stays usable. The public
-// Dirty path cannot reach this under deadlock.Debug (the -race build), where
-// ensureUsableTag rejects such tags during expansion, so setDirty is exercised
-// directly with a synthetic tag.
+// map key, yet jw.mu is still released so the instance stays usable. Public Dirty
+// rejects this tag during expansion, so setDirty is exercised directly with a
+// synthetic tag.
 func TestJaws_setDirtyPanicReleasesLock(t *testing.T) {
 	jw, err := New()
 	if err != nil {
@@ -750,15 +747,13 @@ func TestBroadcast_RejectsUnhashableTagDest(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer jw.Close()
-	var logbuf bytes.Buffer
-	jw.Logger = slog.New(slog.NewTextHandler(&logbuf, nil))
+	logger := &captureErrorLogger{}
+	jw.Logger = logger
 
 	// A statically comparable struct holding a func in an interface field panics
 	// when used as a map key. Such a Dest must be rejected before it reaches the
 	// Serve loop, where wantMessage's tagMap lookup would panic and tear the loop
-	// down. In debug builds tag.TagExpand rejects it; in non-debug (production)
-	// builds TagExpand is lenient and the hashableTag check in Broadcast catches
-	// it. Either way, Broadcast must not panic and must not queue the message.
+	// down. Broadcast must not panic and must not queue the message in any build.
 	type ifaceHolder struct{ any }
 	var panicked bool
 	func() {
@@ -778,11 +773,11 @@ func TestBroadcast_RejectsUnhashableTagDest(t *testing.T) {
 		t.Fatalf("unhashable Dest was queued for the Serve loop: %T(%#v)", msg.Dest, msg.Dest)
 	default:
 	}
-	if logbuf.Len() == 0 {
-		t.Error("expected the rejected broadcast tag to be logged")
+	if !errors.Is(logger.err, tag.ErrNotUsableAsTag) {
+		t.Errorf("logged error = %v, want ErrNotUsableAsTag", logger.err)
 	}
-	if !deadlock.Debug && !strings.Contains(logbuf.String(), "not comparable") {
-		t.Errorf("non-debug build should reject the non-comparable tag, got %q", logbuf.String())
+	if !errors.Is(logger.err, tag.ErrNotComparable) {
+		t.Errorf("logged error = %v, want ErrNotComparable", logger.err)
 	}
 }