refactor: replace time.Time lastWrite with atomic Int64 for lock-free write timestamping

- Replace `lastWrite time.Time` with `lastWriteNano atomic.Int64` to enable lock-free updates in hot paths.
- Update maintenance logic to use monotonic nanoseconds instead of wall-clock time.
- Add `MarkWritten()` method for explicit write-timestamp updates.
- Remove `Rendering atomic.Bool` and associated logic, as it was redundant with the new timestamp mechanism.
- Delete obsolete test `TestRequest_JawsKeyReadsAreLockedDuringRecycle`, which is no longer relevant after lock-free JawsKey access removal.

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

Author: linkdata

recyclerace_test.go

@@ -50,27 +50,26 @@ type ConnectFn = func(rq *Request) error
 // [Request.Log] and [Request.MustLog] let it forward to the logger; both exist only for
 // that diagnostic use, not as a public nil-safe contract.
 type Request struct {
-	Jaws       *Jaws                   // (read-only) the JaWS instance the Request belongs to
-	JawsKey    key.Key                 // (read-only) random key identifying this Request in the WebSocket URI and the broadcast/tail target; read under mu by the identity check (destKey, wantMessage) and the render path (JawsKeyString, HeadHTML)
-	remoteIP   netip.Addr              // (read-only) remote IP, or the zero netip.Addr if unset
-	Rendering  atomic.Bool             // set to true by RequestWriter.Write()
-	running    atomic.Bool             // if ServeHTTP() is running
-	claimed    atomic.Bool             // if UseRequest() has been called for it
-	mu         deadlock.RWMutex        // protects following
-	lastJid    Jid                     // last element Jid allocated within this Request
-	lastWrite  time.Time               // when the initial HTML was last written to, used for automatic cleanup
-	initial    *http.Request           // initial HTTP request passed to Jaws.NewRequest
-	session    *Session                // session, if established
-	todoDirt   []any                   // dirty tags
-	ctx        context.Context         // current context, derived from either Jaws or WS HTTP req
-	httpDoneCh <-chan struct{}         // once claimed, set to http.Request.Context().Done()
-	cancelFn   context.CancelCauseFunc // cancel function
-	connectFn  ConnectFn               // a ConnectFn to call before starting message processing for the Request
-	elems      []*Element              // our Elements
-	tagMap     map[any][]*Element      // maps tags to Elements
-	muQueue    deadlock.Mutex          // protects wsQueue and tailsent
-	wsQueue    []wire.WsMsg            // queued messages to send
-	tailsent   bool
+	Jaws          *Jaws                   // (read-only) the JaWS instance the Request belongs to
+	JawsKey       key.Key                 // (read-only) random key identifying this Request in the WebSocket URI and the broadcast/tail target; read under mu by the identity check (destKey, wantMessage) and the render path (JawsKeyString, HeadHTML)
+	remoteIP      netip.Addr              // (read-only) remote IP, or the zero netip.Addr if unset
+	running       atomic.Bool             // if ServeHTTP() is running
+	claimed       atomic.Bool             // if UseRequest() has been called for it
+	lastWriteNano atomic.Int64            // monotonic nanos (Jaws.nowNano) of the most recent RequestWriter write; lock-free, drives pending-eviction recency (oldestEvictablePendingLocked) and idle expiry (maintenance)
+	mu            deadlock.RWMutex        // protects following
+	lastJid       Jid                     // last element Jid allocated within this Request
+	initial       *http.Request           // initial HTTP request passed to Jaws.NewRequest
+	session       *Session                // session, if established
+	todoDirt      []any                   // dirty tags
+	ctx           context.Context         // current context, derived from either Jaws or WS HTTP req
+	httpDoneCh    <-chan struct{}         // once claimed, set to http.Request.Context().Done()
+	cancelFn      context.CancelCauseFunc // cancel function
+	connectFn     ConnectFn               // a ConnectFn to call before starting message processing for the Request
+	elems         []*Element              // our Elements
+	tagMap        map[any][]*Element      // maps tags to Elements
+	muQueue       deadlock.Mutex          // protects wsQueue and tailsent
+	wsQueue       []wire.WsMsg            // queued messages to send
+	tailsent      bool
 }
 
 type eventFnCall struct {
@@ -122,6 +121,16 @@ func (rq *Request) String() string {
 	return "Request<" + rq.JawsKeyString() + ">"
 }
 
+// MarkWritten records that the Request's initial HTML is being written, so the
+// pending-eviction logic spares it while a render is in flight.
+//
+// [RequestWriter.Write] calls it on every write; the recorded instant drives the
+// recency window in [Jaws.oldestEvictablePendingLocked] and the idle expiry in
+// [Request.maintenance]. It is lock-free and safe to call concurrently.
+func (rq *Request) MarkWritten() {
+	rq.lastWriteNano.Store(rq.Jaws.nowNano())
+}
+
 // destKey returns the Request's current identity key, read under rq.mu, for use as
 // a broadcast destination. Targeting the key value rather than the *Request pointer
 // lets the Serve loop reject a message aimed at a request that was recycled and
@@ -169,10 +178,10 @@ func (rq *Request) claim(r *http.Request) error {
 				}
 			}
 			rq.httpDoneCh = httpDoneCh
