fix: never reuse Request identities, pool only buffers (#195) by linkdata · Pull Request #206 · linkdata/jaws

linkdata · 2026-07-21T13:59:20Z

Problem

Jaws.ServeHTTP could recycle a Request while its initial HTTP renderer still owned the pointer (issue #195, severity: high). An early or malformed GET /jaws/<key> claimed a still-rendering Request, failed the WebSocket upgrade, and the deferred teardown cleared the Request and returned it to a sync.Pool; once reused for another connection, continued rendering corrupted an unrelated client's Request.

Fix

Stop reusing *Request identities — the structural root cause. Each NewRequest allocates a fresh Request with a stable identity that is never handed to another connection; only the internal buffers (todoDirt, elems, tagMap, wsQueue) are pooled via Jaws.requestBufferPool.

No pointer reuse: completion (releaseBuffersLocked) cancels the context, unregisters the identity, detaches the session, and returns the buffers to the pool. The *Request is never re-pooled, so a stale pointer can never alias a different connection.
Non-destructive teardown: completion does not touch render-visible state — it leaves Element.ui/handlers, the Jid counter, and JawsKey intact. Those are read lock-free by an initial JawsRender that may still be running, and resetting the Jid counter would duplicate an already-streamed id. A stale Element needs no inerting since the identity is never reused.
Queue handoff under muQueue: the wsQueue clear/transfer/detach is done entirely under muQueue, so a concurrent Request.queue cannot land a message in a buffer already returned to the pool.
Key tombstoning: the recycle path reserves the finished key with a nil tombstone + runtime.AddCleanup (mirroring retirement) instead of deleting it, so a stable key is never reassigned to a different Request while a page might still emit it.
Prompt/bounded payload release: the outbound drain (getSendMsgs) zeroes drained wsQueue entries, and completion clears only the live length (the shrink paths already zero vacated entries), so empty recycling stays O(1) rather than rescanning a pooled buffer's high-water capacity.

An early callback that claims and tears down a still-rendering Request is now harmless: no corruption, no race, no key reassignment — at worst a degraded render on a page that was already being probed. (An earlier revision gated claims on the initial request's context to make render/WebSocket lifetimes sequential, but that signal is unreliable — client disconnect cancels it early, a detached context stays live — and it broke streamed /noscript probes, so it was removed once teardown was made race-safe.)

Tests

request_identity_test.go adds the #195 reproduction, the identity/late-cancel guarantee, non-destructive-teardown coverage (live JawsRender racing teardown; monotonic Jid across teardown; queue-vs-recycle under -race), the key tombstone (forced K,K,K2), and a /noscript-during-live-render guard. TestGetSendMsgsZeroesDrainedQueue and TestReleaseBuffersLockedZeroesWsQueue cover payload release. Existing pointer-reuse assertions were reworked to the stable-identity invariant.

Verification

go vet ./..., staticcheck (clean), golangci-lint (0 issues), go test -race ./... and go test -tags debug -race ./... all pass. 100% coverage on the changed functions.

Performance

BenchmarkRequestLifecyclePooling/impl=pooled (production path, main whole-Request pool → this buffer-pool + tombstone) and BenchmarkRequestRecycleAfterHighWater:

Cost is a fixed per-request overhead: one fresh *Request struct plus the key-reservation runtime.AddCleanup — +3 allocs/op on the empty-request path.
For real pages it is +0.04–0.43% allocs (100–1000 elements); the large per-page buffers stay pooled.
Empty recycling is flat across the high-water axis (~0.5µs at highwater 0, 1K and 100K), confirming completion does not rescan retained capacity.

A follow-up will consolidate registered/running/claimed/tailsent into a single int32 lifecycle state (kept out of this PR to keep the security fix reviewable).

Fixes #195.

Jaws.ServeHTTP could recycle a Request while its initial HTTP renderer still held the pointer: an early or malformed GET /jaws/<key> claimed a still-rendering Request, failed the WebSocket upgrade, and the deferred stopServe cleared the Request and returned it to a sync.Pool. Once reused from the pool, continued rendering mutated an unrelated client's Request. Stop reusing Request identities entirely. NewRequest now allocates a fresh Request with a stable identity; only the reusable buffers (todoDirt, elems, tagMap, wsQueue) are pooled, via Jaws.requestBufferPool. On completion, releaseBuffersLocked cancels the context, unregisters the Request and returns its buffers to the pool while preserving JawsKey, so a pointer the initial renderer still holds stays valid and inert and is never handed to another connection. Identity-targeted operations (destKey, wantMessage, appendDirtyTags) gate on the new registered flag instead of the former key-zeroing trick, and TagExpanded guards against the detached (nil) tagMap so a render continuing after a racy teardown degrades to a no-op rather than panicking. The large per-page buffers remain pooled, so the change adds only one small Request allocation per request: negligible for real pages and vanishing at 1000 elements (BenchmarkRequestLifecyclePooling, impl=pooled). Fixes #195.

…stone Follow-up to the identity-stable change, addressing four review findings. Teardown no longer mutates render-visible state: releaseBuffersLocked stops clearing Element.ui/handlers and resetting lastJid, which raced an initial renderer still inside JawsRender (those fields are read lock-free) and could duplicate an already-streamed Jid. A stale Element needs no inerting now that a Request is never reused. releaseBuffersLocked holds muQueue across the whole wsQueue clear/transfer/detach, so a concurrent Request.queue can no longer land a message in a buffer already returned to the pool, where it would surface in the next Request. UseRequest refuses to claim a Request while its initial render is still in progress (the initial request's context is live), returning 404 WITHOUT consuming the key. An early or malformed /jaws/<key> callback mid-render can no longer claim and tear down the Request; the real WebSocket, which connects only after the page loads, claims it normally. This is a synchronous check on the initial request's context, so there is no watcher goroutine and no watcher-vs-WebSocket scheduling race. The recycle path reserves the finished key with a nil tombstone and a runtime cleanup instead of deleting it, so a stable key can never be reassigned to a different Request while a page might still emit it, mirroring the retirement path. Docs on Request, Element and the README drop the removed pool-and-reuse model. Tests decouple the initial-render request from the WebSocket request (distinct in production) and cover the render gate, the forced-key tombstone, and a live JawsRender racing teardown.

linkdata · 2026-07-21T15:03:47Z

Thanks for the review — all four are real. Pushed b9d93db addressing them.

[P1] Outbound messages crossing Requests (muQueue gap). releaseBuffersLocked now holds muQueue across the entire wsQueue clear → transfer → detach, so a concurrent Request.queue can no longer append into a slice already handed to the pool.

[P1] Teardown racing the initial renderer. Two changes:

Teardown no longer mutates render-visible state — releaseBuffersLocked stops clearing Element.ui/handlers and stops resetting lastJid. Those raced a live JawsRender (read lock-free) and could duplicate a streamed Jid. Since a Request is never reused, a stale Element needs no inerting; inertness now comes from the Request being unregistered and detached.
Render and WebSocket lifetimes are now made sequential: UseRequest refuses to claim while the initial render is still in progress and returns 404 without consuming the key, so an early/malformed /jaws/<key> callback can't claim (and tear down) mid-render, and the real WebSocket still claims it after the page loads. I implemented the gate you sketched as a synchronous check on the initial request's context rather than a watcher goroutine — a watcher that flips the key claimable on ctx.Done() races the WebSocket, which connects ~1 RTT after the handler returns and may beat the watcher's goroutine, causing spurious 404s. The context check has no such race and needs no per-request goroutine. (A nil / non-cancelable initial context, as httptest uses, is treated as immediately claimable.)

[P2] Finished keys reassigned while reachable. The recycle path now reserves the key with a nil tombstone + runtime.AddCleanup instead of deleting it, mirroring the retirement path, so a stable key is never reassigned while the old Request is reachable. New test TestRequestRecycledKeyNotReusedWhileReachable forces the generator to emit K, K, K2 and asserts the replacement gets K2.

[P3] Contradictory docs. The Request type doc, Element doc, and README no longer describe the removed pool-and-reuse model.

New/adjusted tests: the render gate (early callback 404 + key preserved, then claim after render), the forced-key tombstone, and a live JawsRender racing teardown under -race (you noted the earlier tests only exercised HeadHTML/NewElement/Tag). Test fixtures that conflated the initial-render request and the WebSocket request into one live-context object now decouple them (they're distinct in production). Full gate green: go test -race and -tags debug -race across the module, vet, staticcheck, golangci-lint (0 issues), 100% coverage on the changed functions.

Cost. The tombstone's per-recycle AddCleanup adds a fixed ~2 allocs; combined with the fresh *Request struct it's +3 allocs/op on the empty-request path and +0.04–0.43% allocs for real pages (100–1000 elements) in BenchmarkRequestLifecyclePooling/impl=pooled. The large per-page buffers stay pooled.

Separately: your int32 state-machine idea (initialRender → waitForCallback → running → running+tail → teardown) is a good consolidation of registered/running/claimed/tailsent + the gate — I'd like to do it as a focused follow-up PR rather than fold a concurrency-lifecycle refactor into this security fix, so both review cleanly. The gate here is a context check, not a new boolean, so nothing is lost by deferring.

Second review round. Remove the initial-request-context render gate. The request context is not a reliable render-completion barrier: it is canceled when the client connection closes (before the handler returns) and a detached/background context stays live after it returns, so the gate could admit an early callback mid-render or reject the real WebSocket indefinitely. It also 404'd the streamed <noscript> probe, which the browser never retries, so ErrJavascriptDisabled was never recorded and cleanup waited for timeout. Teardown is already race-safe (it no longer mutates render-visible Element state and reserves the key with a tombstone), so the gate is unnecessary; UseRequest claims as before and the fixtures that decoupled the initial-render request from the WebSocket request revert with it. Clear reusable buffers through capacity on completion, not just length. getSendMsgs now zeroes the drained wsQueue before reslicing to zero so HTML payloads are released promptly, and releaseBuffersLocked clears todoDirt/elems/wsQueue through cap so a drained-to-zero queue cannot pin payloads in the pooled backing array. Correct the lifecycle docs: a finished Request keeps its Element fields, so a retained Element is not a no-op — it is unregistered (no further updates) and its identity is never reused, but its methods still operate on the finished Request. The README maintainer checklist now describes identity-stable buffer pooling and key tombstoning rather than whole-Request pooling and pointer reuse. Tests: a /noscript probe during a live-context render still returns 204 and records ErrJavascriptDisabled; TestReleaseBuffersLockedZeroesWsQueue drains to zero length before releasing to prove the backing array is cleared through capacity.

linkdata · 2026-07-21T15:28:41Z

Round 2 addressed in 4ab6eaf — all four are correct.

[P1] Request context isn't a reliable render barrier. Agreed, and the decisive point is that teardown is now race-safe, so the gate isn't buying correctness. Removed the render gate entirely (initialRenderMayBeActiveLocked + the UseRequest check). The context is canceled on client disconnect before the handler returns, and a detached/background context stays live after it returns, so it could both admit an early callback mid-render and reject the real WebSocket — exactly as you note. With no gate, an early callback may claim and tear down mid-render, but teardown no longer mutates render state, never reuses the identity, and reserves the key, so there's no corruption or race — only a degraded render on a probed page. The fixtures that decoupled the initial-render request from the WebSocket request (needed only for the gate) revert with it.

[P2] Gate broke streamed /noscript. Fixed by the gate removal — /noscript again reaches the Request and returns 204 while recording ErrJavascriptDisabled. Added TestNoscriptDuringLiveRenderRecordsJavascriptDisabled (live cancelable initial context → 204 + ErrJavascriptDisabled) as a regression guard against re-adding any such gate.

[P2] Drained queues retained payloads in pooled capacity. Right — clear(rq.wsQueue) after a reslice-to-zero cleared nothing. Two changes: getSendMsgs now zeroes the drained wsQueue before reslicing (prompt payload release during the request's life), and releaseBuffersLocked clears todoDirt/elems/wsQueue through capacity so a drained-to-zero buffer can't pin HTML in the pooled backing array. TestReleaseBuffersLockedZeroesWsQueue now reslices to zero length before releasing to prove it.

[P3] "inert" was wrong. Corrected Element doc, README, and the maintainer checklist: a finished Request keeps its Element fields, so a retained Element is not a no-op — it's unregistered (no further broadcasts/updates) and its identity is never reused, but its methods still operate on the finished Request. The checklist now describes identity-stable buffer pooling + key tombstoning instead of whole-Request pooling/pointer reuse.

Full gate green: go test -race and -tags debug -race across the module, vet, staticcheck, golangci-lint (0 issues), 100% coverage on the changed functions. Benchmark path is unchanged from the previous round (the tombstone AddCleanup remains the per-recycle cost; gate removal doesn't touch jw.recycle).

Third review round. releaseBuffersLocked clears only the live length of the reusable buffers, not their full capacity. Every path that shrinks a buffer already zeroes the vacated entries (getSendMsgs and drainTailScript for wsQueue, makeUpdateList for todoDirt, slices.DeleteFunc for elems), so the retained capacity holds no live references. Clearing through capacity rescanned a pooled buffer's high-water mark on every recycle, including later empty requests, under rq.mu/muQueue. BenchmarkRequestRecycleAfterHighWater shows empty recycling now stays flat across the high-water axis. Add exact regression coverage for the two teardown races the earlier test missed: TestFinishDoesNotResetJidCounter asserts a monotonic Jid across teardown (a restored lastJid = 0 fails it), and TestRecycleQueueRaceDoesNotLeak races a queue against recycle under -race (moving the wsQueue transfer outside muQueue trips the detector). TestGetSendMsgsZeroesDrainedQueue covers the drain zeroing directly. Finish the lifecycle docs: the Request and Element docs no longer call a finished Request or a retained Element "inert" — their methods still run; they are just unregistered and never reused. The README drops the removed render gate and, in the maintainer checklist, distinguishes completion (releases buffers) from non-running retirement (which preserves them for the initial HTTP handler).

linkdata · 2026-07-21T15:50:27Z

Round 3 addressed in 1e43966 — all three correct.

[P2] Capacity clear on every recycle. Reverted to clearing only the live length. You're right that it's redundant now: every shrink path zeroes vacated entries (getSendMsgs and drainTailScript for wsQueue, makeUpdateList for todoDirt, slices.DeleteFunc for elems), so the retained capacity holds no live references, and clearing through capacity rescanned the high-water mark on every recycle (incl. empty ones) under rq.mu/muQueue. Added BenchmarkRequestRecycleAfterHighWater, which is flat across the high-water axis (~0.5µs at highwater 0 / 1K / 100K) — with the capacity clear it grew with high-water as you measured.

[P2] Exact regression coverage. Added:

TestFinishDoesNotResetJidCounter — asserts a monotonic Jid across teardown; restoring lastJid = 0 fails it.
TestRecycleQueueRaceDoesNotLeak — races rq.queue against recycle under -race; moving the wsQueue transfer outside muQueue trips the detector.
TestGetSendMsgsZeroesDrainedQueue — covers the drain zeroing directly.

[P3] Docs. Request/Element docs no longer say a finished Request or retained Element is "inert" — their methods still run; they're just unregistered and never reused. The README lifecycle section drops the removed render gate, and the maintainer checklist now distinguishes completion (releases buffers, clears live length) from non-running retirement (preserves Elements/buffers for the initial HTTP handler). The PR description is rewritten to match the final design (no "inert"/gate claims; current benchmark numbers).

Full gate green: -race and -tags debug -race across the module, vet, staticcheck, golangci-lint (0 issues), 100% coverage on the changed functions.

Documentation-only. The Request type doc no longer claims lifecycle completion always releases buffers and leaves empty, useless collections: it distinguishes completion after WebSocket serving (which releases buffers to the pool) from retirement of an unclaimed Request (which preserves Elements and buffers for an initial HTTP handler that still holds the pointer), and states only that a finished Request is unregistered, never reused, and receives no further broadcasts or updates — its methods still run but affect no live connection. releaseBuffersLocked's contract no longer says "no other goroutine is still processing rq"; it now states that the WebSocket loop must have exited but an initial renderer may still run concurrently, which is exactly why teardown leaves render-visible Element/Jid state untouched and transfers wsQueue under muQueue. Refresh stale test comments that referenced clearLocked, key zeroing on recycle, or the pool reusing a *Request: recycle now tombstones the key and only buffers are pooled, so those tests exercise finished/unregistered requests and distinct later identities rather than reused pointers.

linkdata · 2026-07-21T16:18:20Z

Round 4 (docs) addressed in 0f9204e.

[P3] Request lifecycle contract. The Request type doc no longer says completion always releases buffers and leaves empty/useless collections. It now:

distinguishes completion from retirement — completion after WebSocket serving releases the buffers to the pool; retirement of an unclaimed Request preserves its Elements and buffers so an initial HTTP handler still holding the pointer can finish rendering;
states only what holds in both cases — a finished Request is unregistered, its identity is never reused, and its methods still run but affect no live connection (the initial renderer may continue and re-materialize throwaway elements/messages).

releaseBuffersLocked contract. Dropped "ensure no other goroutine is still processing rq" — that contradicted the supported renderer race. It now states the WebSocket loop must have exited but an initial renderer may still be running concurrently, which is precisely why teardown leaves render-visible Element/Jid state untouched and transfers wsQueue under muQueue.

Stale test comments. Fixed the two you cited (request_test.go claim/startServe and zero-key-dest) plus the adjacent ones I found in the same sweep (TestServeHTTP_TailScript_RejectsRecycledKey, TestJaws_cancelIfCurrent_IgnoresStaleRequest, TestRequest_TailScriptConcurrentWithRecycle): they referenced clearLocked, key-zeroing on recycle, or the pool reusing a *Request. Recycle now tombstones the key and pools only buffers, so those comments describe finished/unregistered requests and distinct later identities rather than reused pointers. A repo-wide sweep for clearLocked / "reused pool" / "recycled+reused" now comes back clean (only the accurate "never pooled or reused" statements remain).

Docs-only round; full gate still green (-race and -tags debug -race across the module, vet, staticcheck, golangci-lint 0 issues).

…Request guarantee Documentation-only, refining the lifecycle contract. The Request type doc no longer equates the end of the initial HTTP render with the Request finishing: the render handler returning normally leaves the Request pending until UseRequest claims it for the WebSocket. The Request finishes when its WebSocket handling ends, or when a non-running Request is retired ("non-running", not "unclaimed", also in the README lifecycle section). The finished-Request guarantee is scoped to identity-targeted operations: because an identity is never reused, updates and broadcasts aimed at a finished Request reach nothing, but this is not a general no-op — Request.Dirty delegates to instance-wide dirtying and can still update matching Elements on other live Requests. Using a Request past its borrowed lifecycle is documented as unsupported. Finish removing the whole-Request pooling/key-zeroing model from comments: the requestBuffers doc, the buffers field, and appendDirtyTags no longer imply every finished Request returns its buffers (retirement preserves them); the wantMessage and ProducersSkip test comments no longer say recycle zeroes JawsKey (the key is preserved — only registered flips and destKey returns zero).

linkdata · 2026-07-21T17:09:19Z

Round 5 (docs) in 986ca7a.

[P3] Pointer ownership vs completion. The Request type doc no longer treats the end of the initial HTTP render as finishing the Request — the handler returning leaves it pending until UseRequest claims it for the WebSocket. The Request finishes when its WebSocket handling ends, or when a non-running Request is retired (also switched "unclaimed" → "non-running" in the README lifecycle section).

[P3] Scope of the finished-Request guarantee. Limited to identity-targeted operations: since an identity is never reused, updates/broadcasts aimed at a finished Request reach nothing — but it's explicitly not a general no-op, because Request.Dirty delegates to instance-wide dirtying and can still update matching Elements on other live Requests. Post-lifetime use is documented as unsupported.

[P3] Remaining pooling/key-model wording. The requestBuffers doc, the buffers field, and appendDirtyTags no longer imply every finished Request returns its buffers (retirement preserves them). TestRequest_wantMessageConcurrentWithRecycle and TestRequest_ProducersSkipRecycled no longer say recycle zeroes JawsKey — the key is preserved; only registered flips and destKey() returns zero.

(Left README.md:675 as-is: that's the timeout-configuration section describing the user-facing default — an unclaimed Request is retired after 10s — which is accurate framing for that scenario rather than the retirement mechanism.)

Docs-only; full gate green (16/16 packages under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues).

…op rebind fixture Documentation and test-fixture accuracy. Early teardown clears the reusable collections (element list, tag map, dirt list, message queue) while leaving the individual Element fields and the Jid counter intact. Describe it as race-safe rather than non-destructive: releaseBuffersLocked's contract and the README now say a concurrent render may degrade (later GetElements and getElementByJid find nothing) but stays race-safe — no data race, no reused identity, no duplicated id. TestRequestFinishDoesNotPanicOnContinuedRender's comment no longer calls continued registration a no-op: NewElement still creates an Element and advances the Jid; only Tag is dropped (via the nil-map guard). Remove the impossible Request-rebinding fixture in TestSession_ProducersSkipRecycled. It modeled a pooled *Request being cleared and acquiring an unrelated identity, which identity stability makes impossible. It now uses a single entry modeling a finished, Session-detached Request (its own nonzero key, session == nil), which is what the producers' skip branch actually guards; the real snapshot-vs-finish race is covered by TestSessionCloseDoesNotReachLaterRequest.

linkdata · 2026-07-21T17:18:21Z

Round 6 (docs + one test fixture) in df868e1.

[P3] Early teardown is race-safe, not fully preserved. Correct — teardown clears rq.elems, the tag map, and queued fixups while leaving the individual Element fields and the Jid counter intact. releaseBuffersLocked's contract and the README now say a concurrent render may degrade (later GetElements/getElementByJid find nothing) but stays race-safe: no data race, no reused identity, no duplicated id — instead of "is not corrupted." TestRequestFinishDoesNotPanicOnContinuedRender's comment no longer calls continued registration a no-op: NewElement still creates an Element and advances the Jid; only Tag is dropped via the nil-map guard.

[P3] Impossible rebind fixture removed. TestSession_ProducersSkipRecycled no longer fabricates a pooled *Request that was "cleared" and "acquired an unrelated nonzero identity" — identity stability makes that impossible. It now uses a single entry modeling a finished, Session-detached Request (its own nonzero key, session == nil), which is exactly what the producers' skip branch (sessionDestKey → 0) guards. The real snapshot-vs-finish race is covered by TestSessionCloseDoesNotReachLaterRequest as you noted. sessionDestKey stays at 100% coverage with the single entry.

Full gate green: 16/16 packages under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues.

Documentation and test-name accuracy. Qualify the early-teardown contract: clearing the collections forgets the already-rendered elements and tags (a later GetElements finds nothing), but a subsequent NewElement repopulates rq.elems and is again findable by Jid. Correct the locking claim — the lock-free Element fields (ui, handlers) are what stay intact; lastJid is advanced only under rq.mu (not read lock-free) so Jids stay monotonic, and wsQueue transfers under muQueue. Mirror this in the README. Finish the stale lifecycle terminology: the #195 reproduction is renamed TestEarlyCallbackPreservesInitialRenderIdentity, and its comment and failure message now say the callback tears the Request down but must not destroy its stable identity (the test asserts only that the key survives). deadSession's "recycled or rebound to another Session" wording becomes "finished, or detached from this Session". The TestSession_ProducersSkipRecycled fixture comment now says the snapshot is taken under sess.mu and processed after releasing it, not taken after unlocking.

linkdata · 2026-07-21T17:26:09Z

Round 7 (docs) in f57d5b7.

[P3] Post-teardown lookup and locking. Corrected both:

The forgotten-elements claim is qualified — clearing the collections forgets the already-rendered elements/tags (a later GetElements finds nothing), but a subsequent NewElement repopulates rq.elems and is again findable by Jid.
The locking claim is fixed — the lock-free fields are Element.ui/handlers (left intact); lastJid is advanced only under rq.mu (not read lock-free) so Jids stay monotonic/unique, and wsQueue transfers under muQueue. Mirrored in the README.

[P3] Stale lifecycle terminology.

The Jaws.ServeHTTP can recycle a Request while its initial renderer still owns it #195 reproduction is renamed TestEarlyCallbackPreservesInitialRenderIdentity; its comment and failure message now say the callback does tear the Request down (clearing collections) but must not destroy its stable identity — which is all the test asserts (the key survives).
deadSession's "recycled or rebound to another Session" → "finished, or detached from this Session".
TestSession_ProducersSkipRecycled's fixture comment now says the snapshot is taken under sess.mu and processed after releasing it, not "after releasing sess.mu".

Full gate green: 16/16 packages under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues.

…nt to reachability TestEarlyCallbackPreservesInitialRenderIdentity now captures the key before the early callback and asserts it is unchanged afterward, instead of merely checking it is nonempty; the comment says the key stays stable and nonzero (the finished Request is unregistered), not "valid". Scope the key non-reassignment claims to the tombstone lifetime. destKey's doc and the README maintainer checklist now say a finished Request's key is reserved (not reassigned) only while the Request remains reachable; once it is collected the tombstone is cleaned up and the key becomes eligible for random reuse, by which point no stale destination referencing it can remain. A wantMessage test comment is likewise qualified.

linkdata · 2026-07-21T17:34:14Z

Round 8 (docs/test) in 926e116.

[P3] Identity test now asserts stability. TestEarlyCallbackPreservesInitialRenderIdentity captures wantKey before the callback and asserts rq.JawsKeyString() == wantKey afterward (plus that it's nonzero to begin with), instead of only checking non-empty. The comment describes the key as stable and nonzero — the finished Request is unregistered — rather than "valid".

[P3] Key non-reassignment scoped to the tombstone lifetime. destKey's doc and the README checklist now say a finished Request's key is reserved (not reassigned) only while the Request remains reachable; after it is collected the tombstone is cleaned up and the key becomes eligible for random reuse — by which point no stale destination referencing it can remain. Qualified a wantMessage test comment the same way. (The existing "reuse timing is unspecified" note near the key-generation section was already scoped correctly.)

Full gate green: 16/16 packages under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues.

A key VALUE (a copied key.Key, a queued wire.Message.Dest, or a browser /jaws/<key> URL) can outlive the *Request. While the finished Request stays reachable its key is held reserved by a nil tombstone, so a stale destination matches nothing; but after the Request is collected the tombstone is removed and the CSPRNG may eventually reissue that key value to a new Request, which such a stale destination could then match. Only the *Request pointer is never reused, so pointer-derived operations are always safe. destKey's doc no longer claims "no such stale destination can remain" once the Request is collected; it now scopes the guarantee to the reachable window and acknowledges post-collection CSPRNG reuse, separating pointer-derived operations (always safe) from key-value-derived ones. The wantMessage regression test's summary comment is corrected the same way (its inline comment was already scoped). Retaining tombstones for the whole Jaws lifetime would make the absolute guarantee hold, but at the cost of an unbounded jw.requests leak (one dead entry per request ever created), so the cleanup-based, correctly-scoped guarantee is kept.

linkdata · 2026-07-21T17:52:00Z

Round 9 in 0b338d5.

[P3] Stale destinations don't disappear on collection. Correct — a key value (copied key.Key, queued wire.Message.Dest, or a browser /jaws/<key> URL) can outlive the *Request, and after tombstone cleanup the CSPRNG can reissue that value. destKey's doc drops "no such stale destination can remain" and now scopes the guarantee: a captured key value matches nothing only while the finished Request stays reachable (its key held reserved by the tombstone); after collection the tombstone is removed and the value may be reissued to a new Request, which a stale destination could then match. Only the *Request pointer is never reused, so pointer-derived operations are always safe. Fixed the same overclaim in the wantMessage regression test's summary comment.

I kept the code (docs-fix, not permanent tombstones): I ran a small audit to be sure. Verification confirmed the only path by which a captured key value reaches a different Request is a post-cleanup CSPRNG collision (2⁻⁶⁴, further gated by the IP check on the reachable /jaws and .tail paths), and that retaining tombstones for the Jaws lifetime would be an unbounded jw.requests leak (~one dead entry per request ever created). A codebase-wide sweep of all "never reused/reassigned/retargeted" claims (31 of them) found every other one is either pointer-identity (absolute-true, since the *Request is GC'd and never handed out again) or already scoped to "while registered/reachable" — these two were the only overclaims.

Full gate green: 16/16 packages under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues.

…ing + destKey resolution The previous wording claimed "pointer-derived operations never reach a different connection", which is false: Request.Dirty dirties matching Elements on other live Requests by design, and an Alert/Redirect message queued before completion carries a key.Key destination that can outlive the *Request and its tombstone. State only the two guarantees that always hold — the *Request pointer is never reused so it never aliases another connection, and destKey returns zero once the Request has finished so it cannot hand back the finished destination — and explicitly note it is not a blanket pointer-derived-safety claim.

linkdata · 2026-07-21T17:58:13Z

Round 10 in the latest push.

[P3] Narrowed the destKey guarantee. You're right that "pointer-derived operations never reach a different connection" is false — Request.Dirty dirties matching Elements on other live Requests by design, and an Alert/Redirect queued before completion carries a key.Key destination that outlives the pointer/tombstone. destKey's doc now states only the two guarantees that actually hold:

the *Request pointer is never reused, so it never aliases another connection, and
destKey returns zero once the Request has finished, so it cannot hand back the finished Request's destination —

and it explicitly flags that this is not a blanket "pointer-derived operations are safe" claim, naming Request.Dirty and queued Alert/Redirect as the counterexamples.

Full gate green: 16/16 under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues.

…key guarantee The Request godoc no longer claims all identity-targeted operations are never retargeted. It defines the never-reused identity as the *Request pointer (GC'd, never handed out again, so a retained pointer never aliases another connection), and scopes the operational guarantee to destinations resolved from the Request after completion (Alert/Redirect see an empty destKey and reach nothing). It reiterates this is not a blanket no-op: Request.Dirty dirties other Requests by design, and an Alert/Redirect queued before completion carries a key value that can outlive the Request and match a different one after key reuse. The README key-reservation invariant is qualified from "in every case" to registered Requests: a Request created by NewRequest after Jaws.Close is unregistered and installs no tombstone, so two post-close calls could receive the same key — harmless, since neither is claimable.

linkdata · 2026-07-21T18:05:06Z

Round 11 in the latest push.

[P3] Exported Request guarantee narrowed. The godoc now defines the never-reused identity as the *Request pointer (GC'd, never handed out again → a retained pointer never aliases another connection), and scopes the operational guarantee to destinations resolved from the Request after completion — Alert/Redirect see an empty destKey and reach nothing. It reiterates this is not a blanket claim: Request.Dirty dirties other Requests by design, and an Alert/Redirect queued before completion carries a key.Key that can outlive the Request and match a different one after key reuse. (I also aligned the operational paragraph lower down so it doesn't restate the broad "identity-targeted operations reach nothing" wording.)

[P3] Post-Close exception in the README. Qualified "in every case" → registered Requests. A Request created by NewRequest after Jaws.Close is never registered and installs no tombstone, so two post-close calls could receive the same key — harmless, since neither is claimable (both have canceled contexts and UseRequest can't claim them).

Full gate green: 16/16 under -race, -tags debug -race, vet, staticcheck, golangci-lint 0 issues; go doc renders cleanly.

linkdata added 2 commits July 21, 2026 15:58

linkdata merged commit 395e7ec into main Jul 21, 2026
7 checks passed

linkdata deleted the fix/195-stable-request-identity branch July 21, 2026 18:13

linkdata mentioned this pull request Jul 21, 2026

refactor: consolidate Request lifecycle flags into an atomic int32 state #207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: never reuse Request identities, pool only buffers (#195)#206

fix: never reuse Request identities, pool only buffers (#195)#206
linkdata merged 12 commits into
mainfrom
fix/195-stable-request-identity

linkdata commented Jul 21, 2026 •

edited

Loading

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

linkdata commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Tests

Verification

Performance

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

linkdata commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

linkdata commented Jul 21, 2026 •

edited

Loading