-			// Refresh lastWrite so a request claimed long after its initial render
-			// (a throttled or backgrounded tab) is not treated as idle and recycled
-			// in the window before ServeHTTP sets running.
-			rq.lastWrite = time.Now()
+			// Refresh the write timestamp so a request claimed long after its initial
+			// render (a throttled or backgrounded tab) is not treated as idle and
+			// recycled in the window before ServeHTTP sets running.
+			rq.lastWriteNano.Store(rq.Jaws.nowNano())
 			return nil
 		}
 	}
@@ -223,12 +232,11 @@ func (rq *Request) clearLocked() *Request {
 	rq.JawsKey = 0
 	rq.lastJid = 0
 	rq.connectFn = nil
-	rq.lastWrite = time.Time{}
+	rq.lastWriteNano.Store(0)
 	rq.initial = nil
 	rq.killSessionLocked()
 	rq.running.Store(false)
 	rq.claimed.Store(false)
-	rq.Rendering.Store(false)
 	if rq.cancelFn != nil {
 		rq.cancelFn(nil)
 	}
@@ -468,21 +476,19 @@ func (rq *Request) SetContext(fn func(oldCtx context.Context) (newCtx context.Co
 
 // maintenance reports whether rq has expired and should be recycled. For a
 // request that never went live it cancels and reports expiry once it has been
-// idle longer than requestTimeout, or immediately if its context is already
-// done. Called from the Serve loop's maintenance pass while jw.mu is held.
+// idle (no [RequestWriter] write) longer than requestTimeout, or immediately if its
+// context is already done. nowNano is the reference instant (see [Jaws.nowNano]).
+// Called from the Serve loop's maintenance pass while jw.mu is held.
 //
 // It returns the cancellation cause (or nil) rather than logging it, so the caller
 // can log it after releasing jw.mu — logging runs the user [Jaws.Logger], which
 // must not be invoked under a lock.
-func (rq *Request) maintenance(now time.Time, requestTimeout time.Duration) (expired bool, cause error) {
+func (rq *Request) maintenance(nowNano int64, requestTimeout time.Duration) (expired bool, cause error) {
 	if !rq.running.Load() {
 		rq.mu.Lock()
-		if rq.Rendering.Swap(false) {
-			rq.lastWrite = now
-		}
 		if rq.ctx.Err() != nil {
 			expired = true
-		} else if now.Sub(rq.lastWrite) > requestTimeout {
+		} else if nowNano-rq.lastWriteNano.Load() > int64(requestTimeout) {
 			cause = rq.cancelLocked(newErrNoWebSocketRequest(rq))
 			expired = true
 		}

request.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"html/template"
+	"io"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
@@ -676,15 +677,13 @@ func TestRequest_ClaimRefreshesLastWriteAndStartServeGuards(t *testing.T) {
 	rq := jw.NewRequest(hr)
 
 	// Simulate a page that rendered long ago (idle) before its WebSocket connects.
-	rq.mu.Lock()
-	rq.lastWrite = time.Now().Add(-time.Hour)
-	rq.mu.Unlock()
+	rq.lastWriteNano.Store(jw.nowNano() - int64(time.Hour))
 
 	if jw.UseRequest(rq.JawsKey, hr) != rq {
 		t.Fatal("expected claim to succeed")
 	}
-	// claim refreshed lastWrite, so the just-claimed request is not idle-eligible.
-	if expired, _ := rq.maintenance(time.Now(), time.Second); expired {
+	// claim refreshed the write timestamp, so the just-claimed request is not idle.
+	if expired, _ := rq.maintenance(jw.nowNano(), time.Second); expired {
 		t.Fatal("a freshly claimed request must not be treated as idle by maintenance")
 	}
 
@@ -1733,9 +1732,7 @@ func TestCoverage_PendingSubscribeMaintenanceAndParse(t *testing.T) {
 	<-unsubDone
 
 	// Request timeout path.
-	rq.mu.Lock()
-	rq.lastWrite = time.Now().Add(-time.Hour)
-	rq.mu.Unlock()
+	rq.lastWriteNano.Store(jw.nowNano() - int64(time.Hour))
 	jw.maintenance(time.Second)
 	if got := jw.RequestCount(); got != 0 {
 		t.Fatalf("expected request recycled, got %d", got)
@@ -1786,32 +1783,25 @@ func TestCoverage_RequestMaintenanceClaimAndErrors(t *testing.T) {
 		t.Fatalf("unexpected error text: %v", err)
 	}
 
-	now := time.Now()
+	nowNano := jw.nowNano()
 	rqM := jw.NewRequest(httptest.NewRequest("GET", "/", nil))
-	rqM.lastWrite = now.Add(-time.Hour)
-	if expired, _ := rqM.maintenance(now, time.Second); !expired {
+	rqM.lastWriteNano.Store(nowNano - int64(time.Hour))
+	if expired, _ := rqM.maintenance(nowNano, time.Second); !expired {
 		t.Fatal("expected maintenance timeout")
 	}
 	rqR := jw.NewRequest(httptest.NewRequest("GET", "/", nil))
-	nowR := time.Now()
-	rqR.Rendering.Store(true)
-	if expired, _ := rqR.maintenance(nowR, time.Hour); expired {
-		t.Fatal("expected maintenance continue")
-	}
-	rqR.mu.RLock()
-	lastWrite := rqR.lastWrite
-	rqR.mu.RUnlock()
-	if lastWrite != nowR {
-		t.Fatalf("expected lastWrite updated to now, got %v want %v", lastWrite, nowR)
+	rqR.MarkWritten()
+	if expired, _ := rqR.maintenance(jw.nowNano(), time.Hour); expired {
+		t.Fatal("a freshly written request must not be idle-expired")
 	}
 	rqC := jw.NewRequest(httptest.NewRequest("GET", "/", nil))
 	rqC.cancel(errors.New("cancelled"))
-	if expired, _ := rqC.maintenance(time.Now(), time.Hour); !expired {
+	if expired, _ := rqC.maintenance(jw.nowNano(), time.Hour); !expired {
 		t.Fatal("expected maintenance cancellation")
 	}
 	rqOK := jw.NewRequest(httptest.NewRequest("GET", "/", nil))
-	rqOK.lastWrite = time.Now()
-	if expired, _ := rqOK.maintenance(time.Now(), time.Hour); expired {
+	rqOK.MarkWritten()
+	if expired, _ := rqOK.maintenance(jw.nowNano(), time.Hour); expired {
 		t.Fatal("expected maintenance keepalive")
 	}
 
@@ -2862,3 +2852,87 @@ func TestDelRequestNilsVacatedSlot(t *testing.T) {
 		}
 	})
 }
+
+// TestRequest_JawsKeyReadsAreLockedDuringRecycle verifies that the request-key
+// readers used while the application renders the initial HTML page
+// (JawsKeyString, String, HeadHTML and TailHTML) read rq.JawsKey under rq.mu, so
+// they do not race the rq.mu-guarded writes to rq.JawsKey that clearLocked and
+// getRequestLocked perform when a still-pending request is recycled (for example
+// by limitPendingRequestsLocked evicting it). Run with -race.
+func TestRequest_JawsKeyReadsAreLockedDuringRecycle(t *testing.T) {
+	jw, err := New()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(jw.Close)
+
+	rq := jw.NewRequest(httptest.NewRequest(http.MethodGet, "/", nil))
+
+	const iterations = 2000
+	var wg sync.WaitGroup
+	wg.Add(2)
+
+	// Writer: mimic clearLocked / getRequestLocked, which assign rq.JawsKey while
+	// holding rq.mu.
+	go func() {
+		defer wg.Done()
+		for i := range iterations {
+			rq.mu.Lock()
+			rq.JawsKey = key.Key(uint64(i) + 1)
+			rq.mu.Unlock()
+		}
+	}()
+
+	// Reader: the lock-free render-path readers that previously read rq.JawsKey
+	// without holding rq.mu.
+	go func() {
+		defer wg.Done()
+		for range iterations {
+			_ = rq.JawsKeyString()
+			_ = rq.String()
+			_ = rq.HeadHTML(io.Discard)
+			_ = rq.TailHTML(io.Discard)
+		}
+	}()
+
+	wg.Wait()
+}
+
+// TestServe_MarksRequestRunningSoMaintenanceSkips verifies that TestServe marks
+// the request running before driving rq.process, mirroring ServeHTTP/startServe.
+// The maintenance pass recycles only not-running requests (clearing their
+// elements via clearLocked), so a request whose process loop is live must report
+// running and must survive a maintenance pass that would otherwise expire it.
+func TestServe_MarksRequestRunningSoMaintenanceSkips(t *testing.T) {
+	jw, err := New()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(jw.Close)
+
+	go jw.Serve()
+
+	tr := NewTestRequest(jw, nil)
+	if tr == nil {
+		t.Fatal("expected test request")
+	}
+	defer func() {
+		tr.Close()
+		<-tr.DoneCh
+	}()
+
+	<-tr.ReadyCh
+	if !tr.running.Load() {
+		t.Fatal("TestServe must mark the request running so maintenance cannot recycle it mid-process")
+	}
+
+	// Make the request look long-idle, then run a maintenance pass directly. A
+	// running request must not be recycled; before the fix it would be removed
+	// from jw.requests and its elements cleared while process is still using them.
+	tr.lastWriteNano.Store(jw.nowNano() - int64(time.Hour))
+	jw.maintenance(time.Millisecond)
+
+	if got := jw.RequestCount(); got != 1 {
+		t.Fatalf("running request was recycled by maintenance: RequestCount() = %d, want 1", got)
+	}
+}